patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH] ring: enforce reading tail before slots
@ 2019-10-08  6:38 Gavin Hu
  2019-10-24  8:36 ` Luca Boccassi
  0 siblings, 1 reply; 2+ messages in thread
From: Gavin Hu @ 2019-10-08  6:38 UTC (permalink / raw)
  To: stable; +Cc: nd, thomas, nipun.gupta, konstantin.ananyev, gavin.hu

In weak memory models, like arm64, reading the prod.tail may get
reordered after reading the ring slots, which corrupts the ring and
stale data is observed.

This issue was reported by NXP on 8-A72 DPAA2 board. The problem is most
likely caused by missing the acquire semantics when reading
prod.tail (in SC dequeue) which makes it possible to read a
stale value from the ring slots.

For MP (and MC) case, rte_atomic32_cmpset() already provides the required
ordering. For SP case, the control depependency between if-statement (which
depends on the read of r->cons.tail) and the later stores to the ring slots
make RMB unnecessary. About the control dependency, read more at:
https://www.cl.cam.ac.uk/~pes20/ppc-supplemental/test7.pdf

This patch is adding the required read barrier to prevent reading the ring
slots get reordered before reading prod.tail for SC case.

Fixes: 0dfc98c507b1 ("ring: separate out head index manipulation")
Cc: stable@dpdk.org

Signed-off-by: Gavin Hu <gavin.hu@arm.com>
Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
Tested-by: Nipun Gupta <nipun.gupta@nxp.com>
Acked-by: Nipun Gupta <nipun.gupta@nxp.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_ring/rte_ring.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index e924438..9e6ccfe 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -544,11 +544,14 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
 			return 0;
 
 		*new_head = *old_head + n;
-		if (is_sc)
-			r->cons.head = *new_head, success = 1;
-		else
+		if (is_sc) {
+			r->cons.head = *new_head;
+			rte_smp_rmb();
+			success = 1;
+		} else {
 			success = rte_atomic32_cmpset(&r->cons.head, *old_head,
 					*new_head);
+		}
 	} while (unlikely(success == 0));
 	return n;
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [dpdk-stable] [PATCH] ring: enforce reading tail before slots
  2019-10-08  6:38 [dpdk-stable] [PATCH] ring: enforce reading tail before slots Gavin Hu
@ 2019-10-24  8:36 ` Luca Boccassi
  0 siblings, 0 replies; 2+ messages in thread
From: Luca Boccassi @ 2019-10-24  8:36 UTC (permalink / raw)
  To: Gavin Hu, stable; +Cc: nd, thomas, nipun.gupta, konstantin.ananyev

On Tue, 2019-10-08 at 14:38 +0800, Gavin Hu wrote:
> In weak memory models, like arm64, reading the prod.tail may get
> reordered after reading the ring slots, which corrupts the ring and
> stale data is observed.
> 
> This issue was reported by NXP on 8-A72 DPAA2 board. The problem is
> most
> likely caused by missing the acquire semantics when reading
> prod.tail (in SC dequeue) which makes it possible to read a
> stale value from the ring slots.
> 
> For MP (and MC) case, rte_atomic32_cmpset() already provides the
> required
> ordering. For SP case, the control depependency between if-statement
> (which
> depends on the read of r->cons.tail) and the later stores to the ring
> slots
> make RMB unnecessary. About the control dependency, read more at:
> https://www.cl.cam.ac.uk/~pes20/ppc-supplemental/test7.pdf
> 
> 
> This patch is adding the required read barrier to prevent reading the
> ring
> slots get reordered before reading prod.tail for SC case.
> 
> Fixes: 0dfc98c507b1 ("ring: separate out head index manipulation")
> Cc: 
> stable@dpdk.org
> 
> 
> Signed-off-by: Gavin Hu <
> gavin.hu@arm.com
> >
> Reviewed-by: Ola Liljedahl <
> ola.liljedahl@arm.com
> >
> Tested-by: Nipun Gupta <
> nipun.gupta@nxp.com
> >
> Acked-by: Nipun Gupta <
> nipun.gupta@nxp.com
> >
> Acked-by: Konstantin Ananyev <
> konstantin.ananyev@intel.com
> >
> ---
>  lib/librte_ring/rte_ring.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> index e924438..9e6ccfe 100644
> --- a/lib/librte_ring/rte_ring.h
> +++ b/lib/librte_ring/rte_ring.h
> @@ -544,11 +544,14 @@ __rte_ring_move_cons_head(struct rte_ring *r,
> int is_sc,
>  			return 0;
>  
>  		*new_head = *old_head + n;
> -		if (is_sc)
> -			r->cons.head = *new_head, success = 1;
> -		else
> +		if (is_sc) {
> +			r->cons.head = *new_head;
> +			rte_smp_rmb();
> +			success = 1;
> +		} else {
>  			success = rte_atomic32_cmpset(&r->cons.head,
> *old_head,
>  					*new_head);
> +		}
>  	} while (unlikely(success == 0));
>  	return n;
>  }
> 

Acked-by: Luca Boccassi <bluca@debian.org>

Thanks, applied

-- 
Kind regards,
Luca Boccassi

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08  6:38 [dpdk-stable] [PATCH] ring: enforce reading tail before slots Gavin Hu
2019-10-24  8:36 ` Luca Boccassi

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox