[PATCH] sched: Update scheduler stat documentation.

[PATCH] sched: Update scheduler stat documentation.

[Rakib Mullick]

/proc/schedstat's second field reflect to # of time context switch happened not # of times switched to the expired queue.

Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
---

diff --git a/Documentation/scheduler/sched-stats.txt b/Documentation/scheduler/sched-stats.txt
index 1cd5d51..7d8e12c 100644
--- a/Documentation/scheduler/sched-stats.txt
+++ b/Documentation/scheduler/sched-stats.txt
@@ -38,7 +38,8 @@ First field is a sched_yield() statistic:
      1) # of times sched_yield() was called
 
 Next three are schedule() statistics:
-     2) # of times we switched to the expired queue and reused it
+     2) # of times context switch happened. This field is not utilized these
+	days but kept for holding userspace tools integrity.
      3) # of times schedule() was called
      4) # of times schedule() left the processor idle
 

Re: [PATCH] sched: Update scheduler stat documentation.

[Ingo Molnar]

* Rakib Mullick <rakib.mullick@gmail.com> wrote:

> /proc/schedstat's second field reflect to # of time context switch happened not # of times switched to the expired queue.
> 
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> ---
> 
> diff --git a/Documentation/scheduler/sched-stats.txt b/Documentation/scheduler/sched-stats.txt
> index 1cd5d51..7d8e12c 100644
> --- a/Documentation/scheduler/sched-stats.txt
> +++ b/Documentation/scheduler/sched-stats.txt
> @@ -38,7 +38,8 @@ First field is a sched_yield() statistic:
>       1) # of times sched_yield() was called
>  
>  Next three are schedule() statistics:
> -     2) # of times we switched to the expired queue and reused it
> +     2) # of times context switch happened. This field is not utilized these
> +	days but kept for holding userspace tools integrity.

if it's not used by anything but kept for the ABI then we should 
not put context switch number in there, but always keep it zero, 
right?

Thanks,

	Ingo

Re: [PATCH] sched: Update scheduler stat documentation.

[Rakib Mullick]

On Mon, Jan 16, 2012 at 1:56 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Rakib Mullick <rakib.mullick@gmail.com> wrote:
>
>> /proc/schedstat's second field reflect to # of time context switch happened not # of times switched to the expired queue.
>>
>> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
>> ---
>>
>> diff --git a/Documentation/scheduler/sched-stats.txt b/Documentation/scheduler/sched-stats.txt
>> index 1cd5d51..7d8e12c 100644
>> --- a/Documentation/scheduler/sched-stats.txt
>> +++ b/Documentation/scheduler/sched-stats.txt
>> @@ -38,7 +38,8 @@ First field is a sched_yield() statistic:
>>       1) # of times sched_yield() was called
>>
>>  Next three are schedule() statistics:
>> -     2) # of times we switched to the expired queue and reused it
>> +     2) # of times context switch happened. This field is not utilized these
>> +     days but kept for holding userspace tools integrity.
>
> if it's not used by anything but kept for the ABI then we should
> not put context switch number in there, but always keep it zero,
> right?
>
Yes, quite right. It is a bit confusing. Anyone might have think that
context switch numbers are 0. So, instead of saying "# of times
context switch happened", we can say, "This field has been kept for
ABI integrity". Right?

Thanks,
Rakib

Re: [PATCH] sched: Update scheduler stat documentation.

[Ingo Molnar]

* Rakib Mullick <rakib.mullick@gmail.com> wrote:

> On Mon, Jan 16, 2012 at 1:56 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Rakib Mullick <rakib.mullick@gmail.com> wrote:
> >
> >> /proc/schedstat's second field reflect to # of time context switch happened not # of times switched to the expired queue.
> >>
> >> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> >> ---
> >>
> >> diff --git a/Documentation/scheduler/sched-stats.txt b/Documentation/scheduler/sched-stats.txt
> >> index 1cd5d51..7d8e12c 100644
> >> --- a/Documentation/scheduler/sched-stats.txt
> >> +++ b/Documentation/scheduler/sched-stats.txt
> >> @@ -38,7 +38,8 @@ First field is a sched_yield() statistic:
> >>       1) # of times sched_yield() was called
> >>
> >>  Next three are schedule() statistics:
> >> -     2) # of times we switched to the expired queue and reused it
> >> +     2) # of times context switch happened. This field is not utilized these
> >> +     days but kept for holding userspace tools integrity.
> >
> > if it's not used by anything but kept for the ABI then we should
> > not put context switch number in there, but always keep it zero,
> > right?
> >
>
> Yes, quite right. It is a bit confusing. Anyone might have 
> think that context switch numbers are 0. So, instead of saying 
> "# of times context switch happened", we can say, "This field 
> has been kept for ABI integrity". Right?

Yes - but we should also change it to export a value of zero. 
The field is a legacy 'array expirations' field, and as such it 
should be zero.

Thanks,

	Ingo

Re: [PATCH] sched: Update scheduler stat documentation.

[Rakib Mullick]

On Mon, Jan 16, 2012 at 2:36 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
>
> Yes - but we should also change it to export a value of zero.
> The field is a legacy 'array expirations' field, and as such it
> should be zero.
>

Ok, thanks for suggestions. Please, look at the following patch, it's
been roughly created to address your suggestion. If it looks okay,
then I can send formal patch (perhaps two separate patches?). In this
way, I think we can also remove rq->sched_switch field (not done in
this patch)?

diff --git a/Documentation/scheduler/sched-stats.txt
b/Documentation/scheduler/sched-stats.txt
index 1cd5d51..cc2d107 100644
--- a/Documentation/scheduler/sched-stats.txt
+++ b/Documentation/scheduler/sched-stats.txt
@@ -38,7 +38,8 @@ First field is a sched_yield() statistic:
      1) # of times sched_yield() was called

 Next three are schedule() statistics:
-     2) # of times we switched to the expired queue and reused it
+     2) This field is a legacy array expiration field used in O(1) scheduler.
+	But still kept for ABI integrity.
      3) # of times schedule() was called
      4) # of times schedule() left the processor idle

diff --git a/kernel/sched/stats.c b/kernel/sched/stats.c
index 2a581ba..01a52d9 100644
--- a/kernel/sched/stats.c
+++ b/kernel/sched/stats.c
@@ -25,6 +25,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 	seq_printf(seq, "timestamp %lu\n", jiffies);
 	for_each_online_cpu(cpu) {
 		struct rq *rq = cpu_rq(cpu);
+		int rq_sched_switch = 0;
 #ifdef CONFIG_SMP
 		struct sched_domain *sd;
 		int dcount = 0;
@@ -34,7 +35,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
 		seq_printf(seq,
 		    "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_sched_switch, rq->sched_count, rq->sched_goidle,
 		    rq->ttwu_count, rq->ttwu_local,
 		    rq->rq_cpu_time,
 		    rq->rq_sched_info.run_delay, rq->rq_sched_info.pcount);

Re: [PATCH] sched: Update scheduler stat documentation.

[Ingo Molnar]

* Rakib Mullick <rakib.mullick@gmail.com> wrote:

> On Mon, Jan 16, 2012 at 2:36 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> >
> > Yes - but we should also change it to export a value of zero.
> > The field is a legacy 'array expirations' field, and as such it
> > should be zero.
> >
> 
> Ok, thanks for suggestions. Please, look at the following patch, it's
> been roughly created to address your suggestion. If it looks okay,
> then I can send formal patch (perhaps two separate patches?). In this
> way, I think we can also remove rq->sched_switch field (not done in
> this patch)?
> 
> diff --git a/Documentation/scheduler/sched-stats.txt
> b/Documentation/scheduler/sched-stats.txt
> index 1cd5d51..cc2d107 100644
> --- a/Documentation/scheduler/sched-stats.txt
> +++ b/Documentation/scheduler/sched-stats.txt
> @@ -38,7 +38,8 @@ First field is a sched_yield() statistic:
>       1) # of times sched_yield() was called
> 
>  Next three are schedule() statistics:
> -     2) # of times we switched to the expired queue and reused it
> +     2) This field is a legacy array expiration field used in O(1) scheduler.
> +	But still kept for ABI integrity.

I'd formulate it like this:

> +     2) This field is a legacy array expiration count field
> +        used in the O(1) scheduler. We kept it for ABI
> +        compatibility, but it is always set to zero.


> +		int rq_sched_switch = 0;
>  #ifdef CONFIG_SMP
>  		struct sched_domain *sd;
>  		int dcount = 0;
> @@ -34,7 +35,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
>  		seq_printf(seq,
>  		    "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_sched_switch, rq->sched_count, rq->sched_goidle,

printf can print a 0 value just fine as part of the format 
string ... :-)

But yeah, this logic is what i had in mind.

Thanks,

	Ingo

Re: [PATCH] sched: Update scheduler stat documentation.

[Rakib Mullick]

On Tue, Jan 17, 2012 at 3:50 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Rakib Mullick <rakib.mullick@gmail.com> wrote:
>
>> On Mon, Jan 16, 2012 at 2:36 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> >
>> > Yes - but we should also change it to export a value of zero.
>> > The field is a legacy 'array expirations' field, and as such it
>> > should be zero.
>> >
>>
>> Ok, thanks for suggestions. Please, look at the following patch, it's
>> been roughly created to address your suggestion. If it looks okay,
>> then I can send formal patch (perhaps two separate patches?). In this
>> way, I think we can also remove rq->sched_switch field (not done in
>> this patch)?
>>
>> diff --git a/Documentation/scheduler/sched-stats.txt
>> b/Documentation/scheduler/sched-stats.txt
>> index 1cd5d51..cc2d107 100644
>> --- a/Documentation/scheduler/sched-stats.txt
>> +++ b/Documentation/scheduler/sched-stats.txt
>> @@ -38,7 +38,8 @@ First field is a sched_yield() statistic:
>>       1) # of times sched_yield() was called
>>
>>  Next three are schedule() statistics:
>> -     2) # of times we switched to the expired queue and reused it
>> +     2) This field is a legacy array expiration field used in O(1) scheduler.
>> +     But still kept for ABI integrity.
>
> I'd formulate it like this:
>
>> +     2) This field is a legacy array expiration count field
>> +        used in the O(1) scheduler. We kept it for ABI
>> +        compatibility, but it is always set to zero.
>
Okay, I'll take the above description.

>
>> +             int rq_sched_switch = 0;
>>  #ifdef CONFIG_SMP
>>               struct sched_domain *sd;
>>               int dcount = 0;
>> @@ -34,7 +35,7 @@ static int show_schedstat(struct seq_file *seq, void *v)
>>               seq_printf(seq,
>>                   "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_sched_switch, rq->sched_count, rq->sched_goidle,
>
> printf can print a 0 value just fine as part of the format
> string ... :-)
>
Umm... actually, I thought using a variable would have been nice to
indicate it's purpose rather straightly putting 0. Anyway, will change
it.

> But yeah, this logic is what i had in mind.
>
I was confused what you asked for cause present code returns 0 too.
Now got it, will send a patch shortly.

Thanks,
Rakib

> Thanks,
>
>        Ingo