[dpdk-dev] [PATCH] net/fm10k: fix Rx descriptor read timing

[dpdk-dev] [PATCH] net/fm10k: fix Rx descriptor read timing

[Wang Xiao W]

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 <xiao.w.wang@intel.com>
---
 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

Re: [dpdk-dev] [PATCH] net/fm10k: fix Rx descriptor read timing

[Bruce Richardson]

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 <xiao.w.wang@intel.com>

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 <bruce.richardson@intel.com>

> ---
>  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
> 

Re: [dpdk-dev] [PATCH] net/fm10k: fix Rx descriptor read timing

[Bruce Richardson]

On Tue, Jul 05, 2016 at 10:43:42AM +0100, Bruce Richardson wrote:
> 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 <xiao.w.wang@intel.com>
> 
> 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 <bruce.richardson@intel.com>
> 
Applied to dpdk-next-net/rel_16_07

/Bruce