[PATCH] steal_account_process_tick() should return jiffies

[PATCH] steal_account_process_tick() should return jiffies

[Chris Friesen]

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;

Re: [PATCH] steal_account_process_tick() should return jiffies

[Thomas Gleixner]

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

Re: [PATCH] steal_account_process_tick() should return jiffies

[Frederic Weisbecker]

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(&paravirt_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;

Re: [PATCH] steal_account_process_tick() should return jiffies

[Chris Friesen]

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(&paravirt_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;
>

[PATCH v2] sched/cputime: steal_account_process_tick() should return jiffies

[Chris Friesen]

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(&paravirt_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;

Re: [PATCH v2] sched/cputime: steal_account_process_tick() should return jiffies

[Thomas Gleixner]

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>

Re: [PATCH v2] sched/cputime: steal_account_process_tick() should return jiffies

[Frederic Weisbecker]

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!

[tip:sched/core] sched/cputime: Fix steal_account_process_tick() to always return jiffies

[tip-bot for Chris Friesen]

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(&paravirt_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;