LinuxLists.cc - [PATCH] rcu: Check the range of jiffies_till_{first,next}

2018-06-01 02:05:22

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

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 <[email protected]>
---
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;
+}
+
+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,
+ .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(rcu_kick_kthreads, bool, 0644);

/*
@@ -2129,10 +2159,6 @@ static int __noreturn rcu_gp_kthread(void *arg)
/* Handle quiescent-state forcing. */
first_gp_fqs = true;
j = jiffies_till_first_fqs;
- if (j > HZ) {
- j = HZ;
- jiffies_till_first_fqs = HZ;
- }
ret = 0;
for (;;) {
if (!ret) {
@@ -2167,13 +2193,6 @@ static int __noreturn rcu_gp_kthread(void *arg)
WRITE_ONCE(rsp->gp_activity, jiffies);
ret = 0; /* Force full wait till next FQS. */
j = jiffies_till_next_fqs;
- if (j > HZ) {
- j = HZ;
- jiffies_till_next_fqs = HZ;
- } else if (j < 1) {
- j = 1;
- jiffies_till_next_fqs = 1;
- }
} else {
/* Deal with stray signal. */
cond_resched_tasks_rcu_qs();
--
1.9.1

2018-06-02 11:47:54

by Paul E. McKenney

[permalink] [raw]

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

2018-06-03 04:00:46

by Joel Fernandes

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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) <[email protected]>

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?

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)" <[email protected]>
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) <[email protected]>
---
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

2018-06-03 05:39:09

by Byungchul Park

[permalink] [raw]

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

On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <[email protected]> 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 <[email protected]>
>> ---
>> 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) <[email protected]>
>
> 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)" <[email protected]>
> 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) <[email protected]>
> ---
> 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

2018-06-03 06:25:40

by Joel Fernandes

[permalink] [raw]

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

On Sun, Jun 03, 2018 at 02:38:04PM +0900, Byungchul Park wrote:
> On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <[email protected]> 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 <[email protected]>
> >> ---
> >> 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) <[email protected]>
> >
> > 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?

What do you mean? I think you misunderstood. I didn't say value of 0 is being
handled at boot up. What I said is its initialized to something sane that's
non-zero:

If you see, jiffies_till_first_fqs is assigned to ULONG_MAX at compile time:
static ulong jiffies_till_first_fqs = ULONG_MAX;

Then in rcu_init_geometry, we have:
d = RCU_JIFFIES_TILL_FORCE_QS + nr_cpu_ids / RCU_JIFFIES_FQS_DIV;
if (jiffies_till_first_fqs == ULONG_MAX)
jiffies_till_first_fqs = d;

On my system, jiffies_till_first_fqs is assigned to 3 because of this at
boot.

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

My patch was just a suggestion. I didn't know if anyone would want to force
it to 0 or not and was wondering what the value in doing so was at the cost
of adding one more function to handle it. It basically says if you set to 0,
that you never want to wait for a timeout before forcing a qs for the first
time? Then why are we calling swait_event_idle_timeout anyway? Wouldn't a
saner timeout be something not zero?

thanks,

- Joel

2018-06-03 07:52:52

by Byungchul Park

[permalink] [raw]

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

On Sun, Jun 3, 2018 at 3:23 PM, Joel Fernandes <[email protected]> wrote:
> On Sun, Jun 03, 2018 at 02:38:04PM +0900, Byungchul Park wrote:
>> On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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) <[email protected]>
>> >
>> > 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?
>
> What do you mean? I think you misunderstood. I didn't say value of 0 is being
> handled at boot up. What I said is its initialized to something sane that's
> non-zero:
>
> If you see, jiffies_till_first_fqs is assigned to ULONG_MAX at compile time:
> static ulong jiffies_till_first_fqs = ULONG_MAX;
>
> Then in rcu_init_geometry, we have:
> d = RCU_JIFFIES_TILL_FORCE_QS + nr_cpu_ids / RCU_JIFFIES_FQS_DIV;
> if (jiffies_till_first_fqs == ULONG_MAX)
> jiffies_till_first_fqs = d;
>
> On my system, jiffies_till_first_fqs is assigned to 3 because of this at
> boot.
>
>> Furthermore, what if we want to change the value through sysfs to zero
>> on runtime?
>
> My patch was just a suggestion. I didn't know if anyone would want to force
> it to 0 or not and was wondering what the value in doing so was at the cost
> of adding one more function to handle it. It basically says if you set to 0,
> that you never want to wait for a timeout before forcing a qs for the first
> time? Then why are we calling swait_event_idle_timeout anyway? Wouldn't a
> saner timeout be something not zero?

I'm sorry I tried but don't understand your point :(

Did you happen to read the following?

https://lkml.org/lkml/2018/5/29/99

If yes, it would be appreciated if you let me know what I'm missing.

--
Thanks,
Byungchul

2018-06-03 18:47:49

by Joel Fernandes

[permalink] [raw]

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

On Sun, Jun 03, 2018 at 04:51:06PM +0900, Byungchul Park wrote:
> On Sun, Jun 3, 2018 at 3:23 PM, Joel Fernandes <[email protected]> wrote:
> > On Sun, Jun 03, 2018 at 02:38:04PM +0900, Byungchul Park wrote:
> >> On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <[email protected]> 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 <[email protected]>
> >> >> ---
> >> >> 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) <[email protected]>
> >> >
> >> > 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?
> >
> > What do you mean? I think you misunderstood. I didn't say value of 0 is being
> > handled at boot up. What I said is its initialized to something sane that's
> > non-zero:
> >
> > If you see, jiffies_till_first_fqs is assigned to ULONG_MAX at compile time:
> > static ulong jiffies_till_first_fqs = ULONG_MAX;
> >
> > Then in rcu_init_geometry, we have:
> > d = RCU_JIFFIES_TILL_FORCE_QS + nr_cpu_ids / RCU_JIFFIES_FQS_DIV;
> > if (jiffies_till_first_fqs == ULONG_MAX)
> > jiffies_till_first_fqs = d;
> >
> > On my system, jiffies_till_first_fqs is assigned to 3 because of this at
> > boot.
> >
> >> Furthermore, what if we want to change the value through sysfs to zero
> >> on runtime?
> >
> > My patch was just a suggestion. I didn't know if anyone would want to force
> > it to 0 or not and was wondering what the value in doing so was at the cost
> > of adding one more function to handle it. It basically says if you set to 0,
> > that you never want to wait for a timeout before forcing a qs for the first
> > time? Then why are we calling swait_event_idle_timeout anyway? Wouldn't a
> > saner timeout be something not zero?
>
> I'm sorry I tried but don't understand your point :(
> Did you happen to read the following?
>
> https://lkml.org/lkml/2018/5/29/99

No I did not happen to read that and I am very sorry about that. I see Paul
said jiffies_till_first_fqs = 0 is a useful case in that thread, to record
the idle CPUs quickly, which makes sense.

> If yes, it would be appreciated if you let me know what I'm missing.

You are right and not missing anything. I'll be more careful to go through
old threads next time..

Glad to learn something from the discussions. thanks,

BTW one more dumb question - if jiffies_till_first_fqs == 0 is so useful,
then why not default initialize it to 0? I'd think most systems expect
atleast some of the CPUs to idle.

- Joel

2018-06-04 00:31:20

by Byungchul Park

[permalink] [raw]

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

On 2018-06-04 03:47, Joel Fernandes wrote:
> On Sun, Jun 03, 2018 at 04:51:06PM +0900, Byungchul Park wrote:
>> On Sun, Jun 3, 2018 at 3:23 PM, Joel Fernandes <[email protected]> wrote:
>>> On Sun, Jun 03, 2018 at 02:38:04PM +0900, Byungchul Park wrote:
>>>> On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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) <[email protected]>
>>>>>
>>>>> 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?
>>>
>>> What do you mean? I think you misunderstood. I didn't say value of 0 is being
>>> handled at boot up. What I said is its initialized to something sane that's
>>> non-zero:
>>>
>>> If you see, jiffies_till_first_fqs is assigned to ULONG_MAX at compile time:
>>> static ulong jiffies_till_first_fqs = ULONG_MAX;
>>>
>>> Then in rcu_init_geometry, we have:
>>> d = RCU_JIFFIES_TILL_FORCE_QS + nr_cpu_ids / RCU_JIFFIES_FQS_DIV;
>>> if (jiffies_till_first_fqs == ULONG_MAX)
>>> jiffies_till_first_fqs = d;
>>>
>>> On my system, jiffies_till_first_fqs is assigned to 3 because of this at
>>> boot.
>>>
>>>> Furthermore, what if we want to change the value through sysfs to zero
>>>> on runtime?
>>>
>>> My patch was just a suggestion. I didn't know if anyone would want to force
>>> it to 0 or not and was wondering what the value in doing so was at the cost
>>> of adding one more function to handle it. It basically says if you set to 0,
>>> that you never want to wait for a timeout before forcing a qs for the first
>>> time? Then why are we calling swait_event_idle_timeout anyway? Wouldn't a
>>> saner timeout be something not zero?
>>
>> I'm sorry I tried but don't understand your point :(
>> Did you happen to read the following?
>>
>> https://lkml.org/lkml/2018/5/29/99
>
> No I did not happen to read that and I am very sorry about that. I see Paul
> said jiffies_till_first_fqs = 0 is a useful case in that thread, to record
> the idle CPUs quickly, which makes sense.
>
>> If yes, it would be appreciated if you let me know what I'm missing.
>
> You are right and not missing anything. I'll be more careful to go through
> old threads next time..
>
> Glad to learn something from the discussions. thanks,
>
> BTW one more dumb question - if jiffies_till_first_fqs == 0 is so useful,
> then why not default initialize it to 0? I'd think most systems expect
> atleast some of the CPUs to idle.

and some others to be busy tho :)

IMHO, although the case of jiffies_till_first_fqs == 0 is sometimes
useful, the other case is much more useful since we normally don't
want to add overhead by the forcing. The reason we don't force
quiescent states immediately but wait several jiffies by default is
because the overhead such as burning barrier, atomic ops and so on
is needed to force it.

>
> - Joel
>
>

--
Thanks,
Byungchul