diff mbox series

eal: add notes to SMP memory barrier APIs

Message ID 20230621064420.163931-1-ruifeng.wang@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal: add notes to SMP memory barrier APIs |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-abi-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-unit-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS

Commit Message

Ruifeng Wang June 21, 2023, 6:44 a.m. UTC 
  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(+)

Comments

Thomas Monjalon June 21, 2023, 7:29 a.m. UTC | #1 
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"?
Mattias Rönnblom June 22, 2023, 6:19 p.m. UTC | #2 
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);
>   ///@}
Tyler Retzlaff June 23, 2023, 9:51 p.m. UTC | #3 
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);
> >  ///@}
Ruifeng Wang June 25, 2023, 7:55 a.m. UTC | #4 
> -----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

>
Ruifeng Wang June 25, 2023, 8:17 a.m. UTC | #5 
> -----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);
> >   ///@}
Ruifeng Wang June 25, 2023, 8:45 a.m. UTC | #6 
> -----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);  ///@}
Thomas Monjalon June 25, 2023, 3:40 p.m. UTC | #7 
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.
Mattias Rönnblom June 29, 2023, 7:28 p.m. UTC | #8 
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);
>>>    ///@}
Ruifeng Wang July 3, 2023, 6:12 a.m. UTC | #9 
> -----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);
> >>>    ///@}
diff mbox series

Patch

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);
 ///@}