steal_account_process_tick() should return jiffies

PATCH Superseded Chris Friesen 4 participants 8 messages Mar 4, 2016 – Mar 8, 2016

Subsystems (2)

Stable branch Scheduler

lore

Share & tools

lore

Chris Friesen

author

2016-03-04 23:07 UTC #0 lore raw

PATCH 1/1 steal_account_process_tick() should return jiffies

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 <[email protected]>

---

 kernel/sched/cputime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/sched/cputime.c +1 -1

index b2ab2ff..e724496 100644
@@ -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;

Thomas Gleixner

2016-03-05 10:28 UTC #1 lore raw

Chris,

On Fri, 4 Mar 2016, Chris Friesen wrote:

First of all the subject line should contain a subsystem prefix, i.e. "sched/cputime:"

Quoted 6 lines

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.

Quoted 3 lines

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?

Quoted 1 line

As far as I can tell this bug has been present since commit dee08a72.

Which bug?

Quoted 11 lines

Signed-off-by Chris Friesen <[email protected]>
---
 kernel/sched/cputime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/sched/cputime.c +0 -0
index b2ab2ff..e724496 100644

kernel/sched/cputime.c static __always_inline bool steal_account_process_tick(void) · line 279

Quoted 5 lines

kernel/sched/cputime.c +1 -1

@@ -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

Frederic Weisbecker

2016-03-05 13:19 UTC #2 lore raw

Quoted 47 lines · 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:"
Quoted 6 lines
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.
Quoted 3 lines
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?
Quoted 1 line
As far as I can tell this bug has been present since commit dee08a72.
Which bug?
Quoted 11 lines
Signed-off-by Chris Friesen <[email protected]>
---
 kernel/sched/cputime.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/sched/cputime.c +0 -0
index b2ab2ff..e724496 100644
kernel/sched/cputime.c static __always_inline bool steal_account_process_tick(void) · line 279
Quoted 5 lines
kernel/sched/cputime.c +1 -1
@@ -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.

Quoted 2 lines

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:

kernel/sched/cputime.c +4 -4

index b2ab2ff..d38e25f 100644
@@ -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;

Chris Friesen

author

2016-03-06 04:17 UTC #3 lore raw

Quoted 15 lines · Frederic Weisbecker

Quoted 14 lines · 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:"
Quoted 6 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 6 lines

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.

Quoted 31 lines

Something like that:

kernel/sched/cputime.c +4 -4

index b2ab2ff..d38e25f 100644
@@ -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;

Chris Friesen

author

2016-03-06 05:18 UTC #4 lore raw

PATCH v2 1/1 sched/cputime: steal_account_process_tick() should 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 <[email protected]>

---
 kernel/sched/cputime.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

kernel/sched/cputime.c +7 -7

index b2ab2ff..ab2b5fb 100644
@@ -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;

Thomas Gleixner

Reviewed-by on v2 1/1

2016-03-06 11:00 UTC #5 lore raw

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

Quoted 12 lines · 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.

Quoted 7 lines

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 <[email protected]>

Reviewed-by Thomas Gleixner <[email protected]>

Frederic Weisbecker

Acked-by on v2 1/1

2016-03-08 12:29 UTC #6 lore raw

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

In reply to Chris Friesen

Quoted 12 lines · 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.

Quoted 7 lines

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 <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Thanks Chris!

tip-bot for Chris Friesen

merge bot

2016-03-08 13:18 UTC #7 lore raw

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

In reply to Chris Friesen

sched/cputime: Fix steal_account_process_tick() to always return jiffies

Commit-ID:  f9c904b7613b8b4c85b10cd6b33ad41b2843fa9d
Gitweb:     http://git.kernel.org/tip/f9c904b7613b8b4c85b10cd6b33ad41b2843fa9d
Author:     Chris Friesen <[email protected]>
AuthorDate: Sat, 5 Mar 2016 23:18:48 -0600
Committer:  Ingo Molnar <[email protected]>
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.

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 <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Reviewed-by Thomas Gleixner <[email protected]>

Cc <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/cputime.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

kernel/sched/cputime.c +7 -7

index 01d9898..75f98c5 100644
@@ -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;