From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH] net/fm10k: fix Rx descriptor read timing Date: Tue, 5 Jul 2016 10:43:42 +0100 Message-ID: <20160705094342.GD10232@bricha3-MOBL3> References: <1467618668-28611-1-git-send-email-xiao.w.wang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: jing.d.chen@intel.com, dev@dpdk.org To: Wang Xiao W Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 9C3C3530A for ; Tue, 5 Jul 2016 11:43:49 +0200 (CEST) Content-Disposition: inline In-Reply-To: <1467618668-28611-1-git-send-email-xiao.w.wang@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Jul 04, 2016 at 03:51:08PM +0800, Wang Xiao W wrote: > We find that when traffic is light, a few amount of packets will be > wrongly parsed (e.g. packet type), however this issue will not happen > when traffic is heavy. > > The root cause is some fields in fm10k_rx_desc are read at wrong timing. > When the input speed is slower than software's capability, fm10k scalar > Rx function accesses descriptors closely to HW, so there's potential > sequence: some fields like pkt_info in fm10k_rx_desc are read before HW > writeback but some fields like DD bit are read after HW writeback, this > will lead to the later packet parsing function using incorrect value. > > This patch fixes this issue by reading and parsing Rx descriptor after > DD bit is set. > > Fixes: 4b61d3bfa941 ("fm10k: add receive and tranmit") > Fixes: c82dd0a7bfa5 ("fm10k: add scatter receive") > > Signed-off-by: Wang Xiao W Fix looks correct to me. The key part I think is that the read of the descriptor at line "desc = q->hw_ring[next_dd];" is not atomic as it's a read of 16B, allowing part of the descriptor to be read either side of a HW write-back. Acked-by: Bruce Richardson > --- > drivers/net/fm10k/fm10k_rxtx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/fm10k/fm10k_rxtx.c b/drivers/net/fm10k/fm10k_rxtx.c > index dd92a91..5b2d04b 100644 > --- a/drivers/net/fm10k/fm10k_rxtx.c > +++ b/drivers/net/fm10k/fm10k_rxtx.c > @@ -114,10 +114,10 @@ fm10k_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > > nb_pkts = RTE_MIN(nb_pkts, q->alloc_thresh); > for (count = 0; count < nb_pkts; ++count) { > + if (!(q->hw_ring[next_dd].d.staterr & FM10K_RXD_STATUS_DD)) > + break; > mbuf = q->sw_ring[next_dd]; > desc = q->hw_ring[next_dd]; > - if (!(desc.d.staterr & FM10K_RXD_STATUS_DD)) > - break; > #ifdef RTE_LIBRTE_FM10K_DEBUG_RX > dump_rxd(&desc); > #endif > @@ -228,10 +228,10 @@ fm10k_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, > > nb_seg = RTE_MIN(nb_pkts, q->alloc_thresh); > for (count = 0; count < nb_seg; count++) { > + if (!(q->hw_ring[next_dd].d.staterr & FM10K_RXD_STATUS_DD)) > + break; > mbuf = q->sw_ring[next_dd]; > desc = q->hw_ring[next_dd]; > - if (!(desc.d.staterr & FM10K_RXD_STATUS_DD)) > - break; > #ifdef RTE_LIBRTE_FM10K_DEBUG_RX > dump_rxd(&desc); > #endif > -- > 1.9.3 >