* [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them @ 2018-06-01 2:03 Byungchul Park 2018-06-02 11:45 ` Paul E. McKenney 2018-06-03 3:58 ` Joel Fernandes 0 siblings, 2 replies; 8+ messages in thread From: Byungchul Park @ 2018-06-01 2:03 UTC (permalink / raw) To: jiangshanlai, paulmck, josh, rostedt, mathieu.desnoyers Cc: linux-kernel, kernel-team, joel 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@lge.com> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them 2018-06-01 2:03 [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them Byungchul Park @ 2018-06-02 11:45 ` Paul E. McKenney 2018-06-03 3:58 ` Joel Fernandes 1 sibling, 0 replies; 8+ messages in thread From: Paul E. McKenney @ 2018-06-02 11:45 UTC (permalink / raw) To: Byungchul Park Cc: jiangshanlai, josh, rostedt, mathieu.desnoyers, linux-kernel, kernel-team, joel 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@lge.com> Applied for further review and testing, thank you! Thanx, Paul > --- > 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them 2018-06-01 2:03 [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them Byungchul Park 2018-06-02 11:45 ` Paul E. McKenney @ 2018-06-03 3:58 ` Joel Fernandes 2018-06-03 5:38 ` Byungchul Park 1 sibling, 1 reply; 8+ messages in thread From: Joel Fernandes @ 2018-06-03 3:58 UTC (permalink / raw) To: Byungchul Park Cc: jiangshanlai, paulmck, josh, rostedt, mathieu.desnoyers, linux-kernel, kernel-team, kernel-team 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@lge.com> > --- > 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@joelfernandes.org> 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)" <joel@joelfernandes.org> 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@joelfernandes.org> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them 2018-06-03 3:58 ` Joel Fernandes @ 2018-06-03 5:38 ` Byungchul Park 2018-06-03 6:23 ` Joel Fernandes 0 siblings, 1 reply; 8+ messages in thread From: Byungchul Park @ 2018-06-03 5:38 UTC (permalink / raw) To: Joel Fernandes Cc: Byungchul Park, jiangshanlai, Paul McKenney, josh, rostedt, Mathieu Desnoyers, linux-kernel, kernel-team, kernel-team On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <joel@joelfernandes.org> 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@lge.com> >> --- >> 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@joelfernandes.org> > > 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@joelfernandes.org> > 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@joelfernandes.org> > --- > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them 2018-06-03 5:38 ` Byungchul Park @ 2018-06-03 6:23 ` Joel Fernandes 2018-06-03 7:51 ` Byungchul Park 0 siblings, 1 reply; 8+ messages in thread From: Joel Fernandes @ 2018-06-03 6:23 UTC (permalink / raw) To: Byungchul Park Cc: Byungchul Park, jiangshanlai, Paul McKenney, josh, rostedt, Mathieu Desnoyers, linux-kernel, kernel-team, kernel-team On Sun, Jun 03, 2018 at 02:38:04PM +0900, Byungchul Park wrote: > On Sun, Jun 3, 2018 at 12:58 PM, Joel Fernandes <joel@joelfernandes.org> 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@lge.com> > >> --- > >> 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@joelfernandes.org> > > > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them 2018-06-03 6:23 ` Joel Fernandes @ 2018-06-03 7:51 ` Byungchul Park 2018-06-03 18:47 ` Joel Fernandes 0 siblings, 1 reply; 8+ messages in thread From: Byungchul Park @ 2018-06-03 7:51 UTC (permalink / raw) To: Joel Fernandes Cc: Byungchul Park, jiangshanlai, Paul McKenney, josh, rostedt, Mathieu Desnoyers, linux-kernel, kernel-team, kernel-team On Sun, Jun 3, 2018 at 3:23 PM, Joel Fernandes <joel@joelfernandes.org> 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 <joel@joelfernandes.org> 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@lge.com> >> >> --- >> >> 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@joelfernandes.org> >> > >> > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them 2018-06-03 7:51 ` Byungchul Park @ 2018-06-03 18:47 ` Joel Fernandes 2018-06-04 0:30 ` Byungchul Park 0 siblings, 1 reply; 8+ messages in thread From: Joel Fernandes @ 2018-06-03 18:47 UTC (permalink / raw) To: Byungchul Park Cc: Byungchul Park, jiangshanlai, Paul McKenney, josh, rostedt, Mathieu Desnoyers, linux-kernel, kernel-team, kernel-team On Sun, Jun 03, 2018 at 04:51:06PM +0900, Byungchul Park wrote: > On Sun, Jun 3, 2018 at 3:23 PM, Joel Fernandes <joel@joelfernandes.org> 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 <joel@joelfernandes.org> 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@lge.com> > >> >> --- > >> >> 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@joelfernandes.org> > >> > > >> > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them 2018-06-03 18:47 ` Joel Fernandes @ 2018-06-04 0:30 ` Byungchul Park 0 siblings, 0 replies; 8+ messages in thread From: Byungchul Park @ 2018-06-04 0:30 UTC (permalink / raw) To: Joel Fernandes, Byungchul Park Cc: jiangshanlai, Paul McKenney, josh, rostedt, Mathieu Desnoyers, linux-kernel, kernel-team, kernel-team 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 <joel@joelfernandes.org> 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 <joel@joelfernandes.org> 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@lge.com> >>>>>> --- >>>>>> 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@joelfernandes.org> >>>>> >>>>> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-04 0:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-01 2:03 [PATCH] rcu: Check the range of jiffies_till_{first,next}_fqs when setting them Byungchul Park 2018-06-02 11:45 ` Paul E. McKenney 2018-06-03 3:58 ` Joel Fernandes 2018-06-03 5:38 ` Byungchul Park 2018-06-03 6:23 ` Joel Fernandes 2018-06-03 7:51 ` Byungchul Park 2018-06-03 18:47 ` Joel Fernandes 2018-06-04 0:30 ` Byungchul Park
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).