Re: [PATCH] rcu/tree: Default jiffies_to_sched_qs to jiffies_till_sched_qs

[Joel Fernandes <joel@joelfernandes.org>]

On Mon, Mar 11, 2019 at 03:47:42PM -0700, Paul E. McKenney wrote:
> On Mon, Mar 11, 2019 at 01:35:40PM -0400, Joel Fernandes wrote:
> > On Mon, Mar 11, 2019 at 08:19:47AM -0700, Paul E. McKenney wrote:
> > > This time keeping the CC list...
> > > 
> > > 							Thanx, Paul
> > > 
> > > On Mon, Mar 11, 2019 at 08:18:54AM -0700, Paul E. McKenney wrote:
> > > > On Mon, Mar 11, 2019 at 03:16:11PM +0530, Neeraj Upadhyay wrote:
> > > > > Current code does not call adjust_jiffies_till_sched_qs(),
> > > > > if jiffies_till_sched_qs is specified. For the case, where
> > > > > jiffies_till_first_fqs and jiffies_till_next_fqs are default,
> > > > > jiffies_to_sched_qs won't be a correct adjustment of
> > > > > jiffies_till_sched_qs.
> > > > > 
> > > > > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> > > > 
> > > > Good catch!  Queued and pushed.  Please see below for updated
> > > > commit log.  On future patches, could you please first describe
> > > > the problem and consequences, then what the fix is?  This approach
> > > > makes it much easier for people later on who will be trying to
> > > > figure out what is going on, and who might or might not have much
> > > > understanding of RCU.  (For example, they might be doing a bisection
> > > > or some such.)
> > > > 
> > > > Not a big deal, as I can touch this up, but a good habit to get into.
> > > > 
> > > > And no, rcutorture currently does not specify non-default values
> > > > for jiffies_till_sched_qs.  Which should probably be fixed.  I could
> > > > make rcu_implicit_dynticks_qs() complain if jiffies_to_sched_qs is
> > > > zero, but that feels a bit hacky and specific.  :-/
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > ------------------------------------------------------------------------
> > > > 
> > > > commit ee474b85fa0815be940ed89a91e0d84a110a0a92
> > > > Author: Neeraj Upadhyay <neeraju@codeaurora.org>
> > > > Date:   Mon Mar 11 15:16:11 2019 +0530
> > > > 
> > > >     rcu: Default jiffies_to_sched_qs to jiffies_till_sched_qs
> > > >     
> > > >     The current code only calls adjust_jiffies_till_sched_qs() if
> > > >     jiffies_till_sched_qs is left at its default value, so when the
> > > >     jiffies_till_sched_qs kernel-boot parameter actually is specified,
> > > >     jiffies_to_sched_qs will be left with the value zero, which
> > > >     will result in useless slowdowns of cond_resched().  This commit
> > > >     therefore changes rcu_init_geometry() to unconditionally invoke
> > > >     adjust_jiffies_till_sched_qs(), which ensures that jiffies_to_sched_qs
> > > >     will be initialized in all cases, thus maintaining good cond_resched()
> > > >     performance.
> > > >     
> > > >     Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
> > > >     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index ddd5c74e386b..10aeb89395ea 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3259,8 +3259,7 @@ static void __init rcu_init_geometry(void)
> > > >  		jiffies_till_first_fqs = d;
> > > >  	if (jiffies_till_next_fqs == ULONG_MAX)
> > > >  		jiffies_till_next_fqs = d;
> > > > -	if (jiffies_till_sched_qs == ULONG_MAX)
> > > > -		adjust_jiffies_till_sched_qs();
> > > > +	adjust_jiffies_till_sched_qs();
> > > >  
> > > >  	/* If the compile-time values are accurate, just leave. */
> > > >  	if (rcu_fanout_leaf == RCU_FANOUT_LEAF &&
> > > 
> > 
> > Makes sense to me.
> > 
> > Also the comment here needs an update too I think:
> > 
> > static ulong jiffies_to_sched_qs; /* Adjusted version of above if not default */
> > 
> > Seems to me, after your patch jiffies_to_sched_qs will always be an adjusted
> > value of some sort, unless jiffies_till_sched_qs is specified.
> > 
> > Comment should be some thing like this then?
> > 
> > /* Either the above, or an adjusted default version based on
> >  * jiffies_till_{first,next}_fqs if it is not specified */
> 
> Good point, but how about the patch below?

Yes, your comment is better :-) thanks,

 - Joel


> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit b1d5aaf8eff7872a63531f35aa0490f2fc8118d6
> Author: Paul E. McKenney <paulmck@linux.ibm.com>
> Date:   Mon Mar 11 15:45:13 2019 -0700
> 
>     rcu: Update jiffies_to_sched_qs and adjust_jiffies_till_sched_qs() comments
>     
>     This commit better documents the jiffies_to_sched_qs default-value
>     strategy used by adjust_jiffies_till_sched_qs()
>     
>     Reported-by: Joel Fernandes <joel@joelfernandes.org>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 10aeb89395ea..355775a82581 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -400,7 +400,7 @@ static bool rcu_kick_kthreads;
>   */
>  static ulong jiffies_till_sched_qs = ULONG_MAX;
>  module_param(jiffies_till_sched_qs, ulong, 0444);
> -static ulong jiffies_to_sched_qs; /* Adjusted version of above if not default */
> +static ulong jiffies_to_sched_qs; /* See adjust_jiffies_till_sched_qs(). */
>  module_param(jiffies_to_sched_qs, ulong, 0444); /* Display only! */
>  
>  /*
> @@ -418,6 +418,7 @@ static void adjust_jiffies_till_sched_qs(void)
>  		WRITE_ONCE(jiffies_to_sched_qs, jiffies_till_sched_qs);
>  		return;
>  	}
> +	/* Otherwise, set to third fqs scan, but bound below on large system. */
>  	j = READ_ONCE(jiffies_till_first_fqs) +
>  		      2 * READ_ONCE(jiffies_till_next_fqs);
>  	if (j < HZ / 10 + nr_cpu_ids / RCU_JIFFIES_FQS_DIV)
>