Groups > linux.kernel > #1320540 > unrolled thread

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

Started by	Matt Fleming <matt@codeblueprint.co.uk>
First post	2016-01-28 11:40 +0100
Last post	2016-01-28 12:40 +0100
Articles	3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  Re: [PATCH] sched: Make schedstats a runtime tunable that is  disabled by default v2 Matt Fleming <matt@codeblueprint.co.uk> - 2016-01-28 11:40 +0100
    Re: [PATCH] sched: Make schedstats a runtime tunable that is  disabled by default v2 Mel Gorman <mgorman@techsingularity.net> - 2016-01-28 12:00 +0100
      Re: [PATCH] sched: Make schedstats a runtime tunable that is  disabled by default v2 Matt Fleming <matt@codeblueprint.co.uk> - 2016-01-28 12:40 +0100

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

From	Matt Fleming <matt@codeblueprint.co.uk>
Date	2016-01-28 11:40 +0100
Subject	Re: [PATCH] sched: Make schedstats a runtime tunable that is disabled by default v2
Message-ID	<qVS9B-24O-31@gated-at.bofh.it>

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.

> @@ -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?

>  #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.

> @@ -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?

[toc] | [next] | [standalone]

#1320555

From	Mel Gorman <mgorman@techsingularity.net>
Date	2016-01-28 12:00 +0100
Message-ID	<qVSsX-2cx-33@gated-at.bofh.it>
In reply to	#1320540

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

[toc] | [prev] | [next] | [standalone]

#1320590

From	Matt Fleming <matt@codeblueprint.co.uk>
Date	2016-01-28 12:40 +0100
Message-ID	<qVT5E-2Js-19@gated-at.bofh.it>
In reply to	#1320555

On Thu, 28 Jan, at 10:59:25AM, Mel Gorman wrote:
> 
> 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.

Letting the compiler make the decision seems fine to me, but I'll let
other people chime in with their 'static inline' opinions.

My concern was that you didn't intend to delete the keyword, and it
was done by accident. But since that's not the case, it's no big deal.

[toc] | [prev] | [standalone]

csiph-web

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

Contents

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

#1320555

#1320590