Re: [PATCH v2 rcu/dev 1/2] rcu/tree: Reduce wake up for synchronize_rcu() common case

[Uladzislau Rezki]

On Mon, Mar 18, 2024 at 05:05:31PM -0400, Joel Fernandes wrote:
> 
> 
> > On Mar 18, 2024, at 2:58 PM, Uladzislau Rezki <urezki@xxxxxxxxx> wrote:
> > 
> > Hello, Joel!
> > 
> > Sorry for late checking, see below few comments:
> > 
> >> In the synchronize_rcu() common case, we will have less than
> >> SR_MAX_USERS_WAKE_FROM_GP number of users per GP. Waking up the kworker
> >> is pointless just to free the last injected wait head since at that point,
> >> all the users have already been awakened.
> >> 
> >> Introduce a new counter to track this and prevent the wakeup in the
> >> common case.
> >> 
> >> Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> >> ---
> >> Rebased on paul/dev of today.
> >> 
> >> kernel/rcu/tree.c | 36 +++++++++++++++++++++++++++++++-----
> >> kernel/rcu/tree.h |  1 +
> >> 2 files changed, 32 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >> index 9fbb5ab57c84..bd29fe3c76bf 100644
> >> --- a/kernel/rcu/tree.c
> >> +++ b/kernel/rcu/tree.c
> >> @@ -96,6 +96,7 @@ static struct rcu_state rcu_state = {
> >>    .ofl_lock = __ARCH_SPIN_LOCK_UNLOCKED,
> >>    .srs_cleanup_work = __WORK_INITIALIZER(rcu_state.srs_cleanup_work,
> >>        rcu_sr_normal_gp_cleanup_work),
> >> +    .srs_cleanups_pending = ATOMIC_INIT(0),
> >> };
> >> 
> >> /* Dump rcu_node combining tree at boot to verify correct setup. */
> >> @@ -1642,8 +1643,11 @@ static void rcu_sr_normal_gp_cleanup_work(struct work_struct *work)
> >>     * the done tail list manipulations are protected here.
> >>     */
> >>    done = smp_load_acquire(&rcu_state.srs_done_tail);
> >> -    if (!done)
> >> +    if (!done) {
> >> +        /* See comments below. */
> >> +        atomic_dec_return_release(&rcu_state.srs_cleanups_pending);
> >>        return;
> >> +    }
> >> 
> >>    WARN_ON_ONCE(!rcu_sr_is_wait_head(done));
> >>    head = done->next;
> >> @@ -1666,6 +1670,9 @@ static void rcu_sr_normal_gp_cleanup_work(struct work_struct *work)
> >> 
> >>        rcu_sr_put_wait_head(rcu);
> >>    }
> >> +
> >> +    /* Order list manipulations with atomic access. */
> >> +    atomic_dec_return_release(&rcu_state.srs_cleanups_pending);
> >> }
> >> 
> >> /*
> >> @@ -1673,7 +1680,7 @@ static void rcu_sr_normal_gp_cleanup_work(struct work_struct *work)
> >>  */
> >> static void rcu_sr_normal_gp_cleanup(void)
> >> {
> >> -    struct llist_node *wait_tail, *next, *rcu;
> >> +    struct llist_node *wait_tail, *next = NULL, *rcu = NULL;
> >>    int done = 0;
> >> 
> >>    wait_tail = rcu_state.srs_wait_tail;
> >> @@ -1699,16 +1706,35 @@ static void rcu_sr_normal_gp_cleanup(void)
> >>            break;
> >>    }
> >> 
> >> -    // concurrent sr_normal_gp_cleanup work might observe this update.
> >> -    smp_store_release(&rcu_state.srs_done_tail, wait_tail);
> >> +    /*
> >> +     * Fast path, no more users to process. Remove the last wait head
> >> +     * if no inflight-workers. If there are in-flight workers, let them
> >> +     * remove the last wait head.
> >> +     */
> >> +    WARN_ON_ONCE(!rcu);
> >> 
> > This assumption is not correct. An "rcu" can be NULL in fact.
> 
> Hmm I could never trigger that. Are you saying that is true after Neeraj recent patch or something else?
> Note, after Neeraj patch to handle the lack of heads availability, it could be true so I requested
> him to rebase his patch on top of this one.
> 
> However I will revisit my patch and look for if it could occur but please let me know if you knew of a sequence of events to make it NULL.
> > 
I think we should agree on your patch first otherwise it becomes a bit
messy or go with a Neeraj as first step and then work on youth. So, i
reviewed this patch based on latest Paul's dev branch. I see that Neeraj
needs further work.

So this is true without Neeraj patch. Consider the following case:

3     2     1     0
wh -> cb -> cb -> cb -> NULL

we start to process from 2 and handle all clients, in the end,
an "rcu" points to NULL and trigger the WARN_ON_ONCE. I see the
splat during the boot:

<snip>
[    0.927699][   T16] ------------[ cut here ]------------
[    0.930867][   T16] WARNING: CPU: 0 PID: 16 at kernel/rcu/tree.c:1721 rcu_gp_cleanup+0x37b/0x4a0
[    0.930490][    T1] acpiphp: ACPI Hot Plug PCI Controller Driver version: 0.5
[    0.931401][   T16] Modules linked in:
[    0.932400][    T1] PCI: Using configuration type 1 for base access
[    0.932771][   T16]
[    0.932773][   T16] CPU: 0 PID: 16 Comm: rcu_sched Not tainted 6.8.0-rc2-00089-g65ae0a6b86f0-dirty #1156
[    0.937780][   T16] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[    0.939402][   T16] RIP: 0010:rcu_gp_cleanup+0x37b/0x4a0
[    0.940636][   T16] Code: b0 4b bd 72 09 48 81 ff e8 b0 4b bd 76 1e 4c 8b 27 48 83 c7 10 e8 a5 8e fb ff 4c 89 23 83 ed 01 74 0a 4c 89 e7 48 85 ff 75 d2 <0f> 0b 48 8b 35 14 d0 fd 02 48 89 1d 8d 64 d0 01 48 83 c4 08 48 c7
[    0.942402][   T16] RSP: 0018:ffff9b4a8008fe88 EFLAGS: 00010246
[    0.943648][   T16] RAX: 0000000000000000 RBX: ffffffffbd4bb0a8 RCX: 6c9b26c9b26c9b27
[    0.944751][   T16] RDX: 0000000000000000 RSI: 00000000374b92b6 RDI: 0000000000000000
[    0.945757][   T16] RBP: 0000000000000004 R08: fffffffffff54ea1 R09: 0000000000000000
[    0.946753][   T16] R10: ffff89070098c278 R11: 0000000000000001 R12: 0000000000000000
[    0.947752][   T16] R13: fffffffffffffcbc R14: 0000000000000000 R15: ffffffffbd3f1300
[    0.948764][   T16] FS:  0000000000000000(0000) GS:ffff8915efe00000(0000) knlGS:0000000000000000
[    0.950403][   T16] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.951656][   T16] CR2: ffff89163ffff000 CR3: 00000002eae26000 CR4: 00000000000006f0
[    0.952755][   T16] Call Trace:
[    0.953597][   T16]  <TASK>
[    0.955404][   T16]  ? __warn+0x80/0x140
[    0.956608][   T16]  ? rcu_gp_cleanup+0x37b/0x4a0
[    0.957621][   T16]  ? report_bug+0x15d/0x180
[    0.959403][   T16]  ? handle_bug+0x3c/0x70
[    0.960616][   T16]  ? exc_invalid_op+0x17/0x70
[    0.961620][   T16]  ? asm_exc_invalid_op+0x1a/0x20
[    0.962627][   T16]  ? rcu_gp_cleanup+0x37b/0x4a0
[    0.963622][   T16]  ? rcu_gp_cleanup+0x36b/0x4a0
[    0.965403][   T16]  ? __pfx_rcu_gp_kthread+0x10/0x10
[    0.967402][   T16]  rcu_gp_kthread+0xf7/0x180
[    0.968619][   T16]  kthread+0xd3/0x100
[    0.969602][   T16]  ? __pfx_kthread+0x10/0x10
[    0.971402][   T16]  ret_from_fork+0x34/0x50
[    0.972613][   T16]  ? __pfx_kthread+0x10/0x10
[    0.973615][   T16]  ret_from_fork_asm+0x1b/0x30
[    0.974624][   T16]  </TASK>
[    0.975587][   T16] ---[ end trace 0000000000000000 ]---
<snip>

--
Uladzislau Rezki