Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Joel Fernandes <joel@joelfernandes.org>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v3] rcu: Further comment and explain the state space of GP sequences
Date:   Mon, 23 Jan 2023 09:07:43 -0500
Message-Id: <A6B13595-00FE-4DF4-A5DC-0B54DD0C4E80@joelfernandes.org>
References: <Y85868Pei9VutSiW@lothringen>
Cc:     "Paul E . McKenney" <paulmck@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, rcu <rcu@vger.kernel.org>,
        quic_neeraju@quicinc.com, Uladzislau Rezki <urezki@gmail.com>
In-Reply-To: <Y85868Pei9VutSiW@lothringen>
To:     Frederic Weisbecker <frederic@kernel.org>
Precedence: bulk


> On Jan 23, 2023, at 7:26 AM, Frederic Weisbecker <frederic@kernel.org> wro=
te:
>=20
> =EF=BB=BFOn Thu, Jan 19, 2023 at 05:06:27PM +0000, Joel Fernandes wrote:
>>> On Thu, Jan 19, 2023 at 2:15 PM Frederic Weisbecker <frederic@kernel.org=
> wrote:
>>>=20
>>> On Thu, Jan 19, 2023 at 03:11:35PM +0100, Frederic Weisbecker wrote:
>>>> The state space of the GP sequence number isn't documented and the
>>>> definitions of its special values are scattered. Try to gather some
>>>> common knowledge near the GP seq headers.
>>>>=20
>>>> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
>>>> ---
>>>> kernel/rcu/rcu.h | 33 +++++++++++++++++++++++++++++++++
>>>> 1 file changed, 33 insertions(+)
>>>>=20
>>>> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
>>>> index 115616ac3bfa..fb95de039596 100644
>>>> --- a/kernel/rcu/rcu.h
>>>> +++ b/kernel/rcu/rcu.h
>>>> @@ -14,6 +14,39 @@
>>>>=20
>>>> /*
>>>>  * Grace-period counter management.
>>>> + *
>>>> + * The two lowest significant bits gather the control flags.
>>>> + * The higher bits form the RCU sequence counter.
>>>> + *
>>>> + * About the control flags, a common value of 0 means that no GP is in=
 progress.
>>>> + * A value of 1 means that a grace period has started and is in progre=
ss. When
>>>> + * the grace period completes, the control flags are reset to 0 and th=
e sequence
>>>> + * counter is incremented.
>>>> + *
>>>> + * However some specific RCU usages make use of custom values.
>>>> + *
>>>> + * SRCU special control values:
>>>> + *
>>>> + *   SRCU_SNP_INIT_SEQ       :       Invalid/init value set when SRCU n=
ode
>>>> + *                                                   is initialized.
>>>> + *
>>>> + *   SRCU_STATE_IDLE         :       No SRCU gp is in progress
>>>> + *
>>>> + *   SRCU_STATE_SCAN1        :       State set by rcu_seq_start(). Ind=
icates
>>>> + *                                                           we are sc=
anning the inactive readers
>>>> + *                                                           index.
>>=20
>> The term "inactive reader" is confusing. The readers can very much be
>> active during scans. During a scan stage, there might be a reader on
>> any of the 2 indexes that can be right in the middle of their critical
>> section (and we don't know which index because they could have got
>> preempted, right after sampling idx). Maybe "inactive slot" is a
>> better term? And define "inactive slot" as the slot which is no longer
>> going to be sampled by new readers.
>=20
> That's why I used "inactive readers index".
> I guess I should have written "inactive readers' index" to disambiguate
> the fact that inactive refers to "index" and not "readers" but I almost
> never observe plural genitive written that way these days.
>=20
> As for the gory details of "inactive" being actually "bound to become
> inactive", I'm not sure that belongs here but here is what I can do:

Sounds good and the below change looks good to me.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks,

 - Joel


>=20
> ---
> From: Frederic Weisbecker <frederic@kernel.org>
> Date: Thu, 19 Jan 2023 14:29:34 +0100
> Subject: [PATCH] rcu: Further comment and explain the state space of GP
> sequences
>=20
> The state space of the GP sequence number isn't documented and the
> definitions of its special values are scattered. Try to gather some
> common knowledge near the GP seq headers.
>=20
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
> kernel/rcu/rcu.h | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>=20
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index 115616ac3bfa..5be983598b5a 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -14,6 +14,43 @@
>=20
> /*
>  * Grace-period counter management.
> + *
> + * The two lowest significant bits gather the control flags.
> + * The higher bits form the RCU sequence counter.
> + *
> + * About the control flags, a common value of 0 means that no GP is in pr=
ogress.
> + * A value of 1 means that a grace period has started and is in progress.=
 When
> + * the grace period completes, the control flags are reset to 0 and the s=
equence
> + * counter is incremented.
> + *
> + * However some specific RCU usages make use of custom values.
> + *
> + * SRCU special control values:
> + *
> + *    SRCU_SNP_INIT_SEQ    :    Invalid/init value set when SRCU node
> + *                    is initialized.
> + *
> + *    SRCU_STATE_IDLE        :    No SRCU gp is in progress
> + *
> + *    SRCU_STATE_SCAN1    :    State set by rcu_seq_start(). Indicates
> + *                    we are scanning the readers on the slot
> + *                    defined as inactive (though there might
> + *                    be pending readers there but their number
> + *                    is bound).
> + *
> + *    SRCU_STATE_SCAN2    :    State set manually via rcu_seq_set_state()=

> + *                    Indicates we are flipping the readers
> + *                    index and then scanning the readers on the
> + *                    slot newly set as inactive (again there
> + *                    might remain a bound amount of pending
> + *                    readers there).
> + *
> + * RCU polled GP special control value:
> + *
> + *    RCU_GET_STATE_COMPLETED :    State value indicating that a polled G=
P
> + *                    has completed. It's an absolute value
> + *                    covering both the state and the counter of
> + *                    the GP sequence.
>  */
>=20
> #define RCU_SEQ_CTR_SHIFT    2
> --=20
> 2.34.1
>=20