Received-SPF: pass (google.com: domain of linux-kernel+bounces-106666-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: Joel Fernandes <joel@joelfernandes.org>
Precedence: bulk
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v2 rcu/dev 1/2] rcu/tree: Reduce wake up for synchronize_rcu() common case
Date: Mon, 18 Mar 2024 17:05:31 -0400
Message-Id: <404E28F5-B018-4CE7-BE57-D0362B0C9969@joelfernandes.org>
References: <ZfiOwtPfnxXW4JX3@pc636>
Cc: linux-kernel@vger.kernel.org, frederic@kernel.org, boqun.feng@gmail.com,
 neeraj.iitr10@gmail.com, rcu@vger.kernel.org, rostedt@goodmis.org,
 "Paul E. McKenney" <paulmck@kernel.org>,
 Neeraj Upadhyay <neeraj.upadhyay@kernel.org>,
 Josh Triplett <josh@joshtriplett.org>,
 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
 Lai Jiangshan <jiangshanlai@gmail.com>, Zqiang <qiang.zhang1211@gmail.com>
In-Reply-To: <ZfiOwtPfnxXW4JX3@pc636>
To: Uladzislau Rezki <urezki@gmail.com>


> On Mar 18, 2024, at 2:58=E2=80=AFPM, Uladzislau Rezki <urezki@gmail.com> w=
rote:
>=20
> =EF=BB=BFHello, Joel!
>=20
> Sorry for late checking, see below few comments:
>=20
>> 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.
>>=20
>> Introduce a new counter to track this and prevent the wakeup in the
>> common case.
>>=20
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> ---
>> Rebased on paul/dev of today.
>>=20
>> kernel/rcu/tree.c | 36 +++++++++++++++++++++++++++++++-----
>> kernel/rcu/tree.h |  1 +
>> 2 files changed, 32 insertions(+), 5 deletions(-)
>>=20
>> 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 =3D {
>>    .ofl_lock =3D __ARCH_SPIN_LOCK_UNLOCKED,
>>    .srs_cleanup_work =3D __WORK_INITIALIZER(rcu_state.srs_cleanup_work,
>>        rcu_sr_normal_gp_cleanup_work),
>> +    .srs_cleanups_pending =3D ATOMIC_INIT(0),
>> };
>>=20
>> /* Dump rcu_node combining tree at boot to verify correct setup. */
>> @@ -1642,8 +1643,11 @@ static void rcu_sr_normal_gp_cleanup_work(struct w=
ork_struct *work)
>>     * the done tail list manipulations are protected here.
>>     */
>>    done =3D 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;
>> +    }
>>=20
>>    WARN_ON_ONCE(!rcu_sr_is_wait_head(done));
>>    head =3D done->next;
>> @@ -1666,6 +1670,9 @@ static void rcu_sr_normal_gp_cleanup_work(struct wo=
rk_struct *work)
>>=20
>>        rcu_sr_put_wait_head(rcu);
>>    }
>> +
>> +    /* Order list manipulations with atomic access. */
>> +    atomic_dec_return_release(&rcu_state.srs_cleanups_pending);
>> }
>>=20
>> /*
>> @@ -1673,7 +1680,7 @@ static void rcu_sr_normal_gp_cleanup_work(struct wo=
rk_struct *work)
>>  */
>> static void rcu_sr_normal_gp_cleanup(void)
>> {
>> -    struct llist_node *wait_tail, *next, *rcu;
>> +    struct llist_node *wait_tail, *next =3D NULL, *rcu =3D NULL;
>>    int done =3D 0;
>>=20
>>    wait_tail =3D rcu_state.srs_wait_tail;
>> @@ -1699,16 +1706,35 @@ static void rcu_sr_normal_gp_cleanup(void)
>>            break;
>>    }
>>=20
>> -    // 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);
>>=20
> 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 rec=
ent patch or something else? Note, after Neeraj patch to handle the lack of h=
eads availability, it could be true so I requested him to rebase his patch o=
n top of this one.

However I will revisit my patch and look for if it could occur but please le=
t me know if you knew of a sequence of events to make it NULL.
>=20
>>    ASSERT_EXCLUSIVE_WRITER(rcu_state.srs_done_tail);
>>=20
>> +    if (rcu && rcu_sr_is_wait_head(rcu) && rcu->next =3D=3D NULL &&
>> +        /* Order atomic access with list manipulation. */
>> +        !atomic_read_acquire(&rcu_state.srs_cleanups_pending)) {
>> +        wait_tail->next =3D NULL;
>> +        rcu_sr_put_wait_head(rcu);
>> +        smp_store_release(&rcu_state.srs_done_tail, wait_tail);
>> +        return;
>> +    }
>> +
>> +    /* Concurrent sr_normal_gp_cleanup work might observe this update. *=
/
>> +    smp_store_release(&rcu_state.srs_done_tail, wait_tail);
>> +
>>    /*
>>     * We schedule a work in order to perform a final processing
>>     * of outstanding users(if still left) and releasing wait-heads
>>     * added by rcu_sr_normal_gp_init() call.
>>     */
>> -    queue_work(sync_wq, &rcu_state.srs_cleanup_work);
>> +    atomic_inc(&rcu_state.srs_cleanups_pending);
>> +    if (!queue_work(sync_wq, &rcu_state.srs_cleanup_work)) {
>> +        atomic_dec(&rcu_state.srs_cleanups_pending);
>> +    }
>> }
> No need an extra "{}" pair.

I do prefer it for readability but I am ok with dropping it.

Thanks!

 - Joel=20


>=20
>>=20
>> /*
>> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
>> index bae7925c497f..affcb92a358c 100644
>> --- a/kernel/rcu/tree.h
>> +++ b/kernel/rcu/tree.h
>> @@ -420,6 +420,7 @@ struct rcu_state {
>>    struct llist_node *srs_done_tail; /* ready for GP users. */
>>    struct sr_wait_node srs_wait_nodes[SR_NORMAL_GP_WAIT_HEAD_MAX];
>>    struct work_struct srs_cleanup_work;
>> +    atomic_t srs_cleanups_pending; /* srs inflight worker cleanups. */
>> };
>>=20
>> /* Values for rcu_state structure's gp_flags field. */
>> --
>> 2.34.1
>>=20