DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ruifeng Wang <Ruifeng.Wang@arm.com>
To: Lance Richardson <lance.richardson@broadcom.com>,
	"Ajit Khaparde (ajit.khaparde@broadcom.com)"
	<ajit.khaparde@broadcom.com>,
	Somnath Kotur <somnath.kotur@broadcom.com>,
	Bruce Richardson <bruce.richardson@intel.com>,
	Konstantin Ananyev <konstantin.ananyev@intel.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>,
	Stephen Hurd <stephen.hurd@broadcom.com>,
	David Christensen <david.christensen@broadcom.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"stable@dpdk.org" <stable@dpdk.org>, nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH] net/bnxt: fix missing barriers in completion handling
Date: Fri, 9 Jul 2021 05:59:59 +0000	[thread overview]
Message-ID: <HE1PR0802MB2474CDAA57CCF626D8F887999E189@HE1PR0802MB2474.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20210708191501.77972-1-lance.richardson@broadcom.com>

> -----Original Message-----
> From: Lance Richardson <lance.richardson@broadcom.com>
> Sent: Friday, July 9, 2021 3:15 AM
> To: Ajit Khaparde (ajit.khaparde@broadcom.com)
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Bruce Richardson
> <bruce.richardson@intel.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; jerinj@marvell.com; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Stephen Hurd <stephen.hurd@broadcom.com>;
> David Christensen <david.christensen@broadcom.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH] net/bnxt: fix missing barriers in completion handling
> 
> Ensure that Rx/Tx/Async completion entry fields are accessed
> only after the completion's valid flag has been loaded and
> verified. This is needed for correct operation on systems that
> use relaxed memory consistency models.
> 
> Fixes: 2eb53b134aae ("net/bnxt: add initial Rx code")
> Fixes: 6eb3cc2294fd ("net/bnxt: add initial Tx code")
> Cc: stable@dpdk.org
> Signed-off-by: Lance Richardson <lance.richardson@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>  drivers/net/bnxt/bnxt_cpr.h           | 36 ++++++++++++++++++++++++---
>  drivers/net/bnxt/bnxt_ethdev.c        | 16 ++++++------
>  drivers/net/bnxt/bnxt_irq.c           |  7 +++---
>  drivers/net/bnxt/bnxt_rxr.c           |  9 ++++---
>  drivers/net/bnxt/bnxt_rxtx_vec_avx2.c |  2 +-
>  drivers/net/bnxt/bnxt_rxtx_vec_neon.c |  2 +-
>  drivers/net/bnxt/bnxt_rxtx_vec_sse.c  |  2 +-
>  drivers/net/bnxt/bnxt_txr.c           |  2 +-
>  8 files changed, 54 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_cpr.h b/drivers/net/bnxt/bnxt_cpr.h
> index 2a56ec52c..3ee6b74bc 100644
> --- a/drivers/net/bnxt/bnxt_cpr.h
> +++ b/drivers/net/bnxt/bnxt_cpr.h
> @@ -8,13 +8,10 @@
>  #include <stdbool.h>
> 
>  #include <rte_io.h>
> +#include "hsi_struct_def_dpdk.h"
> 
>  struct bnxt_db_info;
> 
> -#define CMP_VALID(cmp, raw_cons, ring)
> 	\
> -	(!!(rte_le_to_cpu_32(((struct cmpl_base *)(cmp))->info3_v) &
> 	\
> -	    CMPL_BASE_V) == !((raw_cons) & ((ring)->ring_size)))
> -
>  #define CMP_TYPE(cmp)						\
>  	(((struct cmpl_base *)cmp)->type & CMPL_BASE_TYPE_MASK)
> 
> @@ -121,4 +118,35 @@ bool bnxt_is_recovery_enabled(struct bnxt *bp);
>  bool bnxt_is_master_func(struct bnxt *bp);
> 
>  void bnxt_stop_rxtx(struct bnxt *bp);
> +
> +/**
> + * Check validity of a completion ring entry. If the entry is valid, include a
> + * C11 __ATOMIC_ACQUIRE fence to ensure that subsequent loads of fields
> in the
> + * completion are not hoisted by the compiler or by the CPU to come before
> the
> + * loading of the "valid" field.
> + *
> + * Note: the caller must not access any fields in the specified completion
> + * entry prior to calling this function.
> + *
> + * @param cmp
Nit, cmpl

> + *   Pointer to an entry in the completion ring.
> + * @param raw_cons
> + *   Raw consumer index of entry in completion ring.
> + * @param ring_size
> + *   Size of completion ring.
> + */
> +static __rte_always_inline bool
> +bnxt_cpr_cmp_valid(const void *cmpl, uint32_t raw_cons, uint32_t
> ring_size)
> +{
> +	const struct cmpl_base *c = cmpl;
> +	bool expected, valid;
> +
> +	expected = !(raw_cons & ring_size);
> +	valid = !!(rte_le_to_cpu_32(c->info3_v) & CMPL_BASE_V);
> +	if (valid == expected) {
> +		rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
> +		return true;
> +	}
> +	return false;
> +}
>  #endif
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> b/drivers/net/bnxt/bnxt_ethdev.c
> index ed09f1bf5..ee6929692 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c

<snip>

> 
>  	/* Check to see if hw has posted a completion for the descriptor. */
> @@ -3327,7 +3327,7 @@ bnxt_tx_descriptor_status_op(void *tx_queue,
> uint16_t offset)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
cpr->cp_ring_struct->ring_size can be used instead of 'ring_mask + 1'?

>  			break;
> 
>  		if (CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2)

<snip>

> diff --git a/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> b/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> index 263e6ec3c..13211060c 100644
> --- a/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> +++ b/drivers/net/bnxt/bnxt_rxtx_vec_neon.c
> @@ -339,7 +339,7 @@ bnxt_handle_tx_cp_vec(struct bnxt_tx_queue *txq)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
Same here. I think cpr->cp_ring_struct->ring_size can be used and it avoids calculation.
Also some places in other vector files.

>  			break;
> 
>  		if (likely(CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2))
> diff --git a/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> b/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> index 9a53d1fba..6e5630532 100644
> --- a/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> +++ b/drivers/net/bnxt/bnxt_rxtx_vec_sse.c
> @@ -320,7 +320,7 @@ bnxt_handle_tx_cp_vec(struct bnxt_tx_queue *txq)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
>  			break;
> 
>  		if (likely(CMP_TYPE(txcmp) == TX_CMPL_TYPE_TX_L2))
> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
> index 9a6b96e04..47824334a 100644
> --- a/drivers/net/bnxt/bnxt_txr.c
> +++ b/drivers/net/bnxt/bnxt_txr.c
> @@ -461,7 +461,7 @@ static int bnxt_handle_tx_cp(struct bnxt_tx_queue
> *txq)
>  		cons = RING_CMPL(ring_mask, raw_cons);
>  		txcmp = (struct tx_cmpl *)&cpr->cp_desc_ring[cons];
> 
> -		if (!CMP_VALID(txcmp, raw_cons, cp_ring_struct))
> +		if (!bnxt_cpr_cmp_valid(txcmp, raw_cons, ring_mask + 1))
>  			break;
> 
>  		opaque = rte_le_to_cpu_32(txcmp->opaque);
> --
> 2.25.1


  reply	other threads:[~2021-07-09  6:00 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-08 19:15 Lance Richardson
2021-07-09  5:59 ` Ruifeng Wang [this message]
2021-07-09 14:48   ` Lance Richardson
2021-07-09 16:38 ` [dpdk-dev] [PATCH v2] " Lance Richardson
2021-07-12  2:34   ` Ruifeng Wang
2021-07-12 21:43     ` Ajit Khaparde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=HE1PR0802MB2474CDAA57CCF626D8F887999E189@HE1PR0802MB2474.eurprd08.prod.outlook.com \
    --to=ruifeng.wang@arm.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.christensen@broadcom.com \
    --cc=dev@dpdk.org \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=lance.richardson@broadcom.com \
    --cc=nd@arm.com \
    --cc=somnath.kotur@broadcom.com \
    --cc=stable@dpdk.org \
    --cc=stephen.hurd@broadcom.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).