Re: [PATCH v3] rcu: Further comment and explain the state space of GP sequences

[Joel Fernandes]

> On Jan 23, 2023, at 7:26 AM, Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
> 
> On Thu, Jan 19, 2023 at 05:06:27PM +0000, Joel Fernandes wrote:
>>> On Thu, Jan 19, 2023 at 2:15 PM Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
>>> 
>>> 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.
>>>> 
>>>> Signed-off-by: Frederic Weisbecker <frederic@xxxxxxxxxx>
>>>> ---
>>>> kernel/rcu/rcu.h | 33 +++++++++++++++++++++++++++++++++
>>>> 1 file changed, 33 insertions(+)
>>>> 
>>>> 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 @@
>>>> 
>>>> /*
>>>>  * 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 progress. When
>>>> + * the grace period completes, the control flags are reset to 0 and the 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 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 inactive readers
>>>> + *                                                           index.
>> 
>> 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.
> 
> 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.
> 
> 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@xxxxxxxxxxxxxxxxx>

Thanks,

 - Joel



> 
> ---
> From: Frederic Weisbecker <frederic@xxxxxxxxxx>
> Date: Thu, 19 Jan 2023 14:29:34 +0100
> Subject: [PATCH] rcu: Further comment and explain the state space of GP
> sequences
> 
> 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.
> 
> Signed-off-by: Frederic Weisbecker <frederic@xxxxxxxxxx>
> ---
> kernel/rcu/rcu.h | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
> 
> 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 @@
> 
> /*
>  * 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 progress. When
> + * the grace period completes, the control flags are reset to 0 and the 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 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 GP
> + *                    has completed. It's an absolute value
> + *                    covering both the state and the counter of
> + *                    the GP sequence.
>  */
> 
> #define RCU_SEQ_CTR_SHIFT    2
> -- 
> 2.34.1
>