From: Ingo Molnar <mingo@kernel.org>
To: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Cc: Balbir Singh <bsingharora@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Peter Zijlstra <a.p.zijlstra@chello.nl>
Subject: Re: [PATCH] proc/schedstat: Expose /proc/<pid>/schedstat if delay accounting is enabled
Date: Fri, 29 May 2015 10:04:25 +0200 [thread overview]
Message-ID: <20150529080424.GA27235@gmail.com> (raw)
In-Reply-To: <20150529061620.GA17421@naverao1-tp.ibm.com>
* Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote:
> /proc/<pid>/schedstat is currently only available if CONFIG_SCHEDSTATS is
> enabled. But, all the fields that this exposes are available and valid
> if CONFIG_TASK_DELAY_ACCT is enabled as well.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> fs/proc/base.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 093ca14..3ece303 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -304,14 +304,17 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
> }
> #endif
>
> +#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
> +#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
> +#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
So such #ifdef parades are ugly and are usually a sign of some problem with the
patch - as in this case.
But there's deeper problems as well:
/*
* Provides /proc/PID/schedstat
*/
static int proc_pid_schedstat(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *task)
{
seq_printf(m, "%llu %llu %lu\n",
(unsigned long long)task->se.sum_exec_runtime,
(unsigned long long)task->sched_info.run_delay,
task->sched_info.pcount);
return 0;
}
- The sum_exec_runtime field is available unconditionally.
- But the sched_info.run_delay field is only maintained if CONFIG_SCHEDSTATS is
enabled.
- Also, the sched_info.pcount field is again only maintained if CONFIG_SCHEDSTATS
is enabled.
So the claim in your changelog that these fields are maintained in the
delayaccounting case is plain false: none of the fields are conditional on delay
accounting.
So what's the purpose of your patch?
Thanks,
Ingo
next prev parent reply other threads:[~2015-05-29 8:04 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-25 7:12 [PATCH] proc/schedstat: Expose /proc/<pid>/schedstat if delay accounting is enabled Naveen N. Rao
2015-05-28 5:11 ` Naveen N. Rao
2015-05-28 9:11 ` Balbir Singh
2015-05-29 6:16 ` Naveen N. Rao
2015-05-29 8:04 ` Ingo Molnar [this message]
2015-05-29 8:55 ` Naveen N. Rao
2015-05-29 9:18 ` Ingo Molnar
2015-05-29 9:45 ` Naveen N. Rao
2015-05-29 9:54 ` Ingo Molnar
2015-05-29 17:06 ` Naveen N. Rao
2015-06-02 7:58 ` Ingo Molnar
2015-06-25 8:39 ` Naveen N. Rao
2015-06-25 12:12 ` Ingo Molnar
2015-06-25 18:00 ` Cong Wang
2015-06-25 18:27 ` Naveen N. Rao
2015-06-25 18:40 ` Cong Wang
2015-06-26 8:39 ` Naveen N. Rao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150529080424.GA27235@gmail.com \
--to=mingo@kernel.org \
--cc=a.p.zijlstra@chello.nl \
--cc=bsingharora@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=naveen.n.rao@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.