* [PATCH] steal_account_process_tick() should return jiffies
@ 2016-03-04 22:59 Chris Friesen
2016-03-05 10:27 ` Thomas Gleixner
0 siblings, 1 reply; 8+ messages in thread
From: Chris Friesen @ 2016-03-04 22:59 UTC (permalink / raw)
To: John Stultz, Thomas Gleixner, Daniel Lezcano, Frederic Weisbecker, lkml
The callers of steal_account_process_tick() expect it to return whether
the last jiffy was stolen or not.
Currently the return value of steal_account_process_tick() is in units
of cputime, which vary between either jiffies or nsecs depending on
CONFIG_VIRT_CPU_ACCOUNTING_GEN.
The fix is to change steal_account_process_tick() to always return
jiffies. If CONFIG_VIRT_CPU_ACCOUNTING_GEN is not enabled then this
is a no-op.
As far as I can tell this bug has been present since commit dee08a72.
Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
---
kernel/sched/cputime.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index b2ab2ff..e724496 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -276,7 +276,7 @@ static __always_inline bool steal_account_process_tick(void)
this_rq()->prev_steal_time += cputime_to_nsecs(steal_ct);
account_steal_time(steal_ct);
- return steal_ct;
+ return cputime_to_jiffies(steal_ct);
}
#endif
return false;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] steal_account_process_tick() should return jiffies
2016-03-04 22:59 [PATCH] steal_account_process_tick() should return jiffies Chris Friesen
@ 2016-03-05 10:27 ` Thomas Gleixner
2016-03-05 13:19 ` Frederic Weisbecker
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2016-03-05 10:27 UTC (permalink / raw)
To: Chris Friesen
Cc: John Stultz, Daniel Lezcano, Frederic Weisbecker, lkml,
Peter Zijlstra, Ingo Molnar, Rik van Riel
Chris,
On Fri, 4 Mar 2016, Chris Friesen wrote:
First of all the subject line should contain a subsystem prefix,
i.e. "sched/cputime:"
> The callers of steal_account_process_tick() expect it to return whether
> the last jiffy was stolen or not.
>
> Currently the return value of steal_account_process_tick() is in units
> of cputime, which vary between either jiffies or nsecs depending on
> CONFIG_VIRT_CPU_ACCOUNTING_GEN.
Sure, but what is the actual problem? The return value is boolean and tells
whether there was stolen time accounted or not.
> The fix is to change steal_account_process_tick() to always return
> jiffies. If CONFIG_VIRT_CPU_ACCOUNTING_GEN is not enabled then this
> is a no-op.
What does that fix?
> As far as I can tell this bug has been present since commit dee08a72.
Which bug?
> Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
> ---
>
> kernel/sched/cputime.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index b2ab2ff..e724496 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -276,7 +276,7 @@ static __always_inline bool steal_account_process_tick(void)
> this_rq()->prev_steal_time += cputime_to_nsecs(steal_ct);
>
> account_steal_time(steal_ct);
> - return steal_ct;
> + return cputime_to_jiffies(steal_ct);
So if steal time is close to a jiffie, then cputime_to_jiffies will return 0
and you account a full jiffie to user/system/whatever.
Without a proper explanation of the problem and the resulting "bug" I really
cannot figure out why we want that change.
Thanks,
tglx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] steal_account_process_tick() should return jiffies
2016-03-05 10:27 ` Thomas Gleixner
@ 2016-03-05 13:19 ` Frederic Weisbecker
2016-03-06 4:17 ` Chris Friesen
0 siblings, 1 reply; 8+ messages in thread
From: Frederic Weisbecker @ 2016-03-05 13:19 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Chris Friesen, John Stultz, Daniel Lezcano, lkml, Peter Zijlstra,
Ingo Molnar, Rik van Riel
On Sat, Mar 05, 2016 at 11:27:01AM +0100, Thomas Gleixner wrote:
> Chris,
>
> On Fri, 4 Mar 2016, Chris Friesen wrote:
>
> First of all the subject line should contain a subsystem prefix,
> i.e. "sched/cputime:"
>
> > The callers of steal_account_process_tick() expect it to return whether
> > the last jiffy was stolen or not.
> >
> > Currently the return value of steal_account_process_tick() is in units
> > of cputime, which vary between either jiffies or nsecs depending on
> > CONFIG_VIRT_CPU_ACCOUNTING_GEN.
>
> Sure, but what is the actual problem? The return value is boolean and tells
> whether there was stolen time accounted or not.
>
> > The fix is to change steal_account_process_tick() to always return
> > jiffies. If CONFIG_VIRT_CPU_ACCOUNTING_GEN is not enabled then this
> > is a no-op.
>
> What does that fix?
>
> > As far as I can tell this bug has been present since commit dee08a72.
>
> Which bug?
>
> > Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
> > ---
> >
> > kernel/sched/cputime.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> > index b2ab2ff..e724496 100644
> > --- a/kernel/sched/cputime.c
> > +++ b/kernel/sched/cputime.c
> > @@ -276,7 +276,7 @@ static __always_inline bool steal_account_process_tick(void)
> > this_rq()->prev_steal_time += cputime_to_nsecs(steal_ct);
> >
> > account_steal_time(steal_ct);
> > - return steal_ct;
> > + return cputime_to_jiffies(steal_ct);
>
> So if steal time is close to a jiffie, then cputime_to_jiffies will return 0
> and you account a full jiffie to user/system/whatever.
>
> Without a proper explanation of the problem and the resulting "bug" I really
> cannot figure out why we want that change.
Indeed the changelog should better explain the problem. So I think the issue is that
if the cputime has nsecs granularity and we have a tiny stolen time to account (lets say
a few nanosecs, in fact anything that is below a jiffy), we are not going to account the
tick on user/system.
But the fix doesn't look right to me because we are still accounting the steal time
if it is lower than a jiffy and that steal time will never be substracted to user/system
time if it never reach a jiffy.
Instead the fix should accumulate the steal time and account it only once it's worth
a jiffy and then substract it from system/user time accordingly. Something like that:
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index b2ab2ff..d38e25f 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -262,7 +262,7 @@ static __always_inline bool steal_account_process_tick(void)
#ifdef CONFIG_PARAVIRT
if (static_key_false(¶virt_steal_enabled)) {
u64 steal;
- cputime_t steal_ct;
+ unsigned long steal_jiffies;
steal = paravirt_steal_clock(smp_processor_id());
steal -= this_rq()->prev_steal_time;
@@ -272,11 +272,11 @@ static __always_inline bool steal_account_process_tick(void)
* based on jiffies). Lets cast the result to cputime
* granularity and account the rest on the next rounds.
*/
- steal_ct = nsecs_to_cputime(steal);
- this_rq()->prev_steal_time += cputime_to_nsecs(steal_ct);
+ steal_jiffies = nsecs_to_jiffies(steal);
+ this_rq()->prev_steal_time += jiffies_to_nsecs(steal_jiffies);
account_steal_time(steal_ct);
- return steal_ct;
+ return steal_jiffies;
}
#endif
return false;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] steal_account_process_tick() should return jiffies
2016-03-05 13:19 ` Frederic Weisbecker
@ 2016-03-06 4:17 ` Chris Friesen
2016-03-06 5:18 ` [PATCH v2] sched/cputime: " Chris Friesen
0 siblings, 1 reply; 8+ messages in thread
From: Chris Friesen @ 2016-03-06 4:17 UTC (permalink / raw)
To: Frederic Weisbecker, Thomas Gleixner
Cc: John Stultz, Daniel Lezcano, lkml, Peter Zijlstra, Ingo Molnar,
Rik van Riel
On 03/05/2016 07:19 AM, Frederic Weisbecker wrote:
> On Sat, Mar 05, 2016 at 11:27:01AM +0100, Thomas Gleixner wrote:
>> Chris,
>>
>> On Fri, 4 Mar 2016, Chris Friesen wrote:
>>
>> First of all the subject line should contain a subsystem prefix,
>> i.e. "sched/cputime:"
>>
>>> The callers of steal_account_process_tick() expect it to return whether
>>> the last jiffy was stolen or not.
>>>
>>> Currently the return value of steal_account_process_tick() is in units
>>> of cputime, which vary between either jiffies or nsecs depending on
>>> CONFIG_VIRT_CPU_ACCOUNTING_GEN.
>>
>> Sure, but what is the actual problem? The return value is boolean and tells
>> whether there was stolen time accounted or not.
> Indeed the changelog should better explain the problem. So I think the issue is that
> if the cputime has nsecs granularity and we have a tiny stolen time to account (lets say
> a few nanosecs, in fact anything that is below a jiffy), we are not going to account the
> tick on user/system.
Yes, this is exactly it. Because of this, if CONFIG_VIRT_CPU_ACCOUNTING_GEN is
enabled in a guest then the idle/system/user stats in /proc/stat can show odd
values, and "top" shows nothing for user/system even if CPU hogs are running.
> But the fix doesn't look right to me because we are still accounting the steal time
> if it is lower than a jiffy and that steal time will never be substracted to user/system
> time if it never reach a jiffy.
>
> Instead the fix should accumulate the steal time and account it only once it's worth
> a jiffy and then substract it from system/user time accordingly.
Yes, on reflection you are correct, and the patch looks pretty close, except
that account_steal_time() is still expecting units of cputime. I'll send a
followup patch.
> Something like that:
>
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index b2ab2ff..d38e25f 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -262,7 +262,7 @@ static __always_inline bool steal_account_process_tick(void)
> #ifdef CONFIG_PARAVIRT
> if (static_key_false(¶virt_steal_enabled)) {
> u64 steal;
> - cputime_t steal_ct;
> + unsigned long steal_jiffies;
>
> steal = paravirt_steal_clock(smp_processor_id());
> steal -= this_rq()->prev_steal_time;
> @@ -272,11 +272,11 @@ static __always_inline bool steal_account_process_tick(void)
> * based on jiffies). Lets cast the result to cputime
> * granularity and account the rest on the next rounds.
> */
> - steal_ct = nsecs_to_cputime(steal);
> - this_rq()->prev_steal_time += cputime_to_nsecs(steal_ct);
> + steal_jiffies = nsecs_to_jiffies(steal);
> + this_rq()->prev_steal_time += jiffies_to_nsecs(steal_jiffies);
>
> account_steal_time(steal_ct);
> - return steal_ct;
> + return steal_jiffies;
> }
> #endif
> return false;
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] sched/cputime: steal_account_process_tick() should return jiffies
2016-03-06 4:17 ` Chris Friesen
@ 2016-03-06 5:18 ` Chris Friesen
2016-03-06 10:58 ` Thomas Gleixner
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Chris Friesen @ 2016-03-06 5:18 UTC (permalink / raw)
To: Frederic Weisbecker, Thomas Gleixner
Cc: John Stultz, Daniel Lezcano, lkml, Peter Zijlstra, Ingo Molnar,
Rik van Riel
The callers of steal_account_process_tick() expect it to return
whether a jiffy should be considered stolen or not.
Currently the return value of steal_account_process_tick() is in
units of cputime, which vary between either jiffies or nsecs
depending on CONFIG_VIRT_CPU_ACCOUNTING_GEN.
If cputime has nsecs granularity and there is a tiny amount of
stolen time (a few nsecs, say) then we will consider the entire
tick stolen and will not account the tick on user/system/idle,
causing /proc/stats to show invalid data.
The fix is to change steal_account_process_tick() to accumulate
the stolen time and only account it once it's worth a jiffy.
(Thanks to Frederic Weisbecker for suggestions to fix a bug in my
first version of the patch.)
Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
---
kernel/sched/cputime.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index b2ab2ff..ab2b5fb 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -262,21 +262,21 @@ static __always_inline bool steal_account_process_tick(void)
#ifdef CONFIG_PARAVIRT
if (static_key_false(¶virt_steal_enabled)) {
u64 steal;
- cputime_t steal_ct;
+ unsigned long steal_jiffies;
steal = paravirt_steal_clock(smp_processor_id());
steal -= this_rq()->prev_steal_time;
/*
- * cputime_t may be less precise than nsecs (eg: if it's
- * based on jiffies). Lets cast the result to cputime
+ * steal is in nsecs but our caller is expecting steal
+ * time in jiffies. Lets cast the result to jiffies
* granularity and account the rest on the next rounds.
*/
- steal_ct = nsecs_to_cputime(steal);
- this_rq()->prev_steal_time += cputime_to_nsecs(steal_ct);
+ steal_jiffies = nsecs_to_jiffies(steal);
+ this_rq()->prev_steal_time += jiffies_to_nsecs(steal_jiffies);
- account_steal_time(steal_ct);
- return steal_ct;
+ account_steal_time(jiffies_to_cputime(steal_jiffies));
+ return steal_jiffies;
}
#endif
return false;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] sched/cputime: steal_account_process_tick() should return jiffies
2016-03-06 5:18 ` [PATCH v2] sched/cputime: " Chris Friesen
@ 2016-03-06 10:58 ` Thomas Gleixner
2016-03-08 12:29 ` Frederic Weisbecker
2016-03-08 13:18 ` [tip:sched/core] sched/cputime: Fix steal_account_process_tick() to always " tip-bot for Chris Friesen
2 siblings, 0 replies; 8+ messages in thread
From: Thomas Gleixner @ 2016-03-06 10:58 UTC (permalink / raw)
To: Chris Friesen
Cc: Frederic Weisbecker, John Stultz, Daniel Lezcano, lkml,
Peter Zijlstra, Ingo Molnar, Rik van Riel
On Sat, 5 Mar 2016, Chris Friesen wrote:
> The callers of steal_account_process_tick() expect it to return
> whether a jiffy should be considered stolen or not.
>
> Currently the return value of steal_account_process_tick() is in
> units of cputime, which vary between either jiffies or nsecs
> depending on CONFIG_VIRT_CPU_ACCOUNTING_GEN.
>
> If cputime has nsecs granularity and there is a tiny amount of
> stolen time (a few nsecs, say) then we will consider the entire
> tick stolen and will not account the tick on user/system/idle,
> causing /proc/stats to show invalid data.
>
> The fix is to change steal_account_process_tick() to accumulate
> the stolen time and only account it once it's worth a jiffy.
>
> (Thanks to Frederic Weisbecker for suggestions to fix a bug in my
> first version of the patch.)
>
> Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] sched/cputime: steal_account_process_tick() should return jiffies
2016-03-06 5:18 ` [PATCH v2] sched/cputime: " Chris Friesen
2016-03-06 10:58 ` Thomas Gleixner
@ 2016-03-08 12:29 ` Frederic Weisbecker
2016-03-08 13:18 ` [tip:sched/core] sched/cputime: Fix steal_account_process_tick() to always " tip-bot for Chris Friesen
2 siblings, 0 replies; 8+ messages in thread
From: Frederic Weisbecker @ 2016-03-08 12:29 UTC (permalink / raw)
To: Chris Friesen
Cc: Thomas Gleixner, John Stultz, Daniel Lezcano, lkml,
Peter Zijlstra, Ingo Molnar, Rik van Riel
On Sat, Mar 05, 2016 at 11:18:48PM -0600, Chris Friesen wrote:
> The callers of steal_account_process_tick() expect it to return
> whether a jiffy should be considered stolen or not.
>
> Currently the return value of steal_account_process_tick() is in
> units of cputime, which vary between either jiffies or nsecs
> depending on CONFIG_VIRT_CPU_ACCOUNTING_GEN.
>
> If cputime has nsecs granularity and there is a tiny amount of
> stolen time (a few nsecs, say) then we will consider the entire
> tick stolen and will not account the tick on user/system/idle,
> causing /proc/stats to show invalid data.
>
> The fix is to change steal_account_process_tick() to accumulate
> the stolen time and only account it once it's worth a jiffy.
>
> (Thanks to Frederic Weisbecker for suggestions to fix a bug in my
> first version of the patch.)
>
> Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Thanks Chris!
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip:sched/core] sched/cputime: Fix steal_account_process_tick() to always return jiffies
2016-03-06 5:18 ` [PATCH v2] sched/cputime: " Chris Friesen
2016-03-06 10:58 ` Thomas Gleixner
2016-03-08 12:29 ` Frederic Weisbecker
@ 2016-03-08 13:18 ` tip-bot for Chris Friesen
2 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Chris Friesen @ 2016-03-08 13:18 UTC (permalink / raw)
To: linux-tip-commits
Cc: cbf123, mingo, stable, linux-kernel, peterz, torvalds, tglx,
chris.friesen, hpa, fweisbec
Commit-ID: f9c904b7613b8b4c85b10cd6b33ad41b2843fa9d
Gitweb: http://git.kernel.org/tip/f9c904b7613b8b4c85b10cd6b33ad41b2843fa9d
Author: Chris Friesen <cbf123@mail.usask.ca>
AuthorDate: Sat, 5 Mar 2016 23:18:48 -0600
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 8 Mar 2016 12:24:56 +0100
sched/cputime: Fix steal_account_process_tick() to always return jiffies
The callers of steal_account_process_tick() expect it to return
whether a jiffy should be considered stolen or not.
Currently the return value of steal_account_process_tick() is in
units of cputime, which vary between either jiffies or nsecs
depending on CONFIG_VIRT_CPU_ACCOUNTING_GEN.
If cputime has nsecs granularity and there is a tiny amount of
stolen time (a few nsecs, say) then we will consider the entire
tick stolen and will not account the tick on user/system/idle,
causing /proc/stats to show invalid data.
The fix is to change steal_account_process_tick() to accumulate
the stolen time and only account it once it's worth a jiffy.
(Thanks to Frederic Weisbecker for suggestions to fix a bug in my
first version of the patch.)
Signed-off-by: Chris Friesen <chris.friesen@windriver.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: <stable@vger.kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/56DBBDB8.40305@mail.usask.ca
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
kernel/sched/cputime.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 01d9898..75f98c5 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -262,21 +262,21 @@ static __always_inline bool steal_account_process_tick(void)
#ifdef CONFIG_PARAVIRT
if (static_key_false(¶virt_steal_enabled)) {
u64 steal;
- cputime_t steal_ct;
+ unsigned long steal_jiffies;
steal = paravirt_steal_clock(smp_processor_id());
steal -= this_rq()->prev_steal_time;
/*
- * cputime_t may be less precise than nsecs (eg: if it's
- * based on jiffies). Lets cast the result to cputime
+ * steal is in nsecs but our caller is expecting steal
+ * time in jiffies. Lets cast the result to jiffies
* granularity and account the rest on the next rounds.
*/
- steal_ct = nsecs_to_cputime(steal);
- this_rq()->prev_steal_time += cputime_to_nsecs(steal_ct);
+ steal_jiffies = nsecs_to_jiffies(steal);
+ this_rq()->prev_steal_time += jiffies_to_nsecs(steal_jiffies);
- account_steal_time(steal_ct);
- return steal_ct;
+ account_steal_time(jiffies_to_cputime(steal_jiffies));
+ return steal_jiffies;
}
#endif
return false;
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-03-08 13:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-04 22:59 [PATCH] steal_account_process_tick() should return jiffies Chris Friesen
2016-03-05 10:27 ` Thomas Gleixner
2016-03-05 13:19 ` Frederic Weisbecker
2016-03-06 4:17 ` Chris Friesen
2016-03-06 5:18 ` [PATCH v2] sched/cputime: " Chris Friesen
2016-03-06 10:58 ` Thomas Gleixner
2016-03-08 12:29 ` Frederic Weisbecker
2016-03-08 13:18 ` [tip:sched/core] sched/cputime: Fix steal_account_process_tick() to always " tip-bot for Chris Friesen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).