linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] sched: Make nr_uninterruptible count a signed value
@ 2012-05-08 21:39 Diwakar Tundlam
  2012-05-08 21:56 ` Peter Zijlstra
  0 siblings, 1 reply; 14+ messages in thread
From: Diwakar Tundlam @ 2012-05-08 21:39 UTC (permalink / raw)
  To: 'Peter Zijlstra'
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

Declare nr_uninterruptible as a signed long to avoid garbage values
seen in cat /proc/sched_debug when a task is moved to the run queue of
a newly online core. This is part of a global counter where only the
total sum over all CPUs matters.

Signed-off-by: Diwakar Tundlam <dtundlam@nvidia.com>
---
 kernel/sched/core.c  |    7 ++++---
 kernel/sched/sched.h |    2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8d5eef6..7a64b5b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2114,7 +2114,8 @@ unsigned long nr_running(void)
 
 unsigned long nr_uninterruptible(void)
 {
-	unsigned long i, sum = 0;
+	unsigned long i;
+	long sum = 0;
 
 	for_each_possible_cpu(i)
 		sum += cpu_rq(i)->nr_uninterruptible;
@@ -2123,7 +2124,7 @@ unsigned long nr_uninterruptible(void)
 	 * Since we read the counters lockless, it might be slightly
 	 * inaccurate. Do not allow it to go below zero though:
 	 */
-	if (unlikely((long)sum < 0))
+	if (unlikely(sum < 0))
 		sum = 0;
 
 	return sum;
@@ -2174,7 +2175,7 @@ static long calc_load_fold_active(struct rq *this_rq)
 	long nr_active, delta = 0;
 
 	nr_active = this_rq->nr_running;
-	nr_active += (long) this_rq->nr_uninterruptible;
+	nr_active += this_rq->nr_uninterruptible;
 
 	if (nr_active != this_rq->calc_load_active) {
 		delta = nr_active - this_rq->calc_load_active;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index fb3acba..2668b07 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -385,7 +385,7 @@ struct rq {
 	 * one CPU and if it got migrated afterwards it may decrease
 	 * it on another CPU. Always updated under the runqueue lock:
 	 */
-	unsigned long nr_uninterruptible;
+	long nr_uninterruptible;
 
 	struct task_struct *curr, *idle, *stop;
 	unsigned long next_balance;
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-08 21:39 [PATCH] sched: Make nr_uninterruptible count a signed value Diwakar Tundlam
@ 2012-05-08 21:56 ` Peter Zijlstra
  2012-05-08 22:14   ` Diwakar Tundlam
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2012-05-08 21:56 UTC (permalink / raw)
  To: Diwakar Tundlam
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On Tue, 2012-05-08 at 14:39 -0700, Diwakar Tundlam wrote:
> Declare nr_uninterruptible as a signed long to avoid garbage values
> seen in cat /proc/sched_debug when a task is moved to the run queue of
> a newly online core. This is part of a global counter where only the
> total sum over all CPUs matters.

Its late here, but do explain how any of this makes any difference what
so ever? Since all we do with that field is add/sub the whole sign issue
shouldn't matter one way or the other.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-08 21:56 ` Peter Zijlstra
@ 2012-05-08 22:14   ` Diwakar Tundlam
  2012-05-08 22:27     ` Peter Zijlstra
  0 siblings, 1 reply; 14+ messages in thread
From: Diwakar Tundlam @ 2012-05-08 22:14 UTC (permalink / raw)
  To: 'Peter Zijlstra'
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

Sorry to bug you when it is late for you..

You're right, there is no real difference at all.
Only cosmetic difference when you look at the output of cat /proc/sched_debug.

But I suddenly realized maybe the increment/decrement of nr_interruptible is reversed.
Maybe that's the source of the problem: decrement in activate task and increment in deactivate task !!

Code snip:

/*
 * activate_task - move a task to the runqueue.
 */
static void activate_task(struct rq *rq, struct task_struct *p, int flags)
{
	if (task_contributes_to_load(p))
		rq->nr_uninterruptible--; <<<<< why decrement in activate task

	enqueue_task(rq, p, flags);
	inc_nr_running(rq);
}

/*
 * deactivate_task - remove a task from the runqueue.
 */
static void deactivate_task(struct rq *rq, struct task_struct *p, int flags)
{
	if (task_contributes_to_load(p))
		rq->nr_uninterruptible++; <<<<< why increment in deactivate task

	dequeue_task(rq, p, flags);
	dec_nr_running(rq);
}

Thanks,
--Diwakar.

-----Original Message-----
From: Peter Zijlstra [mailto:a.p.zijlstra@chello.nl] 
Sent: Tuesday, May 08, 2012 2:57 PM
To: Diwakar Tundlam
Cc: 'Ingo Molnar'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: Re: [PATCH] sched: Make nr_uninterruptible count a signed value

On Tue, 2012-05-08 at 14:39 -0700, Diwakar Tundlam wrote:
> Declare nr_uninterruptible as a signed long to avoid garbage values 
> seen in cat /proc/sched_debug when a task is moved to the run queue of 
> a newly online core. This is part of a global counter where only the 
> total sum over all CPUs matters.

Its late here, but do explain how any of this makes any difference what so ever? Since all we do with that field is add/sub the whole sign issue shouldn't matter one way or the other.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-08 22:14   ` Diwakar Tundlam
@ 2012-05-08 22:27     ` Peter Zijlstra
  2012-05-08 22:29       ` Peter Zijlstra
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2012-05-08 22:27 UTC (permalink / raw)
  To: Diwakar Tundlam
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On Tue, 2012-05-08 at 15:14 -0700, Diwakar Tundlam wrote:
> Sorry to bug you when it is late for you..
> 
Nah, I'm the idiot still behind the screen after midnight, its just the
brain that's slightly slower and needs more hints.

> You're right, there is no real difference at all.
> Only cosmetic difference when you look at the output of
> cat /proc/sched_debug.

Not sure I see that.. the printf is still using %Ld (signed) so the
output shouldn't matter regardless of if the variable is unsigned long
or long.
> 
> But I suddenly realized maybe the increment/decrement of
> nr_interruptible is reversed.
> Maybe that's the source of the problem: decrement in activate task and
> increment in deactivate task !!

No that's right. nr_uninterruptible counts the number of tasks in
uninterruptible sleep, so deactivate_task puts a task to sleep, so we
need to increment the number of sleeping tasks, activate_task wakes a
task up so we need to decrement the number of sleeping tasks.

I think the problem you're having is that we don't match the cpu where
we inc and dec the counter, and that's fully on purpose since its rather
expensive -- it would require atomics.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-08 22:27     ` Peter Zijlstra
@ 2012-05-08 22:29       ` Peter Zijlstra
  2012-05-08 22:46         ` Diwakar Tundlam
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2012-05-08 22:29 UTC (permalink / raw)
  To: Diwakar Tundlam
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On Wed, 2012-05-09 at 00:27 +0200, Peter Zijlstra wrote:
> On Tue, 2012-05-08 at 15:14 -0700, Diwakar Tundlam wrote:
> > Sorry to bug you when it is late for you..
> > 
> Nah, I'm the idiot still behind the screen after midnight, its just the
> brain that's slightly slower and needs more hints.
> 
> > You're right, there is no real difference at all.
> > Only cosmetic difference when you look at the output of
> > cat /proc/sched_debug.
> 
> Not sure I see that.. the printf is still using %Ld (signed) so the
> output shouldn't matter regardless of if the variable is unsigned long
> or long.
> > 
> > But I suddenly realized maybe the increment/decrement of
> > nr_interruptible is reversed.
> > Maybe that's the source of the problem: decrement in activate task and
> > increment in deactivate task !!
> 
> No that's right. nr_uninterruptible counts the number of tasks in
> uninterruptible sleep, so deactivate_task puts a task to sleep, so we
> need to increment the number of sleeping tasks, activate_task wakes a
> task up so we need to decrement the number of sleeping tasks.
> 
> I think the problem you're having is that we don't match the cpu where
> we inc and dec the counter, and that's fully on purpose since its rather
> expensive -- it would require atomics.
> 

FWIW the way to properly read the sched_debug output is something like:

# grep nr_uninterruptible /proc/sched_debug
  .nr_uninterruptible            : -1305
  .nr_uninterruptible            : 336
  .nr_uninterruptible            : -229
  .nr_uninterruptible            : 276
  .nr_uninterruptible            : 105
  .nr_uninterruptible            : 157
  .nr_uninterruptible            : -2782
  .nr_uninterruptible            : 325
  .nr_uninterruptible            : -471
  .nr_uninterruptible            : 9
  .nr_uninterruptible            : 205
  .nr_uninterruptible            : 88
  .nr_uninterruptible            : 7
  .nr_uninterruptible            : 912
  .nr_uninterruptible            : 188
  .nr_uninterruptible            : 66
  .nr_uninterruptible            : 87
  .nr_uninterruptible            : 45
  .nr_uninterruptible            : 194
  .nr_uninterruptible            : 1178
  .nr_uninterruptible            : 185
  .nr_uninterruptible            : 143
  .nr_uninterruptible            : 136
  .nr_uninterruptible            : 145

# awk '/nr_uninterruptible/ {t += $3} END {print t}' /proc/sched_debug
0

The per-cpu value is meaningless, only the sum over all cpus is a
meaningful number.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-08 22:29       ` Peter Zijlstra
@ 2012-05-08 22:46         ` Diwakar Tundlam
  2012-05-09  7:49           ` Michael Wang
  2012-05-09  8:11           ` Peter Zijlstra
  0 siblings, 2 replies; 14+ messages in thread
From: Diwakar Tundlam @ 2012-05-08 22:46 UTC (permalink / raw)
  To: 'Peter Zijlstra'
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

> No that's right. nr_uninterruptible counts the number of tasks in 
> uninterruptible sleep, so deactivate_task puts a task to sleep, so we 
> need to increment the number of sleeping tasks, activate_task wakes a 
> task up so we need to decrement the number of sleeping tasks.

Yep, I looked at the code for task_contributes_to_load() and I understand what it is all about.
The ++ and -- are correct, I see it now.

On the -ve values, strangely inspite of %Ld in the print statement, in my kernel, I see high unsigned values instead of -ve values for nr_uninterruptible.

But the sum is always 0, though.

Maybe it is an artifact of 32-bit machine displaying 64-bit print format.
An (unsigned long)(-24) promoted to (signed long long) ends up as 4294967272.
As seen in my output of sched_debug.

Your machine is probably natively 64-bit.

$ adb shell cat /proc/sched_debug |egrep 'cpu#|nr_'
cpu#0
  .nr_running                    : 1
  .nr_switches                   : 16233
  .nr_load_updates               : 2529
  .nr_uninterruptible            : 4294967272 <<<<< 0xffffffe8 == (-24)
  .nr_spread_over                : 18
  .nr_running                    : 0
  .nr_spread_over                : 101
  .nr_running                    : 1
  .rt_nr_running                 : 0
  .rt_nr_running                 : 0
  .rt_nr_running                 : 0
cpu#1
  .nr_running                    : 1
  .nr_switches                   : 7891
  .nr_load_updates               : 2124
  .nr_uninterruptible            : 18
  .nr_spread_over                : 121
  .nr_running                    : 1
  .rt_nr_running                 : 0
  .rt_nr_running                 : 0
  .rt_nr_running                 : 0
cpu#3
  .nr_running                    : 1
  .nr_switches                   : 13896
  .nr_load_updates               : 1179
  .nr_uninterruptible            : 6
  .nr_spread_over                : 106
  .nr_running                    : 1
  .rt_nr_running                 : 0
  .rt_nr_running                 : 0
  .rt_nr_running                 : 0

Thanks,
--Diwakar.

-----Original Message-----
From: Peter Zijlstra [mailto:peterz@infradead.org] 
Sent: Tuesday, May 08, 2012 3:30 PM
To: Diwakar Tundlam
Cc: 'Ingo Molnar'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: RE: [PATCH] sched: Make nr_uninterruptible count a signed value

On Wed, 2012-05-09 at 00:27 +0200, Peter Zijlstra wrote:
> On Tue, 2012-05-08 at 15:14 -0700, Diwakar Tundlam wrote:
> > Sorry to bug you when it is late for you..
> > 
> Nah, I'm the idiot still behind the screen after midnight, its just 
> the brain that's slightly slower and needs more hints.
> 
> > You're right, there is no real difference at all.
> > Only cosmetic difference when you look at the output of cat 
> > /proc/sched_debug.
> 
> Not sure I see that.. the printf is still using %Ld (signed) so the 
> output shouldn't matter regardless of if the variable is unsigned long 
> or long.
> > 
> > But I suddenly realized maybe the increment/decrement of 
> > nr_interruptible is reversed.
> > Maybe that's the source of the problem: decrement in activate task 
> > and increment in deactivate task !!
> 
> No that's right. nr_uninterruptible counts the number of tasks in 
> uninterruptible sleep, so deactivate_task puts a task to sleep, so we 
> need to increment the number of sleeping tasks, activate_task wakes a 
> task up so we need to decrement the number of sleeping tasks.
> 
> I think the problem you're having is that we don't match the cpu where 
> we inc and dec the counter, and that's fully on purpose since its 
> rather expensive -- it would require atomics.
> 

FWIW the way to properly read the sched_debug output is something like:

# grep nr_uninterruptible /proc/sched_debug
  .nr_uninterruptible            : -1305
  .nr_uninterruptible            : 336
  .nr_uninterruptible            : -229
  .nr_uninterruptible            : 276
  .nr_uninterruptible            : 105
  .nr_uninterruptible            : 157
  .nr_uninterruptible            : -2782
  .nr_uninterruptible            : 325
  .nr_uninterruptible            : -471
  .nr_uninterruptible            : 9
  .nr_uninterruptible            : 205
  .nr_uninterruptible            : 88
  .nr_uninterruptible            : 7
  .nr_uninterruptible            : 912
  .nr_uninterruptible            : 188
  .nr_uninterruptible            : 66
  .nr_uninterruptible            : 87
  .nr_uninterruptible            : 45
  .nr_uninterruptible            : 194
  .nr_uninterruptible            : 1178
  .nr_uninterruptible            : 185
  .nr_uninterruptible            : 143
  .nr_uninterruptible            : 136
  .nr_uninterruptible            : 145

# awk '/nr_uninterruptible/ {t += $3} END {print t}' /proc/sched_debug
0

The per-cpu value is meaningless, only the sum over all cpus is a meaningful number.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-08 22:46         ` Diwakar Tundlam
@ 2012-05-09  7:49           ` Michael Wang
  2012-05-09 18:55             ` Diwakar Tundlam
  2012-05-09  8:11           ` Peter Zijlstra
  1 sibling, 1 reply; 14+ messages in thread
From: Michael Wang @ 2012-05-09  7:49 UTC (permalink / raw)
  To: Diwakar Tundlam
  Cc: 'Peter Zijlstra', 'Ingo Molnar',
	'David Rientjes', 'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On 05/09/2012 06:46 AM, Diwakar Tundlam wrote:

>> No that's right. nr_uninterruptible counts the number of tasks in 
>> uninterruptible sleep, so deactivate_task puts a task to sleep, so we 
>> need to increment the number of sleeping tasks, activate_task wakes a 
>> task up so we need to decrement the number of sleeping tasks.
> 
> Yep, I looked at the code for task_contributes_to_load() and I understand what it is all about.
> The ++ and -- are correct, I see it now.
> 
> On the -ve values, strangely inspite of %Ld in the print statement, in my kernel, I see high unsigned values instead of -ve values for nr_uninterruptible.
> 
> But the sum is always 0, though.
> 
> Maybe it is an artifact of 32-bit machine displaying 64-bit print format.
> An (unsigned long)(-24) promoted to (signed long long) ends up as 4294967272.
> As seen in my output of sched_debug.


This may do some help.

Regards,
Michael Wang
---
 kernel/sched/debug.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 09acaa1..ab9d53f 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -270,7 +270,7 @@ static void print_cpu(struct seq_file *m, int cpu)
                   rq->load.weight);
        P(nr_switches);
        P(nr_load_updates);
-       P(nr_uninterruptible);
+       P((signed long)nr_uninterruptible);
        PN(next_balance);
        P(curr->pid);
        PN(clock);
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* RE: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-08 22:46         ` Diwakar Tundlam
  2012-05-09  7:49           ` Michael Wang
@ 2012-05-09  8:11           ` Peter Zijlstra
  2012-05-09 19:04             ` Diwakar Tundlam
  2012-05-10  3:41             ` Michael Wang
  1 sibling, 2 replies; 14+ messages in thread
From: Peter Zijlstra @ 2012-05-09  8:11 UTC (permalink / raw)
  To: Diwakar Tundlam
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On Tue, 2012-05-08 at 15:46 -0700, Diwakar Tundlam wrote:
> Maybe it is an artifact of 32-bit machine displaying 64-bit print format.
> An (unsigned long)(-24) promoted to (signed long long) ends up as 4294967272.
> As seen in my output of sched_debug.

Ah, quite possible. %Ld is indeed %lld and the value is long, not long
long. So the proper fix is to fudge that printk statement somehow.

> Your machine is probably natively 64-bit. 

Yeah, I gave up on 32bit computing a while ago..


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-09  7:49           ` Michael Wang
@ 2012-05-09 18:55             ` Diwakar Tundlam
  2012-05-10  4:46               ` Michael Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Diwakar Tundlam @ 2012-05-09 18:55 UTC (permalink / raw)
  To: 'Michael Wang'
  Cc: 'Peter Zijlstra', 'Ingo Molnar',
	'David Rientjes', 'linux-kernel@vger.kernel.org',
	Peter De Schrijver

>> -       P(nr_uninterruptible);
>> +       P((signed long)nr_uninterruptible);

I thought of it too, but it won't compile because the P macro expands to dereferencing rq->nr_uninterruptible, so with your change, it will show up as rq->(signed long)nr_uninterruptible which is a syntax error.

--Diwakar.

-----Original Message-----
From: Michael Wang [mailto:wangyun@linux.vnet.ibm.com] 
Sent: Wednesday, May 09, 2012 12:49 AM
To: Diwakar Tundlam
Cc: 'Peter Zijlstra'; 'Ingo Molnar'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: Re: [PATCH] sched: Make nr_uninterruptible count a signed value

On 05/09/2012 06:46 AM, Diwakar Tundlam wrote:

>> No that's right. nr_uninterruptible counts the number of tasks in 
>> uninterruptible sleep, so deactivate_task puts a task to sleep, so we 
>> need to increment the number of sleeping tasks, activate_task wakes a 
>> task up so we need to decrement the number of sleeping tasks.
> 
> Yep, I looked at the code for task_contributes_to_load() and I understand what it is all about.
> The ++ and -- are correct, I see it now.
> 
> On the -ve values, strangely inspite of %Ld in the print statement, in my kernel, I see high unsigned values instead of -ve values for nr_uninterruptible.
> 
> But the sum is always 0, though.
> 
> Maybe it is an artifact of 32-bit machine displaying 64-bit print format.
> An (unsigned long)(-24) promoted to (signed long long) ends up as 4294967272.
> As seen in my output of sched_debug.


This may do some help.

Regards,
Michael Wang
---
 kernel/sched/debug.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c index 09acaa1..ab9d53f 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -270,7 +270,7 @@ static void print_cpu(struct seq_file *m, int cpu)
                   rq->load.weight);
        P(nr_switches);
        P(nr_load_updates);
-       P(nr_uninterruptible);
+       P((signed long)nr_uninterruptible);
        PN(next_balance);
        P(curr->pid);
        PN(clock);
--
1.7.4.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-09  8:11           ` Peter Zijlstra
@ 2012-05-09 19:04             ` Diwakar Tundlam
  2012-05-10  3:41             ` Michael Wang
  1 sibling, 0 replies; 14+ messages in thread
From: Diwakar Tundlam @ 2012-05-09 19:04 UTC (permalink / raw)
  To: 'Peter Zijlstra'
  Cc: 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1717 bytes --]

The issue is because when promoting from 32-bit unsigned to 64-bit signed, the compiler first does SIZE increase and then the sign change. If it did SIGN change first and then size increase (with sign-extension) and it will work correctly.

But then, the compiler is probably doing the right thing here because the starting point is an unsigned value and when it is assigned a negative value, the compiler is perfectly justified in converting it to a large positive value which is what happened here.

The theoretical correct fix is to change the declaration to signed long nr_uninterruptible simply because it can sometimes become -ve.

The other fix is to fix the print statement - but it feels wrong.

It would be good to fix the declaration...

Thanks,
--Diwakar.

-----Original Message-----
From: Peter Zijlstra [mailto:peterz@infradead.org] 
Sent: Wednesday, May 09, 2012 1:12 AM
To: Diwakar Tundlam
Cc: 'Ingo Molnar'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
Subject: RE: [PATCH] sched: Make nr_uninterruptible count a signed value

On Tue, 2012-05-08 at 15:46 -0700, Diwakar Tundlam wrote:
> Maybe it is an artifact of 32-bit machine displaying 64-bit print format.
> An (unsigned long)(-24) promoted to (signed long long) ends up as 4294967272.
> As seen in my output of sched_debug.

Ah, quite possible. %Ld is indeed %lld and the value is long, not long long. So the proper fix is to fudge that printk statement somehow.

> Your machine is probably natively 64-bit. 

Yeah, I gave up on 32bit computing a while ago..

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-09  8:11           ` Peter Zijlstra
  2012-05-09 19:04             ` Diwakar Tundlam
@ 2012-05-10  3:41             ` Michael Wang
  2012-05-10  9:46               ` Peter Zijlstra
  1 sibling, 1 reply; 14+ messages in thread
From: Michael Wang @ 2012-05-10  3:41 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Diwakar Tundlam, 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On 05/09/2012 04:11 PM, Peter Zijlstra wrote:

> On Tue, 2012-05-08 at 15:46 -0700, Diwakar Tundlam wrote:
>> Maybe it is an artifact of 32-bit machine displaying 64-bit print format.
>> An (unsigned long)(-24) promoted to (signed long long) ends up as 4294967272.
>> As seen in my output of sched_debug.
> 
> Ah, quite possible. %Ld is indeed %lld and the value is long, not long
> long. So the proper fix is to fudge that printk statement somehow.


Could we use SEQ_printf for nr_uninterruptible, just like we did on
rq->load.weight?

Regards,
Michael Wang

> 
>> Your machine is probably natively 64-bit. 
> 
> Yeah, I gave up on 32bit computing a while ago..
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-09 18:55             ` Diwakar Tundlam
@ 2012-05-10  4:46               ` Michael Wang
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Wang @ 2012-05-10  4:46 UTC (permalink / raw)
  To: Diwakar Tundlam
  Cc: 'Peter Zijlstra', 'Ingo Molnar',
	'David Rientjes', 'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On 05/10/2012 02:55 AM, Diwakar Tundlam wrote:

>>> -       P(nr_uninterruptible);
>>> +       P((signed long)nr_uninterruptible);
> 
> I thought of it too, but it won't compile because the P macro expands to dereferencing rq->nr_uninterruptible, so with your change, it will show up as rq->(signed long)nr_uninterruptible which is a syntax error.
> 

oh, I see, so looks like it's a little hard to avoid some ugly change...

--Michael Wang

> --Diwakar.
> 
> -----Original Message-----
> From: Michael Wang [mailto:wangyun@linux.vnet.ibm.com] 
> Sent: Wednesday, May 09, 2012 12:49 AM
> To: Diwakar Tundlam
> Cc: 'Peter Zijlstra'; 'Ingo Molnar'; 'David Rientjes'; 'linux-kernel@vger.kernel.org'; Peter De Schrijver
> Subject: Re: [PATCH] sched: Make nr_uninterruptible count a signed value
> 
> On 05/09/2012 06:46 AM, Diwakar Tundlam wrote:
> 
>>> No that's right. nr_uninterruptible counts the number of tasks in 
>>> uninterruptible sleep, so deactivate_task puts a task to sleep, so we 
>>> need to increment the number of sleeping tasks, activate_task wakes a 
>>> task up so we need to decrement the number of sleeping tasks.
>>
>> Yep, I looked at the code for task_contributes_to_load() and I understand what it is all about.
>> The ++ and -- are correct, I see it now.
>>
>> On the -ve values, strangely inspite of %Ld in the print statement, in my kernel, I see high unsigned values instead of -ve values for nr_uninterruptible.
>>
>> But the sum is always 0, though.
>>
>> Maybe it is an artifact of 32-bit machine displaying 64-bit print format.
>> An (unsigned long)(-24) promoted to (signed long long) ends up as 4294967272.
>> As seen in my output of sched_debug.
> 
> 
> This may do some help.
> 
> Regards,
> Michael Wang
> ---
>  kernel/sched/debug.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c index 09acaa1..ab9d53f 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -270,7 +270,7 @@ static void print_cpu(struct seq_file *m, int cpu)
>                    rq->load.weight);
>         P(nr_switches);
>         P(nr_load_updates);
> -       P(nr_uninterruptible);
> +       P((signed long)nr_uninterruptible);
>         PN(next_balance);
>         P(curr->pid);
>         PN(clock);
> --
> 1.7.4.1
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-10  3:41             ` Michael Wang
@ 2012-05-10  9:46               ` Peter Zijlstra
  2012-05-11  2:19                 ` Michael Wang
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2012-05-10  9:46 UTC (permalink / raw)
  To: Michael Wang
  Cc: Diwakar Tundlam, 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

Or something like the below.. I recently changed nr_running to int (see
c82513e51) so P(nr_running) would also benefit.

---
 kernel/sched/debug.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 31e4f61..954fabf 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -260,8 +260,14 @@ static void print_cpu(struct seq_file *m, int cpu)
 	SEQ_printf(m, "\ncpu#%d\n", cpu);
 #endif
 
-#define P(x) \
-	SEQ_printf(m, "  .%-30s: %Ld\n", #x, (long long)(rq->x))
+#define P(x)								\
+do {									\
+	if (sizeof(rq->x) == 4)						\
+		SEQ_printf(m, "  .%-30s: %ld\n", #x, (long)(rq->x));	\
+	else								\
+		SEQ_printf(m, "  .%-30s: %Ld\n", #x, (long long)(rq->x));\
+} while (0)
+		
 #define PN(x) \
 	SEQ_printf(m, "  .%-30s: %Ld.%06ld\n", #x, SPLIT_NS(rq->x))
 


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] sched: Make nr_uninterruptible count a signed value
  2012-05-10  9:46               ` Peter Zijlstra
@ 2012-05-11  2:19                 ` Michael Wang
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Wang @ 2012-05-11  2:19 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Diwakar Tundlam, 'Ingo Molnar', 'David Rientjes',
	'linux-kernel@vger.kernel.org',
	Peter De Schrijver

On 05/10/2012 05:46 PM, Peter Zijlstra wrote:

> Or something like the below.. I recently changed nr_running to int (see
> c82513e51) so P(nr_running) would also benefit.
> 
> ---
>  kernel/sched/debug.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> index 31e4f61..954fabf 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -260,8 +260,14 @@ static void print_cpu(struct seq_file *m, int cpu)
>  	SEQ_printf(m, "\ncpu#%d\n", cpu);
>  #endif
> 
> -#define P(x) \
> -	SEQ_printf(m, "  .%-30s: %Ld\n", #x, (long long)(rq->x))
> +#define P(x)								\
> +do {									\
> +	if (sizeof(rq->x) == 4)						\
> +		SEQ_printf(m, "  .%-30s: %ld\n", #x, (long)(rq->x));	\


Oh, yes, I haven't noticed that sizeof could also be a check point.
So now we can use P(x) freely and don't need to worry about anything.

Regards,
Michael Wang

> +	else								\
> +		SEQ_printf(m, "  .%-30s: %Ld\n", #x, (long long)(rq->x));\
> +} while (0)
> +		
>  #define PN(x) \
>  	SEQ_printf(m, "  .%-30s: %Ld.%06ld\n", #x, SPLIT_NS(rq->x))
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-05-11  2:19 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-08 21:39 [PATCH] sched: Make nr_uninterruptible count a signed value Diwakar Tundlam
2012-05-08 21:56 ` Peter Zijlstra
2012-05-08 22:14   ` Diwakar Tundlam
2012-05-08 22:27     ` Peter Zijlstra
2012-05-08 22:29       ` Peter Zijlstra
2012-05-08 22:46         ` Diwakar Tundlam
2012-05-09  7:49           ` Michael Wang
2012-05-09 18:55             ` Diwakar Tundlam
2012-05-10  4:46               ` Michael Wang
2012-05-09  8:11           ` Peter Zijlstra
2012-05-09 19:04             ` Diwakar Tundlam
2012-05-10  3:41             ` Michael Wang
2012-05-10  9:46               ` Peter Zijlstra
2012-05-11  2:19                 ` Michael Wang

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).