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

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]

Date:	Tue, 2 Feb 2016 20:15:14 +0530
From:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:	Mel Gorman <mgorman@...hsingularity.net>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	Matt Fleming <matt@...eblueprint.co.uk>,
	Mike Galbraith <mgalbraith@...e.de>,
	LKML <linux-kernel@...r.kernel.org>,
	"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Subject: Re: [PATCH 1/1] sched: Make schedstats a runtime tunable that is
 disabled by default v3

> > If we disable schedstats dynamically but CONFIG_TASK_DELAY_ACCT is not
> > set, then sched_info_on will return true,
> 
> Is this really a problem?
> 
> sched_info_on() guards sched_info_dequeued(), sched_info_queued(),
> __sched_info_switch(). These update fields in the sched_info struct with
> the exception of rq->rq_cpu_time. In the case of rq_cpu_time, the values
> it's updated depend on sched_info.
> 
> I'm not spotting the case where the current information for delayacct is
> inaccurate. Where is it? Granted, there is some scope for also disabling
> the delayacct information unless explicitly enabled.

ah, the run_delay gets updated. So yes its not a problem.

> 
> > This could impact guest steal
> > time stats as well as data read from /proc/<PID>/schedstat
> > 
> > Also when schedstats is dynamically disabled, and user tries to enable
> > kernel sleep profiling profile_setup(), the kernel may not be able to do
> > the right profiling since enqueue_sleeper() may not get called. Should
> > we alert the user saying kernel sleep profiling is disabled?
> > 
> 
> Yes. This on top? It's not completely bullet proof as a user could both
> force schedstat disabled and enable sleep profiling but it's a waste of
> memory to guard against it
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index a10494a94cc3..5c2cd37c42e9 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -920,6 +920,14 @@ static inline int sched_info_on(void)
>  #endif
>  }
> 
> +#ifdef CONFIG_SCHEDSTATS
> +void force_schedstat_enabled(void);
> +#else
> +static inline void force_schedstat_enabled(void)
> +{
> +}
> +#endif

One nit:
Since force_schedstat_enabled is called under CONFIG_SCHEDSTATS
we may not want the static define.

> +
>  enum cpu_idle_type {
>  	CPU_IDLE,
>  	CPU_NOT_IDLE,
> diff --git a/kernel/profile.c b/kernel/profile.c
> index 99513e1160e5..51369697466e 100644
> --- a/kernel/profile.c
> +++ b/kernel/profile.c
> @@ -59,6 +59,7 @@ int profile_setup(char *str)
> 
>  	if (!strncmp(str, sleepstr, strlen(sleepstr))) {
>  #ifdef CONFIG_SCHEDSTATS
> +		force_schedstat_enabled();
>  		prof_on = SLEEP_PROFILING;
>  		if (str[strlen(sleepstr)] == ',')
>  			str += strlen(sleepstr) + 1;

-- 
Thanks and Regards
Srikar Dronamraju