Re: [RFC PATCH 13/37] mm: implement speculative handling in __handle_mm_fault().

[Michel Lespinasse]

On Thu, Apr 29, 2021 at 11:34:12AM -0700, Paul E. McKenney wrote:
> ------------------------------------------------------------------------
> 
> commit 97262c64c2cf807bf06825e454c4bedd228fadfb
> Author: Paul E. McKenney <paulmck@xxxxxxxxxx>
> Date:   Thu Apr 29 11:18:01 2021 -0700
> 
>     rcu: Improve comments describing RCU read-side critical sections
>     
>     There are a number of places that call out the fact that preempt-disable
>     regions of code now act as RCU read-side critical sections, where
>     preempt-disable regions of code include irq-disable regions of code,
>     bh-disable regions of code, hardirq handlers, and NMI handlers.  However,
>     someone relying solely on (for example) the call_rcu() header comment
>     might well have no idea that preempt-disable regions of code have RCU
>     semantics.
>     
>     This commit therefore updates the header comments for
>     call_rcu(), synchronize_rcu(), rcu_dereference_bh_check(), and
>     rcu_dereference_sched_check() to call out these new(ish) forms of RCU
>     readers.
>     
>     Reported-by: Michel Lespinasse <michel@xxxxxxxxxxxxxx>
>     Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index a10480f2b4ef..c01b04ad64c4 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -532,7 +532,10 @@ do {									      \
>   * @p: The pointer to read, prior to dereferencing
>   * @c: The conditions under which the dereference will take place
>   *
> - * This is the RCU-bh counterpart to rcu_dereference_check().
> + * This is the RCU-bh counterpart to rcu_dereference_check().  However,
> + * please note that in recent kernels, synchronize_rcu() waits for
> + * local_bh_disable() regions of code in addition to regions of code
> + * demarked by rcu_read_lock() and rcu_read_unlock().

Two things:
- "recent kernels" could be clarified, as Matthew pointed out
- The above is not 100% clear if call_rcu() also waits for
  local_bh_disable() regions of code ? (you did clarify this in tree.c
  but I think it's better to have that here as well)

>   */
>  #define rcu_dereference_bh_check(p, c) \
>  	__rcu_dereference_check((p), (c) || rcu_read_lock_bh_held(), __rcu)
> @@ -543,6 +546,9 @@ do {									      \
>   * @c: The conditions under which the dereference will take place
>   *
>   * This is the RCU-sched counterpart to rcu_dereference_check().
> + * However, please note that in recent kernels, synchronize_rcu() waits
> + * for preemption-disabled regions of code in addition to regions of code
> + * demarked by rcu_read_lock() and rcu_read_unlock().

Same comments regarding "recent kernels" and call_rcu() here.

>   */
>  #define rcu_dereference_sched_check(p, c) \
>  	__rcu_dereference_check((p), (c) || rcu_read_lock_sched_held(), \
> @@ -634,6 +640,12 @@ do {									      \
>   * sections, invocation of the corresponding RCU callback is deferred
>   * until after the all the other CPUs exit their critical sections.
>   *
> + * In recent kernels, synchronize_rcu() and call_rcu() also wait for
> + * regions of code with preemption disabled, including regions of code
> + * with interrupts or softirqs disabled.  If your kernel is old enough
> + * for synchronize_sched() to be defined, only code enclosed within
> + * rcu_read_lock() and rcu_read_unlock() are guaranteed to be waited for.
> + *

Thanks, this is the quote I was looking for, and also I think it's
important for it to be in rcupdate.h rather than any .c implementation
(I think it's more natural to look at headers for this kind of stuff).

Same comment regarding "old enough" / "recent kernels" though.

>   * Note, however, that RCU callbacks are permitted to run concurrently
>   * with new RCU read-side critical sections.  One way that this can happen
>   * is via the following sequence of events: (1) CPU 0 enters an RCU
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c

The tree.c changes look fine to me.

Thanks a lot for looking into this !

--
Michel "walken" Lespinasse