On Fri, Sep 8, 2023 at 4:36 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
>
> The LOAD-ACQUIRE access performed on rdp->nocb_cb_sleep advertizes
> ordering callback execution against grace period completion. However
> this is contradicted by the following:
>
> * This LOAD-ACQUIRE doesn't pair with anything. The only counterpart
> barrier that can be found is the smp_mb() placed after callbacks
> advancing in nocb_gp_wait(). However the barrier is placed _after_
> ->nocb_cb_sleep write.
Hmm, on one side you have:
WRITE_ONCE(rdp->nocb_cb_sleep, false);
smp_mb();
swake_up_one(&rdp->nocb_cb_wq); /* wakeup -- consider this to be a STORE */
And on another side you have:
swait_event_interruptible_exclusive(rdp->nocb_cb_wq, ..cond..) /*
consider this to be a LOAD */
smp_load_acquire(&rdp->nocb_cb_sleep)
/* exec CBs (LOAD operations) */
So there seems to be pairing AFAICS.
But maybe you are referring to pairing between advancing the callbacks
and storing to nocb_cb_sleep. In this case, the RELEASE of the nocb
unlock operation just after advancing should be providing the
ordering, but we still need the acquire this patch deletes.
> * Callbacks can be concurrently advanced between the LOAD-ACQUIRE on
> ->nocb_cb_sleep and the call to rcu_segcblist_extract_done_cbs() in
> rcu_do_batch(), making any ordering based on ->nocb_cb_sleep broken.
If you don't mind, could you elaborate more?
> * Both rcu_segcblist_extract_done_cbs() and rcu_advance_cbs() are called
> under the nocb_lock, the latter hereby providing already the desired
> ACQUIRE semantics.
The acquire orders loads to nocb_cb_sleep with all later loads/stores.
I am not sure how nocb_lock gives that same behavior since that's
doing ACQUIRE on the lock access itself and not on nocb_cb_sleep
access, I'd appreciate it if we can debate this out.
Every few months I need a memory-ordering workout so this can be that.
;-) You could be onto something.
thanks,
- Joel
>
> Therefore it is safe to access ->nocb_cb_sleep with a simple compiler
> barrier.
>
> Signed-off-by: Frederic Weisbecker <frederic@xxxxxxxxxx>
> ---
> kernel/rcu/tree_nocb.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> index b9eab359c597..6e63ba4788e1 100644
> --- a/kernel/rcu/tree_nocb.h
> +++ b/kernel/rcu/tree_nocb.h
> @@ -933,8 +933,7 @@ static void nocb_cb_wait(struct rcu_data *rdp)
> swait_event_interruptible_exclusive(rdp->nocb_cb_wq,
> nocb_cb_wait_cond(rdp));
>
> - // VVV Ensure CB invocation follows _sleep test.
> - if (smp_load_acquire(&rdp->nocb_cb_sleep)) { // ^^^
> + if (READ_ONCE(rdp->nocb_cb_sleep)) {
> WARN_ON(signal_pending(current));
> trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("WokeEmpty"));
> }
> --
> 2.41.0
>
