Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them

[Byungchul Park]

On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> On Fri, Jun 01, 2018 at 11:03:09AM +0900, Byungchul Park wrote:
>> Currently, the range of jiffies_till_{first,next}_fqs are checked and
>> adjusted on and on in the loop of rcu_gp_kthread on runtime.
>>
>> However, it's enough to check them only when setting them, not every
>> time in the loop. So make them handled on a setting time via sysfs.
>>
>> Signed-off-by: Byungchul Park <byungchul.park@xxxxxxx>
>> ---
>>  kernel/rcu/tree.c | 45 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> index 4e96761..eb54d7d 100644
>> --- a/kernel/rcu/tree.c
>> +++ b/kernel/rcu/tree.c
>> @@ -518,8 +518,38 @@ void rcu_all_qs(void)
>>  static ulong jiffies_till_next_fqs = ULONG_MAX;
>>  static bool rcu_kick_kthreads;
>>
>> -module_param(jiffies_till_first_fqs, ulong, 0644);
>> -module_param(jiffies_till_next_fqs, ulong, 0644);
>> +static int param_set_first_fqs_jiffies(const char *val, const struct kernel_param *kp)
>> +{
>> +     ulong j;
>> +     int ret = kstrtoul(val, 0, &j);
>> +
>> +     if (!ret)
>> +             WRITE_ONCE(*(ulong *)kp->arg, (j > HZ) ? HZ : j);
>> +     return ret;
>> +}
>> +
>> +static int param_set_next_fqs_jiffies(const char *val, const struct kernel_param *kp)
>> +{
>> +     ulong j;
>> +     int ret = kstrtoul(val, 0, &j);
>> +
>> +     if (!ret)
>> +             WRITE_ONCE(*(ulong *)kp->arg, (j > HZ) ? HZ : (j ?: 1));
>> +     return ret;
>> +}
>
> Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
>
> Also, can we not combine the 2 param_set_ handlers as well?
>
> Only thing we would be giving up is that jiffies_till_first_fqs = 0 wouldn't
> be allowed (if we go with the param_set_next handler to be the common one)
> but don't think that's a useful/valid usecase since jiffies_till_first_fqs is
> set to a sane non-0 value anyway at boot up because of rcu_init_geometry
> anyway.. Thoughts?

Excuse me. Which code in rcu_init_geometry() makes jiffies_till_first_fqs
be a non-zero in case called with jiffies_till_first_fqs == 0?

Furthermore, what if we want to change the value through sysfs to zero
on runtime?

> If you agree, the below patch could be applied on top of rcu/dev (tested on
> rcu/dev), it saves another 20 lines.
>
> thanks,
>
> - Joel
>
> ---8<-----------------------
>
> From: "Joel Fernandes (Google)" <joel@xxxxxxxxxxxxxxxxx>
> Date: Sat, 2 Jun 2018 20:47:06 -0700
> Subject: [PATCH] rcu: Use common handler for setting
>  jiffies_till_{first,next}_fqs
>
> Recently the checking of jiffies_till_{first,next}_fqs during forcing of
> quiescent states was changed to be done whenever the parameters are set.
>
> Looking at how jiffies_till_first_fqs is used on my system, I noticed a
> value of 0 for it doesn't make much sense and is infact set to a non-0
> value at boot up. In this case, we can combine the module_param handlers
> for setting both these and keep code size small. This patch attempts it.
>
> Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> ---
>  kernel/rcu/tree.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index deb2508be923..6550040f8d46 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -498,17 +498,7 @@ static ulong jiffies_till_first_fqs = ULONG_MAX;
>  static ulong jiffies_till_next_fqs = ULONG_MAX;
>  static bool rcu_kick_kthreads;
>
> -static int param_set_first_fqs_jiffies(const char *val, const struct kernel_param *kp)
> -{
> -       ulong j;
> -       int ret = kstrtoul(val, 0, &j);
> -
> -       if (!ret)
> -               WRITE_ONCE(*(ulong *)kp->arg, (j > HZ) ? HZ : j);
> -       return ret;
> -}
> -
> -static int param_set_next_fqs_jiffies(const char *val, const struct kernel_param *kp)
> +static int param_set_fqs_jiffies(const char *val, const struct kernel_param *kp)
>  {
>         ulong j;
>         int ret = kstrtoul(val, 0, &j);
> @@ -518,18 +508,13 @@ static int param_set_next_fqs_jiffies(const char *val, const struct kernel_param
>         return ret;
>  }
>
> -static struct kernel_param_ops first_fqs_jiffies_ops = {
> -       .set = param_set_first_fqs_jiffies,
> -       .get = param_get_ulong,
> -};
> -
> -static struct kernel_param_ops next_fqs_jiffies_ops = {
> -       .set = param_set_next_fqs_jiffies,
> +static struct kernel_param_ops fqs_jiffies_ops = {
> +       .set = param_set_fqs_jiffies,
>         .get = param_get_ulong,
>  };
>
> -module_param_cb(jiffies_till_first_fqs, &first_fqs_jiffies_ops, &jiffies_till_first_fqs, 0644);
> -module_param_cb(jiffies_till_next_fqs, &next_fqs_jiffies_ops, &jiffies_till_next_fqs, 0644);
> +module_param_cb(jiffies_till_first_fqs, &fqs_jiffies_ops, &jiffies_till_first_fqs, 0644);
> +module_param_cb(jiffies_till_next_fqs, &fqs_jiffies_ops, &jiffies_till_next_fqs, 0644);
>  module_param(rcu_kick_kthreads, bool, 0644);
>
>  /*
> --
> 2.17.1.1185.g55be947832-goog
>



-- 
Thanks,
Byungchul