BUG: using smp_processor_id() in preemptible code: s2disk

In reply to Maxim Levitsky

Quoted 5 lines · Sergey Senozhatsky

Hello,
.35-rc3, x86
Hit the following error today:

I confirm the same issue. Sorry for not reporting it earlier.

Best regards,
Maxim Levitsky

Rafael J. Wysocki

2010-06-13 23:36 UTC #3 lore raw

Quoted 8 lines · Maxim Levitsky

Quoted 5 lines · Sergey Senozhatsky
Hello,
.35-rc3, x86
Hit the following error today:
I confirm the same issue. Sorry for not reporting it earlier.

It looks like a cpufreq issue to me, but it may be related to CPU hotplug as well. I'm not sure who's been messing up with that recently, though.

Rafael

Rafael J. Wysocki

2010-06-13 23:38 UTC #4 lore raw

Quoted 8 lines · Maxim Levitsky

Quoted 5 lines · Sergey Senozhatsky
Hello,
.35-rc3, x86
Hit the following error today:
I confirm the same issue. Sorry for not reporting it earlier.

It looks like a cpufreq issue to me, but it may be related to CPU hotplug as well. I'm not sure who's been messing up with that recently, though.

Rafael

Sergey Senozhatsky

author

2010-06-14 14:06 UTC #5 lore raw

Hello,
Not sure if this simple solution is the correct one.

---

kernel/sched.c +4 -1

index f8b8996..cfb262b 100644
@@ -2866,7 +2866,10 @@ unsigned long nr_iowait(void)
 
 unsigned long nr_iowait_cpu(void)
 {
-	struct rq *this = this_rq();
+	int cpu = get_cpu();
+	struct rq *this = cpu_rq(cpu);
+	put_cpu();
+	
 	return atomic_read(&this->nr_iowait);
 }

Rafael J. Wysocki

2010-06-14 14:23 UTC #6 lore raw

Quoted 2 lines · Sergey Senozhatsky

Hello,
Not sure if this simple solution is the correct one.

Well, let's ask the scheduler people.

Ingo, Peter, what do you think of the patch below?

Rafael

Quoted 18 lines

---

kernel/sched.c +4 -1

index f8b8996..cfb262b 100644
@@ -2866,7 +2866,10 @@ unsigned long nr_iowait(void)
 
 unsigned long nr_iowait_cpu(void)
 {
-	struct rq *this = this_rq();
+	int cpu = get_cpu();
+	struct rq *this = cpu_rq(cpu);
+	put_cpu();
+	
 	return atomic_read(&this->nr_iowait);
 }

Rafael J. Wysocki

2010-06-14 14:25 UTC #7 lore raw

Quoted 2 lines · Sergey Senozhatsky

Hello,
Not sure if this simple solution is the correct one.

Well, let's ask the scheduler people.

Ingo, Peter, what do you think of the patch below?

Rafael

Quoted 18 lines

---

kernel/sched.c +4 -1

index f8b8996..cfb262b 100644
@@ -2866,7 +2866,10 @@ unsigned long nr_iowait(void)
 
 unsigned long nr_iowait_cpu(void)
 {
-	struct rq *this = this_rq();
+	int cpu = get_cpu();
+	struct rq *this = cpu_rq(cpu);
+	put_cpu();
+	
 	return atomic_read(&this->nr_iowait);
 }

Arjan van de Ven

2010-06-14 14:36 UTC #8 lore raw

Quoted 2 lines · On Mon, 14 Jun 2010 17:09:41 +0300 Sergey Senozhatsky…

Hello,
Not sure if this simple solution is the correct one.

it's not; the caller needs to pass in the cpu number I suspect for this to be really correct....

I just returned from family emergency travel and will take a look today

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-14 14:51 UTC #9 lore raw

Quoted 8 lines · Arjan van de Ven

Quoted 2 lines
Hello,
Not sure if this simple solution is the correct one.
it's not; the caller needs to pass in the cpu number I suspect for this to be really correct....
I just returned from family emergency travel and will take a look today

I thought about patching

./drivers/cpuidle/governors/menu.c: if (nr_iowait_cpu())
./drivers/cpuidle/governors/menu.c: mult += 10 * nr_iowait_cpu();
./kernel/time/tick-sched.c: if (nr_iowait_cpu() > 0)

decided to patch nr_iowait_cpu instead.

Sergey

Arjan van de Ven

2010-06-14 14:59 UTC #10 lore raw

Quoted 13 lines · On Mon, 14 Jun 2010 17:54:40 +0300 Sergey Senozhatsky…

Quoted 9 lines · Arjan van de Ven
Quoted 2 lines
Hello,
Not sure if this simple solution is the correct one.
it's not; the caller needs to pass in the cpu number I suspect for this to be really correct....
I just returned from family emergency travel and will take a look today
I thought about patching

Quoted 7 lines

./drivers/cpuidle/governors/menu.c: if (nr_iowait_cpu())
./drivers/cpuidle/governors/menu.c: mult += 10 *
nr_iowait_cpu(); ./kernel/time/tick-sched.c: if
(nr_iowait_cpu() > 0)
decided to patch nr_iowait_cpu instead.

the bug is that it needs to be

nr_iowait_cpu(int cpu)

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-14 15:14 UTC #11 lore raw

Quoted 6 lines · Arjan van de Ven

Quoted 1 line
decided to patch nr_iowait_cpu instead.
the bug is that it needs to be
nr_iowait_cpu(int cpu)

Something similar to this?

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +3 -1

index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;
 	}

Arjan van de Ven

Acked-by

2010-06-15 03:37 UTC #12 lore raw

Quoted 9 lines · On Mon, 14 Jun 2010 18:17:35 +0300 Sergey Senozhatsky…

Quoted 6 lines · Arjan van de Ven
Quoted 1 line
decided to patch nr_iowait_cpu instead.
the bug is that it needs to be
nr_iowait_cpu(int cpu)
Something similar to this?

yup that'll do it

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-15 06:15 UTC #13 lore raw

PATCH 1/1 avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Fixing

 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62
 [<c10027d3>] sysenter_do_call+0x12/0x2d

Initially fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan van de Ven stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to it callers.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +3 -1

index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;
 	}

Arjan van de Ven

2010-06-15 14:22 UTC #14 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 18 lines · On Tue, 15 Jun 2010 09:19:27 +0300 Sergey Senozhatsky…

kernel/time/tick-sched.c +3 -1

index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts,
 ktime_t now, u64 *last_update_time) ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime,
 delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime =
 ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;

Quoted 1 line

}

hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....

they will normally be the same, except for the case where you get your warning...
(and this is also why you can't move the get_cpu() inside nr_iowait_cpu() ... )

Quoted 1 line

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-15 14:46 UTC #15 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 9 lines · Arjan van de Ven

Quoted 7 lines · On Tue, 15 Jun 2010 09:19:27 +0300 Sergey Senozhatsky…
kernel/time/tick-sched.c +0 -0
index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts,
 ktime_t now, u64 *last_update_time) ktime_t delta;
 

Quoted 17 lines

Quoted 11 lines

kernel/time/tick-sched.c +3 -1

 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime,
 delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime =
 ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;

Quoted 1 line

}

hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....

Hmm. Thanks, good catch.
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?

Quoted 5 lines

they will normally be the same, except for the case where you get your warning...
(and this is also why you can't move the get_cpu() inside nr_iowait_cpu() ... )

Sergey

Arjan van de Ven

2010-06-15 15:05 UTC #16 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 9 lines · On Tue, 15 Jun 2010 17:50:29 +0300 Sergey Senozhatsky…

Quoted 4 lines
hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....
Hmm. Thanks, good catch.
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?

that'd be option one
option two is to add a "cpu" member to struct tick_sched.....

if you go for option one, I'd replace the ts argument with the cpu argument.....

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-15 15:19 UTC #17 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 7 lines · Arjan van de Ven

Quoted 5 lines · On Tue, 15 Jun 2010 17:50:29 +0300 Sergey Senozhatsky…
Quoted 4 lines
hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....

Quoted 7 lines

Quoted 4 lines
Hmm. Thanks, good catch.
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?
that'd be option one

We'll have problem in tick_nohz_start_idle(struct tick_sched *ts)

{
	ktime_t now;
	now = ktime_get();
	update_ts_time_stats(ts, now, NULL);

...
So, we also will have to expand it to tick_nohz_start_idle(struct tick_sched *ts, int cpu)

That's why I prefer option #2.

Quoted 2 lines

option two is to add a "cpu" member to struct tick_sched.....

Thought about that. Seems ok to me.

Quoted 4 lines

if you go for option one, I'd replace the ts argument with the cpu argument.....

Hmm, I missed this one (replacing *ts to int cpu). And I like it. Something like:

-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) +update_ts_time_stats(int cpu, ktime_t now, u64 *last_update_time)

{
	ktime_t delta;

+ struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);

	if (ts->idle_active) {
		int cpu = get_cpu();
		delta = ktime_sub(now, ts->idle_entrytime);
		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
		if (nr_iowait_cpu(cpu) > 0)
			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
		put_cpu();
		ts->idle_entrytime = now;
+		ts->idle_active = 0;
	}

	if (last_update_time)
		*last_update_time = ktime_to_us(now);

}

static void tick_nohz_stop_idle(int cpu, ktime_t now) {
- struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);

	update_ts_time_stats(cpu, now, NULL);

- ts->idle_active = 0;

	sched_clock_idle_wakeup_event(0);
}

and so on.

Sergey

Sergey Senozhatsky

author

2010-06-15 15:27 UTC #18 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Review

Quoted 8 lines · Sergey Senozhatsky

....

		delta = ktime_sub(now, ts->idle_entrytime);
		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
		if (nr_iowait_cpu(cpu) > 0)
			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
		put_cpu();
		ts->idle_entrytime = now;

(quoted diff) +1 -0

+		ts->idle_active = 0;

		^^^^^^

This part is wrong. Sorry.

Quoted 6 lines

	}

	if (last_update_time)
		*last_update_time = ktime_to_us(now);

}

Sergey Senozhatsky

author

2010-06-15 15:31 UTC #19 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Review

Quoted 8 lines · Sergey Senozhatsky

....

		delta = ktime_sub(now, ts->idle_entrytime);
		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
		if (nr_iowait_cpu(cpu) > 0)
			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
		put_cpu();
		ts->idle_entrytime = now;

(quoted diff) +1 -0

+		ts->idle_active = 0;

		^^^^^^

This part is wrong. Sorry.

Quoted 6 lines

	}

	if (last_update_time)
		*last_update_time = ktime_to_us(now);

}

Sergey Senozhatsky

author

2010-06-15 15:23 UTC #20 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 7 lines · Arjan van de Ven

Quoted 5 lines · On Tue, 15 Jun 2010 17:50:29 +0300 Sergey Senozhatsky…
Quoted 4 lines
hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....

Quoted 7 lines

Quoted 4 lines
Hmm. Thanks, good catch.
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?
that'd be option one

We'll have problem in tick_nohz_start_idle(struct tick_sched *ts)

{
	ktime_t now;
	now = ktime_get();
	update_ts_time_stats(ts, now, NULL);

...
So, we also will have to expand it to tick_nohz_start_idle(struct tick_sched *ts, int cpu)

That's why I prefer option #2.

Quoted 2 lines

option two is to add a "cpu" member to struct tick_sched.....

Thought about that. Seems ok to me.

Quoted 4 lines

if you go for option one, I'd replace the ts argument with the cpu argument.....

Hmm, I missed this one (replacing *ts to int cpu). And I like it. Something like:

-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) +update_ts_time_stats(int cpu, ktime_t now, u64 *last_update_time)

{
	ktime_t delta;

+ struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);

	if (ts->idle_active) {
		int cpu = get_cpu();
		delta = ktime_sub(now, ts->idle_entrytime);
		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
		if (nr_iowait_cpu(cpu) > 0)
			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
		put_cpu();
		ts->idle_entrytime = now;
+		ts->idle_active = 0;
	}

	if (last_update_time)
		*last_update_time = ktime_to_us(now);

}

static void tick_nohz_stop_idle(int cpu, ktime_t now) {
- struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);

	update_ts_time_stats(cpu, now, NULL);

- ts->idle_active = 0;

	sched_clock_idle_wakeup_event(0);
}

and so on.

Sergey

Sergey Senozhatsky

author

2010-06-15 16:09 UTC #21 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

I've changed struct tick_sched to match passed *ts and cpu. Also changed "&per_cpu(tick_cpu_sched, cpu)" call to "struct tick_sched *tick_get_tick_sched(int cpu)" which we already have.

But I don't really like this part:
struct tick_sched *tick_get_tick_sched(int cpu)

{
	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
	ts->cpu = cpu;
	^^^^^^^^^^^^^
	return ts;
}

Please kindly review.

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

include/linux/tick.h +1 -0

index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +14 ~~-11~~

index 1d7b9bc..5105345 100644
@@ -38,7 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
-	return &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	ts->cpu = cpu;
+	return ts;
 }
 
 /*

@@ -137,7 +139,7 @@ __setup("nohz=", setup_tick_nohz);
 static void tick_nohz_update_jiffies(ktime_t now)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	unsigned long flags;
 
 	cpumask_clear_cpu(cpu, nohz_cpu_mask);

@@ -159,9 +161,10 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = ts->cpu;
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -173,7 +176,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 
 static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	update_ts_time_stats(ts, now, NULL);
 	ts->idle_active = 0;

@@ -211,7 +214,7 @@ static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
  */
 u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -237,7 +240,7 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time_us);
  */
 u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -267,7 +270,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	local_irq_save(flags);
 
 	cpu = smp_processor_id();
-	ts = &per_cpu(tick_cpu_sched, cpu);
+	ts = tick_get_tick_sched(cpu);
 
 	/*
 	 * Call to tick_nohz_start_idle stops the last_update_time from being

@@ -508,7 +511,7 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
 void tick_nohz_restart_sched_tick(void)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING
 	unsigned long ticks;
 #endif

@@ -671,7 +674,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 #if 0
 	/* Switch back to 2.6.27 behaviour */
 
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t delta;
 
 	/*

@@ -688,7 +691,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 
 static inline void tick_check_nohz(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t now;
 
 	if (!ts->idle_active && !ts->tick_stopped)

@@ -818,7 +821,7 @@ void tick_setup_sched_timer(void)
 #if defined CONFIG_NO_HZ || defined CONFIG_HIGH_RES_TIMERS
 void tick_cancel_sched_timer(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 # ifdef CONFIG_HIGH_RES_TIMERS
 	if (ts->sched_timer.base)

Arjan van de Ven

2010-06-16 06:03 UTC #22 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 7 lines · On Tue, 15 Jun 2010 19:13:03 +0300 Sergey Senozhatsky…

I've changed struct tick_sched to match passed *ts and cpu. Also changed "&per_cpu(tick_cpu_sched, cpu)" call to "struct tick_sched *tick_get_tick_sched(int cpu)" which we already have.
But I don't really like this part:
struct tick_sched *tick_get_tick_sched(int cpu)

Quoted 8 lines

{
	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
	ts->cpu = cpu;
	^^^^^^^^^^^^^
	return ts;
}

Please kindly review.

can we do this bit once, when the ts structure gets initialized? it's not like the cpu value will ever change...

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-16 09:31 UTC #23 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 4 lines · Arjan van de Ven

can we do this bit once, when the ts structure gets initialized? it's not like the cpu value will ever change...

Hello Arjan,

Sure we can. The question is where is the "proper place"? for_each_possible_cpu in __init?

or something like

static DEFINE_PER_CPU(struct tick_sched, tick_cpu_sched); int cpu = get_cpu();
&per_cpu(tick_cpu_sched, cpu)->cpu = cpu; put_cpu();

Sergey

Sergey Senozhatsky

author

2010-06-16 09:34 UTC #24 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 4 lines · Arjan van de Ven

can we do this bit once, when the ts structure gets initialized? it's not like the cpu value will ever change...

Hello Arjan,

Sure we can. The question is where is the "proper place"? for_each_possible_cpu in __init?

or something like

static DEFINE_PER_CPU(struct tick_sched, tick_cpu_sched); int cpu = get_cpu();
&per_cpu(tick_cpu_sched, cpu)->cpu = cpu; put_cpu();

Sergey

Arjan van de Ven

2010-06-16 06:05 UTC #25 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 7 lines · On Tue, 15 Jun 2010 19:13:03 +0300 Sergey Senozhatsky…

I've changed struct tick_sched to match passed *ts and cpu. Also changed "&per_cpu(tick_cpu_sched, cpu)" call to "struct tick_sched *tick_get_tick_sched(int cpu)" which we already have.
But I don't really like this part:
struct tick_sched *tick_get_tick_sched(int cpu)

Quoted 8 lines

{
	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
	ts->cpu = cpu;
	^^^^^^^^^^^^^
	return ts;
}

Please kindly review.

can we do this bit once, when the ts structure gets initialized? it's not like the cpu value will ever change...

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-15 16:13 UTC #26 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

I've changed struct tick_sched to match passed *ts and cpu. Also changed "&per_cpu(tick_cpu_sched, cpu)" call to "struct tick_sched *tick_get_tick_sched(int cpu)" which we already have.

But I don't really like this part:
struct tick_sched *tick_get_tick_sched(int cpu)

{
	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
	ts->cpu = cpu;
	^^^^^^^^^^^^^
	return ts;
}

Please kindly review.

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

include/linux/tick.h +1 -0

index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +14 ~~-11~~

index 1d7b9bc..5105345 100644
@@ -38,7 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
-	return &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	ts->cpu = cpu;
+	return ts;
 }
 
 /*

@@ -137,7 +139,7 @@ __setup("nohz=", setup_tick_nohz);
 static void tick_nohz_update_jiffies(ktime_t now)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	unsigned long flags;
 
 	cpumask_clear_cpu(cpu, nohz_cpu_mask);

@@ -159,9 +161,10 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = ts->cpu;
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -173,7 +176,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 
 static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	update_ts_time_stats(ts, now, NULL);
 	ts->idle_active = 0;

@@ -211,7 +214,7 @@ static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
  */
 u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -237,7 +240,7 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time_us);
  */
 u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -267,7 +270,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	local_irq_save(flags);
 
 	cpu = smp_processor_id();
-	ts = &per_cpu(tick_cpu_sched, cpu);
+	ts = tick_get_tick_sched(cpu);
 
 	/*
 	 * Call to tick_nohz_start_idle stops the last_update_time from being

@@ -508,7 +511,7 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
 void tick_nohz_restart_sched_tick(void)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING
 	unsigned long ticks;
 #endif

@@ -671,7 +674,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 #if 0
 	/* Switch back to 2.6.27 behaviour */
 
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t delta;
 
 	/*

@@ -688,7 +691,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 
 static inline void tick_check_nohz(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t now;
 
 	if (!ts->idle_active && !ts->tick_stopped)

@@ -818,7 +821,7 @@ void tick_setup_sched_timer(void)
 #if defined CONFIG_NO_HZ || defined CONFIG_HIGH_RES_TIMERS
 void tick_cancel_sched_timer(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 # ifdef CONFIG_HIGH_RES_TIMERS
 	if (ts->sched_timer.base)

Sergey Senozhatsky

author

2010-06-17 06:26 UTC #27 lore raw

PATCH 1/1 cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Fix

 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74   
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85   
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48 
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2 
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a  
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62  
 [<c10027d3>] sysenter_do_call+0x12/0x2d

The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.

Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

include/linux/tick.h +1 -0

index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +13 ~~-10~~

index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

@@ -137,7 +140,7 @@ __setup("nohz=", setup_tick_nohz);
 static void tick_nohz_update_jiffies(ktime_t now)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	unsigned long flags;
 
 	cpumask_clear_cpu(cpu, nohz_cpu_mask);

@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -173,7 +176,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 
 static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	update_ts_time_stats(ts, now, NULL);
 	ts->idle_active = 0;

@@ -211,7 +214,7 @@ static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
  */
 u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -237,7 +240,7 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time_us);
  */
 u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -267,7 +270,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	local_irq_save(flags);
 
 	cpu = smp_processor_id();
-	ts = &per_cpu(tick_cpu_sched, cpu);
+	ts = tick_get_tick_sched(cpu);
 
 	/*
 	 * Call to tick_nohz_start_idle stops the last_update_time from being

@@ -508,7 +511,7 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
 void tick_nohz_restart_sched_tick(void)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING
 	unsigned long ticks;
 #endif

@@ -671,7 +674,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 #if 0
 	/* Switch back to 2.6.27 behaviour */
 
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t delta;
 
 	/*

@@ -688,7 +691,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 
 static inline void tick_check_nohz(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t now;
 
 	if (!ts->idle_active && !ts->tick_stopped)

@@ -818,7 +821,7 @@ void tick_setup_sched_timer(void)
 #if defined CONFIG_NO_HZ || defined CONFIG_HIGH_RES_TIMERS
 void tick_cancel_sched_timer(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 # ifdef CONFIG_HIGH_RES_TIMERS
 	if (ts->sched_timer.base)

Arjan van de Ven

Acked-by on v1 1/1

2010-06-17 06:43 UTC #28 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 44 lines · On Thu, 17 Jun 2010 09:29:50 +0300 Sergey Senozhatsky…

Fix

 BUG: using smp_processor_id() in preemptible [00000000] code:
s2disk/3392 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74   
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85   
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48 
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2 
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a  
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62  
 [<c10027d3>] sysenter_do_call+0x12/0x2d

The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.

Quoted 6 lines

Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
Signed-off-by Sergey Senozhatsky <[email protected]>

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Arjan van de Ven

Acked-by on v1 1/1

2010-06-17 06:43 UTC #29 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 44 lines · On Thu, 17 Jun 2010 09:29:50 +0300 Sergey Senozhatsky…

Fix

 BUG: using smp_processor_id() in preemptible [00000000] code:
s2disk/3392 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74   
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85   
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48 
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2 
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a  
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62  
 [<c10027d3>] sysenter_do_call+0x12/0x2d

The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.

Quoted 6 lines

Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
Signed-off-by Sergey Senozhatsky <[email protected]>

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Andrew Morton

2010-06-17 06:59 UTC #30 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 51 lines · On Thu, 17 Jun 2010 09:29:50 +0300 Sergey Senozhatsky…

Fix

 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74   
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85   
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48 
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2 
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a  
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62  
 [<c10027d3>] sysenter_do_call+0x12/0x2d

The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.

...

struct tick_sched *tick_get_tick_sched(int cpu)
{

Review

Quoted 5 lines

(quoted diff) +3 -0

+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

That's just weird. And by doing a write it does require that this cahcheline be probably-read and written back regularly, which is more bus traffic.

It should be OK to initialise these guys with a for_each_possible_cpu() loop in a new module_init() function in tick-sched.c - if someone runs update_ts_time_stats() before the initcalls then conceivably the `swapper' process's accounting will go a little bit wrong, but I doubt it.

Still, it'd be better to do it earlier, I guess. tick_init() is called super-early and that would be a good place. tick_init() is presently a no-op if !CONFIG_GENERIC_CLOCKEVENTS, but all this code depends on CONFIG_GENERIC_CLOCKEVENTS anwyay.

So how does this look? If "OK" then would you be able to test it please?

[ Sigh. The field tick_sched.cpu shouldn't even exist on
uniprocessor builds. Ifdeffing it away is trivial and a bit messy, but it's still only a partial solution. Passing the `cpu' argument to nr_iowait_cpu() will generate additional code, and it's unneeded on uniprocessor builds.]

 include/linux/tick.h      |    1 +
 kernel/time/tick-common.c |    1 +
 kernel/time/tick-sched.c  |   11 ++++++++---
 3 files changed, 10 insertions(+), 3 deletions(-)

include/linux/tick.h renamed from include/linux/tick.h~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix +1 -0

diff -puN include/linux/tick.h~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix include/linux/tick.h
@@ -71,6 +71,7 @@ struct tick_sched {
 };
 
 extern void __init tick_init(void);
+extern void __init tick_sched_init(void);
 extern int tick_is_oneshot_available(void);
 extern struct tick_device *tick_get_device(int cpu);

kernel/time/tick-sched.c renamed from kernel/time/tick-sched.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix +8 -3

diff -puN kernel/time/tick-sched.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix kernel/time/tick-sched.c
@@ -38,9 +38,6 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
-	/*FIXME: Arjan van de Ven:
-	 can we do this bit once, when the ts structure gets initialized?*/
-	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

@@ -880,3 +877,11 @@ int tick_check_oneshot_change(int allow_
 	tick_nohz_switch_to_nohz();
 	return 0;
 }
+
+void __init tick_sched_init(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		per_cpu(tick_cpu_sched, cpu).cpu = cpu;
+}

kernel/time/tick-common.c renamed from kernel/time/tick-common.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix +1 -0

diff -puN kernel/time/tick-common.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix kernel/time/tick-common.c
@@ -413,4 +413,5 @@ static struct notifier_block tick_notifi
 void __init tick_init(void)
 {
 	clockevents_register_notifier(&tick_notifier);
+	tick_sched_init();
 }

Andrew Morton

2010-06-17 06:59 UTC #31 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 51 lines · On Thu, 17 Jun 2010 09:29:50 +0300 Sergey Senozhatsky…

Fix

 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74   
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85   
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48 
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2 
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a  
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62  
 [<c10027d3>] sysenter_do_call+0x12/0x2d

The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.

...

struct tick_sched *tick_get_tick_sched(int cpu)
{

Review

Quoted 5 lines

(quoted diff) +3 -0

+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

That's just weird. And by doing a write it does require that this cahcheline be probably-read and written back regularly, which is more bus traffic.

So how does this look? If "OK" then would you be able to test it please?

 include/linux/tick.h      |    1 +
 kernel/time/tick-common.c |    1 +
 kernel/time/tick-sched.c  |   11 ++++++++---
 3 files changed, 10 insertions(+), 3 deletions(-)

include/linux/tick.h renamed from include/linux/tick.h~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix +1 -0

diff -puN include/linux/tick.h~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix include/linux/tick.h
@@ -71,6 +71,7 @@ struct tick_sched {
 };
 
 extern void __init tick_init(void);
+extern void __init tick_sched_init(void);
 extern int tick_is_oneshot_available(void);
 extern struct tick_device *tick_get_device(int cpu);

kernel/time/tick-sched.c renamed from kernel/time/tick-sched.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix +8 -3

diff -puN kernel/time/tick-sched.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix kernel/time/tick-sched.c
@@ -38,9 +38,6 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
-	/*FIXME: Arjan van de Ven:
-	 can we do this bit once, when the ts structure gets initialized?*/
-	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

@@ -880,3 +877,11 @@ int tick_check_oneshot_change(int allow_
 	tick_nohz_switch_to_nohz();
 	return 0;
 }
+
+void __init tick_sched_init(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		per_cpu(tick_cpu_sched, cpu).cpu = cpu;
+}

kernel/time/tick-common.c renamed from kernel/time/tick-common.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix +1 -0

diff -puN kernel/time/tick-common.c~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix kernel/time/tick-common.c
@@ -413,4 +413,5 @@ static struct notifier_block tick_notifi
 void __init tick_init(void)
 {
 	clockevents_register_notifier(&tick_notifier);
+	tick_sched_init();
 }

Andrew Morton

2010-06-17 07:04 UTC #32 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 1 line · On Wed, 16 Jun 2010 23:59:07 -0700 Andrew Morton <[email protected]>…

So how does this look? If "OK" then would you be able to test it please?

I saw it first!

fix !CONFIG_TICK_ONESHOT

include/linux/tick.h renamed from include/linux/tick.h~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix-fix +3 -1

@@ -71,7 +71,6 @@ struct tick_sched {
 };
 
 extern void __init tick_init(void);
-extern void __init tick_sched_init(void);
 extern int tick_is_oneshot_available(void);
 extern struct tick_device *tick_get_device(int cpu);

@@ -93,6 +92,9 @@ extern struct cpumask *tick_get_broadcas
 
 #  ifdef CONFIG_TICK_ONESHOT
 extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
+extern void __init tick_sched_init(void);
+#  else
+static inline void tick_sched_init(void) { }
 #  endif
 
 # endif /* BROADCAST */

Andrew Morton

2010-06-17 07:04 UTC #33 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 1 line · On Wed, 16 Jun 2010 23:59:07 -0700 Andrew Morton <[email protected]>…

So how does this look? If "OK" then would you be able to test it please?

I saw it first!

fix !CONFIG_TICK_ONESHOT

include/linux/tick.h renamed from include/linux/tick.h~cpuidle-avoid-using-smp_processor_id-in-preemptible-code-nr_iowait_cpu-v4-fix-fix +3 -1

@@ -71,7 +71,6 @@ struct tick_sched {
 };
 
 extern void __init tick_init(void);
-extern void __init tick_sched_init(void);
 extern int tick_is_oneshot_available(void);
 extern struct tick_device *tick_get_device(int cpu);

@@ -93,6 +92,9 @@ extern struct cpumask *tick_get_broadcas
 
 #  ifdef CONFIG_TICK_ONESHOT
 extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
+extern void __init tick_sched_init(void);
+#  else
+static inline void tick_sched_init(void) { }
 #  endif
 
 # endif /* BROADCAST */

Sergey Senozhatsky

author

2010-06-17 07:30 UTC #34 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 5 lines · Andrew Morton

[..] if someone runs
update_ts_time_stats() before the initcalls then conceivably the `swapper' process's accounting will go a little bit wrong, but I doubt it.

That was the sing that scared me - update_ts_time_stats call before init. Having ".cpu = cpu" in tick_get_tick_sched guarantees correct .cpu and... and it sucks.

Quoted 8 lines

Still, it'd be better to do it earlier, I guess. tick_init() is called super-early and that would be a good place. tick_init() is presently a no-op if !CONFIG_GENERIC_CLOCKEVENTS, but all this code depends on CONFIG_GENERIC_CLOCKEVENTS anwyay.
So how does this look? If "OK" then would you be able to test it please?

I'll test it in 2 hours. Thanks.

Quoted 7 lines

[ Sigh. The field tick_sched.cpu shouldn't even exist on
uniprocessor builds. Ifdeffing it away is trivial and a bit messy, but it's still only a partial solution. Passing the `cpu' argument to nr_iowait_cpu() will generate additional code, and it's unneeded on uniprocessor builds.]

You're right.

Sergey

Sergey Senozhatsky

author

2010-06-17 07:34 UTC #35 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 5 lines · Andrew Morton

[..] if someone runs
update_ts_time_stats() before the initcalls then conceivably the `swapper' process's accounting will go a little bit wrong, but I doubt it.

That was the sing that scared me - update_ts_time_stats call before init. Having ".cpu = cpu" in tick_get_tick_sched guarantees correct .cpu and... and it sucks.

Quoted 8 lines

Still, it'd be better to do it earlier, I guess. tick_init() is called super-early and that would be a good place. tick_init() is presently a no-op if !CONFIG_GENERIC_CLOCKEVENTS, but all this code depends on CONFIG_GENERIC_CLOCKEVENTS anwyay.
So how does this look? If "OK" then would you be able to test it please?

I'll test it in 2 hours. Thanks.

Quoted 7 lines

[ Sigh. The field tick_sched.cpu shouldn't even exist on
uniprocessor builds. Ifdeffing it away is trivial and a bit messy, but it's still only a partial solution. Passing the `cpu' argument to nr_iowait_cpu() will generate additional code, and it's unneeded on uniprocessor builds.]

You're right.

Sergey

Peter Zijlstra

maintainer

2010-06-25 14:39 UTC #36 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 42 lines · Sergey Senozhatsky

Fix
 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".
This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.
Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
include/linux/tick.h +1 -0
index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;
kernel/time/tick-sched.c +3 -0
index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }
 

kernel/time/tick-sched.c update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) · line 170

Quoted 8 lines

kernel/time/tick-sched.c +1 -1

@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

This all seems extremely silly, why not something like:

---
 kernel/time/tick-sched.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

kernel/time/tick-sched.c +8 -8

index 5f171f0..1363d3a 100644
@@ -154,14 +154,14 @@ static void tick_nohz_update_jiffies(ktime_t now)
  * Updates the per cpu time idle statistics counters
  */
 static void
-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
+update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 {
 	ktime_t delta;
 
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -175,19 +175,19 @@ static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 	ts->idle_active = 0;
 
 	sched_clock_idle_wakeup_event(0);
 }
 
-static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
+static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 {
 	ktime_t now;
 
 	now = ktime_get();
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 
 	ts->idle_entrytime = now;
 	ts->idle_active = 1;

@@ -216,7 +216,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->idle_sleeptime);
 }

@@ -242,7 +242,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->iowait_sleeptime);
 }

@@ -284,7 +284,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	 */
 	ts->inidle = 1;
 
-	now = tick_nohz_start_idle(ts);
+	now = tick_nohz_start_idle(cpu, ts);
 
 	/*
 	 * If this cpu is offline and it is the one which updates

Peter Zijlstra

maintainer

2010-06-25 14:40 UTC #37 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

In reply to Peter Zijlstra

Quoted 42 lines · Sergey Senozhatsky

Fix
 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".
This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.
Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
include/linux/tick.h +1 -0
index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;
kernel/time/tick-sched.c +3 -0
index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }
 

kernel/time/tick-sched.c update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) · line 170

Quoted 8 lines

kernel/time/tick-sched.c +1 -1

@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

This all seems extremely silly, why not something like:

---
 kernel/time/tick-sched.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

kernel/time/tick-sched.c +8 -8

index 5f171f0..1363d3a 100644
@@ -154,14 +154,14 @@ static void tick_nohz_update_jiffies(ktime_t now)
  * Updates the per cpu time idle statistics counters
  */
 static void
-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
+update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 {
 	ktime_t delta;
 
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -175,19 +175,19 @@ static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 	ts->idle_active = 0;
 
 	sched_clock_idle_wakeup_event(0);
 }
 
-static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
+static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 {
 	ktime_t now;
 
 	now = ktime_get();
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 
 	ts->idle_entrytime = now;
 	ts->idle_active = 1;

@@ -216,7 +216,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->idle_sleeptime);
 }

@@ -242,7 +242,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->iowait_sleeptime);
 }

@@ -284,7 +284,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	 */
 	ts->inidle = 1;
 
-	now = tick_nohz_start_idle(ts);
+	now = tick_nohz_start_idle(cpu, ts);
 
 	/*
 	 * If this cpu is offline and it is the one which updates

Andrew Morton

2010-06-30 19:58 UTC #38 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 42 lines · On Fri, 25 Jun 2010 16:39:33 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 40 lines · Sergey Senozhatsky
Fix
 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".
This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.
Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
include/linux/tick.h +1 -0
index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;
kernel/time/tick-sched.c +3 -0
index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

Quoted 12 lines

kernel/time/tick-sched.c update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) · line 170
Quoted 9 lines
kernel/time/tick-sched.c +1 -1
@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}
This all seems extremely silly, why not something like:

Does it work?

c'mon guys, it's taking us weeks and weeks to fix one simple bug. It's a regression! We should be in panic mode.

Andrew Morton

2010-06-30 19:59 UTC #39 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 42 lines · On Fri, 25 Jun 2010 16:39:33 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 40 lines · Sergey Senozhatsky
Fix
 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".
This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.
Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
include/linux/tick.h +1 -0
index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;
kernel/time/tick-sched.c +3 -0
index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

Quoted 12 lines

kernel/time/tick-sched.c update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) · line 170
Quoted 9 lines
kernel/time/tick-sched.c +1 -1
@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}
This all seems extremely silly, why not something like:

Does it work?

c'mon guys, it's taking us weeks and weeks to fix one simple bug. It's a regression! We should be in panic mode.

Sergey Senozhatsky

author

2010-07-01 06:13 UTC #40 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 63 lines · Andrew Morton

Subject: Re: [PATCH] cpuidle: avoid using smp_processor_id() in
preemptible
code (nr_iowait_cpu) v4
X-Mailer: Sylpheed 2.4.8 (GTK+ 2.12.9; x86_64-pc-linux-gnu)
Quoted 42 lines · On Fri, 25 Jun 2010 16:39:33 +0200 Peter Zijlstra <[email protected]> wrote:
Quoted 40 lines · Sergey Senozhatsky
Fix
 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".
This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.
Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
include/linux/tick.h +1 -0
index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;
kernel/time/tick-sched.c +3 -0
index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }
Quoted 12 lines
kernel/time/tick-sched.c update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) · line 170
Quoted 9 lines
kernel/time/tick-sched.c +1 -1
@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}
This all seems extremely silly, why not something like:

Quoted 5 lines

Does it work?
c'mon guys, it's taking us weeks and weeks to fix one simple bug. It's a regression! We should be in panic mode.

Hello,

Quoted 3 lines · Sergey Senozhatsky

Quoted 3 lines
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?

Quoted 2 lines · Arjan van de Ven

that'd be option one
option two is to add a "cpu" member to struct tick_sched.....

So, it's been discussed. I chose option #2 however and made a mistake. Personally I prefer Peter's patch. I need some time to test it.

Sergey

Sergey Senozhatsky

author

2010-07-01 06:17 UTC #41 lore raw

Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Quoted 63 lines · Andrew Morton

Subject: Re: [PATCH] cpuidle: avoid using smp_processor_id() in
preemptible
code (nr_iowait_cpu) v4
X-Mailer: Sylpheed 2.4.8 (GTK+ 2.12.9; x86_64-pc-linux-gnu)
Quoted 42 lines · On Fri, 25 Jun 2010 16:39:33 +0200 Peter Zijlstra <[email protected]> wrote:
Quoted 40 lines · Sergey Senozhatsky
Fix
 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".
This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.
Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats since we "pick the current cpu, rather than the one denoted by ts" in that case. To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
include/linux/tick.h +1 -0
index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;
kernel/time/tick-sched.c +3 -0
index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }
Quoted 12 lines
kernel/time/tick-sched.c update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time) · line 170
Quoted 9 lines
kernel/time/tick-sched.c +1 -1
@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}
This all seems extremely silly, why not something like:

Quoted 5 lines

Does it work?
c'mon guys, it's taking us weeks and weeks to fix one simple bug. It's a regression! We should be in panic mode.

Hello,

Quoted 3 lines · Sergey Senozhatsky

Quoted 3 lines
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?

Quoted 2 lines · Arjan van de Ven

that'd be option one
option two is to add a "cpu" member to struct tick_sched.....

So, it's been discussed. I chose option #2 however and made a mistake. Personally I prefer Peter's patch. I need some time to test it.

Sergey

Peter Zijlstra

maintainer

2010-07-01 07:07 UTC #42 lore raw

PATCH 1/1 sched: Cure nr_iowait_cpu() users

With 0224cf4c5e (sched: Intoduce get_cpu_iowait_time_us()) Arjan broke things by not making sure preemption was indeed disabled by the callers of nr_iowait_cpu() which took the iowait value of the current cpu.

This resulted in a heap of preempt warnings. Cure this by making nr_iowait_cpu() take a cpu number and fix up the callers to pass in the right number.

---
Confirmed to work..

 drivers/cpuidle/governors/menu.c |    4 ++--
 include/linux/sched.h            |    2 +-
 kernel/sched.c                   |    4 ++--
 kernel/time/tick-sched.c         |   16 ++++++++--------
 4 files changed, 13 insertions(+), 13 deletions(-)

drivers/cpuidle/governors/menu.c +2 -2

index 52ff8aa..1b12870 100644
@@ -143,7 +143,7 @@ static inline int which_bucket(unsigned int duration)
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(smp_processor_id()))
 		bucket = BUCKETS/2;
 
 	if (duration < 10)

@@ -175,7 +175,7 @@ static inline int performance_multiplier(void)
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(smp_processor_id());
 
 	return mult;
 }

include/linux/sched.h +1 -1

index a61c08c..1f25798 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index 71e3dc8..d3c0262 100644
@@ -2946,9 +2946,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +8 -8

index e0707ea..17525ca 100644
@@ -154,14 +154,14 @@ static void tick_nohz_update_jiffies(ktime_t now)
  * Updates the per cpu time idle statistics counters
  */
 static void
-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
+update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 {
 	ktime_t delta;
 
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -175,19 +175,19 @@ static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 	ts->idle_active = 0;
 
 	sched_clock_idle_wakeup_event(0);
 }
 
-static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
+static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 {
 	ktime_t now;
 
 	now = ktime_get();
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 
 	ts->idle_entrytime = now;
 	ts->idle_active = 1;

@@ -216,7 +216,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->idle_sleeptime);
 }

@@ -242,7 +242,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->iowait_sleeptime);
 }

@@ -284,7 +284,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	 */
 	ts->inidle = 1;
 
-	now = tick_nohz_start_idle(ts);
+	now = tick_nohz_start_idle(cpu, ts);
 
 	/*
 	 * If this cpu is offline and it is the one which updates

Sergey Senozhatsky

author Acked-by on v1 1/1

2010-07-01 08:14 UTC #43 lore raw

Re: [PATCH] sched: Cure nr_iowait_cpu() users

Acked-by Sergey Senozhatsky <[email protected]> (??)

Sergey

Quoted 144 lines · Peter Zijlstra

Subject: [PATCH] sched: Cure nr_iowait_cpu() users
X-Mailer: Evolution 2.28.3

This resulted in a heap of preempt warnings. Cure this by making nr_iowait_cpu() take a cpu number and fix up the callers to pass in the right number.

In reply to Peter Zijlstra

---
Confirmed to work..

 drivers/cpuidle/governors/menu.c |    4 ++--
 include/linux/sched.h            |    2 +-
 kernel/sched.c                   |    4 ++--
 kernel/time/tick-sched.c         |   16 ++++++++--------
 4 files changed, 13 insertions(+), 13 deletions(-)

drivers/cpuidle/governors/menu.c +2 -2

index 52ff8aa..1b12870 100644
@@ -143,7 +143,7 @@ static inline int which_bucket(unsigned int duration)
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(smp_processor_id()))
 		bucket = BUCKETS/2;
 
 	if (duration < 10)

@@ -175,7 +175,7 @@ static inline int performance_multiplier(void)
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(smp_processor_id());
 
 	return mult;
 }

include/linux/sched.h +1 -1

index a61c08c..1f25798 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index 71e3dc8..d3c0262 100644
@@ -2946,9 +2946,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +8 -8

index e0707ea..17525ca 100644
@@ -154,14 +154,14 @@ static void tick_nohz_update_jiffies(ktime_t now)
  * Updates the per cpu time idle statistics counters
  */
 static void
-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
+update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 {
 	ktime_t delta;
 
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -175,19 +175,19 @@ static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 	ts->idle_active = 0;
 
 	sched_clock_idle_wakeup_event(0);
 }
 
-static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
+static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 {
 	ktime_t now;
 
 	now = ktime_get();
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 
 	ts->idle_entrytime = now;
 	ts->idle_active = 1;

@@ -216,7 +216,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->idle_sleeptime);
 }

@@ -242,7 +242,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->iowait_sleeptime);
 }

@@ -284,7 +284,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	 */
 	ts->inidle = 1;
 
-	now = tick_nohz_start_idle(ts);
+	now = tick_nohz_start_idle(cpu, ts);
 
 	/*
 	 * If this cpu is offline and it is the one which updates

Sergey Senozhatsky

author Acked-by on v1 1/1

2010-07-01 08:18 UTC #44 lore raw

Re: [PATCH] sched: Cure nr_iowait_cpu() users

Acked-by Sergey Senozhatsky <[email protected]> (??)

Sergey

Quoted 144 lines · Peter Zijlstra

Subject: [PATCH] sched: Cure nr_iowait_cpu() users
X-Mailer: Evolution 2.28.3

This resulted in a heap of preempt warnings. Cure this by making nr_iowait_cpu() take a cpu number and fix up the callers to pass in the right number.

In reply to Peter Zijlstra

---
Confirmed to work..

 drivers/cpuidle/governors/menu.c |    4 ++--
 include/linux/sched.h            |    2 +-
 kernel/sched.c                   |    4 ++--
 kernel/time/tick-sched.c         |   16 ++++++++--------
 4 files changed, 13 insertions(+), 13 deletions(-)

drivers/cpuidle/governors/menu.c +2 -2

index 52ff8aa..1b12870 100644
@@ -143,7 +143,7 @@ static inline int which_bucket(unsigned int duration)
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(smp_processor_id()))
 		bucket = BUCKETS/2;
 
 	if (duration < 10)

@@ -175,7 +175,7 @@ static inline int performance_multiplier(void)
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(smp_processor_id());
 
 	return mult;
 }

include/linux/sched.h +1 -1

index a61c08c..1f25798 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index 71e3dc8..d3c0262 100644
@@ -2946,9 +2946,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +8 -8

index e0707ea..17525ca 100644
@@ -154,14 +154,14 @@ static void tick_nohz_update_jiffies(ktime_t now)
  * Updates the per cpu time idle statistics counters
  */
 static void
-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
+update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 {
 	ktime_t delta;
 
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -175,19 +175,19 @@ static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 	ts->idle_active = 0;
 
 	sched_clock_idle_wakeup_event(0);
 }
 
-static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
+static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 {
 	ktime_t now;
 
 	now = ktime_get();
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 
 	ts->idle_entrytime = now;
 	ts->idle_active = 1;

@@ -216,7 +216,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->idle_sleeptime);
 }

@@ -242,7 +242,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->iowait_sleeptime);
 }

@@ -284,7 +284,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	 */
 	ts->inidle = 1;
 
-	now = tick_nohz_start_idle(ts);
+	now = tick_nohz_start_idle(cpu, ts);
 
 	/*
 	 * If this cpu is offline and it is the one which updates

tip-bot for Peter Zijlstra

maintainer

2010-07-01 08:47 UTC #45 lore raw

[tip:sched/urgent] sched: Cure nr_iowait_cpu() users

Commit-ID:  8c215bd3890c347dfb6a2db4779755f8b9c298a9
Gitweb:     http://git.kernel.org/tip/8c215bd3890c347dfb6a2db4779755f8b9c298a9
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Thu, 1 Jul 2010 09:07:17 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 1 Jul 2010 09:39:48 +0200

sched: Cure nr_iowait_cpu() users

Commit 0224cf4c5e (sched: Intoduce get_cpu_iowait_time_us()) broke things by not making sure preemption was indeed disabled by the callers of nr_iowait_cpu() which took the iowait value of the current cpu.

This resulted in a heap of preempt warnings. Cure this by making nr_iowait_cpu() take a cpu number and fix up the callers to pass in the right number.

Cc Arjan van de Ven <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Maxim Levitsky <[email protected]>

Cc Len Brown <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Jiri Slaby <[email protected]>

Cc [email protected]

LKML-Reference: <1277968037.1868.120.camel@laptop>

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

---
 drivers/cpuidle/governors/menu.c |    4 ++--
 include/linux/sched.h            |    2 +-
 kernel/sched.c                   |    4 ++--
 kernel/time/tick-sched.c         |   16 ++++++++--------
 4 files changed, 13 insertions(+), 13 deletions(-)

drivers/cpuidle/governors/menu.c +2 -2

index 52ff8aa..1b12870 100644
@@ -143,7 +143,7 @@ static inline int which_bucket(unsigned int duration)
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(smp_processor_id()))
 		bucket = BUCKETS/2;
 
 	if (duration < 10)

@@ -175,7 +175,7 @@ static inline int performance_multiplier(void)
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(smp_processor_id());
 
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index a24d6d5..f87abe3 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +8 -8

index 1d7b9bc..1a6f828 100644
@@ -154,14 +154,14 @@ static void tick_nohz_update_jiffies(ktime_t now)
  * Updates the per cpu time idle statistics counters
  */
 static void
-update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
+update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 {
 	ktime_t delta;
 
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -175,19 +175,19 @@ static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 	ts->idle_active = 0;
 
 	sched_clock_idle_wakeup_event(0);
 }
 
-static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
+static ktime_t tick_nohz_start_idle(int cpu, struct tick_sched *ts)
 {
 	ktime_t now;
 
 	now = ktime_get();
 
-	update_ts_time_stats(ts, now, NULL);
+	update_ts_time_stats(cpu, ts, now, NULL);
 
 	ts->idle_entrytime = now;
 	ts->idle_active = 1;

@@ -216,7 +216,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->idle_sleeptime);
 }

@@ -242,7 +242,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 	if (!tick_nohz_enabled)
 		return -1;
 
-	update_ts_time_stats(ts, ktime_get(), last_update_time);
+	update_ts_time_stats(cpu, ts, ktime_get(), last_update_time);
 
 	return ktime_to_us(ts->iowait_sleeptime);
 }

@@ -284,7 +284,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	 */
 	ts->inidle = 1;
 
-	now = tick_nohz_start_idle(ts);
+	now = tick_nohz_start_idle(cpu, ts);
 
 	/*
 	 * If this cpu is offline and it is the one which updates

Sergey Senozhatsky

author

2010-06-17 06:29 UTC #46 lore raw

PATCH 1/1 cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

Fix

 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74   
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85   
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48 
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2 
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a  
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62  
 [<c10027d3>] sysenter_do_call+0x12/0x2d

The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

include/linux/tick.h +1 -0

index b232ccc..db14691 100644
@@ -51,6 +51,7 @@ struct tick_sched {
 	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
 	ktime_t				idle_tick;
+	int				cpu;
 	int				inidle;
 	int				tick_stopped;
 	unsigned long			idle_jiffies;

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +13 ~~-10~~

index 1d7b9bc..1907037 100644
@@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
 
 struct tick_sched *tick_get_tick_sched(int cpu)
 {
+	/*FIXME: Arjan van de Ven:
+	 can we do this bit once, when the ts structure gets initialized?*/
+	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
 	return &per_cpu(tick_cpu_sched, cpu);
 }

@@ -137,7 +140,7 @@ __setup("nohz=", setup_tick_nohz);
 static void tick_nohz_update_jiffies(ktime_t now)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	unsigned long flags;
 
 	cpumask_clear_cpu(cpu, nohz_cpu_mask);

@@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(ts->cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
 		ts->idle_entrytime = now;
 	}

@@ -173,7 +176,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 
 static void tick_nohz_stop_idle(int cpu, ktime_t now)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	update_ts_time_stats(ts, now, NULL);
 	ts->idle_active = 0;

@@ -211,7 +214,7 @@ static ktime_t tick_nohz_start_idle(struct tick_sched *ts)
  */
 u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -237,7 +240,7 @@ EXPORT_SYMBOL_GPL(get_cpu_idle_time_us);
  */
 u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 	if (!tick_nohz_enabled)
 		return -1;

@@ -267,7 +270,7 @@ void tick_nohz_stop_sched_tick(int inidle)
 	local_irq_save(flags);
 
 	cpu = smp_processor_id();
-	ts = &per_cpu(tick_cpu_sched, cpu);
+	ts = tick_get_tick_sched(cpu);
 
 	/*
 	 * Call to tick_nohz_start_idle stops the last_update_time from being

@@ -508,7 +511,7 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
 void tick_nohz_restart_sched_tick(void)
 {
 	int cpu = smp_processor_id();
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING
 	unsigned long ticks;
 #endif

@@ -671,7 +674,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 #if 0
 	/* Switch back to 2.6.27 behaviour */
 
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t delta;
 
 	/*

@@ -688,7 +691,7 @@ static void tick_nohz_kick_tick(int cpu, ktime_t now)
 
 static inline void tick_check_nohz(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 	ktime_t now;
 
 	if (!ts->idle_active && !ts->tick_stopped)

@@ -818,7 +821,7 @@ void tick_setup_sched_timer(void)
 #if defined CONFIG_NO_HZ || defined CONFIG_HIGH_RES_TIMERS
 void tick_cancel_sched_timer(int cpu)
 {
-	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
+	struct tick_sched *ts = tick_get_tick_sched(cpu);
 
 # ifdef CONFIG_HIGH_RES_TIMERS
 	if (ts->sched_timer.base)

Arjan van de Ven

2010-06-15 15:08 UTC #47 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 9 lines · On Tue, 15 Jun 2010 17:50:29 +0300 Sergey Senozhatsky…

Quoted 4 lines
hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....
Hmm. Thanks, good catch.
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?

that'd be option one
option two is to add a "cpu" member to struct tick_sched.....

if you go for option one, I'd replace the ts argument with the cpu argument.....

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-15 14:50 UTC #48 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 9 lines · Arjan van de Ven

Quoted 7 lines · On Tue, 15 Jun 2010 09:19:27 +0300 Sergey Senozhatsky…
kernel/time/tick-sched.c +0 -0
index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts,
 ktime_t now, u64 *last_update_time) ktime_t delta;
 

Quoted 17 lines

Quoted 11 lines

kernel/time/tick-sched.c +3 -1

 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime,
 delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime =
 ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;

Quoted 1 line

}

hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....

Hmm. Thanks, good catch.
Well, there is something I'm missing. How can I match given *ts and cpu in update_ts_time_stats (except for introducing update_ts_time_stats(..., int cpu)) ?

Quoted 5 lines

they will normally be the same, except for the case where you get your warning...
(and this is also why you can't move the get_cpu() inside nr_iowait_cpu() ... )

Sergey

Arjan van de Ven

2010-06-15 14:24 UTC #49 lore raw

Re: [PATCH] avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Quoted 18 lines · On Tue, 15 Jun 2010 09:19:27 +0300 Sergey Senozhatsky…

kernel/time/tick-sched.c +3 -1

index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts,
 ktime_t now, u64 *last_update_time) ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime,
 delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime =
 ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;

Quoted 1 line

}

hmm this part is wrong
you pick the current cpu, rather than the one denoted by ts.....

they will normally be the same, except for the case where you get your warning...
(and this is also why you can't move the get_cpu() inside nr_iowait_cpu() ... )

Quoted 1 line

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-15 06:19 UTC #50 lore raw

PATCH 1/1 avoid using smp_processor_id() in preemptible code (nr_iowait_cpu)

Fixing

 BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
 caller is nr_iowait_cpu+0xe/0x1e
 Pid: 3392, comm: s2disk Not tainted 2.6.35-rc3-dbg-00106-ga75e02b #2
 Call Trace:
 [<c1184c55>] debug_smp_processor_id+0xa5/0xbc
 [<c10282a5>] nr_iowait_cpu+0xe/0x1e
 [<c104ab7c>] update_ts_time_stats+0x32/0x6c
 [<c104ac73>] get_cpu_idle_time_us+0x36/0x58
 [<c124229b>] get_cpu_idle_time+0x12/0x74
 [<c1242963>] cpufreq_governor_dbs+0xc3/0x2dc
 [<c1240437>] __cpufreq_governor+0x51/0x85
 [<c1241190>] __cpufreq_set_policy+0x10c/0x13d
 [<c12413d3>] cpufreq_add_dev_interface+0x212/0x233
 [<c1241b1e>] ? handle_update+0x0/0xd
 [<c1241a18>] cpufreq_add_dev+0x34b/0x35a
 [<c103c973>] ? schedule_delayed_work_on+0x11/0x13
 [<c12c14db>] cpufreq_cpu_callback+0x59/0x63
 [<c1042f39>] notifier_call_chain+0x26/0x48
 [<c1042f7d>] __raw_notifier_call_chain+0xe/0x10
 [<c102efb9>] __cpu_notify+0x15/0x29
 [<c102efda>] cpu_notify+0xd/0xf
 [<c12bfb30>] _cpu_up+0xaf/0xd2
 [<c12b3ad4>] enable_nonboot_cpus+0x3d/0x94
 [<c1055eef>] hibernation_snapshot+0x104/0x1a2
 [<c1058b49>] snapshot_ioctl+0x24b/0x53e
 [<c1028ad1>] ? sub_preempt_count+0x7c/0x89
 [<c10ab91d>] vfs_ioctl+0x2e/0x8c
 [<c10588fe>] ? snapshot_ioctl+0x0/0x53e
 [<c10ac2c7>] do_vfs_ioctl+0x42f/0x45a
 [<c10a0ba5>] ? fsnotify_modify+0x4f/0x5a
 [<c11e9dc3>] ? tty_write+0x0/0x1d0
 [<c10a12d6>] ? vfs_write+0xa2/0xda
 [<c10ac333>] sys_ioctl+0x41/0x62
 [<c10027d3>] sysenter_do_call+0x12/0x2d

Initially fix was to use get_cpu/put_cpu in nr_iowait_cpu. However, Arjan van de Ven stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".

This patch introduces nr_iowait_cpu(int cpu) and changes to it callers.

Signed-off-by Sergey Senozhatsky <[email protected]>

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +3 -1

index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;
 	}

Arjan van de Ven

Acked-by

2010-06-15 03:40 UTC #51 lore raw

Quoted 9 lines · On Mon, 14 Jun 2010 18:17:35 +0300 Sergey Senozhatsky…

Quoted 6 lines · Arjan van de Ven
Quoted 1 line
decided to patch nr_iowait_cpu instead.
the bug is that it needs to be
nr_iowait_cpu(int cpu)
Something similar to this?

yup that'll do it

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-14 15:17 UTC #52 lore raw

Quoted 6 lines · Arjan van de Ven

Quoted 1 line
decided to patch nr_iowait_cpu instead.
the bug is that it needs to be
nr_iowait_cpu(int cpu)

Something similar to this?

---

drivers/cpuidle/governors/menu.c +9 -3

index 52ff8aa..4871ed5 100644
@@ -137,14 +137,17 @@ static inline int which_bucket(unsigned int duration)
 {
 	int bucket = 0;
 
+	int cpu = get_cpu();
 	/*
 	 * We keep two groups of stats; one with no
 	 * IO pending, one without.
 	 * This allows us to calculate
 	 * E(duration)|iowait
 	 */
-	if (nr_iowait_cpu())
+	if (nr_iowait_cpu(cpu))
 		bucket = BUCKETS/2;
+	
+	put_cpu();
 
 	if (duration < 10)
 		return bucket;

@@ -169,14 +172,17 @@ static inline int which_bucket(unsigned int duration)
 static inline int performance_multiplier(void)
 {
 	int mult = 1;
-
+	int cpu = get_cpu();
+	
 	/* for higher loadavg, we are more reluctant */
 
 	mult += 2 * get_loadavg();
 
 	/* for IO wait tasks (per cpu!) we add 5x each */
-	mult += 10 * nr_iowait_cpu();
+	mult += 10 * nr_iowait_cpu(cpu);
 
+	put_cpu();
+	
 	return mult;
 }

include/linux/sched.h +1 -1

index f118809..747fcae 100644
@@ -139,7 +139,7 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
-extern unsigned long nr_iowait_cpu(void);
+extern unsigned long nr_iowait_cpu(int cpu);
 extern unsigned long this_cpu_load(void);

kernel/sched.c +2 -2

index f8b8996..f61b48e 100644
@@ -2864,9 +2864,9 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
-unsigned long nr_iowait_cpu(void)
+unsigned long nr_iowait_cpu(int cpu)
 {
-	struct rq *this = this_rq();
+	struct rq *this = cpu_rq(cpu);
 	return atomic_read(&this->nr_iowait);
 }

kernel/time/tick-sched.c +3 -1

index 1d7b9bc..101e8aa 100644
@@ -159,10 +159,12 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
 	ktime_t delta;
 
 	if (ts->idle_active) {
+		int cpu = get_cpu();
 		delta = ktime_sub(now, ts->idle_entrytime);
 		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
-		if (nr_iowait_cpu() > 0)
+		if (nr_iowait_cpu(cpu) > 0)
 			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
+		put_cpu();
 		ts->idle_entrytime = now;
 	}

Arjan van de Ven

2010-06-14 15:01 UTC #53 lore raw

Quoted 13 lines · On Mon, 14 Jun 2010 17:54:40 +0300 Sergey Senozhatsky…

Quoted 9 lines · Arjan van de Ven
Quoted 2 lines
Hello,
Not sure if this simple solution is the correct one.
it's not; the caller needs to pass in the cpu number I suspect for this to be really correct....
I just returned from family emergency travel and will take a look today
I thought about patching

Quoted 7 lines

./drivers/cpuidle/governors/menu.c: if (nr_iowait_cpu())
./drivers/cpuidle/governors/menu.c: mult += 10 *
nr_iowait_cpu(); ./kernel/time/tick-sched.c: if
(nr_iowait_cpu() > 0)
decided to patch nr_iowait_cpu instead.

the bug is that it needs to be

nr_iowait_cpu(int cpu)

Signature · 3 lines

Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Sergey Senozhatsky

author

2010-06-14 14:54 UTC #54 lore raw

Quoted 8 lines · Arjan van de Ven

Quoted 2 lines
Hello,
Not sure if this simple solution is the correct one.
it's not; the caller needs to pass in the cpu number I suspect for this to be really correct....
I just returned from family emergency travel and will take a look today

I thought about patching

./drivers/cpuidle/governors/menu.c: if (nr_iowait_cpu())
./drivers/cpuidle/governors/menu.c: mult += 10 * nr_iowait_cpu();
./kernel/time/tick-sched.c: if (nr_iowait_cpu() > 0)

decided to patch nr_iowait_cpu instead.

Sergey

Arjan van de Ven

2010-06-14 14:38 UTC #55 lore raw