procfs: do not confuse jiffies with cputime64_t

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies.

Signed-off-by Andreas Schwab <[email protected]>

---
 fs/proc/stat.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

fs/proc/stat.c +2 -2

index 2a30d67..88f8a55 100644
@@ -32,7 +32,7 @@ static cputime64_t get_idle_time(int cpu)
 		idle = kstat_cpu(cpu).cpustat.idle;
 		idle = cputime64_add(idle, arch_idle_time(cpu));
 	} else
-		idle = nsecs_to_jiffies64(1000 * idle_time);
+		idle = jiffies64_to_cputime64(nsecs_to_jiffies64(1000 * idle_time));
 
 	return idle;
 }

@@ -46,7 +46,7 @@ static cputime64_t get_iowait_time(int cpu)
 		/* !NO_HZ so we can rely on cpustat.iowait */
 		iowait = kstat_cpu(cpu).cpustat.iowait;
 	else
-		iowait = nsecs_to_jiffies64(1000 * iowait_time);
+		iowait = jiffies64_to_cputime64(nsecs_to_jiffies64(1000 * iowait_time));
 
 	return iowait;
 }

Signature · 7 lines

1.7.8


-- 
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Review static cputime64_t get_iowait_time(int cpu) · line 51

Quoted 8 lines · Andreas Schwab

(quoted diff) +1 -1

@@ -46,7 +46,7 @@ static cputime64_t get_iowait_time(int cpu)
                /* !NO_HZ so we can rely on cpustat.iowait */
                iowait = kstat_cpu(cpu).cpustat.iowait;
        else
-               iowait = nsecs_to_jiffies64(1000 * iowait_time);
+               iowait = jiffies64_to_cputime64(nsecs_to_jiffies64(1000 * iowait_time));
 
        return iowait;

Hmm, shouldn't this be using nsecs_to_cputime64()? For some reason however, that function is (incorrectly?) defined as

include/asm-generic/cputime.h:#define nsecs_to_cputime64(__ct) nsecs_to_jiffies64(__ct)

and only used in one place, in

kernel/sched.c: if (cputime64_gt(nsecs_to_cputime64(latest_ns), cpustat->irq)) kernel/sched.c: if (cputime64_gt(nsecs_to_cputime64(latest_ns), cpustat->softirq))

I'm not sure what the correct solution is, but I would assume that ia64 and powerpc should fix their definitions of nsecs_to_cputime64() anyway.

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

Signed-off-by Andreas Schwab <[email protected]>

---
 arch/ia64/include/asm/cputime.h    |    1 +
 arch/powerpc/include/asm/cputime.h |    2 ++
 fs/proc/stat.c                     |    4 ++--
 include/asm-generic/cputime.h      |    1 +
 4 files changed, 6 insertions(+), 2 deletions(-)

arch/ia64/include/asm/cputime.h +1 -0

index 6073b18..5a274af 100644
@@ -60,6 +60,7 @@ typedef u64 cputime64_t;
  */
 #define cputime_to_usecs(__ct)		((__ct) / NSEC_PER_USEC)
 #define usecs_to_cputime(__usecs)	((__usecs) * NSEC_PER_USEC)
+#define usecs_to_cputime64(__usecs)	usecs_to_cputime(__usecs)
 
 /*
  * Convert cputime <-> seconds

arch/powerpc/include/asm/cputime.h +2 -0

index 33a3580..fa3f921 100644
@@ -150,6 +150,8 @@ static inline cputime_t usecs_to_cputime(const unsigned long us)
 	return ct;
 }
 
+#define usecs_to_cputime64(us)		usecs_to_cputime(us)
+
 /*
  * Convert cputime <-> seconds
  */

fs/proc/stat.c +2 -2

index 2a30d67..0855e6f 100644
@@ -32,7 +32,7 @@ static cputime64_t get_idle_time(int cpu)
 		idle = kstat_cpu(cpu).cpustat.idle;
 		idle = cputime64_add(idle, arch_idle_time(cpu));
 	} else
-		idle = nsecs_to_jiffies64(1000 * idle_time);
+		idle = usecs_to_cputime64(idle_time);
 
 	return idle;
 }

@@ -46,7 +46,7 @@ static cputime64_t get_iowait_time(int cpu)
 		/* !NO_HZ so we can rely on cpustat.iowait */
 		iowait = kstat_cpu(cpu).cpustat.iowait;
 	else
-		iowait = nsecs_to_jiffies64(1000 * iowait_time);
+		iowait = usecs_to_cputime64(iowait_time);
 
 	return iowait;
 }

include/asm-generic/cputime.h +1 -0

index 62ce682..12a1764 100644
@@ -40,6 +40,7 @@ typedef u64 cputime64_t;
  */
 #define cputime_to_usecs(__ct)		jiffies_to_usecs(__ct)
 #define usecs_to_cputime(__msecs)	usecs_to_jiffies(__msecs)
+#define usecs_to_cputime64(__msecs)	nsecs_to_jiffies64((__msecs) * 1000)
 
 /*
  * Convert cputime to seconds and back.

Signature · 12 lines

1.7.8


-- 
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

Signed-off-by Andreas Schwab <[email protected]>

---
 arch/ia64/include/asm/cputime.h    |    1 +
 arch/powerpc/include/asm/cputime.h |    2 ++
 arch/s390/include/asm/cputime.h    |    2 ++
 fs/proc/stat.c                     |    4 ++--
 include/asm-generic/cputime.h      |    1 +
 5 files changed, 8 insertions(+), 2 deletions(-)

arch/ia64/include/asm/cputime.h +1 -0

index 6073b18..5a274af 100644
@@ -60,6 +60,7 @@ typedef u64 cputime64_t;
  */
 #define cputime_to_usecs(__ct)		((__ct) / NSEC_PER_USEC)
 #define usecs_to_cputime(__usecs)	((__usecs) * NSEC_PER_USEC)
+#define usecs_to_cputime64(__usecs)	usecs_to_cputime(__usecs)
 
 /*
  * Convert cputime <-> seconds

arch/powerpc/include/asm/cputime.h +2 -0

index 33a3580..fa3f921 100644
@@ -150,6 +150,8 @@ static inline cputime_t usecs_to_cputime(const unsigned long us)
 	return ct;
 }
 
+#define usecs_to_cputime64(us)		usecs_to_cputime(us)
+
 /*
  * Convert cputime <-> seconds
  */

arch/s390/include/asm/cputime.h +2 -0

index 0814348..b9acaaa 100644
@@ -87,6 +87,8 @@ usecs_to_cputime(const unsigned int m)
 	return (cputime_t) m * 4096;
 }
 
+#define usecs_to_cputime64(m)		usecs_to_cputime(m)
+
 /*
  * Convert cputime to milliseconds and back.
  */

fs/proc/stat.c +2 -2

index 2a30d67..0855e6f 100644
@@ -32,7 +32,7 @@ static cputime64_t get_idle_time(int cpu)
 		idle = kstat_cpu(cpu).cpustat.idle;
 		idle = cputime64_add(idle, arch_idle_time(cpu));
 	} else
-		idle = nsecs_to_jiffies64(1000 * idle_time);
+		idle = usecs_to_cputime64(idle_time);
 
 	return idle;
 }

@@ -46,7 +46,7 @@ static cputime64_t get_iowait_time(int cpu)
 		/* !NO_HZ so we can rely on cpustat.iowait */
 		iowait = kstat_cpu(cpu).cpustat.iowait;
 	else
-		iowait = nsecs_to_jiffies64(1000 * iowait_time);
+		iowait = usecs_to_cputime64(iowait_time);
 
 	return iowait;
 }

include/asm-generic/cputime.h +1 -0

index 62ce682..12a1764 100644
@@ -40,6 +40,7 @@ typedef u64 cputime64_t;
  */
 #define cputime_to_usecs(__ct)		jiffies_to_usecs(__ct)
 #define usecs_to_cputime(__msecs)	usecs_to_jiffies(__msecs)
+#define usecs_to_cputime64(__msecs)	nsecs_to_jiffies64((__msecs) * 1000)
 
 /*
  * Convert cputime to seconds and back.

Signature · 11 lines

1.7.8

-- 
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

From 1fca39b21f3b344c90c30d98db6dcdcdc6815797 Mon Sep 17 00:00:00 2001

Hmm, it seems that this bugfix (for 3.2) stalled. I guess that it is primarily because it is multiarch fix.

I am sorry to bother you Andrew but could we push this through you, please?

The full patch for reference:
---

Quoted 1 line

From 1fca39b21f3b344c90c30d98db6dcdcdc6815797 Mon Sep 17 00:00:00 2001

From: Andreas Schwab <[email protected]>
Date: Mon, 12 Dec 2011 14:07:53 +0100
Subject: [PATCH] procfs: do not confuse jiffies with cputime64_t

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

Hmm, it seems that this bugfix (for 3.2) stalled. I guess that it is primarily because it is multiarch fix.
I am sorry to bother you Andrew but could we push this through you, please?

The full patch for reference:
---

Quoted 1 line

From 1fca39b21f3b344c90c30d98db6dcdcdc6815797 Mon Sep 17 00:00:00 2001

From: Andreas Schwab <[email protected]>
Date: Mon, 12 Dec 2011 14:07:53 +0100
Subject: [PATCH] procfs: do not confuse jiffies with cputime64_t

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

From: Andreas Schwab <[email protected]>
Subject: procfs: do not confuse jiffies with cputime64_t

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

Quoted 3 lines · Andrew Morton

From: Andreas Schwab <[email protected]>
Subject: procfs: do not confuse jiffies with cputime64_t

Quoted 2 lines

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

For most architectures (which use the asm-generic/cputime.h) jiffies use the same unit as cputime64_t values, but ia64, powerpc and s390 make them different.

Quoted 8 lines · On Thu, 22 Dec 2011 00:50:06 +0100 Andreas Schwab <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

From: Andreas Schwab <[email protected]>
Subject: procfs: do not confuse jiffies with cputime64_t

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

Quoted 4 lines

For most architectures (which use the asm-generic/cputime.h) jiffies use the same unit as cputime64_t values, but ia64, powerpc and s390 make them different.

OK, but

a) what effect does this problem have upon users of the kernel?

b) which kernel version(s) are affected?

Quoted 1 line

a) what effect does this problem have upon users of the kernel?

get_idle_time returns the value in the wrong unit.

Quoted 3 lines · On Thu, 22 Dec 2011 01:14:39 +0100 Andreas Schwab <[email protected]> wrote:

Quoted 1 line

a) what effect does this problem have upon users of the kernel?

get_idle_time returns the value in the wrong unit.

So the "idle" fields in /proc/stat are wrong.

We're getting there! Now, in what way are they wrong?

How could a kernel maintainer recognise that this patch is needed?

Also, in include/asm-generic/cputime.h we have:

#define usecs_to_cputime64(__msecs) nsecs_to_jiffies64((__msecs) * 1000)

But it would be neater to have used nsecs_to_cputime64(), surely.

Quoted 2 lines · Andrew Morton

Also, in include/asm-generic/cputime.h we have:

Quoted 3 lines

#define usecs_to_cputime64(__msecs) nsecs_to_jiffies64((__msecs) * 1000)

But it would be neater to have used nsecs_to_cputime64(), surely.

The procfs interface wants to convert usecs to cputime64, but generic cputime does not have usecs_to_jiffies64. Once someone writes the latter it can be used here.

Quoted 4 lines · On Thu, 22 Dec 2011 00:55:07 +0100 Andreas Schwab <[email protected]> wrote:

Quoted 2 lines · Andrew Morton

Also, in include/asm-generic/cputime.h we have:

Quoted 8 lines

Quoted 3 lines

#define usecs_to_cputime64(__msecs) nsecs_to_jiffies64((__msecs) * 1000)

But it would be neater to have used nsecs_to_cputime64(), surely.

The procfs interface wants to convert usecs to cputime64, but generic cputime does not have usecs_to_jiffies64. Once someone writes the latter it can be used here.

That doesn't address my suggestion.

I'm saying that this:

#define usecs_to_cputime64(__msecs) nsecs_to_jiffies64((__msecs) * 1000)

should have instead been

#define usecs_to_cputime64(__msecs) nsecs_to_cputime64((__msecs) * 1000)

Quoted 12 lines · On Wed, 21 Dec 2011 11:03:34 +0100 Michal Hocko <[email protected]> wrote:

Hmm, it seems that this bugfix (for 3.2) stalled. I guess that it is primarily because it is multiarch fix.
I am sorry to bother you Andrew but could we push this through you, please?

The full patch for reference:
---

Quoted 1 line

From 1fca39b21f3b344c90c30d98db6dcdcdc6815797 Mon Sep 17 00:00:00 2001

From: Andreas Schwab <[email protected]>
Date: Mon, 12 Dec 2011 14:07:53 +0100
Subject: [PATCH] procfs: do not confuse jiffies with cputime64_t

Quoted 2 lines

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.

This makes a huge mess when mixed with Martin's "cputime: add sparse checking and cleanup" in linux-next. I think I got it fixed up but it needs careful checking - I don't _think_ I needed to add more __force thingies. The version against today's linux-next is below.

(I did party tricks with this so I could carry the against-mainline and against-next versions in the same tree).

Also, in include/asm-generic/cputime.h we have:

#define usecs_to_cputime64(__msecs) nsecs_to_jiffies64((__msecs) * 1000)

But it would be neater to have used nsecs_to_cputime64(), surely.

From: Andreas Schwab <[email protected]>
Subject: procfs: do not confuse jiffies with cputime64_t

get_{idle,iowait}_time are supposed to return cputime64_t values, not jiffies. Add usecs_to_cputime64 for this.