Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

[Ingo Molnar]

* Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx> wrote:

> If a process invokes synchronize_srcu(), is delayed just the right amount
> of time, and thus does not sleep when waiting for the grace period to
> complete, there is no ordering between the end of the grace period and
> the code following the synchronize_srcu().  Similarly, there can be a
> lack of ordering between the end of the SRCU grace period and callback
> invocation.
> 
> This commit adds the necessary ordering.
> 
> Reported-by: Lance Roy <ldr709@xxxxxxxxx>
> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> ---
>  include/linux/rcupdate.h | 12 ++++++++++++
>  kernel/rcu/srcu.c        |  5 +++++
>  kernel/rcu/tree.h        | 12 ------------
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 01f71e1d2e94..608d56f908f2 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -1161,5 +1161,17 @@ do { \
>  		ftrace_dump(oops_dump_mode); \
>  } while (0)
>  
> +/*
> + * Place this after a lock-acquisition primitive to guarantee that
> + * an UNLOCK+LOCK pair act as a full barrier.  This guarantee applies
> + * if the UNLOCK and LOCK are executed by the same CPU or if the
> + * UNLOCK and LOCK operate on the same lock variable.

minor typo:

  s/an UNLOCK+LOCK pair act as
    an UNLOCK+LOCK pair acts as

> + */
> +#ifdef CONFIG_PPC
> +#define smp_mb__after_unlock_lock()	smp_mb()  /* Full ordering for lock. */
> +#else /* #ifdef CONFIG_PPC */
> +#define smp_mb__after_unlock_lock()	do { } while (0)
> +#endif /* #else #ifdef CONFIG_PPC */

Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
#ifdefs into generic headers is generally frowned upon.

The canonical approach would be either to define a helper Kconfig variable that 
can be set by PPC (but other architectures don't need to set it), or to expose a 
suitable macro (function) for architectures to define in their barrier.h arch 
header file.

Thanks,

	Ingo