From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755943AbcBBOq0 (ORCPT ); Tue, 2 Feb 2016 09:46:26 -0500 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:42453 "EHLO e23smtp07.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755536AbcBBOqX (ORCPT ); Tue, 2 Feb 2016 09:46:23 -0500 X-IBM-Helo: d23dlp02.au.ibm.com X-IBM-MailFrom: srikar@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Tue, 2 Feb 2016 20:15:14 +0530 From: Srikar Dronamraju To: Mel Gorman Cc: Peter Zijlstra , Ingo Molnar , Matt Fleming , Mike Galbraith , LKML , "Naveen N. Rao" Subject: Re: [PATCH 1/1] sched: Make schedstats a runtime tunable that is disabled by default v3 Message-ID: <20160202144514.GA28611@linux.vnet.ibm.com> Reply-To: Srikar Dronamraju References: <1454319426-2658-1-git-send-email-mgorman@techsingularity.net> <20160202093207.GA9494@linux.vnet.ibm.com> <20160202115815.GD8337@techsingularity.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20160202115815.GD8337@techsingularity.net> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16020214-0025-0000-0000-000002E15E54 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > > 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//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