[PATCH v1] ring: enforce reading the tails before ring operations

[PATCH v1] ring: enforce reading the tails before ring operations

[gavin hu]

In weak memory models, like arm64, reading the {prod,cons}.tail may get
reordered after reading or writing 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 cons.tail (in
SP enqueue) or prod.tail (in SC dequeue) which makes it possible to read a
stale value from the ring slots.  There must be a read fence before writing
or reading the ring slots, rte_atomic32_cmpset() provides the same ordering
for MP (and MC) case. This patch is to enforce this ordering for SP (and
SC) case.

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>
---
 lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
index ea7dbe5..1bd3dfd 100644
--- a/lib/librte_ring/rte_ring_generic.h
+++ b/lib/librte_ring/rte_ring_generic.h
@@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
 			return 0;
 
 		*new_head = *old_head + n;
-		if (is_sp)
-			r->prod.head = *new_head, success = 1;
-		else
+		if (is_sp) {
+			r->prod.head = *new_head;
+			rte_smp_rmb();
+			success = 1;
+		} else
 			success = rte_atomic32_cmpset(&r->prod.head,
 					*old_head, *new_head);
 	} while (unlikely(success == 0));
@@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring *r, unsigned 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));
-- 
2.7.4

Re: [v1] ring: enforce reading the tails before ring operations

[Ilya Maximets]

On 06.03.2019 6:07, gavin hu wrote:
> In weak memory models, like arm64, reading the {prod,cons}.tail may get
> reordered after reading or writing 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 cons.tail (in
> SP enqueue) or prod.tail (in SC dequeue) which makes it possible to read a
> stale value from the ring slots.  There must be a read fence before writing

Sorry, but the phrase "There must be a read fence before writing" makes no sense.
Could you please rephrase or describe in details which reads you're trying to
keep in exact order?

> or reading the ring slots, rte_atomic32_cmpset() provides the same ordering
> for MP (and MC) case. This patch is to enforce this ordering for SP (and
> SC) case.
> 
> 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>
> ---
>  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
> index ea7dbe5..1bd3dfd 100644
> --- a/lib/librte_ring/rte_ring_generic.h
> +++ b/lib/librte_ring/rte_ring_generic.h
> @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
>  			return 0;
>  
>  		*new_head = *old_head + n;
> -		if (is_sp)
> -			r->prod.head = *new_head, success = 1;
> -		else
> +		if (is_sp) {
> +			r->prod.head = *new_head;
> +			rte_smp_rmb();
> +			success = 1;
> +		} else
>  			success = rte_atomic32_cmpset(&r->prod.head,
>  					*old_head, *new_head);
>  	} while (unlikely(success == 0));
> @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring *r, unsigned 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));
> 

Re: [v1] ring: enforce reading the tails before ring operations

[Gavin Hu (Arm Technology China)]

Hi ilya,

Thanks for your comments, inline comments and new commit message in V2.

/Gavin

> -----Original Message-----
> From: Ilya Maximets <i.maximets@samsung.com>
> Sent: Wednesday, March 6, 2019 7:49 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> Subject: Re: [v1] ring: enforce reading the tails before ring operations
> 
> On 06.03.2019 6:07, gavin hu wrote:
> > In weak memory models, like arm64, reading the {prod,cons}.tail may get
> > reordered after reading or writing 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 cons.tail (in
> > SP enqueue) or prod.tail (in SC dequeue) which makes it possible to read
> a
> > stale value from the ring slots.  There must be a read fence before
> writing
> 
> Sorry, but the phrase "There must be a read fence before writing" makes
> no sense.
> Could you please rephrase or describe in details which reads you're trying
> to
> keep in exact order?

This patch is to keep in exact order for reading the tails(to calculate the available
/free slots of ring when moving the heads) before reading or writing the ring slots.

I rephrased the commit message in V2, could you have a look? 

> > or reading the ring slots, rte_atomic32_cmpset() provides the same
> ordering
> > for MP (and MC) case. This patch is to enforce this ordering for SP (and
> > SC) case.
> >
> > 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>
> > ---
> >  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/librte_ring/rte_ring_generic.h
> b/lib/librte_ring/rte_ring_generic.h
> > index ea7dbe5..1bd3dfd 100644
> > --- a/lib/librte_ring/rte_ring_generic.h
> > +++ b/lib/librte_ring/rte_ring_generic.h
> > @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r,
> unsigned int is_sp,
> >  			return 0;
> >
> >  		*new_head = *old_head + n;
> > -		if (is_sp)
> > -			r->prod.head = *new_head, success = 1;
> > -		else
> > +		if (is_sp) {
> > +			r->prod.head = *new_head;
> > +			rte_smp_rmb();
> > +			success = 1;
> > +		} else
> >  			success = rte_atomic32_cmpset(&r->prod.head,
> >  					*old_head, *new_head);
> >  	} while (unlikely(success == 0));
> > @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring *r,
> unsigned 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));
> >

[PATCH v2] ring: enforce reading the tails before ring operations

[gavin hu]

In weak memory models, like arm64, reading the {prod,cons}.tail may get
reordered after reading or writing 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 cons.tail (in
SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
reordered before reading {prod,cons}.tail for SP (and SC) case.

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>
---
 lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
index ea7dbe5..1bd3dfd 100644
--- a/lib/librte_ring/rte_ring_generic.h
+++ b/lib/librte_ring/rte_ring_generic.h
@@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
 			return 0;
 
 		*new_head = *old_head + n;
-		if (is_sp)
-			r->prod.head = *new_head, success = 1;
-		else
+		if (is_sp) {
+			r->prod.head = *new_head;
+			rte_smp_rmb();
+			success = 1;
+		} else
 			success = rte_atomic32_cmpset(&r->prod.head,
 					*old_head, *new_head);
 	} while (unlikely(success == 0));
@@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring *r, unsigned 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));
-- 
2.7.4

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Ilya Maximets]

On 07.03.2019 9:45, gavin hu wrote:
> In weak memory models, like arm64, reading the {prod,cons}.tail may get
> reordered after reading or writing 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 cons.tail (in
> SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
> reordered before reading {prod,cons}.tail for SP (and SC) case.

Read barrier rte_smp_rmb() is OK to prevent reading the ring get reordered
before reading the tail. However, to prevent *writing* the ring get reordered
*before reading* the tail you need a full memory barrier, i.e. rte_smp_mb().

> 
> 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>
> ---
>  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
> index ea7dbe5..1bd3dfd 100644
> --- a/lib/librte_ring/rte_ring_generic.h
> +++ b/lib/librte_ring/rte_ring_generic.h
> @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
>  			return 0;
>  
>  		*new_head = *old_head + n;
> -		if (is_sp)
> -			r->prod.head = *new_head, success = 1;
> -		else
> +		if (is_sp) {
> +			r->prod.head = *new_head;
> +			rte_smp_rmb();
> +			success = 1;
> +		} else
>  			success = rte_atomic32_cmpset(&r->prod.head,
>  					*old_head, *new_head);
>  	} while (unlikely(success == 0));
> @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring *r, unsigned 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));
> 

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: Ilya Maximets <i.maximets@samsung.com>
> Sent: Thursday, March 7, 2019 4:52 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> operations
> 
> On 07.03.2019 9:45, gavin hu wrote:
> > In weak memory models, like arm64, reading the {prod,cons}.tail may get
> > reordered after reading or writing 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 cons.tail (in
> > SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
> > reordered before reading {prod,cons}.tail for SP (and SC) case.
> 
> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> reordered
> before reading the tail. However, to prevent *writing* the ring get
> reordered
> *before reading* the tail you need a full memory barrier, i.e.
> rte_smp_mb().

ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while WMB(ST Option) orders ST/ST.
For more details, please refer to: Table B2-1 Encoding of the DMB and DSB <option> parameter  in
https://developer.arm.com/docs/ddi0487/latest/arm-architecture-reference-manual-armv8-for-armv8-a-architecture-profile 

> 
> >
> > 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>
> > ---
> >  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/librte_ring/rte_ring_generic.h
> b/lib/librte_ring/rte_ring_generic.h
> > index ea7dbe5..1bd3dfd 100644
> > --- a/lib/librte_ring/rte_ring_generic.h
> > +++ b/lib/librte_ring/rte_ring_generic.h
> > @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r,
> unsigned int is_sp,
> >  			return 0;
> >
> >  		*new_head = *old_head + n;
> > -		if (is_sp)
> > -			r->prod.head = *new_head, success = 1;
> > -		else
> > +		if (is_sp) {
> > +			r->prod.head = *new_head;
> > +			rte_smp_rmb();
> > +			success = 1;
> > +		} else
> >  			success = rte_atomic32_cmpset(&r->prod.head,
> >  					*old_head, *new_head);
> >  	} while (unlikely(success == 0));
> > @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring *r,
> unsigned 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));
> >

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Ilya Maximets]

On 07.03.2019 12:27, Gavin Hu (Arm Technology China) wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets@samsung.com>
>> Sent: Thursday, March 7, 2019 4:52 PM
>> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
>> dev@dpdk.org
>> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
>> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
>> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
>> operations
>>
>> On 07.03.2019 9:45, gavin hu wrote:
>>> In weak memory models, like arm64, reading the {prod,cons}.tail may get
>>> reordered after reading or writing 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 cons.tail (in
>>> SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
>>> reordered before reading {prod,cons}.tail for SP (and SC) case.
>>
>> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
>> reordered
>> before reading the tail. However, to prevent *writing* the ring get
>> reordered
>> *before reading* the tail you need a full memory barrier, i.e.
>> rte_smp_mb().
> 
> ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while WMB(ST Option) orders ST/ST.
> For more details, please refer to: Table B2-1 Encoding of the DMB and DSB <option> parameter  in
> https://developer.arm.com/docs/ddi0487/latest/arm-architecture-reference-manual-armv8-for-armv8-a-architecture-profile 

I see. But you have to change the rte_smp_rmb() function definition in
lib/librte_eal/common/include/generic/rte_atomic.h and assure that all
other architectures follows same rules.
Otherwise, this change is logically wrong, because read barrier in current
definition could not be used to order Load with Store.

> 
>>
>>>
>>> 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>
>>> ---
>>>  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
>>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/librte_ring/rte_ring_generic.h
>> b/lib/librte_ring/rte_ring_generic.h
>>> index ea7dbe5..1bd3dfd 100644
>>> --- a/lib/librte_ring/rte_ring_generic.h
>>> +++ b/lib/librte_ring/rte_ring_generic.h
>>> @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r,
>> unsigned int is_sp,
>>>  			return 0;
>>>
>>>  		*new_head = *old_head + n;
>>> -		if (is_sp)
>>> -			r->prod.head = *new_head, success = 1;
>>> -		else
>>> +		if (is_sp) {
>>> +			r->prod.head = *new_head;
>>> +			rte_smp_rmb();
>>> +			success = 1;
>>> +		} else
>>>  			success = rte_atomic32_cmpset(&r->prod.head,
>>>  					*old_head, *new_head);
>>>  	} while (unlikely(success == 0));
>>> @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring *r,
>> unsigned 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));
>>>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: Ilya Maximets <i.maximets@samsung.com>
> Sent: Thursday, March 7, 2019 5:48 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> operations
> 
> On 07.03.2019 12:27, Gavin Hu (Arm Technology China) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ilya Maximets <i.maximets@samsung.com>
> >> Sent: Thursday, March 7, 2019 4:52 PM
> >> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> >> dev@dpdk.org
> >> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> >> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> >> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> >> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> >> operations
> >>
> >> On 07.03.2019 9:45, gavin hu wrote:
> >>> In weak memory models, like arm64, reading the {prod,cons}.tail may get
> >>> reordered after reading or writing 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 cons.tail (in
> >>> SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
> >>> reordered before reading {prod,cons}.tail for SP (and SC) case.
> >>
> >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> >> reordered
> >> before reading the tail. However, to prevent *writing* the ring get
> >> reordered
> >> *before reading* the tail you need a full memory barrier, i.e.
> >> rte_smp_mb().
> >
> > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while WMB(ST
> Option) orders ST/ST.
> > For more details, please refer to: Table B2-1 Encoding of the DMB and DSB
> <option> parameter  in
> > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> reference-manual-armv8-for-armv8-a-architecture-profile
> 
> I see. But you have to change the rte_smp_rmb() function definition in
> lib/librte_eal/common/include/generic/rte_atomic.h and assure that all
> other architectures follows same rules.
> Otherwise, this change is logically wrong, because read barrier in current
> definition could not be used to order Load with Store.
> 

Good points, let me re-think how to handle for other architectures. 
Full MB is required for other architectures(x86? Ppc?), but for arm, read barrier(load/store and load/load) is enough. 

> >
> >>
> >>>
> >>> 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>
> >>> ---
> >>>  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
> >>>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/lib/librte_ring/rte_ring_generic.h
> >> b/lib/librte_ring/rte_ring_generic.h
> >>> index ea7dbe5..1bd3dfd 100644
> >>> --- a/lib/librte_ring/rte_ring_generic.h
> >>> +++ b/lib/librte_ring/rte_ring_generic.h
> >>> @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r,
> >> unsigned int is_sp,
> >>>  			return 0;
> >>>
> >>>  		*new_head = *old_head + n;
> >>> -		if (is_sp)
> >>> -			r->prod.head = *new_head, success = 1;
> >>> -		else
> >>> +		if (is_sp) {
> >>> +			r->prod.head = *new_head;
> >>> +			rte_smp_rmb();
> >>> +			success = 1;
> >>> +		} else
> >>>  			success = rte_atomic32_cmpset(&r->prod.head,
> >>>  					*old_head, *new_head);
> >>>  	} while (unlikely(success == 0));
> >>> @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring
> *r,
> >> unsigned 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));
> >>>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Ananyev, Konstantin]

Hi Gavin,

> > >>> In weak memory models, like arm64, reading the {prod,cons}.tail may get
> > >>> reordered after reading or writing 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 cons.tail (in
> > >>> SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
> > >>> reordered before reading {prod,cons}.tail for SP (and SC) case.
> > >>
> > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > >> reordered
> > >> before reading the tail. However, to prevent *writing* the ring get
> > >> reordered
> > >> *before reading* the tail you need a full memory barrier, i.e.
> > >> rte_smp_mb().
> > >
> > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while WMB(ST
> > Option) orders ST/ST.
> > > For more details, please refer to: Table B2-1 Encoding of the DMB and DSB
> > <option> parameter  in
> > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > reference-manual-armv8-for-armv8-a-architecture-profile
> >
> > I see. But you have to change the rte_smp_rmb() function definition in
> > lib/librte_eal/common/include/generic/rte_atomic.h and assure that all
> > other architectures follows same rules.
> > Otherwise, this change is logically wrong, because read barrier in current
> > definition could not be used to order Load with Store.
> >
> 
> Good points, let me re-think how to handle for other architectures.
> Full MB is required for other architectures(x86? Ppc?), but for arm, read barrier(load/store and load/load) is enough.

For x86, I don't think you need any barrier here, as with IA memory mode:
-  Reads are not reordered with other reads.
- Writes are not reordered with older reads.

BTW, could you explain a bit more why barrier is necessary even on arm here?
As I can see, there is a data dependency between the tail value and
subsequent address calculations for ring writes/reads.
Isn't that sufficient to prevent re-ordering even for weak memory model?
Konstantin
 

> 
> > >
> > >>
> > >>>
> > >>> 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>
> > >>> ---
> > >>>  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
> > >>>  1 file changed, 10 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/lib/librte_ring/rte_ring_generic.h
> > >> b/lib/librte_ring/rte_ring_generic.h
> > >>> index ea7dbe5..1bd3dfd 100644
> > >>> --- a/lib/librte_ring/rte_ring_generic.h
> > >>> +++ b/lib/librte_ring/rte_ring_generic.h
> > >>> @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring *r,
> > >> unsigned int is_sp,
> > >>>  			return 0;
> > >>>
> > >>>  		*new_head = *old_head + n;
> > >>> -		if (is_sp)
> > >>> -			r->prod.head = *new_head, success = 1;
> > >>> -		else
> > >>> +		if (is_sp) {
> > >>> +			r->prod.head = *new_head;
> > >>> +			rte_smp_rmb();
> > >>> +			success = 1;
> > >>> +		} else
> > >>>  			success = rte_atomic32_cmpset(&r->prod.head,
> > >>>  					*old_head, *new_head);
> > >>>  	} while (unlikely(success == 0));
> > >>> @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct rte_ring
> > *r,
> > >> unsigned 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));
> > >>>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Honnappa Nagarahalli]

> Hi Gavin,
> 
> > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail
> > > >>> may get reordered after reading or writing 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
> > > >>> cons.tail (in SP enqueue) or 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. This patch is to prevent reading and writing the ring
> > > >>> slots get reordered before reading {prod,cons}.tail for SP (and SC)
> case.
> > > >>
> > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > >> reordered before reading the tail. However, to prevent *writing*
> > > >> the ring get reordered *before reading* the tail you need a full
> > > >> memory barrier, i.e.
> > > >> rte_smp_mb().
> > > >
> > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > > > WMB(ST
> > > Option) orders ST/ST.
> > > > For more details, please refer to: Table B2-1 Encoding of the DMB
> > > > and DSB
> > > <option> parameter  in
> > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > reference-manual-armv8-for-armv8-a-architecture-profile
> > >
> > > I see. But you have to change the rte_smp_rmb() function definition
> > > in lib/librte_eal/common/include/generic/rte_atomic.h and assure
> > > that all other architectures follows same rules.
> > > Otherwise, this change is logically wrong, because read barrier in
> > > current definition could not be used to order Load with Store.
> > >
> >
> > Good points, let me re-think how to handle for other architectures.
> > Full MB is required for other architectures(x86? Ppc?), but for arm, read
> barrier(load/store and load/load) is enough.
> 
> For x86, I don't think you need any barrier here, as with IA memory mode:
> -  Reads are not reordered with other reads.
> - Writes are not reordered with older reads.
Agree

> 
> BTW, could you explain a bit more why barrier is necessary even on arm here?
> As I can see, there is a data dependency between the tail value and
> subsequent address calculations for ring writes/reads.
> Isn't that sufficient to prevent re-ordering even for weak memory model?
The tail value affects 'n'. But, the value of 'n' can be speculated because of the following 'if' statement:

if (unlikely(n > *free_entries))
                        n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 : *free_entries;

The address calculations for actual ring writes/reads do not depend on the tail value. Since 'n' can be speculated, the writes/reads can be moved up before the load of the tail value.

> Konstantin
> 
> 
<snip>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Gavin Hu (Arm Technology China)]

Hi Konstantin,

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Friday, March 8, 2019 11:21 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Gavin Hu (Arm
> Technology China) <Gavin.Hu@arm.com>; Ilya Maximets
> <i.maximets@samsung.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; olivier.matz@6wind.com;
> Richardson, Bruce <bruce.richardson@intel.com>;
> chaozhu@linux.vnet.ibm.com; nd <nd@arm.com>
> Subject: RE: [PATCH v2] ring: enforce reading the tails before ring operations
> 
> > Hi Gavin,
> >
> > > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail
> > > > >>> may get reordered after reading or writing 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
> > > > >>> cons.tail (in SP enqueue) or 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. This patch is to prevent reading and writing the ring
> > > > >>> slots get reordered before reading {prod,cons}.tail for SP (and SC)
> > case.
> > > > >>
> > > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > > >> reordered before reading the tail. However, to prevent *writing*
> > > > >> the ring get reordered *before reading* the tail you need a full
> > > > >> memory barrier, i.e.
> > > > >> rte_smp_mb().
> > > > >
> > > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > > > > WMB(ST
> > > > Option) orders ST/ST.
> > > > > For more details, please refer to: Table B2-1 Encoding of the DMB
> > > > > and DSB
> > > > <option> parameter  in
> > > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > > reference-manual-armv8-for-armv8-a-architecture-profile
> > > >
> > > > I see. But you have to change the rte_smp_rmb() function definition
> > > > in lib/librte_eal/common/include/generic/rte_atomic.h and assure
> > > > that all other architectures follows same rules.
> > > > Otherwise, this change is logically wrong, because read barrier in
> > > > current definition could not be used to order Load with Store.
> > > >
> > >
> > > Good points, let me re-think how to handle for other architectures.
> > > Full MB is required for other architectures(x86? Ppc?), but for arm, read
> > barrier(load/store and load/load) is enough.
> >
> > For x86, I don't think you need any barrier here, as with IA memory mode:
> > -  Reads are not reordered with other reads.
> > - Writes are not reordered with older reads.
> Agree

I understand herein no instruction level barriers are required for IA, but how about the
compiler barrier: rte_compiler_barrier? 

> 
> >
> > BTW, could you explain a bit more why barrier is necessary even on arm
> here?
> > As I can see, there is a data dependency between the tail value and
> > subsequent address calculations for ring writes/reads.
> > Isn't that sufficient to prevent re-ordering even for weak memory model?
> The tail value affects 'n'. But, the value of 'n' can be speculated because of
> the following 'if' statement:
> 
> if (unlikely(n > *free_entries))
>                         n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 : *free_entries;
> 
> The address calculations for actual ring writes/reads do not depend on the
> tail value. Since 'n' can be speculated, the writes/reads can be moved up
> before the load of the tail value.

Good explanation. The address calculations does not depend on tail/n, only the
limit/last one depends on it, while it can be speculated. 

> > Konstantin
> >
> >
> <snip>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Ananyev, Konstantin]

> > > > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail
> > > > > >>> may get reordered after reading or writing 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
> > > > > >>> cons.tail (in SP enqueue) or 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. This patch is to prevent reading and writing the ring
> > > > > >>> slots get reordered before reading {prod,cons}.tail for SP (and SC)
> > > case.
> > > > > >>
> > > > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > > > >> reordered before reading the tail. However, to prevent *writing*
> > > > > >> the ring get reordered *before reading* the tail you need a full
> > > > > >> memory barrier, i.e.
> > > > > >> rte_smp_mb().
> > > > > >
> > > > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > > > > > WMB(ST
> > > > > Option) orders ST/ST.
> > > > > > For more details, please refer to: Table B2-1 Encoding of the DMB
> > > > > > and DSB
> > > > > <option> parameter  in
> > > > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > > > reference-manual-armv8-for-armv8-a-architecture-profile
> > > > >
> > > > > I see. But you have to change the rte_smp_rmb() function definition
> > > > > in lib/librte_eal/common/include/generic/rte_atomic.h and assure
> > > > > that all other architectures follows same rules.
> > > > > Otherwise, this change is logically wrong, because read barrier in
> > > > > current definition could not be used to order Load with Store.
> > > > >
> > > >
> > > > Good points, let me re-think how to handle for other architectures.
> > > > Full MB is required for other architectures(x86? Ppc?), but for arm, read
> > > barrier(load/store and load/load) is enough.
> > >
> > > For x86, I don't think you need any barrier here, as with IA memory mode:
> > > -  Reads are not reordered with other reads.
> > > - Writes are not reordered with older reads.
> > Agree
> 
> I understand herein no instruction level barriers are required for IA, but how about the
> compiler barrier: rte_compiler_barrier?
> 
> >
> > >
> > > BTW, could you explain a bit more why barrier is necessary even on arm
> > here?
> > > As I can see, there is a data dependency between the tail value and
> > > subsequent address calculations for ring writes/reads.
> > > Isn't that sufficient to prevent re-ordering even for weak memory model?
> > The tail value affects 'n'. But, the value of 'n' can be speculated because of
> > the following 'if' statement:
> >
> > if (unlikely(n > *free_entries))
> >                         n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 : *free_entries;
> >
> > The address calculations for actual ring writes/reads do not depend on the
> > tail value. 

Ok, agree I formulated it wrongly, only limit value is dependent on cons.tail. 
Address is not. 

>Since 'n' can be speculated, the writes/reads can be moved up
> > before the load of the tail value.

For my curiosity: ok, I understand that 'n' value can be speculated,
and speculative stores could start before n is calculated properly...
But are you saying that such speculative store results might be visible to the 
other observers (different cpu)? 

> 
> Good explanation. The address calculations does not depend on tail/n, only the
> limit/last one depends on it, while it can be speculated.
> 
> > > Konstantin
> > >
> > >
> > <snip>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Honnappa Nagarahalli]

> > > > > > >>> In weak memory models, like arm64, reading the
> > > > > > >>> {prod,cons}.tail may get reordered after reading or
> > > > > > >>> writing 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 cons.tail (in SP enqueue) or 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. This patch is to prevent reading and writing the
> > > > > > >>> ring slots get reordered before reading {prod,cons}.tail
> > > > > > >>> for SP (and SC)
> > > > case.
> > > > > > >>
> > > > > > >> Read barrier rte_smp_rmb() is OK to prevent reading the
> > > > > > >> ring get reordered before reading the tail. However, to
> > > > > > >> prevent *writing* the ring get reordered *before reading*
> > > > > > >> the tail you need a full memory barrier, i.e.
> > > > > > >> rte_smp_mb().
> > > > > > >
> > > > > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST,
> > > > > > > while WMB(ST
> > > > > > Option) orders ST/ST.
> > > > > > > For more details, please refer to: Table B2-1 Encoding of
> > > > > > > the DMB and DSB
> > > > > > <option> parameter  in
> > > > > > > https://developer.arm.com/docs/ddi0487/latest/arm-architectu
> > > > > > > re-
> > > > > > reference-manual-armv8-for-armv8-a-architecture-profile
> > > > > >
> > > > > > I see. But you have to change the rte_smp_rmb() function
> > > > > > definition in
> > > > > > lib/librte_eal/common/include/generic/rte_atomic.h and assure that
> all other architectures follows same rules.
> > > > > > Otherwise, this change is logically wrong, because read
> > > > > > barrier in current definition could not be used to order Load with
> Store.
> > > > > >
> > > > >
> > > > > Good points, let me re-think how to handle for other architectures.
> > > > > Full MB is required for other architectures(x86? Ppc?), but for
> > > > > arm, read
> > > > barrier(load/store and load/load) is enough.
> > > >
> > > > For x86, I don't think you need any barrier here, as with IA memory
> mode:
> > > > -  Reads are not reordered with other reads.
> > > > - Writes are not reordered with older reads.
> > > Agree
> >
> > I understand herein no instruction level barriers are required for IA,
> > but how about the compiler barrier: rte_compiler_barrier?
> >
> > >
> > > >
> > > > BTW, could you explain a bit more why barrier is necessary even on
> > > > arm
> > > here?
> > > > As I can see, there is a data dependency between the tail value
> > > > and subsequent address calculations for ring writes/reads.
> > > > Isn't that sufficient to prevent re-ordering even for weak memory model?
> > > The tail value affects 'n'. But, the value of 'n' can be speculated
> > > because of the following 'if' statement:
> > >
> > > if (unlikely(n > *free_entries))
> > >                         n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 :
> > > *free_entries;
> > >
> > > The address calculations for actual ring writes/reads do not depend
> > > on the tail value.
> 
> Ok, agree I formulated it wrongly, only limit value is dependent on cons.tail.
> Address is not.
> 
> >Since 'n' can be speculated, the writes/reads can be moved up
> > > before the load of the tail value.
> 
> For my curiosity: ok, I understand that 'n' value can be speculated, and
> speculative stores could start before n is calculated properly...
> But are you saying that such speculative store results might be visible to the
> other observers (different cpu)?
> 
You are correct. The speculative stores will NOT be visible to other observers till the value of 'n' is fixed. Speculative stores might have to be discarded depending on the value of 'n' (which will affect cache performance).
There is also a control dependency between the load of cons.tail and the stores to the ring. That should also keep the load and stores from getting reordered (though I am not sure if it still allows for speculative stores).
So, IMO, the barrier in enqueue is not needed. Is this what you wanted to drive at?

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Ananyev, Konstantin]

> > > > > > > >>> In weak memory models, like arm64, reading the
> > > > > > > >>> {prod,cons}.tail may get reordered after reading or
> > > > > > > >>> writing 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 cons.tail (in SP enqueue) or 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. This patch is to prevent reading and writing the
> > > > > > > >>> ring slots get reordered before reading {prod,cons}.tail
> > > > > > > >>> for SP (and SC)
> > > > > case.
> > > > > > > >>
> > > > > > > >> Read barrier rte_smp_rmb() is OK to prevent reading the
> > > > > > > >> ring get reordered before reading the tail. However, to
> > > > > > > >> prevent *writing* the ring get reordered *before reading*
> > > > > > > >> the tail you need a full memory barrier, i.e.
> > > > > > > >> rte_smp_mb().
> > > > > > > >
> > > > > > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST,
> > > > > > > > while WMB(ST
> > > > > > > Option) orders ST/ST.
> > > > > > > > For more details, please refer to: Table B2-1 Encoding of
> > > > > > > > the DMB and DSB
> > > > > > > <option> parameter  in
> > > > > > > > https://developer.arm.com/docs/ddi0487/latest/arm-architectu
> > > > > > > > re-
> > > > > > > reference-manual-armv8-for-armv8-a-architecture-profile
> > > > > > >
> > > > > > > I see. But you have to change the rte_smp_rmb() function
> > > > > > > definition in
> > > > > > > lib/librte_eal/common/include/generic/rte_atomic.h and assure that
> > all other architectures follows same rules.
> > > > > > > Otherwise, this change is logically wrong, because read
> > > > > > > barrier in current definition could not be used to order Load with
> > Store.
> > > > > > >
> > > > > >
> > > > > > Good points, let me re-think how to handle for other architectures.
> > > > > > Full MB is required for other architectures(x86? Ppc?), but for
> > > > > > arm, read
> > > > > barrier(load/store and load/load) is enough.
> > > > >
> > > > > For x86, I don't think you need any barrier here, as with IA memory
> > mode:
> > > > > -  Reads are not reordered with other reads.
> > > > > - Writes are not reordered with older reads.
> > > > Agree
> > >
> > > I understand herein no instruction level barriers are required for IA,
> > > but how about the compiler barrier: rte_compiler_barrier?
> > >
> > > >
> > > > >
> > > > > BTW, could you explain a bit more why barrier is necessary even on
> > > > > arm
> > > > here?
> > > > > As I can see, there is a data dependency between the tail value
> > > > > and subsequent address calculations for ring writes/reads.
> > > > > Isn't that sufficient to prevent re-ordering even for weak memory model?
> > > > The tail value affects 'n'. But, the value of 'n' can be speculated
> > > > because of the following 'if' statement:
> > > >
> > > > if (unlikely(n > *free_entries))
> > > >                         n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 :
> > > > *free_entries;
> > > >
> > > > The address calculations for actual ring writes/reads do not depend
> > > > on the tail value.
> >
> > Ok, agree I formulated it wrongly, only limit value is dependent on cons.tail.
> > Address is not.
> >
> > >Since 'n' can be speculated, the writes/reads can be moved up
> > > > before the load of the tail value.
> >
> > For my curiosity: ok, I understand that 'n' value can be speculated, and
> > speculative stores could start before n is calculated properly...
> > But are you saying that such speculative store results might be visible to the
> > other observers (different cpu)?
> >
> You are correct. The speculative stores will NOT be visible to other observers till the value of 'n' is fixed. Speculative stores might have to be
> discarded depending on the value of 'n' (which will affect cache performance).
> There is also a control dependency between the load of cons.tail and the stores to the ring. That should also keep the load and stores from
> getting reordered (though I am not sure if it still allows for speculative stores).
> So, IMO, the barrier in enqueue is not needed. Is this what you wanted to drive at?

Yes, that was my thought.
Thanks
Konstantin

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: Gavin Hu (Arm Technology China)
> Sent: Thursday, March 7, 2019 6:45 PM
> To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com;
> bruce.richardson@intel.com; konstantin.ananyev@intel.com;
> chaozhu@linux.vnet.ibm.com
> Subject: RE: [PATCH v2] ring: enforce reading the tails before ring operations
> 
> 
> 
> > -----Original Message-----
> > From: Ilya Maximets <i.maximets@samsung.com>
> > Sent: Thursday, March 7, 2019 5:48 PM
> > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> dev@dpdk.org
> > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > operations
> >
> > On 07.03.2019 12:27, Gavin Hu (Arm Technology China) wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Ilya Maximets <i.maximets@samsung.com>
> > >> Sent: Thursday, March 7, 2019 4:52 PM
> > >> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> > >> dev@dpdk.org
> > >> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > >> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> Nagarahalli
> > >> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > >> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > >> operations
> > >>
> > >> On 07.03.2019 9:45, gavin hu wrote:
> > >>> In weak memory models, like arm64, reading the {prod,cons}.tail may
> get
> > >>> reordered after reading or writing 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 cons.tail
> (in
> > >>> SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
> > >>> reordered before reading {prod,cons}.tail for SP (and SC) case.
> > >>
> > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > >> reordered
> > >> before reading the tail. However, to prevent *writing* the ring get
> > >> reordered
> > >> *before reading* the tail you need a full memory barrier, i.e.
> > >> rte_smp_mb().
> > >
> > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> WMB(ST
> > Option) orders ST/ST.
> > > For more details, please refer to: Table B2-1 Encoding of the DMB and
> DSB
> > <option> parameter  in
> > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > reference-manual-armv8-for-armv8-a-architecture-profile
> >
> > I see. But you have to change the rte_smp_rmb() function definition in
> > lib/librte_eal/common/include/generic/rte_atomic.h and assure that all
> > other architectures follows same rules.
> > Otherwise, this change is logically wrong, because read barrier in current
> > definition could not be used to order Load with Store.
> >
> 
> Good points, let me re-think how to handle for other architectures.
> Full MB is required for other architectures(x86? Ppc?), but for arm, read
> barrier(load/store and load/load) is enough.

Hi Ilya,

I would expand the rmb definition to cover load/store, in addition to load/load.
For X86, as a strong memory order model, rmb is actually equivalent to mb, as implemented as a compiler barrier: rte_compiler_barrier, arm32 is also this case.
For PPC, both 32 and 64-bit, rmb=wmb=mb, lwsync/sync orders load/store, load/load, store/load, store/store, looking at the table on this page:
https://www.ibm.com/developerworks/systems/articles/powerpc.html 

In summary, we are safe to expand this definition for all the architectures DPDK support? 
Any comments are welcome!

BR. Gavin

> > >
> > >>
> > >>>
> > >>> 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>
> > >>> ---
> > >>>  lib/librte_ring/rte_ring_generic.h | 16 ++++++++++------
> > >>>  1 file changed, 10 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/lib/librte_ring/rte_ring_generic.h
> > >> b/lib/librte_ring/rte_ring_generic.h
> > >>> index ea7dbe5..1bd3dfd 100644
> > >>> --- a/lib/librte_ring/rte_ring_generic.h
> > >>> +++ b/lib/librte_ring/rte_ring_generic.h
> > >>> @@ -90,9 +90,11 @@ __rte_ring_move_prod_head(struct rte_ring
> *r,
> > >> unsigned int is_sp,
> > >>>  			return 0;
> > >>>
> > >>>  		*new_head = *old_head + n;
> > >>> -		if (is_sp)
> > >>> -			r->prod.head = *new_head, success = 1;
> > >>> -		else
> > >>> +		if (is_sp) {
> > >>> +			r->prod.head = *new_head;
> > >>> +			rte_smp_rmb();
> > >>> +			success = 1;
> > >>> +		} else
> > >>>  			success = rte_atomic32_cmpset(&r->prod.head,
> > >>>  					*old_head, *new_head);
> > >>>  	} while (unlikely(success == 0));
> > >>> @@ -158,9 +160,11 @@ __rte_ring_move_cons_head(struct
> rte_ring
> > *r,
> > >> unsigned 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));
> > >>>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Honnappa Nagarahalli]

> > > >> On 07.03.2019 9:45, gavin hu wrote:
> > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail
> > > >>> may
> > get
> > > >>> reordered after reading or writing 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
> > > >>> cons.tail
> > (in
> > > >>> SP enqueue) or 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. This patch is to prevent reading and writing the ring
> > > >>> slots get reordered before reading {prod,cons}.tail for SP (and SC)
> case.
> > > >>
> > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > >> reordered before reading the tail. However, to prevent *writing*
> > > >> the ring get reordered *before reading* the tail you need a full
> > > >> memory barrier, i.e.
> > > >> rte_smp_mb().
> > > >
> > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > WMB(ST
> > > Option) orders ST/ST.
> > > > For more details, please refer to: Table B2-1 Encoding of the DMB
> > > > and
> > DSB
> > > <option> parameter  in
> > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > reference-manual-armv8-for-armv8-a-architecture-profile
> > >
> > > I see. But you have to change the rte_smp_rmb() function definition
> > > in lib/librte_eal/common/include/generic/rte_atomic.h and assure
> > > that all other architectures follows same rules.
> > > Otherwise, this change is logically wrong, because read barrier in
> > > current definition could not be used to order Load with Store.
> > >
> >
> > Good points, let me re-think how to handle for other architectures.
> > Full MB is required for other architectures(x86? Ppc?), but for arm,
> > read barrier(load/store and load/load) is enough.
> 
> Hi Ilya,
> 
> I would expand the rmb definition to cover load/store, in addition to
> load/load.
> For X86, as a strong memory order model, rmb is actually equivalent to mb,
> as implemented as a compiler barrier: rte_compiler_barrier, arm32 is also
> this case.
> For PPC, both 32 and 64-bit, rmb=wmb=mb, lwsync/sync orders load/store,
> load/load, store/load, store/store, looking at the table on this page:
> https://www.ibm.com/developerworks/systems/articles/powerpc.html
> 
> In summary, we are safe to expand this definition for all the architectures
> DPDK support?
Essentially, it is a documentation bug. i.e. the current implementation of rte_smp_rmb() already behaves as load/load and load/store barrier.

> Any comments are welcome!
> 
> BR. Gavin
> 

<snip>

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Ananyev, Konstantin]

> -----Original Message-----
> From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> Sent: Friday, March 8, 2019 4:23 AM
> To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com; hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; chaozhu@linux.vnet.ibm.com
> Subject: RE: [PATCH v2] ring: enforce reading the tails before ring operations
> 
> 
> 
> > -----Original Message-----
> > From: Gavin Hu (Arm Technology China)
> > Sent: Thursday, March 7, 2019 6:45 PM
> > To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com;
> > bruce.richardson@intel.com; konstantin.ananyev@intel.com;
> > chaozhu@linux.vnet.ibm.com
> > Subject: RE: [PATCH v2] ring: enforce reading the tails before ring operations
> >
> >
> >
> > > -----Original Message-----
> > > From: Ilya Maximets <i.maximets@samsung.com>
> > > Sent: Thursday, March 7, 2019 5:48 PM
> > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> > dev@dpdk.org
> > > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > > hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > > Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > > operations
> > >
> > > On 07.03.2019 12:27, Gavin Hu (Arm Technology China) wrote:
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Ilya Maximets <i.maximets@samsung.com>
> > > >> Sent: Thursday, March 7, 2019 4:52 PM
> > > >> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> > > >> dev@dpdk.org
> > > >> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > > >> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> > Nagarahalli
> > > >> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > > >> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > > >> operations
> > > >>
> > > >> On 07.03.2019 9:45, gavin hu wrote:
> > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail may
> > get
> > > >>> reordered after reading or writing 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 cons.tail
> > (in
> > > >>> SP enqueue) or 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. This patch is to prevent reading and writing the ring slots get
> > > >>> reordered before reading {prod,cons}.tail for SP (and SC) case.
> > > >>
> > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > >> reordered
> > > >> before reading the tail. However, to prevent *writing* the ring get
> > > >> reordered
> > > >> *before reading* the tail you need a full memory barrier, i.e.
> > > >> rte_smp_mb().
> > > >
> > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > WMB(ST
> > > Option) orders ST/ST.
> > > > For more details, please refer to: Table B2-1 Encoding of the DMB and
> > DSB
> > > <option> parameter  in
> > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > reference-manual-armv8-for-armv8-a-architecture-profile
> > >
> > > I see. But you have to change the rte_smp_rmb() function definition in
> > > lib/librte_eal/common/include/generic/rte_atomic.h and assure that all
> > > other architectures follows same rules.
> > > Otherwise, this change is logically wrong, because read barrier in current
> > > definition could not be used to order Load with Store.
> > >
> >
> > Good points, let me re-think how to handle for other architectures.
> > Full MB is required for other architectures(x86? Ppc?), but for arm, read
> > barrier(load/store and load/load) is enough.
> 
> Hi Ilya,
> 
> I would expand the rmb definition to cover load/store, in addition to load/load.
> For X86, as a strong memory order model, rmb is actually equivalent to mb,

That's not exactly the case, on x86 we have:
smp_rmb == compiler_barrier
smp_mb is a proper memory barrier.

Konstantin

> as implemented as a compiler barrier: rte_compiler_barrier,
> arm32 is also this case.
> For PPC, both 32 and 64-bit, rmb=wmb=mb, lwsync/sync orders load/store, load/load, store/load, store/store, looking at the table on this
> page:
> https://www.ibm.com/developerworks/systems/articles/powerpc.html
> 
> In summary, we are safe to expand this definition for all the architectures DPDK support?
> Any comments are welcome!
> 
> BR. Gavin
> 

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Gavin Hu (Arm Technology China)]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Friday, March 8, 2019 8:13 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Ilya
> Maximets <i.maximets@samsung.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com; Richardson,
> Bruce <bruce.richardson@intel.com>; chaozhu@linux.vnet.ibm.com
> Subject: RE: [PATCH v2] ring: enforce reading the tails before ring
> operations
> 
> 
> 
> > -----Original Message-----
> > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > Sent: Friday, March 8, 2019 4:23 AM
> > To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> > Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> olivier.matz@6wind.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>;
> chaozhu@linux.vnet.ibm.com
> > Subject: RE: [PATCH v2] ring: enforce reading the tails before ring
> operations
> >
> >
> >
> > > -----Original Message-----
> > > From: Gavin Hu (Arm Technology China)
> > > Sent: Thursday, March 7, 2019 6:45 PM
> > > To: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org
> > > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > > hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com;
> > > bruce.richardson@intel.com; konstantin.ananyev@intel.com;
> > > chaozhu@linux.vnet.ibm.com
> > > Subject: RE: [PATCH v2] ring: enforce reading the tails before ring
> operations
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ilya Maximets <i.maximets@samsung.com>
> > > > Sent: Thursday, March 7, 2019 5:48 PM
> > > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> > > dev@dpdk.org
> > > > Cc: nd <nd@arm.com>; thomas@monjalon.net; jerinj@marvell.com;
> > > > hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > > > Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > > > operations
> > > >
> > > > On 07.03.2019 12:27, Gavin Hu (Arm Technology China) wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Ilya Maximets <i.maximets@samsung.com>
> > > > >> Sent: Thursday, March 7, 2019 4:52 PM
> > > > >> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> > > > >> dev@dpdk.org
> > > > >> Cc: nd <nd@arm.com>; thomas@monjalon.net;
> jerinj@marvell.com;
> > > > >> hemant.agrawal@nxp.com; Nipun.gupta@nxp.com; Honnappa
> > > Nagarahalli
> > > > >> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com
> > > > >> Subject: Re: [PATCH v2] ring: enforce reading the tails before ring
> > > > >> operations
> > > > >>
> > > > >> On 07.03.2019 9:45, gavin hu wrote:
> > > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail
> may
> > > get
> > > > >>> reordered after reading or writing 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
> cons.tail
> > > (in
> > > > >>> SP enqueue) or 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. This patch is to prevent reading and writing the ring
> slots get
> > > > >>> reordered before reading {prod,cons}.tail for SP (and SC) case.
> > > > >>
> > > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > > >> reordered
> > > > >> before reading the tail. However, to prevent *writing* the ring get
> > > > >> reordered
> > > > >> *before reading* the tail you need a full memory barrier, i.e.
> > > > >> rte_smp_mb().
> > > > >
> > > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > > WMB(ST
> > > > Option) orders ST/ST.
> > > > > For more details, please refer to: Table B2-1 Encoding of the DMB
> and
> > > DSB
> > > > <option> parameter  in
> > > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > > reference-manual-armv8-for-armv8-a-architecture-profile
> > > >
> > > > I see. But you have to change the rte_smp_rmb() function definition
> in
> > > > lib/librte_eal/common/include/generic/rte_atomic.h and assure that
> all
> > > > other architectures follows same rules.
> > > > Otherwise, this change is logically wrong, because read barrier in
> current
> > > > definition could not be used to order Load with Store.
> > > >
> > >
> > > Good points, let me re-think how to handle for other architectures.
> > > Full MB is required for other architectures(x86? Ppc?), but for arm,
> read
> > > barrier(load/store and load/load) is enough.
> >
> > Hi Ilya,
> >
> > I would expand the rmb definition to cover load/store, in addition to
> load/load.
> > For X86, as a strong memory order model, rmb is actually equivalent to
> mb,
> 
> That's not exactly the case, on x86 we have:
> smp_rmb == compiler_barrier
> smp_mb is a proper memory barrier.
> 
> Konstantin

Sorry I did not make it clear.
Anyway, on x86, smp_rmb, as a compiler barrier, applies to load/store, not only load/load.
This is the case also for arm, arm64, ppc32, ppc64.
I will submit a patch to expand the definition of this API. 

> 
> > as implemented as a compiler barrier: rte_compiler_barrier,
> > arm32 is also this case.
> > For PPC, both 32 and 64-bit, rmb=wmb=mb, lwsync/sync orders
> load/store, load/load, store/load, store/store, looking at the table on this
> > page:
> > https://www.ibm.com/developerworks/systems/articles/powerpc.html
> >
> > In summary, we are safe to expand this definition for all the
> architectures DPDK support?
> > Any comments are welcome!
> >
> > BR. Gavin
> >

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Ananyev, Konstantin]

> > > > > >>> In weak memory models, like arm64, reading the {prod,cons}.tail
> > may
> > > > get
> > > > > >>> reordered after reading or writing 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
> > cons.tail
> > > > (in
> > > > > >>> SP enqueue) or 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. This patch is to prevent reading and writing the ring
> > slots get
> > > > > >>> reordered before reading {prod,cons}.tail for SP (and SC) case.
> > > > > >>
> > > > > >> Read barrier rte_smp_rmb() is OK to prevent reading the ring get
> > > > > >> reordered
> > > > > >> before reading the tail. However, to prevent *writing* the ring get
> > > > > >> reordered
> > > > > >> *before reading* the tail you need a full memory barrier, i.e.
> > > > > >> rte_smp_mb().
> > > > > >
> > > > > > ISHLD(rte_smp_rmb is DMB(ishld) orders LD/LD and LD/ST, while
> > > > WMB(ST
> > > > > Option) orders ST/ST.
> > > > > > For more details, please refer to: Table B2-1 Encoding of the DMB
> > and
> > > > DSB
> > > > > <option> parameter  in
> > > > > > https://developer.arm.com/docs/ddi0487/latest/arm-architecture-
> > > > > reference-manual-armv8-for-armv8-a-architecture-profile
> > > > >
> > > > > I see. But you have to change the rte_smp_rmb() function definition
> > in
> > > > > lib/librte_eal/common/include/generic/rte_atomic.h and assure that
> > all
> > > > > other architectures follows same rules.
> > > > > Otherwise, this change is logically wrong, because read barrier in
> > current
> > > > > definition could not be used to order Load with Store.
> > > > >
> > > >
> > > > Good points, let me re-think how to handle for other architectures.
> > > > Full MB is required for other architectures(x86? Ppc?), but for arm,
> > read
> > > > barrier(load/store and load/load) is enough.
> > >
> > > Hi Ilya,
> > >
> > > I would expand the rmb definition to cover load/store, in addition to
> > load/load.
> > > For X86, as a strong memory order model, rmb is actually equivalent to
> > mb,
> >
> > That's not exactly the case, on x86 we have:
> > smp_rmb == compiler_barrier
> > smp_mb is a proper memory barrier.
> >
> > Konstantin
> 
> Sorry I did not make it clear.
> Anyway, on x86, smp_rmb, as a compiler barrier, applies to load/store, not only load/load.

Yes, that's true, but I think that's happened by coincidence, 
not intentionally.

> This is the case also for arm, arm64, ppc32, ppc64.
> I will submit a patch to expand the definition of this API.

I understand your intention, but that does mean we would also need
to change not only rte_smp_rmb() but rte_rmb() too (to keep things consistent)?
That sounds worring.  
Might be better to keep smp_rmb() definition as it is, and introduce new function
that fits your purposes (smp_rwmb or smp_load_store_barrier)?

Konstantin

> 
> >
> > > as implemented as a compiler barrier: rte_compiler_barrier,
> > > arm32 is also this case.
> > > For PPC, both 32 and 64-bit, rmb=wmb=mb, lwsync/sync orders
> > load/store, load/load, store/load, store/store, looking at the table on this
> > > page:
> > > https://www.ibm.com/developerworks/systems/articles/powerpc.html
> > >
> > > In summary, we are safe to expand this definition for all the
> > architectures DPDK support?
> > > Any comments are welcome!
> > >
> > > BR. Gavin
> > >

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Thomas Monjalon]

08/03/2019 16:50, Ananyev, Konstantin:
> 08/03/2019 16:05, Gavin Hu (Arm Technology China):
> > Anyway, on x86, smp_rmb, as a compiler barrier, applies to load/store, not only load/load.
> 
> Yes, that's true, but I think that's happened by coincidence, 
> not intentionally.
> 
> > This is the case also for arm, arm64, ppc32, ppc64.
> > I will submit a patch to expand the definition of this API.
> 
> I understand your intention, but that does mean we would also need
> to change not only rte_smp_rmb() but rte_rmb() too (to keep things consistent)?
> That sounds worring.  
> Might be better to keep smp_rmb() definition as it is, and introduce new function
> that fits your purposes (smp_rwmb or smp_load_store_barrier)?

How is it managed in other projects?

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Honnappa Nagarahalli]

> 08/03/2019 16:50, Ananyev, Konstantin:
> > 08/03/2019 16:05, Gavin Hu (Arm Technology China):
> > > Anyway, on x86, smp_rmb, as a compiler barrier, applies to load/store, not
> only load/load.
> >
> > Yes, that's true, but I think that's happened by coincidence, not
> > intentionally.
> >
> > > This is the case also for arm, arm64, ppc32, ppc64.
> > > I will submit a patch to expand the definition of this API.
> >
> > I understand your intention, but that does mean we would also need to
> > change not only rte_smp_rmb() but rte_rmb() too (to keep things consistent)?
> > That sounds worring.
> > Might be better to keep smp_rmb() definition as it is, and introduce
> > new function that fits your purposes (smp_rwmb or smp_load_store_barrier)?
Looking at rte_rmb, rte_io_rmb, rte_cio_rmb implementations for Arm, they all provide load/store barrier as well. If other architectures also provide load/store barrier with rte_xxx_rmb, then we could extend the meaning of the existing APIs.

Even if a new API is provided, we need to do provide the same APIs for IO and CIO variants.

> 
> How is it managed in other projects?
In my experience, I usually have been changing the algorithms to use C11 memory model. So, I have not come across this issue yet. Others can comment.

> 

Re: [PATCH v2] ring: enforce reading the tails before ring operations

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Saturday, March 9, 2019 7:48 AM
> To: thomas@monjalon.net; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>
> Cc: Ilya Maximets <i.maximets@samsung.com>; dev@dpdk.org; nd
> <nd@arm.com>; jerinj@marvell.com; hemant.agrawal@nxp.com;
> Nipun.gupta@nxp.com; olivier.matz@6wind.com; Richardson, Bruce
> <bruce.richardson@intel.com>; chaozhu@linux.vnet.ibm.com; nd
> <nd@arm.com>
> Subject: RE: [PATCH v2] ring: enforce reading the tails before ring
> operations
> 
> > 08/03/2019 16:50, Ananyev, Konstantin:
> > > 08/03/2019 16:05, Gavin Hu (Arm Technology China):
> > > > Anyway, on x86, smp_rmb, as a compiler barrier, applies to
> load/store, not
> > only load/load.
> > >
> > > Yes, that's true, but I think that's happened by coincidence, not
> > > intentionally.
> > >
> > > > This is the case also for arm, arm64, ppc32, ppc64.
> > > > I will submit a patch to expand the definition of this API.
> > >
> > > I understand your intention, but that does mean we would also need
> to
> > > change not only rte_smp_rmb() but rte_rmb() too (to keep things
> consistent)?
> > > That sounds worring.
> > > Might be better to keep smp_rmb() definition as it is, and introduce
> > > new function that fits your purposes (smp_rwmb or
> smp_load_store_barrier)?
> Looking at rte_rmb, rte_io_rmb, rte_cio_rmb implementations for Arm,
> they all provide load/store barrier as well. If other architectures also
> provide load/store barrier with rte_xxx_rmb, then we could extend the
> meaning of the existing APIs.
Further looking at rte_rmb, rte_io_rmb, rte_cio_rmb implementations for PPC64 and x86,
They also provide load/store barrier. It is safe to extend the meaning of the existing rte_XXX_rmb API.
> 
> Even if a new API is provided, we need to do provide the same APIs for IO
> and CIO variants.
Since rte_XXX_rmbs API for all architectures already provide the desired load/store ordering,
a new API is redundant and not needed. 

> >
> > How is it managed in other projects?
> In my experience, I usually have been changing the algorithms to use C11
> memory model. So, I have not come across this issue yet. Others can
> comment.
> 
> >

[PATCH v3 0/1] ring: enforce reading the tail before reading ring slots

[Gavin Hu]

Change log:
  -V1: First version of the patch.

  -V2: Update the commit message to describe the ordering of detailed memory
accesses.

  -V3: As suggested by Konstantin Ananyev, remove the rte_smp_rmb for enqueue
case, as there is a control dependency in place, making rmb unnecessary. For
dequeue case, it is still required as the control dependency does not
order loads with loads.

gavin hu (1):
  ring: enforce reading the tail before reading ring slots

 lib/librte_ring/rte_ring_generic.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH v3 1/1] ring: enforce reading the tail before reading ring slots

[Gavin Hu]

From: gavin hu <gavin.hu@arm.com>

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: c9fb3c62896f ("ring: move code in a new header file")
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>
---
 lib/librte_ring/rte_ring_generic.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
index ea7dbe5..953cdbb 100644
--- a/lib/librte_ring/rte_ring_generic.h
+++ b/lib/librte_ring/rte_ring_generic.h
@@ -158,11 +158,14 @@ __rte_ring_move_cons_head(struct rte_ring *r, unsigned 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

Re: [PATCH v3 1/1] ring: enforce reading the tail before reading ring slots

[Nipun Gupta]

> -----Original Message-----
> From: Gavin Hu [mailto:gavin.hu@arm.com]
> Sent: Tuesday, March 12, 2019 10:29 PM
> To: dev@dpdk.org
> Cc: nd@arm.com; gavin hu <gavin.hu@arm.com>; thomas@monjalon.net;
> konstantin.ananyev@intel.com; jerinj@marvell.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Nipun Gupta <nipun.gupta@nxp.com>;
> Honnappa.Nagarahalli@arm.com; i.maximets@samsung.com;
> chaozhu@linux.vnet.ibm.com; stable@dpdk.org
> Subject: [PATCH v3 1/1] ring: enforce reading the tail before reading ring slots
> 
> From: gavin hu <gavin.hu@arm.com>
> 
> 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://emea01.safelinks.protection.outlook.com/?url=https:%2F%2Fwww.c
> l.cam.ac.uk%2F~pes20%2Fppc-
> supplemental%2Ftest7.pdf&amp;data=02%7C01%7Cnipun.gupta%40nxp.co
> m%7Cc3df030ec49449bbaf0508d6a70c0b9f%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C636880067514602937&amp;sdata=ogPdd%2F4UGYWE8
> nAwY1lkXPCB9MpUIFY0VOQr2N1lwe4%3D&amp;reserved=0
> 
> This patch is adding the required read barrier to prevent reading the ring
> slots get reordered before reading prod.tail for SC case.
> 
> Fixes: c9fb3c62896f ("ring: move code in a new header file")
> 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>

Also, I have revalidated this updated patch.

Re: [PATCH v3 1/1] ring: enforce reading the tail before reading ring slots

[Ananyev, Konstantin]

> -----Original Message-----
> From: Gavin Hu [mailto:gavin.hu@arm.com]
> Sent: Tuesday, March 12, 2019 4:59 PM
> To: dev@dpdk.org
> Cc: nd@arm.com; gavin hu <gavin.hu@arm.com>; thomas@monjalon.net; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> jerinj@marvell.com; hemant.agrawal@nxp.com; nipun.gupta@nxp.com; Honnappa.Nagarahalli@arm.com; i.maximets@samsung.com;
> chaozhu@linux.vnet.ibm.com; stable@dpdk.org
> Subject: [PATCH v3 1/1] ring: enforce reading the tail before reading ring slots
> 
> From: gavin hu <gavin.hu@arm.com>
> 
> 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: c9fb3c62896f ("ring: move code in a new header file")
> 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>
> ---
>  lib/librte_ring/rte_ring_generic.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
> index ea7dbe5..953cdbb 100644
> --- a/lib/librte_ring/rte_ring_generic.h
> +++ b/lib/librte_ring/rte_ring_generic.h
> @@ -158,11 +158,14 @@ __rte_ring_move_cons_head(struct rte_ring *r, unsigned 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: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.7.4

Re: [PATCH v3 1/1] ring: enforce reading the tail before reading ring slots

[Thomas Monjalon]

> > 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: c9fb3c62896f ("ring: move code in a new header file")
> > 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: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks