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

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