Re: [PATCH] sched: Make schedstats a runtime tunable that is disabled by default v2

[Mel Gorman <mgorman@techsingularity.net>]

On Thu, Jan 28, 2016 at 10:32:08AM +0000, Matt Fleming wrote:
> On Wed, 27 Jan, at 03:29:26PM, Mel Gorman wrote:
> > +#ifdef CONFIG_SCHEDSTATS
> > +void set_schedstats(bool enabled)
> > +{
> > +	if (enabled)
> > +		static_branch_enable(&sched_schedstats);
> > +	else
> > +		static_branch_disable(&sched_schedstats);
> > +}
> 
> This function should probably be 'static'; it has no users outside of
> this file.
> 

Yes.

> > @@ -313,17 +317,19 @@ do {									\
> >  #define P(n) SEQ_printf(m, "  .%-30s: %d\n", #n, rq->n);
> >  #define P64(n) SEQ_printf(m, "  .%-30s: %Ld\n", #n, rq->n);
> >  
> > -	P(yld_count);
> > +	if (schedstat_enabled()) {
> > +		P(yld_count);
> >  
> > -	P(sched_count);
> > -	P(sched_goidle);
> > +		P(sched_count);
> > +		P(sched_goidle);
> >  #ifdef CONFIG_SMP
> > -	P64(avg_idle);
> > -	P64(max_idle_balance_cost);
> > +		P64(avg_idle);
> > +		P64(max_idle_balance_cost);
> 
> These two fields are still updated without any kind of
> schedstat_enabled() guard. We probably shouldn't refuse to print them
> if we're maintaining these counters, right?
> 

Right.

> >  #undef P
> >  #undef P64
> > @@ -569,38 +575,38 @@ void proc_sched_show_task(struct task_struct *p, struct seq_file *m)
> >  	nr_switches = p->nvcsw + p->nivcsw;
> >  
> >  #ifdef CONFIG_SCHEDSTATS
> > -	PN(se.statistics.sum_sleep_runtime);
> > -	PN(se.statistics.wait_start);
> > -	PN(se.statistics.sleep_start);
> > -	PN(se.statistics.block_start);
> > -	PN(se.statistics.sleep_max);
> > -	PN(se.statistics.block_max);
> > -	PN(se.statistics.exec_max);
> > -	PN(se.statistics.slice_max);
> > -	PN(se.statistics.wait_max);
> > -	PN(se.statistics.wait_sum);
> > -	P(se.statistics.wait_count);
> > -	PN(se.statistics.iowait_sum);
> > -	P(se.statistics.iowait_count);
> > -	P(se.nr_migrations);
> > -	P(se.statistics.nr_migrations_cold);
> > -	P(se.statistics.nr_failed_migrations_affine);
> > -	P(se.statistics.nr_failed_migrations_running);
> > -	P(se.statistics.nr_failed_migrations_hot);
> > -	P(se.statistics.nr_forced_migrations);
> > -	P(se.statistics.nr_wakeups);
> > -	P(se.statistics.nr_wakeups_sync);
> > -	P(se.statistics.nr_wakeups_migrate);
> > -	P(se.statistics.nr_wakeups_local);
> > -	P(se.statistics.nr_wakeups_remote);
> > -	P(se.statistics.nr_wakeups_affine);
> > -	P(se.statistics.nr_wakeups_affine_attempts);
> > -	P(se.statistics.nr_wakeups_passive);
> > -	P(se.statistics.nr_wakeups_idle);
> > -
> > -	{
> > +	if (schedstat_enabled()) {
> >  		u64 avg_atom, avg_per_cpu;
> >  
> > +		PN(se.statistics.sum_sleep_runtime);
> > +		PN(se.statistics.wait_start);
> > +		PN(se.statistics.sleep_start);
> > +		PN(se.statistics.block_start);
> > +		PN(se.statistics.sleep_max);
> > +		PN(se.statistics.block_max);
> > +		PN(se.statistics.exec_max);
> > +		PN(se.statistics.slice_max);
> > +		PN(se.statistics.wait_max);
> > +		PN(se.statistics.wait_sum);
> > +		P(se.statistics.wait_count);
> > +		PN(se.statistics.iowait_sum);
> > +		P(se.statistics.iowait_count);
> > +		P(se.nr_migrations);
> 
> Ditto for se.nr_migrations. It has no schedstat_enabled() wrapper.
> 

Yes.

> > @@ -801,8 +793,8 @@ static void update_stats_enqueue(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  		update_stats_wait_start(cfs_rq, se);
> >  }
> >  
> > -static inline void
> > -update_stats_dequeue(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > +static void
> > +update_stats_dequeue(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> >  {
> >  	/*
> >  	 * Mark the end of the wait period if dequeueing a
> 
> You dropped the 'inline' from this function. Since there is only one
> caller, I'm guessing that was unintentional?

It wasn't really. The patch increased the function size by enough that
I uninlined it and let the compiler make the decision. In this case,
it should automatically inline but I can leave the inline in.

-- 
Mel Gorman
SUSE Labs