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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160128103208.GB2571@codeblueprint.co.uk>
Date:	Thu, 28 Jan 2016 10:32:08 +0000
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	Mel Gorman <mgorman@...hsingularity.net>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	Mike Galbraith <mgalbraith@...e.de>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sched: Make schedstats a runtime tunable that is
 disabled by default v2

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?