LinuxLists.cc - [PATCH v3 -tip] sched: Clean unused fields from struct rq

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

On Wed, Mar 18, 2009 at 10:51:37PM +0000, Luis Henriques wrote:
> Since they are used on in statistics and are always set to zero, the following
> fields from struct rq have been removed: yld_exp_empty, yld_act_empty and
> yld_both_empty.
>
> Both Sched Debug and SCHEDSTAT_VERSION versions has also been incremented since
> ABIs have been changed.
>
> Signed-off-by: Luis Henriques <[email protected]>
> ---
> kernel/sched.c | 3 ---
> kernel/sched_debug.c | 5 +----
> kernel/sched_stats.h | 7 +++----
> 3 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 8a579e2..4469034 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -646,9 +646,6 @@ struct rq {
> /* could above be rq->cfs_rq.exec_clock + rq->rt_rq.rt_runtime ? */
>
> /* sys_sched_yield() stats */
> - unsigned int yld_exp_empty;
> - unsigned int yld_act_empty;
> - unsigned int yld_both_empty;
> unsigned int yld_count;
>
> /* schedule() stats */
> diff --git a/kernel/sched_debug.c b/kernel/sched_debug.c
> index 4daebff..467ca72 100644
> --- a/kernel/sched_debug.c
> +++ b/kernel/sched_debug.c
> @@ -286,9 +286,6 @@ static void print_cpu(struct seq_file *m, int cpu)
> #ifdef CONFIG_SCHEDSTATS
> #define P(n) SEQ_printf(m, " .%-30s: %d\n", #n, rq->n);
>
> - P(yld_exp_empty);
> - P(yld_act_empty);
> - P(yld_both_empty);
> P(yld_count);
>
> P(sched_switch);
> @@ -313,7 +310,7 @@ static int sched_debug_show(struct seq_file *m, void *v)
> u64 now = ktime_to_ns(ktime_get());
> int cpu;
>
> - SEQ_printf(m, "Sched Debug Version: v0.08, %s %.*s\n",
> + SEQ_printf(m, "Sched Debug Version: v0.09, %s %.*s\n",
> init_utsname()->release,
> (int)strcspn(init_utsname()->version, " "),
> init_utsname()->version);
> diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
> index a8f93dd..32d2bd4 100644
> --- a/kernel/sched_stats.h
> +++ b/kernel/sched_stats.h
> @@ -4,7 +4,7 @@
> * bump this up when changing the output format or the meaning of an existing
> * format, so that tools can adapt (or abort)
> */
> -#define SCHEDSTAT_VERSION 14
> +#define SCHEDSTAT_VERSION 15
>
> static int show_schedstat(struct seq_file *seq, void *v)
> {
> @@ -26,9 +26,8 @@ static int show_schedstat(struct seq_file *seq, void *v)
>
> /* runqueue-specific stats */
> seq_printf(seq,
> - "cpu%d %u %u %u %u %u %u %u %u %u %llu %llu %lu",
> - cpu, rq->yld_both_empty,
> - rq->yld_act_empty, rq->yld_exp_empty, rq->yld_count,
> + "cpu%d %u %u %u %u %u %u %llu %llu %lu",
> + cpu, rq->yld_count,
> rq->sched_switch, rq->sched_count, rq->sched_goidle,
> rq->ttwu_count, rq->ttwu_local,
> rq->rq_cpu_time,
> --
> 1.6.2

Btw: I tried Greg schedtop with this patch and the app behaviour is as expected:

$ ./schedtop
Exception: unsupported version

Regards,
--
Luis Henriques

2009-03-19 07:50:41

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

* Luis Henriques <[email protected]> wrote:

> On Wed, Mar 18, 2009 at 10:51:37PM +0000, Luis Henriques wrote:
> > Since they are used on in statistics and are always set to zero, the following
> > fields from struct rq have been removed: yld_exp_empty, yld_act_empty and
> > yld_both_empty.
> >
> > Both Sched Debug and SCHEDSTAT_VERSION versions has also been incremented since
> > ABIs have been changed.
> >
> > Signed-off-by: Luis Henriques <[email protected]>
> > ---
> > kernel/sched.c | 3 ---
> > kernel/sched_debug.c | 5 +----
> > kernel/sched_stats.h | 7 +++----
> > 3 files changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/sched.c b/kernel/sched.c
> > index 8a579e2..4469034 100644
> > --- a/kernel/sched.c
> > +++ b/kernel/sched.c
> > @@ -646,9 +646,6 @@ struct rq {
> > /* could above be rq->cfs_rq.exec_clock + rq->rt_rq.rt_runtime ? */
> >
> > /* sys_sched_yield() stats */
> > - unsigned int yld_exp_empty;
> > - unsigned int yld_act_empty;
> > - unsigned int yld_both_empty;
> > unsigned int yld_count;
> >
> > /* schedule() stats */
> > diff --git a/kernel/sched_debug.c b/kernel/sched_debug.c
> > index 4daebff..467ca72 100644
> > --- a/kernel/sched_debug.c
> > +++ b/kernel/sched_debug.c
> > @@ -286,9 +286,6 @@ static void print_cpu(struct seq_file *m, int cpu)
> > #ifdef CONFIG_SCHEDSTATS
> > #define P(n) SEQ_printf(m, " .%-30s: %d\n", #n, rq->n);
> >
> > - P(yld_exp_empty);
> > - P(yld_act_empty);
> > - P(yld_both_empty);
> > P(yld_count);
> >
> > P(sched_switch);
> > @@ -313,7 +310,7 @@ static int sched_debug_show(struct seq_file *m, void *v)
> > u64 now = ktime_to_ns(ktime_get());
> > int cpu;
> >
> > - SEQ_printf(m, "Sched Debug Version: v0.08, %s %.*s\n",
> > + SEQ_printf(m, "Sched Debug Version: v0.09, %s %.*s\n",
> > init_utsname()->release,
> > (int)strcspn(init_utsname()->version, " "),
> > init_utsname()->version);
> > diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
> > index a8f93dd..32d2bd4 100644
> > --- a/kernel/sched_stats.h
> > +++ b/kernel/sched_stats.h
> > @@ -4,7 +4,7 @@
> > * bump this up when changing the output format or the meaning of an existing
> > * format, so that tools can adapt (or abort)
> > */
> > -#define SCHEDSTAT_VERSION 14
> > +#define SCHEDSTAT_VERSION 15
> >
> > static int show_schedstat(struct seq_file *seq, void *v)
> > {
> > @@ -26,9 +26,8 @@ static int show_schedstat(struct seq_file *seq, void *v)
> >
> > /* runqueue-specific stats */
> > seq_printf(seq,
> > - "cpu%d %u %u %u %u %u %u %u %u %u %llu %llu %lu",
> > - cpu, rq->yld_both_empty,
> > - rq->yld_act_empty, rq->yld_exp_empty, rq->yld_count,
> > + "cpu%d %u %u %u %u %u %u %llu %llu %lu",
> > + cpu, rq->yld_count,
> > rq->sched_switch, rq->sched_count, rq->sched_goidle,
> > rq->ttwu_count, rq->ttwu_local,
> > rq->rq_cpu_time,
> > --
> > 1.6.2
>
> Btw: I tried Greg schedtop with this patch and the app behaviour is as expected:
>
> $ ./schedtop
> Exception: unsupported version

Mind updating the app too and post it here please? It's the only app
that relies on this file AFAIK.

Ingo

2009-03-19 11:53:53

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

Ingo Molnar wrote:
> * Luis Henriques <[email protected]> wrote:
>
>
>>
>> Btw: I tried Greg schedtop with this patch and the app behaviour is as expected:
>>
>> $ ./schedtop
>> Exception: unsupported version
>>
>
> Mind updating the app too and post it here please? It's the only app
> that relies on this file AFAIK.
>

I can take care of the update if Luis can just confirm that this patch
works as expected against his new ABI?

--
commit 336a22f597769bb5759d561773d05ce666019677
Author: Gregory Haskins <[email protected]>
Date: Thu Mar 19 07:54:10 2009 -0400

Update to proposed v15 ABI

Signed-off-by: Gregory Haskins <[email protected]>

diff --git a/schedtop.cc b/schedtop.cc
index 4d4c510..f3c9468 100644
--- a/schedtop.cc
+++ b/schedtop.cc
@@ -114,7 +114,7 @@ public:
throw std::runtime_error("error parsing
version");

lis >> m_version;
- if (m_version != 14)
+ if ((m_version < 14) || (m_version > 15))
throw std::runtime_error("unsupported version");

state = state_timestamp;
@@ -219,9 +219,11 @@ private:
std::string basename("/" + FormIndex("cpu", m_cpu) + "/rq/");
Importer importer(m_smap, is, basename);

- importer += "yld_both_empty";
- importer += "yld_act_empty";
- importer += "yld_exp_empty";
+ if (m_version < 15){
+ importer += "yld_both_empty";
+ importer += "yld_act_empty";
+ importer += "yld_exp_empty";
+ }
importer += "yld_count";
importer += "sched_switch";
importer += "sched_count";

----------------

If this patch works, you have my "Acked-by" for Luis' kernel-side patch.

-Greg

Attachments:

2009-03-19 18:26:04

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

On Thu, Mar 19, 2009 at 07:56:04AM -0400, Gregory Haskins wrote:
> Ingo Molnar wrote:
> > * Luis Henriques <[email protected]> wrote:
> >
> >
> >>
> >> Btw: I tried Greg schedtop with this patch and the app behaviour is as expected:
> >>
> >> $ ./schedtop
> >> Exception: unsupported version
> >>
> >
> > Mind updating the app too and post it here please? It's the only app
> > that relies on this file AFAIK.
> >
>
> I can take care of the update if Luis can just confirm that this patch
> works as expected against his new ABI?

Thanks, Greg. I just tested your patch to schedtop and I can confirm that it
works just fine in both versions (for v14 and v15).

With respect to the changes to the sched_debug ABI, from what I understood no
one is using it at the moment, right?

> --
> commit 336a22f597769bb5759d561773d05ce666019677
> Author: Gregory Haskins <[email protected]>
> Date: Thu Mar 19 07:54:10 2009 -0400
>
> Update to proposed v15 ABI
>
> Signed-off-by: Gregory Haskins <[email protected]>
>
> diff --git a/schedtop.cc b/schedtop.cc
> index 4d4c510..f3c9468 100644
> --- a/schedtop.cc
> +++ b/schedtop.cc
> @@ -114,7 +114,7 @@ public:
> throw std::runtime_error("error parsing
> version");
>
> lis >> m_version;
> - if (m_version != 14)
> + if ((m_version < 14) || (m_version > 15))
> throw std::runtime_error("unsupported version");
>
> state = state_timestamp;
> @@ -219,9 +219,11 @@ private:
> std::string basename("/" + FormIndex("cpu", m_cpu) + "/rq/");
> Importer importer(m_smap, is, basename);
>
> - importer += "yld_both_empty";
> - importer += "yld_act_empty";
> - importer += "yld_exp_empty";
> + if (m_version < 15){
> + importer += "yld_both_empty";
> + importer += "yld_act_empty";
> + importer += "yld_exp_empty";
> + }
> importer += "yld_count";
> importer += "sched_switch";
> importer += "sched_count";
>
>
>
> ----------------
>
> If this patch works, you have my "Acked-by" for Luis' kernel-side patch.

Great! :-)

Regards,
--
Luis Henriques

2009-03-19 18:41:03

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

Luis Henriques wrote:
> On Wed, Mar 18, 2009 at 10:51:37PM +0000, Luis Henriques wrote:
>
>> Since they are used on in statistics and are always set to zero, the following
>> fields from struct rq have been removed: yld_exp_empty, yld_act_empty and
>> yld_both_empty.
>>
>> Both Sched Debug and SCHEDSTAT_VERSION versions has also been incremented since
>> ABIs have been changed.
>>
>> Signed-off-by: Luis Henriques <[email protected]>
>>
Acked-by: Gregory Haskins <[email protected]>

>> ---
>> kernel/sched.c | 3 ---
>> kernel/sched_debug.c | 5 +----
>> kernel/sched_stats.h | 7 +++----
>> 3 files changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index 8a579e2..4469034 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -646,9 +646,6 @@ struct rq {
>> /* could above be rq->cfs_rq.exec_clock + rq->rt_rq.rt_runtime ? */
>>
>> /* sys_sched_yield() stats */
>> - unsigned int yld_exp_empty;
>> - unsigned int yld_act_empty;
>> - unsigned int yld_both_empty;
>> unsigned int yld_count;
>>
>> /* schedule() stats */
>> diff --git a/kernel/sched_debug.c b/kernel/sched_debug.c
>> index 4daebff..467ca72 100644
>> --- a/kernel/sched_debug.c
>> +++ b/kernel/sched_debug.c
>> @@ -286,9 +286,6 @@ static void print_cpu(struct seq_file *m, int cpu)
>> #ifdef CONFIG_SCHEDSTATS
>> #define P(n) SEQ_printf(m, " .%-30s: %d\n", #n, rq->n);
>>
>> - P(yld_exp_empty);
>> - P(yld_act_empty);
>> - P(yld_both_empty);
>> P(yld_count);
>>
>> P(sched_switch);
>> @@ -313,7 +310,7 @@ static int sched_debug_show(struct seq_file *m, void *v)
>> u64 now = ktime_to_ns(ktime_get());
>> int cpu;
>>
>> - SEQ_printf(m, "Sched Debug Version: v0.08, %s %.*s\n",
>> + SEQ_printf(m, "Sched Debug Version: v0.09, %s %.*s\n",
>> init_utsname()->release,
>> (int)strcspn(init_utsname()->version, " "),
>> init_utsname()->version);
>> diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
>> index a8f93dd..32d2bd4 100644
>> --- a/kernel/sched_stats.h
>> +++ b/kernel/sched_stats.h
>> @@ -4,7 +4,7 @@
>> * bump this up when changing the output format or the meaning of an existing
>> * format, so that tools can adapt (or abort)
>> */
>> -#define SCHEDSTAT_VERSION 14
>> +#define SCHEDSTAT_VERSION 15
>>
>> static int show_schedstat(struct seq_file *seq, void *v)
>> {
>> @@ -26,9 +26,8 @@ static int show_schedstat(struct seq_file *seq, void *v)
>>
>> /* runqueue-specific stats */
>> seq_printf(seq,
>> - "cpu%d %u %u %u %u %u %u %u %u %u %llu %llu %lu",
>> - cpu, rq->yld_both_empty,
>> - rq->yld_act_empty, rq->yld_exp_empty, rq->yld_count,
>> + "cpu%d %u %u %u %u %u %u %llu %llu %lu",
>> + cpu, rq->yld_count,
>> rq->sched_switch, rq->sched_count, rq->sched_goidle,
>> rq->ttwu_count, rq->ttwu_local,
>> rq->rq_cpu_time,
>> --
>> 1.6.2
>>
>
> Btw: I tried Greg schedtop with this patch and the app behaviour is as expected:
>
> $ ./schedtop
> Exception: unsupported version
>
> Regards,
>

Attachments:

2009-03-19 18:49:44

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

Luis Henriques wrote:
>
> With respect to the changes to the sched_debug ABI, from what I understood no
> one is using it at the moment, right?
>
>

I am not aware of any users personally, but that doesn't mean a whole
heck of a lot ;) Ingo and Peter probably have better insight here.

However, you did the proper thing from my perspective and bumped the
version number. So even if there are users out there, it stands to
reason they will likely pick up on the change in a deterministic manner
anyway. Therefore, IMO you are all set.

Thanks Luis,
-Greg

Attachments:

2009-03-23 17:55:11

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

2009-03-24 14:10:37

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

2009-03-24 15:20:10

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

Ingo Molnar wrote:
> * Luis Henriques <[email protected]> wrote:
>
>
>> On Wed, Mar 18, 2009 at 10:51:37PM +0000, Luis Henriques wrote:
>>
>>> Since they are used on in statistics and are always set to zero, the following
>>> fields from struct rq have been removed: yld_exp_empty, yld_act_empty and
>>> yld_both_empty.
>>>
>>> Both Sched Debug and SCHEDSTAT_VERSION versions has also been incremented since
>>> ABIs have been changed.
>>>
>>>
>> Hi Ingo,
>>
>> Sorry to bother you but I can not find this patch in -tip. Just
>> would like to confirm with you that it was NACK'ed or you just
>> forgot to merge it to the tree.
>>
>
> Was held up by the schedstat tool discussions. Please resend the
> patch with a link to the updated tool in the commit log perhaps (if
> such a link exists), and with Gregory's ack in place.
>
> Thanks,
>
> Ingo
>
Hi Ingo,
I was waiting to merge the tool patch until I was sure this would be
your blessed v15 of the ABI. If you are comfy with the kernel patch but
want the tool updated before the kernel, I can do that, no problem. The
only thing that I ask is that if for some reason this kernel patch
doesn't make it in, please require any future patches that change this
ABI to be versioned > 15 ;)

Thanks!
-Greg

Attachments:

2009-03-24 15:42:06

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

* Gregory Haskins <[email protected]> wrote:

> Ingo Molnar wrote:
> > * Luis Henriques <[email protected]> wrote:
> >
> >
> >> On Wed, Mar 18, 2009 at 10:51:37PM +0000, Luis Henriques wrote:
> >>
> >>> Since they are used on in statistics and are always set to zero, the following
> >>> fields from struct rq have been removed: yld_exp_empty, yld_act_empty and
> >>> yld_both_empty.
> >>>
> >>> Both Sched Debug and SCHEDSTAT_VERSION versions has also been incremented since
> >>> ABIs have been changed.
> >>>
> >>>
> >> Hi Ingo,
> >>
> >> Sorry to bother you but I can not find this patch in -tip. Just
> >> would like to confirm with you that it was NACK'ed or you just
> >> forgot to merge it to the tree.
> >>
> >
> > Was held up by the schedstat tool discussions. Please resend the
> > patch with a link to the updated tool in the commit log perhaps (if
> > such a link exists), and with Gregory's ack in place.
> >
> > Thanks,
> >
> > Ingo
> >
> Hi Ingo,
> I was waiting to merge the tool patch until I was sure this would be
> your blessed v15 of the ABI. If you are comfy with the kernel patch but
> want the tool updated before the kernel, I can do that, no problem. The
> only thing that I ask is that if for some reason this kernel patch
> doesn't make it in, please require any future patches that change this
> ABI to be versioned > 15 ;)

How about moving schedstat to Documentation/sched/schedstat.c or so?
It's small and trivial enough, and that way changes would go hand in
hand with the app.

Ingo

2009-03-24 15:57:30

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

Ingo Molnar wrote:
> * Gregory Haskins <[email protected]> wrote:
>
>
>> Ingo Molnar wrote:
>>
>>> * Luis Henriques <[email protected]> wrote:
>>>
>>>
>>>
>>>> On Wed, Mar 18, 2009 at 10:51:37PM +0000, Luis Henriques wrote:
>>>>
>>>>
>>>>> Since they are used on in statistics and are always set to zero, the following
>>>>> fields from struct rq have been removed: yld_exp_empty, yld_act_empty and
>>>>> yld_both_empty.
>>>>>
>>>>> Both Sched Debug and SCHEDSTAT_VERSION versions has also been incremented since
>>>>> ABIs have been changed.
>>>>>
>>>>>
>>>>>
>>>> Hi Ingo,
>>>>
>>>> Sorry to bother you but I can not find this patch in -tip. Just
>>>> would like to confirm with you that it was NACK'ed or you just
>>>> forgot to merge it to the tree.
>>>>
>>>>
>>> Was held up by the schedstat tool discussions. Please resend the
>>> patch with a link to the updated tool in the commit log perhaps (if
>>> such a link exists), and with Gregory's ack in place.
>>>
>>> Thanks,
>>>
>>> Ingo
>>>
>>>
>> Hi Ingo,
>> I was waiting to merge the tool patch until I was sure this would be
>> your blessed v15 of the ABI. If you are comfy with the kernel patch but
>> want the tool updated before the kernel, I can do that, no problem. The
>> only thing that I ask is that if for some reason this kernel patch
>> doesn't make it in, please require any future patches that change this
>> ABI to be versioned > 15 ;)
>>
>
> How about moving schedstat to Documentation/sched/schedstat.c or so?
> It's small and trivial enough, and that way changes would go hand in
> hand with the app.
>
Oh, I misunderstood. The tool patch I was referencing is for my
schedtop tool that is in a separate tree and written in C++. In
retrospect, you probably don't care about the relative state of my tool
coincident with the kernel side change, then. I agree that this other
schedstat tool should probably be in-tree and patched at the same time
as Luis' kernel patch.

FWIW: I have no problem with schedtop.cc going into the kernel as well
if that is what you would like, but I figured I would be burned at the
stake for suggestion such heresy as C++ in the tree ;)

-Greg

Attachments:

2009-03-24 18:24:58

[permalink] [raw]

Subject: Re: [PATCH v3 -tip] sched: Clean unused fields from struct rq

Hi,

On Tue, Mar 24, 2009 at 11:59:34AM -0400, Gregory Haskins wrote:
> > How about moving schedstat to Documentation/sched/schedstat.c or so?
> > It's small and trivial enough, and that way changes would go hand in
> > hand with the app.
> >
> Oh, I misunderstood. The tool patch I was referencing is for my
> schedtop tool that is in a separate tree and written in C++. In
> retrospect, you probably don't care about the relative state of my tool
> coincident with the kernel side change, then. I agree that this other
> schedstat tool should probably be in-tree and patched at the same time
> as Luis' kernel patch.
>
> FWIW: I have no problem with schedtop.cc going into the kernel as well
> if that is what you would like, but I figured I would be burned at the
> stake for suggestion such heresy as C++ in the tree ;)

I also agree that there will be issues adding schedtop to the kernel tree, for
two reasons: 1) it is written in C++ 2) it has dependencies on external
libraries (libboost). So, unless the tool is re-written, I guess it will be
difficult have it accepted. But that's just me saying this :-)

Now, to summarise and to check I understood everything correctly: I need to
resend my patch (the kernel patch), adding a reference to the URL where
schedtop can be obtained. Is this correct? Shall I use the URL to the git
repository or to the rt wiki? Or both?

--
Luis Henriques

2009-03-24 21:05:04