[PATCH RESEND] timer stats: reset entries when disable the timer usage statistics

[PATCH RESEND] timer stats: reset entries when disable the timer usage statistics

[Dong Zhu]

>From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
From: Dong Zhu <bluezhudong@gmail.com> 
Date: Thu, 10 Oct 2013 10:38:13 +0800

When we stop timer statistics collection (via echo 0 >
/proc/timers_stats), the statistics data is still exported as if it were
correct, which can cause applicaitons to misuse the statistics.

This patch resets the statistics when we stop collecting them, to avoid
this problem.

Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
---
 kernel/time/timer_stats.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
index 0b537f2..43f05e7 100644
--- a/kernel/time/timer_stats.c
+++ b/kernel/time/timer_stats.c
@@ -371,6 +371,7 @@ static ssize_t tstats_write(struct file *file, const char __user *buf,
 	switch (ctl[0]) {
 	case '0':
 		if (timer_stats_active) {
+			reset_entries();
 			timer_stats_active = 0;
 			time_stop = ktime_get();
 			sync_access();
-- 
1.7.11.7

-- 
Best Regards,
Dong Zhu

Re: [PATCH RESEND] timer stats: reset entries when disable the timer usage statistics

[John Stultz]

On 10/09/2013 07:59 PM, Dong Zhu wrote:
> From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> From: Dong Zhu <bluezhudong@gmail.com> 
> Date: Thu, 10 Oct 2013 10:38:13 +0800
>
> When we stop timer statistics collection (via echo 0 >
> /proc/timers_stats), the statistics data is still exported as if it were
> correct, which can cause applicaitons to misuse the statistics.
>
> This patch resets the statistics when we stop collecting them, to avoid
> this problem.
>
> Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
> ---
>  kernel/time/timer_stats.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
> index 0b537f2..43f05e7 100644
> --- a/kernel/time/timer_stats.c
> +++ b/kernel/time/timer_stats.c
> @@ -371,6 +371,7 @@ static ssize_t tstats_write(struct file *file, const char __user *buf,
>  	switch (ctl[0]) {
>  	case '0':
>  		if (timer_stats_active) {
> +			reset_entries();
>  			timer_stats_active = 0;
>  			time_stop = ktime_get();
>  			sync_access();

This looks ok to me. Thomas, do you have any objections here?

thanks
-john

Re: [PATCH RESEND] timer stats: reset entries when disable the timer usage statistics

[Ingo Molnar]

* Dong Zhu <bluezhudong@gmail.com> wrote:

> From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> From: Dong Zhu <bluezhudong@gmail.com> 
> Date: Thu, 10 Oct 2013 10:38:13 +0800
> 
> When we stop timer statistics collection (via echo 0 > 
> /proc/timers_stats), the statistics data is still exported as if it were 
> correct, which can cause applicaitons to misuse the statistics.

What misuse do you mean?

> This patch resets the statistics when we stop collecting them, to avoid 
> this problem.

Well, this loses the handy 'snapshot' property of /proc/timer_stats. 
Before this change one could do:

	echo 1 > /proc/timers_stats
	sleep 60 # run system workload
	echo 0 > /proc/timers_stats

and examine the 1-minute collection result without it changing. Your 
change, if I understand it correctly, zeroes it all out.

Instead of this change I'd suggest adding a 'status' line, with two 
outputs:

	Status: collection active

	Status: collection disabled

Thanks,

	Ingo

Re: [PATCH RESEND] timer stats: reset entries when disable the timer usage statistics

[John Stultz]

On 10/09/2013 09:35 PM, Ingo Molnar wrote:
> * Dong Zhu <bluezhudong@gmail.com> wrote:
>
>> From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
>> From: Dong Zhu <bluezhudong@gmail.com> 
>> Date: Thu, 10 Oct 2013 10:38:13 +0800
>>
>> When we stop timer statistics collection (via echo 0 > 
>> /proc/timers_stats), the statistics data is still exported as if it were 
>> correct, which can cause applicaitons to misuse the statistics.
> What misuse do you mean?
>
>> This patch resets the statistics when we stop collecting them, to avoid 
>> this problem.
> Well, this loses the handy 'snapshot' property of /proc/timer_stats. 
> Before this change one could do:
>
> 	echo 1 > /proc/timers_stats
> 	sleep 60 # run system workload
> 	echo 0 > /proc/timers_stats
>
> and examine the 1-minute collection result without it changing. Your 
> change, if I understand it correctly, zeroes it all out.


Good point. I misunderstood the patch and thought it was an issue that
the statistics are never cleared even if the collection was disabled,
but looking closer I see we clear on enable (if we were disabled). So
yes, this change doesn't seem right.



> Instead of this change I'd suggest adding a 'status' line, with two 
> outputs:
>
> 	Status: collection active
>
> 	Status: collection disabled

Agreed.

thanks
-john

Re: [PATCH v2] timer stats: add a 'status' line to timer usage statistics

[Dong Zhu]

Hi Ingo and John,

On Thu, Oct 10, 2013 at 06:35:19AM +0200, Ingo Molnar wrote:
> 
> * Dong Zhu <bluezhudong@gmail.com> wrote:
> 
> > From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> > From: Dong Zhu <bluezhudong@gmail.com> 
> > Date: Thu, 10 Oct 2013 10:38:13 +0800
> > 
> > When we stop timer statistics collection (via echo 0 > 
> > /proc/timers_stats), the statistics data is still exported as if it were 
> > correct, which can cause applicaitons to misuse the statistics.
> 
> What misuse do you mean?
> 
> > This patch resets the statistics when we stop collecting them, to avoid 
> > this problem.
> 
> Well, this loses the handy 'snapshot' property of /proc/timer_stats. 
> Before this change one could do:
> 
> 	echo 1 > /proc/timers_stats
> 	sleep 60 # run system workload
> 	echo 0 > /proc/timers_stats
> 
> and examine the 1-minute collection result without it changing. Your 
> change, if I understand it correctly, zeroes it all out.

Yes, I am wrong about it. Thanks for pointing this out and pretty sorry
for confusing you John.

> 
> Instead of this change I'd suggest adding a 'status' line, with two 
> outputs:
> 
> 	Status: collection active
> 
> 	Status: collection disabled
> 

Agree, I modified this patch and resubmited it again, Could you help
reviewing it again ? Thanks !

>From 263c40abea8011c82582b2d671ae783b26f44bd5 Mon Sep 17 00:00:00 2001
From: Dong Zhu <bluezhudong@gmail.com> 
Date: Thu, 10 Oct 2013 13:46:08 +0800

We can enable|disable timer statistics collection (via echo [1|0] >
/proc/timers_stats), this patch adds a 'status' line to display the
current timer collection status.

Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
---
 kernel/time/timer_stats.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
index 0b537f2..bac5e91 100644
--- a/kernel/time/timer_stats.c
+++ b/kernel/time/timer_stats.c
@@ -303,6 +303,10 @@ static int tstats_show(struct seq_file *m, void *v)
 	if (atomic_read(&overflow_count))
 		seq_printf(m, "Overflow: %d entries\n",
 			atomic_read(&overflow_count));
+	if (timer_stats_active)
+		seq_puts(m, "Status: collection active\n");
+	else
+		seq_puts(m, "Status: collection disabled\n");
 
 	for (i = 0; i < nr_entries; i++) {
 		entry = entries + i;
-- 
1.7.11.7


-- 
Best Regards,
Dong Zhu

Re: [PATCH v2] timer stats: add a 'status' line to timer usage statistics

[Ingo Molnar]

* Dong Zhu <bluezhudong@gmail.com> wrote:

> Hi Ingo and John,
> 
> On Thu, Oct 10, 2013 at 06:35:19AM +0200, Ingo Molnar wrote:
> > 
> > * Dong Zhu <bluezhudong@gmail.com> wrote:
> > 
> > > From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> > > From: Dong Zhu <bluezhudong@gmail.com> 
> > > Date: Thu, 10 Oct 2013 10:38:13 +0800
> > > 
> > > When we stop timer statistics collection (via echo 0 > 
> > > /proc/timers_stats), the statistics data is still exported as if it were 
> > > correct, which can cause applicaitons to misuse the statistics.
> > 
> > What misuse do you mean?
> > 
> > > This patch resets the statistics when we stop collecting them, to avoid 
> > > this problem.
> > 
> > Well, this loses the handy 'snapshot' property of /proc/timer_stats. 
> > Before this change one could do:
> > 
> > 	echo 1 > /proc/timers_stats
> > 	sleep 60 # run system workload
> > 	echo 0 > /proc/timers_stats
> > 
> > and examine the 1-minute collection result without it changing. Your 
> > change, if I understand it correctly, zeroes it all out.
> 
> Yes, I am wrong about it. Thanks for pointing this out and pretty sorry
> for confusing you John.
> 
> > 
> > Instead of this change I'd suggest adding a 'status' line, with two 
> > outputs:
> > 
> > 	Status: collection active
> > 
> > 	Status: collection disabled
> > 
> 
> Agree, I modified this patch and resubmited it again, Could you help
> reviewing it again ? Thanks !
> 
> From 263c40abea8011c82582b2d671ae783b26f44bd5 Mon Sep 17 00:00:00 2001
> From: Dong Zhu <bluezhudong@gmail.com> 
> Date: Thu, 10 Oct 2013 13:46:08 +0800
> 
> We can enable|disable timer statistics collection (via echo [1|0] >
> /proc/timers_stats), this patch adds a 'status' line to display the
> current timer collection status.
> 
> Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
> ---
>  kernel/time/timer_stats.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
> index 0b537f2..bac5e91 100644
> --- a/kernel/time/timer_stats.c
> +++ b/kernel/time/timer_stats.c
> @@ -303,6 +303,10 @@ static int tstats_show(struct seq_file *m, void *v)
>  	if (atomic_read(&overflow_count))
>  		seq_printf(m, "Overflow: %d entries\n",
>  			atomic_read(&overflow_count));
> +	if (timer_stats_active)
> +		seq_puts(m, "Status: collection active\n");
> +	else
> +		seq_puts(m, "Status: collection disabled\n");

I suspect we could do something like:

	seq_printf("Status: collection %s\n", timer_stats_active ? "enabled" : "disabled");

and save a bit of kernel image size?

Also, please bump up the version to v0.3, to give parsers a chance.

Otherwise it looks good to me.

Thanks,

	Ingo

Re: [PATCH v0.3] timer stats: add a 'status' line to timer usage statistics

[Dong Zhu]

> > Agree, I modified this patch and resubmited it again, Could you help
> > reviewing it again ? Thanks !
> > 
> > From 263c40abea8011c82582b2d671ae783b26f44bd5 Mon Sep 17 00:00:00 2001
> > From: Dong Zhu <bluezhudong@gmail.com> 
> > Date: Thu, 10 Oct 2013 13:46:08 +0800
> > 
> > We can enable|disable timer statistics collection (via echo [1|0] >
> > /proc/timers_stats), this patch adds a 'status' line to display the
> > current timer collection status.
> > 
> > Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
> > ---
> >  kernel/time/timer_stats.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
> > index 0b537f2..bac5e91 100644
> > --- a/kernel/time/timer_stats.c
> > +++ b/kernel/time/timer_stats.c
> > @@ -303,6 +303,10 @@ static int tstats_show(struct seq_file *m, void *v)
> >  	if (atomic_read(&overflow_count))
> >  		seq_printf(m, "Overflow: %d entries\n",
> >  			atomic_read(&overflow_count));
> > +	if (timer_stats_active)
> > +		seq_puts(m, "Status: collection active\n");
> > +	else
> > +		seq_puts(m, "Status: collection disabled\n");
> 
> I suspect we could do something like:
> 
> 	seq_printf("Status: collection %s\n", timer_stats_active ? "enabled" : "disabled");
> 
> and save a bit of kernel image size?
> 
> Also, please bump up the version to v0.3, to give parsers a chance.
> 
> Otherwise it looks good to me.
> 
Thanks for your comments, I refined this patch as below: 
>From 1035e4e2b7ff28b1b2fccd407929179b5de8fbd4 Mon Sep 17 00:00:00 2001
From: Dong Zhu <bluezhudong@gmail.com> 
Date: Thu, 10 Oct 2013 14:48:24 +0800

We can enable|disable timer statistics collection (via echo [1|0] >
/proc/timers_stats), this patch adds a 'status' line to display the
current timer collection status.

Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
---
 kernel/time/timer_stats.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
index 0b537f2..5236b45 100644
--- a/kernel/time/timer_stats.c
+++ b/kernel/time/timer_stats.c
@@ -303,6 +303,8 @@ static int tstats_show(struct seq_file *m, void *v)
 	if (atomic_read(&overflow_count))
 		seq_printf(m, "Overflow: %d entries\n",
 			atomic_read(&overflow_count));
+	seq_printf(m, "Status: collection %s\n",
+		timer_stats_active ? "enabled" : "disabled");
 
 	for (i = 0; i < nr_entries; i++) {
 		entry = entries + i;
-- 
1.7.11.7


-- 
Best Regards,
Dong Zhu

Re: [PATCH v0.3] timer stats: add a 'status' line to timer usage statistics

[Ingo Molnar]

* Dong Zhu <bluezhudong@gmail.com> wrote:

> > > Agree, I modified this patch and resubmited it again, Could you help
> > > reviewing it again ? Thanks !
> > > 
> > > From 263c40abea8011c82582b2d671ae783b26f44bd5 Mon Sep 17 00:00:00 2001
> > > From: Dong Zhu <bluezhudong@gmail.com> 
> > > Date: Thu, 10 Oct 2013 13:46:08 +0800
> > > 
> > > We can enable|disable timer statistics collection (via echo [1|0] >
> > > /proc/timers_stats), this patch adds a 'status' line to display the
> > > current timer collection status.
> > > 
> > > Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
> > > ---
> > >  kernel/time/timer_stats.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
> > > index 0b537f2..bac5e91 100644
> > > --- a/kernel/time/timer_stats.c
> > > +++ b/kernel/time/timer_stats.c
> > > @@ -303,6 +303,10 @@ static int tstats_show(struct seq_file *m, void *v)
> > >  	if (atomic_read(&overflow_count))
> > >  		seq_printf(m, "Overflow: %d entries\n",
> > >  			atomic_read(&overflow_count));
> > > +	if (timer_stats_active)
> > > +		seq_puts(m, "Status: collection active\n");
> > > +	else
> > > +		seq_puts(m, "Status: collection disabled\n");
> > 
> > I suspect we could do something like:
> > 
> > 	seq_printf("Status: collection %s\n", timer_stats_active ? "enabled" : "disabled");
> > 
> > and save a bit of kernel image size?
> > 
> > Also, please bump up the version to v0.3, to give parsers a chance.

I think you forgot about this suggestion of mine.

Thanks,

	Ingo

Re: [PATCH v4] timer stats: add a 'status' line to timer usage statistics

[Dong Zhu]

> > > I suspect we could do something like:
> > > 
> > > 	seq_printf("Status: collection %s\n", timer_stats_active ? "enabled" : "disabled");
> > > 
> > > and save a bit of kernel image size?
> > > 
> > > Also, please bump up the version to v0.3, to give parsers a chance.
> 
> I think you forgot about this suggestion of mine.
> 

Oh, oops, sorry about that.

I modified the patch again as below, hope it could be applied this time.

>From 92a67f74766e02b5f4702b1af2af9861bf70d60b Mon Sep 17 00:00:00 2001
From: Dong Zhu <bluezhudong@gmail.com> 
Date: Thu, 10 Oct 2013 15:48:29 +0800

We can enable|disable timer statistics collection (via echo [1|0] >
/proc/timers_stats), this patch adds a 'status' line to display the
current timer collection status. In the meantime I also bump up the
timer stats version to v0.3.

Signed-off-by: Dong Zhu <bluezhudong@gmail.com> 
---
 kernel/time/timer_stats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
index 0b537f2..7c31ab7 100644
--- a/kernel/time/timer_stats.c
+++ b/kernel/time/timer_stats.c
@@ -298,11 +298,13 @@ static int tstats_show(struct seq_file *m, void *v)
 	period = ktime_to_timespec(time);
 	ms = period.tv_nsec / 1000000;
 
-	seq_puts(m, "Timer Stats Version: v0.2\n");
+	seq_puts(m, "Timer Stats Version: v0.3\n");
 	seq_printf(m, "Sample period: %ld.%03ld s\n", period.tv_sec, ms);
 	if (atomic_read(&overflow_count))
 		seq_printf(m, "Overflow: %d entries\n",
 			atomic_read(&overflow_count));
+	seq_printf(m, "Status: collection %s\n",
+		timer_stats_active ? "enabled" : "disabled");
 
 	for (i = 0; i < nr_entries; i++) {
 		entry = entries + i;
-- 
1.7.11.7


-- 
Best Regards,
Dong Zhu

[tip:timers/core] timer stats: Add a 'Collection: active/inactive ' line to timer usage statistics

[tip-bot for Dong Zhu]

Commit-ID:  2cb763614c1c5baef58045af9304265075f22d0a
Gitweb:     http://git.kernel.org/tip/2cb763614c1c5baef58045af9304265075f22d0a
Author:     Dong Zhu <bluezhudong@gmail.com>
AuthorDate: Thu, 10 Oct 2013 15:56:18 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 10 Oct 2013 09:59:25 +0200

timer stats: Add a 'Collection: active/inactive' line to timer usage statistics

We can enable/disable timer statistics collection via:

  echo [1|0] > /proc/timers_stats

and it would be nice if apps had the ability to check
what the current collection status is.

This patch adds a 'Collection: active/inactive' line to display the
current timer collection status.

Also bump up the timer stats version to v0.3.

Signed-off-by: Dong Zhu <bluezhudong@gmail.com>
Cc: John Stultz <john.stultz@linaro.org>
Link: http://lkml.kernel.org/r/20131010075618.GH2139@zhudong.nay.redhat.com
[ Improved the changelog and the code. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/time/timer_stats.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/time/timer_stats.c b/kernel/time/timer_stats.c
index 0b537f2..1fb08f2 100644
--- a/kernel/time/timer_stats.c
+++ b/kernel/time/timer_stats.c
@@ -298,15 +298,15 @@ static int tstats_show(struct seq_file *m, void *v)
 	period = ktime_to_timespec(time);
 	ms = period.tv_nsec / 1000000;
 
-	seq_puts(m, "Timer Stats Version: v0.2\n");
+	seq_puts(m, "Timer Stats Version: v0.3\n");
 	seq_printf(m, "Sample period: %ld.%03ld s\n", period.tv_sec, ms);
 	if (atomic_read(&overflow_count))
-		seq_printf(m, "Overflow: %d entries\n",
-			atomic_read(&overflow_count));
+		seq_printf(m, "Overflow: %d entries\n", atomic_read(&overflow_count));
+	seq_printf(m, "Collection: %s\n", timer_stats_active ? "active" : "inactive");
 
 	for (i = 0; i < nr_entries; i++) {
 		entry = entries + i;
- 		if (entry->timer_flag & TIMER_STATS_FLAG_DEFERRABLE) {
+		if (entry->timer_flag & TIMER_STATS_FLAG_DEFERRABLE) {
 			seq_printf(m, "%4luD, %5d %-16s ",
 				entry->count, entry->pid, entry->comm);
 		} else {