Re: [PATCH] Display -rt related stats in /proc/schedstat

Re: [PATCH] Display -rt related stats in /proc/schedstat

[Joachim Deguara]

On Monday 23 July 2007 14:59:25 Ankita Garg wrote:
> Hi,
>
> This patch adds support to display captured -rt stats under
> /proc/schedstat.

You need to +1 SCHEDSTAT_VERSION if you are going to do this.  I would also 
suggest the number of fields printed out it in schedstats is invariable of 
CONFIG_PREEMPT_RT rather whether the fields have meaning data or zeros 
depends on the define.  Finally an addition to Documentation/sched-stats.txt 
is always desirable with such a change.

-Joachim

[PATCH] Display -rt related stats in /proc/schedstat

[Ankita Garg]

Hi,

This patch adds support to display captured -rt stats under /proc/schedstat.


Signed-off-by: Ankita Garg <ankita@in.ibm.com>
--
 sched_stats.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6.22.1/kernel/sched_stats.h
===================================================================
--- linux-2.6.22.1.orig/kernel/sched_stats.h	2007-07-23 18:15:57.000000000 +0530
+++ linux-2.6.22.1/kernel/sched_stats.h	2007-07-23 18:24:03.000000000 +0530
@@ -21,12 +21,15 @@
 
 		/* runqueue-specific stats */
 		seq_printf(seq,
-		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %llu %lu",
+		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %lu %lu %lu %llu %lu",
 		    cpu, rq->yld_both_empty,
 		    rq->yld_act_empty, rq->yld_exp_empty, rq->yld_cnt,
 		    rq->sched_switch, rq->sched_cnt, rq->sched_goidle,
 		    rq->ttwu_cnt, rq->ttwu_local,
 		    rq->rq_sched_info.cpu_time,
+#ifdef CONFIG_PREEMPT_RT
+		    rq->rto_schedule, rq->rto_wakeup, rq->rto_pulled,
+#endif
 		    rq->rq_sched_info.run_delay, rq->rq_sched_info.pcnt);
 
 		seq_printf(seq, "\n");


-- 
Regards,
Ankita Garg (ankita@in.ibm.com)
Linux Technology Center
IBM India Systems & Technology Labs, 
Bangalore, India   

Re: [PATCH] Display -rt related stats in /proc/schedstat

[Ankita Garg]

On Mon, Jul 23, 2007 at 02:24:34PM +0200, Joachim Deguara wrote:
> On Monday 23 July 2007 14:59:25 Ankita Garg wrote:
> > Hi,
> >
> > This patch adds support to display captured -rt stats under
> > /proc/schedstat.
> 
> You need to +1 SCHEDSTAT_VERSION if you are going to do this.  I would also 
> suggest the number of fields printed out it in schedstats is invariable of 
> CONFIG_PREEMPT_RT rather whether the fields have meaning data or zeros 
> depends on the define.  Finally an addition to Documentation/sched-stats.txt 
> is always desirable with such a change.

Yep, that sounds fine to me, because the fields are being defined
irrespective of CONFIG_PREEMPT_RT. Besides, it looks much cleaner!
Have updated the SCHEDSTAT_VERSION to 15.

As for documentation update, Documentation/sched-stats.txt seems to be a little old.
Under CPU statistics, it indicates 28 fields being printed, but version 14 
seems to be printing only 12 (iiuc). So, requires some update. Will send it along 
in the next patch.


Signed-off-by: Ankita Garg <ankita@in.ibm.com>
--
 sched_stats.h |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6.22.1/kernel/sched_stats.h
===================================================================
--- linux-2.6.22.1.orig/kernel/sched_stats.h	2007-07-23 18:15:57.000000000 +0530
+++ linux-2.6.22.1/kernel/sched_stats.h	2007-07-23 20:19:21.000000000 +0530
@@ -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)
 {
@@ -21,12 +21,13 @@
 
 		/* runqueue-specific stats */
 		seq_printf(seq,
-		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %llu %lu",
+		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %lu %lu %lu %llu %lu",
 		    cpu, rq->yld_both_empty,
 		    rq->yld_act_empty, rq->yld_exp_empty, rq->yld_cnt,
 		    rq->sched_switch, rq->sched_cnt, rq->sched_goidle,
 		    rq->ttwu_cnt, rq->ttwu_local,
 		    rq->rq_sched_info.cpu_time,
+		    rq->rto_schedule, rq->rto_wakeup, rq->rto_pulled,
 		    rq->rq_sched_info.run_delay, rq->rq_sched_info.pcnt);
 
 		seq_printf(seq, "\n");

-- 
Regards,
Ankita Garg (ankita@in.ibm.com)
Linux Technology Center
IBM India Systems & Technology Labs, 
Bangalore, India   

Re: [PATCH] Display -rt related stats in /proc/schedstat

[Joachim Deguara]

On Monday 23 July 2007 16:58:43 ankita@in.ibm.com wrote:
> As for documentation update, Documentation/sched-stats.txt seems to be a
> little old.

Look in the -mm tree, my patch to bring it up to date got in there last 
week ;)

-Joachim

Re: [PATCH] Display -rt related stats in /proc/schedstat

[John Sigler]

Ankita Garg wrote:

> This patch adds support to display captured -rt stats under /proc/schedstat.
> 
> Signed-off-by: Ankita Garg <ankita@in.ibm.com>
> --
>  sched_stats.h |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.22.1/kernel/sched_stats.h
> ===================================================================
> --- linux-2.6.22.1.orig/kernel/sched_stats.h	2007-07-23 18:15:57.000000000 +0530
> +++ linux-2.6.22.1/kernel/sched_stats.h	2007-07-23 18:24:03.000000000 +0530
> @@ -21,12 +21,15 @@
>  
>  		/* runqueue-specific stats */
>  		seq_printf(seq,
> -		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %llu %lu",
> +		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %lu %lu %lu %llu %lu",
>  		    cpu, rq->yld_both_empty,
>  		    rq->yld_act_empty, rq->yld_exp_empty, rq->yld_cnt,
>  		    rq->sched_switch, rq->sched_cnt, rq->sched_goidle,
>  		    rq->ttwu_cnt, rq->ttwu_local,
>  		    rq->rq_sched_info.cpu_time,
> +#ifdef CONFIG_PREEMPT_RT
> +		    rq->rto_schedule, rq->rto_wakeup, rq->rto_pulled,
> +#endif
>  		    rq->rq_sched_info.run_delay, rq->rq_sched_info.pcnt);
>  
>  		seq_printf(seq, "\n");

I know you've written a new patch that makes this one obsolete, but I 
wanted to discuss this specific patch.

Originally, there were 13 conversion specifications, and your patch
adds 3, for a total of 16 conversion specifications.

However, if CONFIG_PREEMPT_RT is not defined, then seq_printf is
called with only 13 arguments.

Is it safe to call seq_printf with too few arguments?

Calling printf(3) with too few arguments leads to undefined behavior.

seq_printf is a wrapper around vsnprintf() so there might be no issue?

Regards.

Re: [PATCH] Display -rt related stats in /proc/schedstat

[Ankita Garg]

On Tue, Jul 24, 2007 at 12:36:22PM +0200, John Sigler wrote:
> Ankita Garg wrote:
> 
> >This patch adds support to display captured -rt stats under 
> >/proc/schedstat.
> >
> >Signed-off-by: Ankita Garg <ankita@in.ibm.com>
> >--
> > sched_stats.h |    5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> >Index: linux-2.6.22.1/kernel/sched_stats.h
> >===================================================================
> >--- linux-2.6.22.1.orig/kernel/sched_stats.h	2007-07-23 
> >18:15:57.000000000 +0530
> >+++ linux-2.6.22.1/kernel/sched_stats.h	2007-07-23 
> >18:24:03.000000000 +0530
> >@@ -21,12 +21,15 @@
> > 
> > 		/* runqueue-specific stats */
> > 		seq_printf(seq,
> >-		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %llu 
> >%lu",
> >+		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %lu %lu 
> >%lu %llu %lu",
> > 		    cpu, rq->yld_both_empty,
> > 		    rq->yld_act_empty, rq->yld_exp_empty, rq->yld_cnt,
> > 		    rq->sched_switch, rq->sched_cnt, rq->sched_goidle,
> > 		    rq->ttwu_cnt, rq->ttwu_local,
> > 		    rq->rq_sched_info.cpu_time,
> >+#ifdef CONFIG_PREEMPT_RT
> >+		    rq->rto_schedule, rq->rto_wakeup, rq->rto_pulled,
> >+#endif
> > 		    rq->rq_sched_info.run_delay, rq->rq_sched_info.pcnt);
> > 
> > 		seq_printf(seq, "\n");
> 
> I know you've written a new patch that makes this one obsolete, but I 
> wanted to discuss this specific patch.
> 
> Originally, there were 13 conversion specifications, and your patch
> adds 3, for a total of 16 conversion specifications.
> 
> However, if CONFIG_PREEMPT_RT is not defined, then seq_printf is
> called with only 13 arguments.
> 
> Is it safe to call seq_printf with too few arguments?

So I tried to run the code disabling CONFIG_PREEMPT_RT, to see the
behavior vsnprintf() on supplying fewer arguments. Arbit/garbage value is 
printed for conversion specifications that do not have a matching argument.
It did not cause any issue at all beyond that point. But considering the
correctness and quality of code, was going to post the updated patch (if
the re-work had not happened)!

> 
> Calling printf(3) with too few arguments leads to undefined behavior.
> 
> seq_printf is a wrapper around vsnprintf() so there might be no issue?
> 
> Regards.

-- 
Regards,
Ankita Garg (ankita@in.ibm.com)
Linux Technology Center
IBM India Systems & Technology Labs, 
Bangalore, India   

Re: [PATCH] Display -rt related stats in /proc/schedstat

[Ingo Molnar]

* Ankita Garg <ankita@in.ibm.com> wrote:

> Hi,
> 
> This patch adds support to display captured -rt stats under 
> /proc/schedstat.

hm, could you add it to /proc/sched_debug instead? That's where all the 
runqueue values are showing up normally. I'm also a bit wary about 
introducing a new schedstats version for -rt.

	Ingo

Re: [PATCH] Display -rt related stats in /proc/schedstat

[Ankita Garg]

On Wed, Jul 25, 2007 at 10:05:04AM +0200, Ingo Molnar wrote:
> 
> * Ankita Garg <ankita@in.ibm.com> wrote:
> 
> > Hi,
> > 
> > This patch adds support to display captured -rt stats under 
> > /proc/schedstat.
> 
> hm, could you add it to /proc/sched_debug instead? That's where all the 
> runqueue values are showing up normally. I'm also a bit wary about 
> introducing a new schedstats version for -rt.

So, I have merged my previous patch (to display rt_nr_running info in
sched_debug.c) with this one.


Signed-off-by: Ankita Garg <ankita@in.ibm.com>
--
 sched_debug.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Index: linux-2.6.22.1/kernel/sched_debug.c
===================================================================
--- linux-2.6.22.1.orig/kernel/sched_debug.c	2007-07-25 14:26:53.000000000 +0530
+++ linux-2.6.22.1/kernel/sched_debug.c	2007-07-25 14:58:07.000000000 +0530
@@ -161,6 +161,15 @@
 	P(cpu_load[2]);
 	P(cpu_load[3]);
 	P(cpu_load[4]);
+#ifdef CONFIG_PREEMPT_RT
+	/* Print rt related rq stats */
+	P(rt_nr_running);
+	P(rt_nr_uninterruptible);
+	P(rto_schedule);
+	P(rto_wakeup);
+	P(rto_pulled);
+#endif
+
 #undef P
 
 	print_cfs_stats(m, cpu, now);


-- 
Regards,
Ankita Garg (ankita@in.ibm.com)
Linux Technology Center
IBM India Systems & Technology Labs, 
Bangalore, India   

Re: [PATCH] Display -rt related stats in /proc/schedstat

[Ingo Molnar]

* Ankita Garg <ankita@in.ibm.com> wrote:

> > > This patch adds support to display captured -rt stats under 
> > > /proc/schedstat.
> > 
> > hm, could you add it to /proc/sched_debug instead? That's where all 
> > the runqueue values are showing up normally. I'm also a bit wary 
> > about introducing a new schedstats version for -rt.
> 
> So, I have merged my previous patch (to display rt_nr_running info in 
> sched_debug.c) with this one.

thanks, applied.

	Ingo

Re: [PATCH] Display -rt related stats in /proc/schedstat

[John Sigler]

Ankita Garg wrote:
> On Tue, Jul 24, 2007 at 12:36:22PM +0200, John Sigler wrote:
>> Ankita Garg wrote:
>>
>>> This patch adds support to display captured -rt stats under 
>>> /proc/schedstat.
>>>
>>> Signed-off-by: Ankita Garg <ankita@in.ibm.com>
>>> --
>>> sched_stats.h |    5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> Index: linux-2.6.22.1/kernel/sched_stats.h
>>> ===================================================================
>>> --- linux-2.6.22.1.orig/kernel/sched_stats.h	2007-07-23 
>>> 18:15:57.000000000 +0530
>>> +++ linux-2.6.22.1/kernel/sched_stats.h	2007-07-23 
>>> 18:24:03.000000000 +0530
>>> @@ -21,12 +21,15 @@
>>>
>>> 		/* runqueue-specific stats */
>>> 		seq_printf(seq,
>>> -		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %llu 
>>> %lu",
>>> +		    "cpu%d %lu %lu %lu %lu %lu %lu %lu %lu %lu %llu %lu %lu 
>>> %lu %llu %lu",
>>> 		    cpu, rq->yld_both_empty,
>>> 		    rq->yld_act_empty, rq->yld_exp_empty, rq->yld_cnt,
>>> 		    rq->sched_switch, rq->sched_cnt, rq->sched_goidle,
>>> 		    rq->ttwu_cnt, rq->ttwu_local,
>>> 		    rq->rq_sched_info.cpu_time,
>>> +#ifdef CONFIG_PREEMPT_RT
>>> +		    rq->rto_schedule, rq->rto_wakeup, rq->rto_pulled,
>>> +#endif
>>> 		    rq->rq_sched_info.run_delay, rq->rq_sched_info.pcnt);
>>>
>>> 		seq_printf(seq, "\n");
>>
>> I know you've written a new patch that makes this one obsolete, but I 
>> wanted to discuss this specific patch.
>>
>> Originally, there were 13 conversion specifications, and your patch
>> adds 3, for a total of 16 conversion specifications.
>>
>> However, if CONFIG_PREEMPT_RT is not defined, then seq_printf is
>> called with only 13 arguments.
>>
>> Is it safe to call seq_printf with too few arguments?
> 
> So I tried to run the code disabling CONFIG_PREEMPT_RT, to see the
> behavior vsnprintf() on supplying fewer arguments. Arbit/garbage value is
> printed for conversion specifications that do not have a matching argument.
> It did not cause any issue at all beyond that point. But considering the
> correctness and quality of code, was going to post the updated patch (if
> the re-work had not happened)!

I double-checked. Calling v*printf with too few parameters also leads to 
undefined behavior.

You didn't notice any ill effects on your particular compiler / platform 
combination, but the code above remains a time bomb :-)

Once in UB land, all bets are off.

Regards.