[PATCH] eal: add notes to SMP memory barrier APIs

[PATCH] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

The rte_smp_xx() APIs are deprecated. But it is not mentioned
in the function header.
Added notes in function header for clarification.

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/eal/include/generic/rte_atomic.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
index 58df843c54..542a2c16ff 100644
--- a/lib/eal/include/generic/rte_atomic.h
+++ b/lib/eal/include/generic/rte_atomic.h
@@ -55,6 +55,11 @@ static inline void rte_rmb(void);
  * Guarantees that the LOAD and STORE operations that precede the
  * rte_smp_mb() call are globally visible across the lcores
  * before the LOAD and STORE operations that follows it.
+ *
+ * @note
+ *  This function is deprecated. It adds complexity to the memory model
+ *  used by this project. C11 memory model should always be used.
+ *  rte_atomic_thread_fence() should be used instead.
  */
 static inline void rte_smp_mb(void);
 
@@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
  * Guarantees that the STORE operations that precede the
  * rte_smp_wmb() call are globally visible across the lcores
  * before the STORE operations that follows it.
+ *
+ * @note
+ *  This function is deprecated. It adds complexity to the memory model
+ *  used by this project. C11 memory model should always be used.
+ *  rte_atomic_thread_fence() should be used instead.
  */
 static inline void rte_smp_wmb(void);
 
@@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
  * Guarantees that the LOAD operations that precede the
  * rte_smp_rmb() call are globally visible across the lcores
  * before the LOAD operations that follows it.
+ *
+ * @note
+ *  This function is deprecated. It adds complexity to the memory model
+ *  used by this project. C11 memory model should always be used.
+ *  rte_atomic_thread_fence() should be used instead.
  */
 static inline void rte_smp_rmb(void);
 ///@}
-- 
2.25.1

Re: [PATCH] eal: add notes to SMP memory barrier APIs

[Thomas Monjalon]

21/06/2023 08:44, Ruifeng Wang:
> + * @note
> + *  This function is deprecated. It adds complexity to the memory model
> + *  used by this project. C11 memory model should always be used.
> + *  rte_atomic_thread_fence() should be used instead.
>   */
>  static inline void rte_smp_mb(void);

I think it should be more explicit:
"the memory model used by this project" -> the DPDK memory model
Why it adds complexity?
What do you mean by "C11 memory model"?

RE: [PATCH] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 21, 2023 3:30 PM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
> Cc: david.marchand@redhat.com; dev@dpdk.org; konstantin.v.ananyev@yandex.ru; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH] eal: add notes to SMP memory barrier APIs
> 
> 21/06/2023 08:44, Ruifeng Wang:
> > + * @note
> > + *  This function is deprecated. It adds complexity to the memory
> > + model
> > + *  used by this project. C11 memory model should always be used.
> > + *  rte_atomic_thread_fence() should be used instead.
> >   */
> >  static inline void rte_smp_mb(void);
> 
> I think it should be more explicit:
> "the memory model used by this project" -> the DPDK memory model Why it adds complexity?

The rte_smp_xx APIs define a set of memory order semantics. It is redundant given we are
using memory order semantics defined in the C language.
I'll make it more explicit in the next version.

> What do you mean by "C11 memory model"?

I mean the memory order semantics:
https://en.cppreference.com/w/c/atomic/memory_order

> 

Re: [PATCH] eal: add notes to SMP memory barrier APIs

[Mattias Rönnblom]

On 2023-06-21 08:44, Ruifeng Wang wrote:
> The rte_smp_xx() APIs are deprecated. But it is not mentioned
> in the function header.
> Added notes in function header for clarification.
> 
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>   lib/eal/include/generic/rte_atomic.h | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
> index 58df843c54..542a2c16ff 100644
> --- a/lib/eal/include/generic/rte_atomic.h
> +++ b/lib/eal/include/generic/rte_atomic.h
> @@ -55,6 +55,11 @@ static inline void rte_rmb(void);
>    * Guarantees that the LOAD and STORE operations that precede the
>    * rte_smp_mb() call are globally visible across the lcores
>    * before the LOAD and STORE operations that follows it.
> + *
> + * @note
> + *  This function is deprecated. It adds complexity to the memory model
> + *  used by this project. C11 memory model should always be used.
> + *  rte_atomic_thread_fence() should be used instead.

It's somewhat confusing to learn I should use the C11 memory model, and 
then in the next sentence that I should call a function which is not in C11.

I think it would be helpful to say which memory_model parameters should 
be used to replace the rte_smp_*mb() calls, and if there are any 
difference in semantics between the Linux kernel-style barriers and 
their C11 (near-)equivalents.

Is there some particular reason these functions aren't marked 
__rte_deprecated? Too many warnings?

>    */
>   static inline void rte_smp_mb(void);
>   
> @@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
>    * Guarantees that the STORE operations that precede the
>    * rte_smp_wmb() call are globally visible across the lcores
>    * before the STORE operations that follows it.
> + *
> + * @note
> + *  This function is deprecated. It adds complexity to the memory model
> + *  used by this project. C11 memory model should always be used.
> + *  rte_atomic_thread_fence() should be used instead.
>    */
>   static inline void rte_smp_wmb(void);
>   
> @@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
>    * Guarantees that the LOAD operations that precede the
>    * rte_smp_rmb() call are globally visible across the lcores
>    * before the LOAD operations that follows it.
> + *
> + * @note
> + *  This function is deprecated. It adds complexity to the memory model
> + *  used by this project. C11 memory model should always be used.
> + *  rte_atomic_thread_fence() should be used instead.
>    */
>   static inline void rte_smp_rmb(void);
>   ///@}

Re: [PATCH] eal: add notes to SMP memory barrier APIs

[Tyler Retzlaff]

On Thu, Jun 22, 2023 at 08:19:30PM +0200, Mattias Rönnblom wrote:
> On 2023-06-21 08:44, Ruifeng Wang wrote:
> >The rte_smp_xx() APIs are deprecated. But it is not mentioned
> >in the function header.
> >Added notes in function header for clarification.
> >
> >Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >---
> >  lib/eal/include/generic/rte_atomic.h | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> >diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
> >index 58df843c54..542a2c16ff 100644
> >--- a/lib/eal/include/generic/rte_atomic.h
> >+++ b/lib/eal/include/generic/rte_atomic.h
> >@@ -55,6 +55,11 @@ static inline void rte_rmb(void);
> >   * Guarantees that the LOAD and STORE operations that precede the
> >   * rte_smp_mb() call are globally visible across the lcores
> >   * before the LOAD and STORE operations that follows it.
> >+ *
> >+ * @note
> >+ *  This function is deprecated. It adds complexity to the memory model
> >+ *  used by this project. C11 memory model should always be used.
> >+ *  rte_atomic_thread_fence() should be used instead.
> 
> It's somewhat confusing to learn I should use the C11 memory model,
> and then in the next sentence that I should call a function which is
> not in C11.

i wonder if we can just do without the comments until we begin to adopt
changes for 23.11 release because the guidance will be short lived.

in 23.07 we want to say that only gcc builtins that align with the
standard C++ memory model should be used.

in 23.11 we want to say that only standard C11 atomics should be used.

my suggestion i guess is just adapt the patch to be appropriate for
23.11 and only merge it after 23.07 release? might be easier to manage.

> 
> I think it would be helpful to say which memory_model parameters
> should be used to replace the rte_smp_*mb() calls, and if there are
> any difference in semantics between the Linux kernel-style barriers
> and their C11 (near-)equivalents.
> 
> Is there some particular reason these functions aren't marked
> __rte_deprecated? Too many warnings?
> 
> >   */
> >  static inline void rte_smp_mb(void);
> >@@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
> >   * Guarantees that the STORE operations that precede the
> >   * rte_smp_wmb() call are globally visible across the lcores
> >   * before the STORE operations that follows it.
> >+ *
> >+ * @note
> >+ *  This function is deprecated. It adds complexity to the memory model
> >+ *  used by this project. C11 memory model should always be used.
> >+ *  rte_atomic_thread_fence() should be used instead.
> >   */
> >  static inline void rte_smp_wmb(void);
> >@@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
> >   * Guarantees that the LOAD operations that precede the
> >   * rte_smp_rmb() call are globally visible across the lcores
> >   * before the LOAD operations that follows it.
> >+ *
> >+ * @note
> >+ *  This function is deprecated. It adds complexity to the memory model
> >+ *  used by this project. C11 memory model should always be used.
> >+ *  rte_atomic_thread_fence() should be used instead.
> >   */
> >  static inline void rte_smp_rmb(void);
> >  ///@}

RE: [PATCH] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

> -----Original Message-----
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Sent: Saturday, June 24, 2023 5:51 AM
> To: Mattias Rönnblom <hofors@lysator.liu.se>
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net; david.marchand@redhat.com;
> dev@dpdk.org; konstantin.v.ananyev@yandex.ru; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH] eal: add notes to SMP memory barrier APIs
> 
> On Thu, Jun 22, 2023 at 08:19:30PM +0200, Mattias R�nnblom wrote:
> > On 2023-06-21 08:44, Ruifeng Wang wrote:
> > >The rte_smp_xx() APIs are deprecated. But it is not mentioned in the
> > >function header.
> > >Added notes in function header for clarification.
> > >
> > >Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > >---
> > >  lib/eal/include/generic/rte_atomic.h | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > >diff --git a/lib/eal/include/generic/rte_atomic.h
> > >b/lib/eal/include/generic/rte_atomic.h
> > >index 58df843c54..542a2c16ff 100644
> > >--- a/lib/eal/include/generic/rte_atomic.h
> > >+++ b/lib/eal/include/generic/rte_atomic.h
> > >@@ -55,6 +55,11 @@ static inline void rte_rmb(void);
> > >   * Guarantees that the LOAD and STORE operations that precede the
> > >   * rte_smp_mb() call are globally visible across the lcores
> > >   * before the LOAD and STORE operations that follows it.
> > >+ *
> > >+ * @note
> > >+ *  This function is deprecated. It adds complexity to the memory
> > >+ model
> > >+ *  used by this project. C11 memory model should always be used.
> > >+ *  rte_atomic_thread_fence() should be used instead.
> >
> > It's somewhat confusing to learn I should use the C11 memory model,
> > and then in the next sentence that I should call a function which is
> > not in C11.
> 
> i wonder if we can just do without the comments until we begin to adopt changes for 23.11
> release because the guidance will be short lived.
> 
> in 23.07 we want to say that only gcc builtins that align with the standard C++ memory
> model should be used.
> 
> in 23.11 we want to say that only standard C11 atomics should be used.

Good point. The memory order parameter will change in 23.11.

> 
> my suggestion i guess is just adapt the patch to be appropriate for
> 23.11 and only merge it after 23.07 release? might be easier to manage.

Agree to only merge it after 23.07. 
I will update the comment for standard C11 atomics.

> 
> >
> > I think it would be helpful to say which memory_model parameters
> > should be used to replace the rte_smp_*mb() calls, and if there are
> > any difference in semantics between the Linux kernel-style barriers
> > and their C11 (near-)equivalents.
> >
> > Is there some particular reason these functions aren't marked
> > __rte_deprecated? Too many warnings?
> >
> > >   */
> > >  static inline void rte_smp_mb(void); @@ -64,6 +69,11 @@ static
> > >inline void rte_smp_mb(void);
> > >   * Guarantees that the STORE operations that precede the
> > >   * rte_smp_wmb() call are globally visible across the lcores
> > >   * before the STORE operations that follows it.
> > >+ *
> > >+ * @note
> > >+ *  This function is deprecated. It adds complexity to the memory
> > >+ model
> > >+ *  used by this project. C11 memory model should always be used.
> > >+ *  rte_atomic_thread_fence() should be used instead.
> > >   */
> > >  static inline void rte_smp_wmb(void); @@ -73,6 +83,11 @@ static
> > >inline void rte_smp_wmb(void);
> > >   * Guarantees that the LOAD operations that precede the
> > >   * rte_smp_rmb() call are globally visible across the lcores
> > >   * before the LOAD operations that follows it.
> > >+ *
> > >+ * @note
> > >+ *  This function is deprecated. It adds complexity to the memory
> > >+ model
> > >+ *  used by this project. C11 memory model should always be used.
> > >+ *  rte_atomic_thread_fence() should be used instead.
> > >   */
> > >  static inline void rte_smp_rmb(void);  ///@}

Re: [PATCH] eal: add notes to SMP memory barrier APIs

[Thomas Monjalon]

25/06/2023 10:45, Ruifeng Wang:
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > On Thu, Jun 22, 2023 at 08:19:30PM +0200, Mattias R�nnblom wrote:
> > > On 2023-06-21 08:44, Ruifeng Wang wrote:
> > > >+ *  This function is deprecated. It adds complexity to the memory
> > > >+ model
> > > >+ *  used by this project. C11 memory model should always be used.
> > > >+ *  rte_atomic_thread_fence() should be used instead.
> > >
> > > It's somewhat confusing to learn I should use the C11 memory model,
> > > and then in the next sentence that I should call a function which is
> > > not in C11.
> > 
> > i wonder if we can just do without the comments until we begin to adopt changes for 23.11
> > release because the guidance will be short lived.
> > 
> > in 23.07 we want to say that only gcc builtins that align with the standard C++ memory
> > model should be used.
> > 
> > in 23.11 we want to say that only standard C11 atomics should be used.
> 
> Good point. The memory order parameter will change in 23.11.
> 
> > 
> > my suggestion i guess is just adapt the patch to be appropriate for
> > 23.11 and only merge it after 23.07 release? might be easier to manage.
> 
> Agree to only merge it after 23.07. 
> I will update the comment for standard C11 atomics.

I would prefer having each step documented
so it will be clearer what is new in 23.11.

RE: [PATCH] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

> -----Original Message-----
> From: Mattias Rönnblom <hofors@lysator.liu.se>
> Sent: Friday, June 23, 2023 2:20 AM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net; david.marchand@redhat.com
> Cc: dev@dpdk.org; konstantin.v.ananyev@yandex.ru; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH] eal: add notes to SMP memory barrier APIs
> 
> On 2023-06-21 08:44, Ruifeng Wang wrote:
> > The rte_smp_xx() APIs are deprecated. But it is not mentioned in the
> > function header.
> > Added notes in function header for clarification.
> >
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >   lib/eal/include/generic/rte_atomic.h | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> >
> > diff --git a/lib/eal/include/generic/rte_atomic.h
> > b/lib/eal/include/generic/rte_atomic.h
> > index 58df843c54..542a2c16ff 100644
> > --- a/lib/eal/include/generic/rte_atomic.h
> > +++ b/lib/eal/include/generic/rte_atomic.h
> > @@ -55,6 +55,11 @@ static inline void rte_rmb(void);
> >    * Guarantees that the LOAD and STORE operations that precede the
> >    * rte_smp_mb() call are globally visible across the lcores
> >    * before the LOAD and STORE operations that follows it.
> > + *
> > + * @note
> > + *  This function is deprecated. It adds complexity to the memory
> > + model
> > + *  used by this project. C11 memory model should always be used.
> > + *  rte_atomic_thread_fence() should be used instead.
> 
> It's somewhat confusing to learn I should use the C11 memory model, and then in the next
> sentence that I should call a function which is not in C11.

I should say "memory order semantics". It will be more specific.
The wrapper function rte_atomic_thread_fence is a special case. It provides an optimized implementation
for __ATOMIC_SEQ_CST for x86:
https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/

> 
> I think it would be helpful to say which memory_model parameters should be used to replace
> the rte_smp_*mb() calls, and if there are any difference in semantics between the Linux
> kernel-style barriers and their C11 (near-)equivalents.

As compiler atomic built-ins are being used. The memory model parameters should be the ones listed in:
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
We are not taking Linux kernel-style barriers. So no need to mention that.

> 
> Is there some particular reason these functions aren't marked __rte_deprecated? Too many
> warnings?

Yes, warnings will come up. Some occurrences still remain in the project. 

> 
> >    */
> >   static inline void rte_smp_mb(void);
> >
> > @@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
> >    * Guarantees that the STORE operations that precede the
> >    * rte_smp_wmb() call are globally visible across the lcores
> >    * before the STORE operations that follows it.
> > + *
> > + * @note
> > + *  This function is deprecated. It adds complexity to the memory
> > + model
> > + *  used by this project. C11 memory model should always be used.
> > + *  rte_atomic_thread_fence() should be used instead.
> >    */
> >   static inline void rte_smp_wmb(void);
> >
> > @@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
> >    * Guarantees that the LOAD operations that precede the
> >    * rte_smp_rmb() call are globally visible across the lcores
> >    * before the LOAD operations that follows it.
> > + *
> > + * @note
> > + *  This function is deprecated. It adds complexity to the memory
> > + model
> > + *  used by this project. C11 memory model should always be used.
> > + *  rte_atomic_thread_fence() should be used instead.
> >    */
> >   static inline void rte_smp_rmb(void);
> >   ///@}

Re: [PATCH] eal: add notes to SMP memory barrier APIs

[Mattias Rönnblom]

On 2023-06-25 10:17, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: Mattias Rönnblom <hofors@lysator.liu.se>
>> Sent: Friday, June 23, 2023 2:20 AM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net; david.marchand@redhat.com
>> Cc: dev@dpdk.org; konstantin.v.ananyev@yandex.ru; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH] eal: add notes to SMP memory barrier APIs
>>
>> On 2023-06-21 08:44, Ruifeng Wang wrote:
>>> The rte_smp_xx() APIs are deprecated. But it is not mentioned in the
>>> function header.
>>> Added notes in function header for clarification.
>>>
>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>> ---
>>>    lib/eal/include/generic/rte_atomic.h | 15 +++++++++++++++
>>>    1 file changed, 15 insertions(+)
>>>
>>> diff --git a/lib/eal/include/generic/rte_atomic.h
>>> b/lib/eal/include/generic/rte_atomic.h
>>> index 58df843c54..542a2c16ff 100644
>>> --- a/lib/eal/include/generic/rte_atomic.h
>>> +++ b/lib/eal/include/generic/rte_atomic.h
>>> @@ -55,6 +55,11 @@ static inline void rte_rmb(void);
>>>     * Guarantees that the LOAD and STORE operations that precede the
>>>     * rte_smp_mb() call are globally visible across the lcores
>>>     * before the LOAD and STORE operations that follows it.
>>> + *
>>> + * @note
>>> + *  This function is deprecated. It adds complexity to the memory
>>> + model
>>> + *  used by this project. C11 memory model should always be used.
>>> + *  rte_atomic_thread_fence() should be used instead.
>>
>> It's somewhat confusing to learn I should use the C11 memory model, and then in the next
>> sentence that I should call a function which is not in C11.
> 
> I should say "memory order semantics". It will be more specific.
> The wrapper function rte_atomic_thread_fence is a special case. It provides an optimized implementation
> for __ATOMIC_SEQ_CST for x86:
> https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
> 
>>
>> I think it would be helpful to say which memory_model parameters should be used to replace
>> the rte_smp_*mb() calls, and if there are any difference in semantics between the Linux
>> kernel-style barriers and their C11 (near-)equivalents.
> 
> As compiler atomic built-ins are being used. The memory model parameters should be the ones listed in:
> https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> We are not taking Linux kernel-style barriers. So no need to mention that.
> 

Yeah, sure. But which one of the C11 memory models, for respective 
legacy barrier?

What you are moving from is Linux kernel-style barriers, so if you are 
to recommend a migration path, their semantics will matter.

>>
>> Is there some particular reason these functions aren't marked __rte_deprecated? Too many
>> warnings?
> 
> Yes, warnings will come up. Some occurrences still remain in the project.
> 
>>
>>>     */
>>>    static inline void rte_smp_mb(void);
>>>
>>> @@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
>>>     * Guarantees that the STORE operations that precede the
>>>     * rte_smp_wmb() call are globally visible across the lcores
>>>     * before the STORE operations that follows it.
>>> + *
>>> + * @note
>>> + *  This function is deprecated. It adds complexity to the memory
>>> + model
>>> + *  used by this project. C11 memory model should always be used.
>>> + *  rte_atomic_thread_fence() should be used instead.
>>>     */
>>>    static inline void rte_smp_wmb(void);
>>>
>>> @@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
>>>     * Guarantees that the LOAD operations that precede the
>>>     * rte_smp_rmb() call are globally visible across the lcores
>>>     * before the LOAD operations that follows it.
>>> + *
>>> + * @note
>>> + *  This function is deprecated. It adds complexity to the memory
>>> + model
>>> + *  used by this project. C11 memory model should always be used.
>>> + *  rte_atomic_thread_fence() should be used instead.
>>>     */
>>>    static inline void rte_smp_rmb(void);
>>>    ///@}

RE: [PATCH] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

> -----Original Message-----
> From: Mattias Rönnblom <hofors@lysator.liu.se>
> Sent: Friday, June 30, 2023 3:28 AM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net; david.marchand@redhat.com
> Cc: dev@dpdk.org; konstantin.v.ananyev@yandex.ru; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH] eal: add notes to SMP memory barrier APIs
> 
> On 2023-06-25 10:17, Ruifeng Wang wrote:
> >> -----Original Message-----
> >> From: Mattias Rönnblom <hofors@lysator.liu.se>
> >> Sent: Friday, June 23, 2023 2:20 AM
> >> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> >> david.marchand@redhat.com
> >> Cc: dev@dpdk.org; konstantin.v.ananyev@yandex.ru; Honnappa
> >> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [PATCH] eal: add notes to SMP memory barrier APIs
> >>
> >> On 2023-06-21 08:44, Ruifeng Wang wrote:
> >>> The rte_smp_xx() APIs are deprecated. But it is not mentioned in the
> >>> function header.
> >>> Added notes in function header for clarification.
> >>>
> >>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>> ---
> >>>    lib/eal/include/generic/rte_atomic.h | 15 +++++++++++++++
> >>>    1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/lib/eal/include/generic/rte_atomic.h
> >>> b/lib/eal/include/generic/rte_atomic.h
> >>> index 58df843c54..542a2c16ff 100644
> >>> --- a/lib/eal/include/generic/rte_atomic.h
> >>> +++ b/lib/eal/include/generic/rte_atomic.h
> >>> @@ -55,6 +55,11 @@ static inline void rte_rmb(void);
> >>>     * Guarantees that the LOAD and STORE operations that precede the
> >>>     * rte_smp_mb() call are globally visible across the lcores
> >>>     * before the LOAD and STORE operations that follows it.
> >>> + *
> >>> + * @note
> >>> + *  This function is deprecated. It adds complexity to the memory
> >>> + model
> >>> + *  used by this project. C11 memory model should always be used.
> >>> + *  rte_atomic_thread_fence() should be used instead.
> >>
> >> It's somewhat confusing to learn I should use the C11 memory model,
> >> and then in the next sentence that I should call a function which is not in C11.
> >
> > I should say "memory order semantics". It will be more specific.
> > The wrapper function rte_atomic_thread_fence is a special case. It
> > provides an optimized implementation for __ATOMIC_SEQ_CST for x86:
> > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
> >
> >>
> >> I think it would be helpful to say which memory_model parameters
> >> should be used to replace the rte_smp_*mb() calls, and if there are
> >> any difference in semantics between the Linux kernel-style barriers and their C11
> (near-)equivalents.
> >
> > As compiler atomic built-ins are being used. The memory model parameters should be the
> ones listed in:
> > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> > We are not taking Linux kernel-style barriers. So no need to mention that.
> >
> 
> Yeah, sure. But which one of the C11 memory models, for respective legacy barrier?
> 
> What you are moving from is Linux kernel-style barriers, so if you are to recommend a
> migration path, their semantics will matter.

Got it. I can add the suggested memory_model parameters for respective legacy barrier.

> 
> >>
> >> Is there some particular reason these functions aren't marked
> >> __rte_deprecated? Too many warnings?
> >
> > Yes, warnings will come up. Some occurrences still remain in the project.
> >
> >>
> >>>     */
> >>>    static inline void rte_smp_mb(void);
> >>>
> >>> @@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
> >>>     * Guarantees that the STORE operations that precede the
> >>>     * rte_smp_wmb() call are globally visible across the lcores
> >>>     * before the STORE operations that follows it.
> >>> + *
> >>> + * @note
> >>> + *  This function is deprecated. It adds complexity to the memory
> >>> + model
> >>> + *  used by this project. C11 memory model should always be used.
> >>> + *  rte_atomic_thread_fence() should be used instead.
> >>>     */
> >>>    static inline void rte_smp_wmb(void);
> >>>
> >>> @@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
> >>>     * Guarantees that the LOAD operations that precede the
> >>>     * rte_smp_rmb() call are globally visible across the lcores
> >>>     * before the LOAD operations that follows it.
> >>> + *
> >>> + * @note
> >>> + *  This function is deprecated. It adds complexity to the memory
> >>> + model
> >>> + *  used by this project. C11 memory model should always be used.
> >>> + *  rte_atomic_thread_fence() should be used instead.
> >>>     */
> >>>    static inline void rte_smp_rmb(void);
> >>>    ///@}

[PATCH v2] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

The rte_smp_xx() APIs are deprecated. But it is not mentioned
in the function header.
Added notes in function header for clarification.

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v2: Made the notes more specific.

 lib/eal/include/generic/rte_atomic.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
index 58df843c54..35e0041ce6 100644
--- a/lib/eal/include/generic/rte_atomic.h
+++ b/lib/eal/include/generic/rte_atomic.h
@@ -55,6 +55,11 @@ static inline void rte_rmb(void);
  * Guarantees that the LOAD and STORE operations that precede the
  * rte_smp_mb() call are globally visible across the lcores
  * before the LOAD and STORE operations that follows it.
+ *
+ * @note
+ *  This function is deprecated. It provides fence synchronization
+ *  primitive but doesn't take memory order parameter.
+ *  rte_atomic_thread_fence() should be used instead.
  */
 static inline void rte_smp_mb(void);
 
@@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
  * Guarantees that the STORE operations that precede the
  * rte_smp_wmb() call are globally visible across the lcores
  * before the STORE operations that follows it.
+ *
+ * @note
+ *  This function is deprecated. It provides fence synchronization
+ *  primitive but doesn't take memory order parameter.
+ *  rte_atomic_thread_fence() should be used instead.
  */
 static inline void rte_smp_wmb(void);
 
@@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
  * Guarantees that the LOAD operations that precede the
  * rte_smp_rmb() call are globally visible across the lcores
  * before the LOAD operations that follows it.
+ *
+ * @note
+ *  This function is deprecated. It provides fence synchronization
+ *  primitive but doesn't take memory order parameter.
+ *  rte_atomic_thread_fence() should be used instead.
  */
 static inline void rte_smp_rmb(void);
 ///@}
@@ -122,6 +137,10 @@ static inline void rte_io_rmb(void);
 
 /**
  * Synchronization fence between threads based on the specified memory order.
+ *
+ * @param memorder
+ *   The memory order defined by compiler atomic builtin at:
+ *   https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
  */
 static inline void rte_atomic_thread_fence(int memorder);
 
-- 
2.25.1

Re: [PATCH v2] eal: add notes to SMP memory barrier APIs

[Mattias Rönnblom]

On 2023-06-26 09:12, Ruifeng Wang wrote:
> The rte_smp_xx() APIs are deprecated. But it is not mentioned
> in the function header.
> Added notes in function header for clarification.
> 
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v2: Made the notes more specific.
> 
>   lib/eal/include/generic/rte_atomic.h | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
> index 58df843c54..35e0041ce6 100644
> --- a/lib/eal/include/generic/rte_atomic.h
> +++ b/lib/eal/include/generic/rte_atomic.h
> @@ -55,6 +55,11 @@ static inline void rte_rmb(void);
>    * Guarantees that the LOAD and STORE operations that precede the
>    * rte_smp_mb() call are globally visible across the lcores
>    * before the LOAD and STORE operations that follows it.
> + *
> + * @note
> + *  This function is deprecated. It provides fence synchronization
> + *  primitive but doesn't take memory order parameter.
> + *  rte_atomic_thread_fence() should be used instead.

I can't see why coding the memory model semantics into the name, rather 
than by specification-by-means-of-a-parameter, could be the real issue. 
Could you explain? Seems like just different syntax to me.

The old <rte_atomic.h> atomic arithmetic and atomic load/store 
operations suffered from unspecified semantics in regards to any 
ordering they imposed on other memory accesses. I guess that shortcoming 
could be described as a "missing parameter", although that too would be 
misleading. Unclear semantics seems not be the case for the kernel-style 
barriers though.

>    */
>   static inline void rte_smp_mb(void);
>   
> @@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
>    * Guarantees that the STORE operations that precede the
>    * rte_smp_wmb() call are globally visible across the lcores
>    * before the STORE operations that follows it.
> + *
> + * @note
> + *  This function is deprecated. It provides fence synchronization
> + *  primitive but doesn't take memory order parameter.
> + *  rte_atomic_thread_fence() should be used instead.
>    */
>   static inline void rte_smp_wmb(void);
>   
> @@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
>    * Guarantees that the LOAD operations that precede the
>    * rte_smp_rmb() call are globally visible across the lcores
>    * before the LOAD operations that follows it.
> + *
> + * @note
> + *  This function is deprecated. It provides fence synchronization
> + *  primitive but doesn't take memory order parameter.
> + *  rte_atomic_thread_fence() should be used instead.
>    */
>   static inline void rte_smp_rmb(void);
>   ///@}
> @@ -122,6 +137,10 @@ static inline void rte_io_rmb(void);
>   
>   /**
>    * Synchronization fence between threads based on the specified memory order.
> + *
> + * @param memorder
> + *   The memory order defined by compiler atomic builtin at:
> + *   https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
>    */
>   static inline void rte_atomic_thread_fence(int memorder);
>   

RE: [PATCH v2] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

> -----Original Message-----
> From: Mattias Rönnblom <hofors@lysator.liu.se>
> Sent: Friday, June 30, 2023 3:44 AM
> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net; david.marchand@redhat.com
> Cc: dev@dpdk.org; roretzla@linux.microsoft.com; konstantin.v.ananyev@yandex.ru; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v2] eal: add notes to SMP memory barrier APIs
> 
> On 2023-06-26 09:12, Ruifeng Wang wrote:
> > The rte_smp_xx() APIs are deprecated. But it is not mentioned in the
> > function header.
> > Added notes in function header for clarification.
> >
> > Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > v2: Made the notes more specific.
> >
> >   lib/eal/include/generic/rte_atomic.h | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> >
> > diff --git a/lib/eal/include/generic/rte_atomic.h
> > b/lib/eal/include/generic/rte_atomic.h
> > index 58df843c54..35e0041ce6 100644
> > --- a/lib/eal/include/generic/rte_atomic.h
> > +++ b/lib/eal/include/generic/rte_atomic.h
> > @@ -55,6 +55,11 @@ static inline void rte_rmb(void);
> >    * Guarantees that the LOAD and STORE operations that precede the
> >    * rte_smp_mb() call are globally visible across the lcores
> >    * before the LOAD and STORE operations that follows it.
> > + *
> > + * @note
> > + *  This function is deprecated. It provides fence synchronization
> > + *  primitive but doesn't take memory order parameter.
> > + *  rte_atomic_thread_fence() should be used instead.
> 
> I can't see why coding the memory model semantics into the name, rather than by
> specification-by-means-of-a-parameter, could be the real issue.
> Could you explain? Seems like just different syntax to me.

Yes, rte_smp_xx and rte_atomic_thread_fence have different syntaxes.

The compiler atomic builtins were accepted for memory ordering. It comprises atomic arithmetic,
atomic load/store, and atomic fence. It is simpler and clearer to do memory ordering by using
the atomic builtins whenever possible.
rte_smp_xx has functionality overlap with atomic fence builtins but with different memory model
semantics and different syntaxes. Because of the differences, it will make memory ordering a little
more complex if rte_smp_xx is kept aside atomic builtins suite.

> 
> The old <rte_atomic.h> atomic arithmetic and atomic load/store operations suffered from
> unspecified semantics in regards to any ordering they imposed on other memory accesses. I
> guess that shortcoming could be described as a "missing parameter", although that too
> would be misleading. Unclear semantics seems not be the case for the kernel-style barriers
> though.
> 
> >    */
> >   static inline void rte_smp_mb(void);
> >
> > @@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
> >    * Guarantees that the STORE operations that precede the
> >    * rte_smp_wmb() call are globally visible across the lcores
> >    * before the STORE operations that follows it.
> > + *
> > + * @note
> > + *  This function is deprecated. It provides fence synchronization
> > + *  primitive but doesn't take memory order parameter.
> > + *  rte_atomic_thread_fence() should be used instead.
> >    */
> >   static inline void rte_smp_wmb(void);
> >
> > @@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
> >    * Guarantees that the LOAD operations that precede the
> >    * rte_smp_rmb() call are globally visible across the lcores
> >    * before the LOAD operations that follows it.
> > + *
> > + * @note
> > + *  This function is deprecated. It provides fence synchronization
> > + *  primitive but doesn't take memory order parameter.
> > + *  rte_atomic_thread_fence() should be used instead.
> >    */
> >   static inline void rte_smp_rmb(void);
> >   ///@}
> > @@ -122,6 +137,10 @@ static inline void rte_io_rmb(void);
> >
> >   /**
> >    * Synchronization fence between threads based on the specified memory order.
> > + *
> > + * @param memorder
> > + *   The memory order defined by compiler atomic builtin at:
> > + *   https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> >    */
> >   static inline void rte_atomic_thread_fence(int memorder);
> >

Re: [PATCH v2] eal: add notes to SMP memory barrier APIs

[Mattias Rönnblom]

On 2023-07-03 09:02, Ruifeng Wang wrote:
>> -----Original Message-----
>> From: Mattias Rönnblom <hofors@lysator.liu.se>
>> Sent: Friday, June 30, 2023 3:44 AM
>> To: Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net; david.marchand@redhat.com
>> Cc: dev@dpdk.org; roretzla@linux.microsoft.com; konstantin.v.ananyev@yandex.ru; Honnappa
>> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH v2] eal: add notes to SMP memory barrier APIs
>>
>> On 2023-06-26 09:12, Ruifeng Wang wrote:
>>> The rte_smp_xx() APIs are deprecated. But it is not mentioned in the
>>> function header.
>>> Added notes in function header for clarification.
>>>
>>> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>> ---
>>> v2: Made the notes more specific.
>>>
>>>    lib/eal/include/generic/rte_atomic.h | 19 +++++++++++++++++++
>>>    1 file changed, 19 insertions(+)
>>>
>>> diff --git a/lib/eal/include/generic/rte_atomic.h
>>> b/lib/eal/include/generic/rte_atomic.h
>>> index 58df843c54..35e0041ce6 100644
>>> --- a/lib/eal/include/generic/rte_atomic.h
>>> +++ b/lib/eal/include/generic/rte_atomic.h
>>> @@ -55,6 +55,11 @@ static inline void rte_rmb(void);
>>>     * Guarantees that the LOAD and STORE operations that precede the
>>>     * rte_smp_mb() call are globally visible across the lcores
>>>     * before the LOAD and STORE operations that follows it.
>>> + *
>>> + * @note
>>> + *  This function is deprecated. It provides fence synchronization
>>> + *  primitive but doesn't take memory order parameter.
>>> + *  rte_atomic_thread_fence() should be used instead.
>>
>> I can't see why coding the memory model semantics into the name, rather than by
>> specification-by-means-of-a-parameter, could be the real issue.
>> Could you explain? Seems like just different syntax to me.
> 
> Yes, rte_smp_xx and rte_atomic_thread_fence have different syntaxes.
> 
> The compiler atomic builtins were accepted for memory ordering. It comprises atomic arithmetic,
> atomic load/store, and atomic fence. It is simpler and clearer to do memory ordering by using
> the atomic builtins whenever possible.
> rte_smp_xx has functionality overlap with atomic fence builtins but with different memory model
> semantics and different syntaxes. Because of the differences, it will make memory ordering a little
> more complex if rte_smp_xx is kept aside atomic builtins suite.
> 

I wasn't arguing for keeping Linux kernel-style barriers. It was just 
the rationale that seemed flawed to me.

If Linux kernel-style memory barriers took a memory model parameter, we 
would still prefer C11-style GCC barrier intrinsics (for this release).

>>
>> The old <rte_atomic.h> atomic arithmetic and atomic load/store operations suffered from
>> unspecified semantics in regards to any ordering they imposed on other memory accesses. I
>> guess that shortcoming could be described as a "missing parameter", although that too
>> would be misleading. Unclear semantics seems not be the case for the kernel-style barriers
>> though.
>>
>>>     */
>>>    static inline void rte_smp_mb(void);
>>>
>>> @@ -64,6 +69,11 @@ static inline void rte_smp_mb(void);
>>>     * Guarantees that the STORE operations that precede the
>>>     * rte_smp_wmb() call are globally visible across the lcores
>>>     * before the STORE operations that follows it.
>>> + *
>>> + * @note
>>> + *  This function is deprecated. It provides fence synchronization
>>> + *  primitive but doesn't take memory order parameter.
>>> + *  rte_atomic_thread_fence() should be used instead.
>>>     */
>>>    static inline void rte_smp_wmb(void);
>>>
>>> @@ -73,6 +83,11 @@ static inline void rte_smp_wmb(void);
>>>     * Guarantees that the LOAD operations that precede the
>>>     * rte_smp_rmb() call are globally visible across the lcores
>>>     * before the LOAD operations that follows it.
>>> + *
>>> + * @note
>>> + *  This function is deprecated. It provides fence synchronization
>>> + *  primitive but doesn't take memory order parameter.
>>> + *  rte_atomic_thread_fence() should be used instead.
>>>     */
>>>    static inline void rte_smp_rmb(void);
>>>    ///@}
>>> @@ -122,6 +137,10 @@ static inline void rte_io_rmb(void);
>>>
>>>    /**
>>>     * Synchronization fence between threads based on the specified memory order.
>>> + *
>>> + * @param memorder
>>> + *   The memory order defined by compiler atomic builtin at:
>>> + *   https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
>>>     */
>>>    static inline void rte_atomic_thread_fence(int memorder);
>>>

[PATCH v3] eal: add notes to SMP memory barrier APIs

[Ruifeng Wang]

The rte_smp_xx() APIs are deprecated. But it is not mentioned
in the function header.
Added notes in function header for clarification.

Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v3: Added suggested memory ordering semantic for replacement.
    Refined deprecation explanation.
v2: Made the notes more specific.

 lib/eal/include/generic/rte_atomic.h | 30 ++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
index 58df843c54..aef44e2455 100644
--- a/lib/eal/include/generic/rte_atomic.h
+++ b/lib/eal/include/generic/rte_atomic.h
@@ -55,6 +55,14 @@ static inline void rte_rmb(void);
  * Guarantees that the LOAD and STORE operations that precede the
  * rte_smp_mb() call are globally visible across the lcores
  * before the LOAD and STORE operations that follows it.
+ *
+ * @note
+ *  This function is deprecated.
+ *  It provides similar synchronization primitive as atomic fence,
+ *  but has different syntax and memory ordering semantic. Hence
+ *  deprecated for the simplicity of memory ordering semantics in use.
+ *
+ *  rte_atomic_thread_fence(__ATOMIC_ACQ_REL) should be used instead.
  */
 static inline void rte_smp_mb(void);
 
@@ -64,6 +72,17 @@ static inline void rte_smp_mb(void);
  * Guarantees that the STORE operations that precede the
  * rte_smp_wmb() call are globally visible across the lcores
  * before the STORE operations that follows it.
+ *
+ * @note
+ *  This function is deprecated.
+ *  It provides similar synchronization primitive as atomic fence,
+ *  but has different syntax and memory ordering semantic. Hence
+ *  deprecated for the simplicity of memory ordering semantics in use.
+ *
+ *  rte_atomic_thread_fence(__ATOMIC_RELEASE) should be used instead.
+ *  The fence also guarantees LOAD operations that precede the call
+ *  are globally visible across the lcores before the STORE operations
+ *  that follows it.
  */
 static inline void rte_smp_wmb(void);
 
@@ -73,6 +92,17 @@ static inline void rte_smp_wmb(void);
  * Guarantees that the LOAD operations that precede the
  * rte_smp_rmb() call are globally visible across the lcores
  * before the LOAD operations that follows it.
+ *
+ * @note
+ *  This function is deprecated.
+ *  It provides similar synchronization primitive as atomic fence,
+ *  but has different syntax and memory ordering semantic. Hence
+ *  deprecated for the simplicity of memory ordering semantics in use.
+ *
+ *  rte_atomic_thread_fence(__ATOMIC_ACQUIRE) should be used instead.
+ *  The fence also guarantees LOAD operations that precede the call
+ *  are globally visible across the lcores before the STORE operations
+ *  that follows it.
  */
 static inline void rte_smp_rmb(void);
 ///@}
-- 
2.25.1

Re: [PATCH v3] eal: add notes to SMP memory barrier APIs

[Thomas Monjalon]

03/07/2023 11:56, Ruifeng Wang:
> The rte_smp_xx() APIs are deprecated. But it is not mentioned
> in the function header.
> Added notes in function header for clarification.
> 
> Signed-off-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v3: Added suggested memory ordering semantic for replacement.
>     Refined deprecation explanation.
> v2: Made the notes more specific.
> 
>  lib/eal/include/generic/rte_atomic.h | 30 ++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
> index 58df843c54..aef44e2455 100644
> --- a/lib/eal/include/generic/rte_atomic.h
> +++ b/lib/eal/include/generic/rte_atomic.h
> @@ -55,6 +55,14 @@ static inline void rte_rmb(void);
>   * Guarantees that the LOAD and STORE operations that precede the
>   * rte_smp_mb() call are globally visible across the lcores
>   * before the LOAD and STORE operations that follows it.
> + *
> + * @note
> + *  This function is deprecated.
> + *  It provides similar synchronization primitive as atomic fence,
> + *  but has different syntax and memory ordering semantic. Hence
> + *  deprecated for the simplicity of memory ordering semantics in use.
> + *
> + *  rte_atomic_thread_fence(__ATOMIC_ACQ_REL) should be used instead.
>   */
>  static inline void rte_smp_mb(void);
>  
> @@ -64,6 +72,17 @@ static inline void rte_smp_mb(void);
>   * Guarantees that the STORE operations that precede the
>   * rte_smp_wmb() call are globally visible across the lcores
>   * before the STORE operations that follows it.
> + *
> + * @note
> + *  This function is deprecated.
> + *  It provides similar synchronization primitive as atomic fence,
> + *  but has different syntax and memory ordering semantic. Hence
> + *  deprecated for the simplicity of memory ordering semantics in use.
> + *
> + *  rte_atomic_thread_fence(__ATOMIC_RELEASE) should be used instead.
> + *  The fence also guarantees LOAD operations that precede the call
> + *  are globally visible across the lcores before the STORE operations
> + *  that follows it.
>   */
>  static inline void rte_smp_wmb(void);
>  
> @@ -73,6 +92,17 @@ static inline void rte_smp_wmb(void);
>   * Guarantees that the LOAD operations that precede the
>   * rte_smp_rmb() call are globally visible across the lcores
>   * before the LOAD operations that follows it.
> + *
> + * @note
> + *  This function is deprecated.
> + *  It provides similar synchronization primitive as atomic fence,
> + *  but has different syntax and memory ordering semantic. Hence
> + *  deprecated for the simplicity of memory ordering semantics in use.
> + *
> + *  rte_atomic_thread_fence(__ATOMIC_ACQUIRE) should be used instead.
> + *  The fence also guarantees LOAD operations that precede the call
> + *  are globally visible across the lcores before the STORE operations
> + *  that follows it.
>   */
>  static inline void rte_smp_rmb(void);
>  ///@}

There was no more comment.

Applied, thanks.