>From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
From: Dong Zhu <[email protected]>
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 <[email protected]>
---
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
On 10/09/2013 07:59 PM, Dong Zhu wrote:
> From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> From: Dong Zhu <[email protected]>
> 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 <[email protected]>
> ---
> 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
* Dong Zhu <[email protected]> wrote:
> From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> From: Dong Zhu <[email protected]>
> 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
On 10/09/2013 09:35 PM, Ingo Molnar wrote:
> * Dong Zhu <[email protected]> wrote:
>
>> From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
>> From: Dong Zhu <[email protected]>
>> 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
Hi Ingo and John,
On Thu, Oct 10, 2013 at 06:35:19AM +0200, Ingo Molnar wrote:
>
> * Dong Zhu <[email protected]> wrote:
>
> > From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> > From: Dong Zhu <[email protected]>
> > 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 <[email protected]>
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 <[email protected]>
---
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
* Dong Zhu <[email protected]> wrote:
> Hi Ingo and John,
>
> On Thu, Oct 10, 2013 at 06:35:19AM +0200, Ingo Molnar wrote:
> >
> > * Dong Zhu <[email protected]> wrote:
> >
> > > From f41628c61d8a9172677ba33a55b61e37ce28f7a6 Mon Sep 17 00:00:00 2001
> > > From: Dong Zhu <[email protected]>
> > > 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
> > 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 <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]>
---
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
* Dong Zhu <[email protected]> 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 <[email protected]>
> > > 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 <[email protected]>
> > > ---
> > > 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
> > > 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 <[email protected]>
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 <[email protected]>
---
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
Commit-ID: 2cb763614c1c5baef58045af9304265075f22d0a
Gitweb: http://git.kernel.org/tip/2cb763614c1c5baef58045af9304265075f22d0a
Author: Dong Zhu <[email protected]>
AuthorDate: Thu, 10 Oct 2013 15:56:18 +0800
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: John Stultz <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ Improved the changelog and the code. ]
Signed-off-by: Ingo Molnar <[email protected]>
---
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 {