Re: [PATCH tip/core/rcu 4/4] rcu: Make rcutorture's shuffler task shuffle recently added tasks

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Sun, Jan 27, 2013 at 09:47:47PM +1100, Josh Triplett wrote:
> On Sat, Jan 26, 2013 at 04:05:20PM -0800, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > A number of kthreads have been added to rcutorture, but the shuffler
> > task was not informed of them, and thus did not shuffle them.  This
> > commit therefore adds the requisite shuffling.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> This also makes an unrelated semantic change, and several unrelated
> whitespace changes.
> 
> > ---
> >  kernel/rcutorture.c |   24 ++++++++++++++++++++----
> >  1 files changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/rcutorture.c b/kernel/rcutorture.c
> > index a583f1c..3ebc8bf 100644
> > --- a/kernel/rcutorture.c
> > +++ b/kernel/rcutorture.c
> > @@ -846,7 +846,7 @@ static int rcu_torture_boost(void *arg)
> >  		/* Wait for the next test interval. */
> >  		oldstarttime = boost_starttime;
> >  		while (ULONG_CMP_LT(jiffies, oldstarttime)) {
> > -			schedule_timeout_uninterruptible(1);
> > +			schedule_timeout_interruptible(oldstarttime - jiffies);
> 
> This change doesn't seem related, and the commit message doesn't explain
> it either.  Could you split it out into a separate commit and document
> the rationale, please?

Ah, it is related because the shuffling is trying to keep CPUs idle,
which means that we don't want all the boost kthreads waking up every
CPU every jiffy.  I agree that this is not obvious, so I have updated
the commit message to call this out.

> >  			rcu_stutter_wait("rcu_torture_boost");
> >  			if (kthread_should_stop() ||
> >  			    fullstop != FULLSTOP_DONTSTOP)
> > @@ -1318,19 +1318,35 @@ static void rcu_torture_shuffle_tasks(void)
> >  				set_cpus_allowed_ptr(reader_tasks[i],
> >  						     shuffle_tmp_mask);
> >  	}
> > -
> >  	if (fakewriter_tasks) {
> >  		for (i = 0; i < nfakewriters; i++)
> >  			if (fakewriter_tasks[i])
> >  				set_cpus_allowed_ptr(fakewriter_tasks[i],
> >  						     shuffle_tmp_mask);
> >  	}
> > -
> >  	if (writer_task)
> >  		set_cpus_allowed_ptr(writer_task, shuffle_tmp_mask);
> > -
> 
> These three whitespace changes seem unrelated as well.

Some cleanup while in the neighborhood.  ;-)

I added this to the commit message as well.

							Thanx, Paul

> >  	if (stats_task)
> >  		set_cpus_allowed_ptr(stats_task, shuffle_tmp_mask);
> > +	if (stutter_task)
> > +		set_cpus_allowed_ptr(stutter_task, shuffle_tmp_mask);
> > +	if (fqs_task)
> > +		set_cpus_allowed_ptr(fqs_task, shuffle_tmp_mask);
> > +	if (shutdown_task)
> > +		set_cpus_allowed_ptr(shutdown_task, shuffle_tmp_mask);
> > +#ifdef CONFIG_HOTPLUG_CPU
> > +	if (onoff_task)
> > +		set_cpus_allowed_ptr(onoff_task, shuffle_tmp_mask);
> > +#endif /* #ifdef CONFIG_HOTPLUG_CPU */
> > +	if (stall_task)
> > +		set_cpus_allowed_ptr(stall_task, shuffle_tmp_mask);
> > +	if (barrier_cbs_tasks)
> > +		for (i = 0; i < n_barrier_cbs; i++)
> > +			if (barrier_cbs_tasks[i])
> > +				set_cpus_allowed_ptr(barrier_cbs_tasks[i],
> > +						     shuffle_tmp_mask);
> > +	if (barrier_task)
> > +		set_cpus_allowed_ptr(barrier_task, shuffle_tmp_mask);
> 
> The rest of this seems fine.
> 
> - Josh Triplett
>