generic-ipi: fix the smp_mb() placement

PATCH In review Suresh Siddha 4 participants 16 messages Oct 29, 2008 – Nov 5, 2008

Share & tools

Suresh Siddha

author

2008-10-29 22:43 UTC #0 lore raw

PATCH 1/1 generic-ipi: fix the smp_mb() placement

While looking at some other issue recently, we encountered this smp_mb() placement issue. x86 specific code also needs some similar fixes. Patch for that will follow soon.

Please review the appended generic-ipi fix.

thanks,
suresh
---

From: Suresh Siddha <[email protected]>
Subject: generic-ipi: fix the smp_mb() placement

smp_mb() is needed (to make the memory operations visible globally) before sending the ipi on the sender and the receiver (on Alpha atleast) needs smp_read_barrier_depends() in the handler before reading the call_single_queue list in a lock-free fashion.

On x86, x2apic mode register accesses for sending IPI's don't have serializing semantics. So the need for smp_mb() before sending the IPI becomes more critical in x2apic mode.

Remove the unnecessary smp_mb() in csd_flag_wait(), as the presence of that smp_mb() doesn't mean anything on the sender, when the ipi receiver is not doing any thing special (like memory fence) after clearing the CSD_FLAG_WAIT.

Signed-off-by Suresh Siddha <[email protected]>

---

kernel/smp.c +12 -6

index f362a85..75c8dde 100644
@@ -51,10 +51,6 @@ static void csd_flag_wait(struct call_single_data *data)
 {
 	/* Wait for response */
 	do {
-		/*
-		 * We need to see the flags store in the IPI handler
-		 */
-		smp_mb();
 		if (!(data->flags & CSD_FLAG_WAIT))
 			break;
 		cpu_relax();

@@ -76,6 +72,11 @@ static void generic_exec_single(int cpu, struct call_single_data *data)
 	list_add_tail(&data->list, &dst->list);
 	spin_unlock_irqrestore(&dst->lock, flags);
 
+	/*
+	 * Make the list addition visible before sending the ipi.
+	 */
+	smp_mb();
+
 	if (ipi)
 		arch_send_call_function_single_ipi(cpu);

@@ -157,7 +158,7 @@ void generic_smp_call_function_single_interrupt(void)
 	 * Need to see other stores to list head for checking whether
 	 * list is empty without holding q->lock
 	 */
-	smp_mb();
+	smp_read_barrier_depends();
 	while (!list_empty(&q->list)) {
 		unsigned int data_flags;

@@ -191,7 +192,7 @@ void generic_smp_call_function_single_interrupt(void)
 		/*
 		 * See comment on outer loop
 		 */
-		smp_mb();
+		smp_read_barrier_depends();
 	}
 }

@@ -370,6 +371,11 @@ int smp_call_function_mask(cpumask_t mask, void (*func)(void *), void *info,
 	list_add_tail_rcu(&data->csd.list, &call_function_queue);
 	spin_unlock_irqrestore(&call_function_lock, flags);
 
+	/*
+	 * Make the list addition visible before sending the ipi.
+	 */
+	smp_mb();
+
 	/* Send a message to all CPUs in the map */
 	arch_send_call_function_ipi(mask);

Jens Axboe

2008-10-30 07:21 UTC #1 lore raw

Quoted 5 lines · Suresh Siddha

While looking at some other issue recently, we encountered this smp_mb() placement issue. x86 specific code also needs some similar fixes. Patch for that will follow soon.
Please review the appended generic-ipi fix.

Looks good, nice debugging! A few comments below.

Quoted 40 lines

thanks,
suresh
---
From: Suresh Siddha <[email protected]>
Subject: generic-ipi: fix the smp_mb() placement
smp_mb() is needed (to make the memory operations visible globally) before sending the ipi on the sender and the receiver (on Alpha atleast) needs smp_read_barrier_depends() in the handler before reading the call_single_queue list in a lock-free fashion.
On x86, x2apic mode register accesses for sending IPI's don't have serializing semantics. So the need for smp_mb() before sending the IPI becomes more critical in x2apic mode.
Remove the unnecessary smp_mb() in csd_flag_wait(), as the presence of that smp_mb() doesn't mean anything on the sender, when the ipi receiver is not doing any thing special (like memory fence) after clearing the CSD_FLAG_WAIT.
Signed-off-by Suresh Siddha <[email protected]>
---
kernel/smp.c +0 -4
index f362a85..75c8dde 100644
@@ -51,10 +51,6 @@ static void csd_flag_wait(struct call_single_data *data)
 {
 	/* Wait for response */
 	do {
-		/*
-		 * We need to see the flags store in the IPI handler
-		 */
-		smp_mb();
 		if (!(data->flags & CSD_FLAG_WAIT))
 			break;
 		cpu_relax();

kernel/smp.c static void generic_exec_single(int cpu, struct call_single_data *data) · line 81

Quoted 10 lines

kernel/smp.c +5 -0

@@ -76,6 +72,11 @@ static void generic_exec_single(int cpu, struct call_single_data *data)
 	list_add_tail(&data->list, &dst->list);
 	spin_unlock_irqrestore(&dst->lock, flags);
 
+	/*
+	 * Make the list addition visible before sending the ipi.
+	 */
+	smp_mb();
+
 	if (ipi)
 		arch_send_call_function_single_ipi(cpu);

We can downgrade this to a smp_wmb().

Quoted 20 lines

kernel/smp.c +2 -2

@@ -157,7 +158,7 @@ void generic_smp_call_function_single_interrupt(void)
 	 * Need to see other stores to list head for checking whether
 	 * list is empty without holding q->lock
 	 */
-	smp_mb();
+	smp_read_barrier_depends();
 	while (!list_empty(&q->list)) {
 		unsigned int data_flags;

@@ -191,7 +192,7 @@ void generic_smp_call_function_single_interrupt(void)
 		/*
 		 * See comment on outer loop
 		 */
-		smp_mb();
+		smp_read_barrier_depends();
 	}
 }

kernel/smp.c int smp_call_function_mask(cpumask_t mask, void (*func)(void *), void *info, · line 380

Quoted 10 lines

kernel/smp.c +5 -0

@@ -370,6 +371,11 @@ int smp_call_function_mask(cpumask_t mask, void (*func)(void *), void *info,
 	list_add_tail_rcu(&data->csd.list, &call_function_queue);
 	spin_unlock_irqrestore(&call_function_lock, flags);
 
+	/*
+	 * Make the list addition visible before sending the ipi.
+	 */
+	smp_mb();
+
 	/* Send a message to all CPUs in the map */
 	arch_send_call_function_ipi(mask);

Ditto

Signature · 1 line

Jens Axboe

Suresh Siddha

author

2008-10-30 16:30 UTC #2 lore raw

Quoted 4 lines · Jens Axboe

Quoted 4 lines

kernel/smp.c +0 -0

@@ -76,6 +72,11 @@ static void generic_exec_single(int cpu, struct call_single_data *data)
 	list_add_tail(&data->list, &dst->list);
 	spin_unlock_irqrestore(&dst->lock, flags);

Quoted 9 lines

kernel/smp.c static void generic_exec_single(int cpu, struct call_single_data *data) · line 81
Quoted 7 lines
kernel/smp.c +5 -0
+	/*
+	 * Make the list addition visible before sending the ipi.
+	 */
+	smp_mb();
+
 	if (ipi)
 		arch_send_call_function_single_ipi(cpu);
We can downgrade this to a smp_wmb().

No. We want the ipi receiver to see the new consistent data rather than possible old consistent data.

And on x86, smp_wmb() is a simple barrier() (in !CONFIG_X86_OOSTORE) and which doesn't do much in this case.

on x86 mfence (smp_mb()) will ensure that msr based APIC (x2apic) accesses (ipi) will be visible only after the memory operations before smp_mb() are made visible.

thanks,
suresh

Jens Axboe

2008-10-30 17:26 UTC #3 lore raw

Quoted 15 lines · Suresh Siddha

Quoted 4 lines · Jens Axboe
Quoted 4 lines
kernel/smp.c +0 -0
@@ -76,6 +72,11 @@ static void generic_exec_single(int cpu, struct call_single_data *data)
 	list_add_tail(&data->list, &dst->list);
 	spin_unlock_irqrestore(&dst->lock, flags);
 
Quoted 9 lines
kernel/smp.c static void generic_exec_single(int cpu, struct call_single_data *data) · line 81
Quoted 7 lines
kernel/smp.c +5 -0
+	/*
+	 * Make the list addition visible before sending the ipi.
+	 */
+	smp_mb();
+
 	if (ipi)
 		arch_send_call_function_single_ipi(cpu);
We can downgrade this to a smp_wmb().

Quoted 2 lines

No. We want the ipi receiver to see the new consistent data rather than possible old consistent data.

Oh right, we need visibility here and not just store ordering.

Quoted 7 lines

And on x86, smp_wmb() is a simple barrier() (in !CONFIG_X86_OOSTORE) and which doesn't do much in this case.
on x86 mfence (smp_mb()) will ensure that msr based APIC (x2apic) accesses (ipi) will be visible only after the memory operations before smp_mb() are made visible.

OK, I'm convinced. I'll queue up the patch, thanks!

Signature · 1 line

Jens Axboe

Ingo Molnar

2008-10-30 18:54 UTC #4 lore raw

In reply to Suresh Siddha

Quoted 7 lines · Suresh Siddha

Subject: generic-ipi: fix the smp_mb() placement
smp_mb() is needed (to make the memory operations visible globally) before sending the ipi on the sender and the receiver (on Alpha atleast) needs smp_read_barrier_depends() in the handler before reading the call_single_queue list in a lock-free fashion.

Quoted 8 lines

On x86, x2apic mode register accesses for sending IPI's don't have serializing semantics. So the need for smp_mb() before sending the IPI becomes more critical in x2apic mode.
Remove the unnecessary smp_mb() in csd_flag_wait(), as the presence of that smp_mb() doesn't mean anything on the sender, when the ipi receiver is not doing any thing special (like memory fence) after clearing the CSD_FLAG_WAIT.

nice! Did you see an actual lockup due to this? Seems like a v2.6.28 fix to me in any case.

Ingo

Suresh Siddha

author

2008-10-30 20:23 UTC #5 lore raw

Quoted 14 lines · Ingo Molnar

Quoted 11 lines · Suresh Siddha
Subject: generic-ipi: fix the smp_mb() placement
smp_mb() is needed (to make the memory operations visible globally) before sending the ipi on the sender and the receiver (on Alpha atleast) needs smp_read_barrier_depends() in the handler before reading the call_single_queue list in a lock-free fashion.
On x86, x2apic mode register accesses for sending IPI's don't have serializing semantics. So the need for smp_mb() before sending the IPI becomes more critical in x2apic mode.

Quoted 6 lines

Quoted 4 lines
Remove the unnecessary smp_mb() in csd_flag_wait(), as the presence of that smp_mb() doesn't mean anything on the sender, when the ipi receiver is not doing any thing special (like memory fence) after clearing the CSD_FLAG_WAIT.
nice! Did you see an actual lockup due to this?

We didn't see the lockup in our tests but Xen folks reported similar failures with their smp call function code.

Quoted 1 line

Seems like a v2.6.28 fix to me in any case.

Yes.

thanks,
suresh

Jeremy Fitzhardinge

2008-10-31 05:10 UTC #6 lore raw

Quoted 3 lines · Suresh Siddha

We didn't see the lockup in our tests but Xen folks reported similar failures with their smp call function code.

...really? I don't remember anything like that, but perhaps I'm forgetting something. In Xen the IPI is sent with a hypercall, which is definitely a solid enough barrier for these purposes.

Ingo Molnar

2008-10-31 09:40 UTC #7 lore raw

Quoted 8 lines · Jeremy Fitzhardinge

Quoted 3 lines · Suresh Siddha
We didn't see the lockup in our tests but Xen folks reported similar failures with their smp call function code.
...really? I don't remember anything like that, but perhaps I'm forgetting something. In Xen the IPI is sent with a hypercall, which is definitely a solid enough barrier for these purposes.

i think Suresh might be referring to some of the fragilities Xen had with generic-ipi. But those AFAICT were due to the on-stack lifetime bug that Nick fixed via the kmalloc? v2.6.26-ish issue.

Ingo

Jeremy Fitzhardinge

2008-10-31 11:12 UTC #8 lore raw

Quoted 5 lines · Ingo Molnar

Quoted 2 lines · Jeremy Fitzhardinge
Suresh Siddha wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
We didn't see the lockup in our tests but Xen folks reported similar failures with their smp call function code.
...really? I don't remember anything like that, but perhaps I'm forgetting something. In Xen the IPI is sent with a hypercall, which is definitely a solid enough barrier for these purposes.
i think Suresh might be referring to some of the fragilities Xen had with generic-ipi. But those AFAICT were due to the on-stack lifetime bug that Nick fixed via the kmalloc? v2.6.26-ish issue.

Right, that's all I could think of.

Suresh Siddha

author

2008-10-31 16:53 UTC #9 lore raw

Quoted 15 lines · Jeremy Fitzhardinge

Quoted 14 lines · Ingo Molnar
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 4 lines · Suresh Siddha
We didn't see the lockup in our tests but Xen folks reported similar failures with their smp call function code.
...really? I don't remember anything like that, but perhaps I'm forgetting something. In Xen the IPI is sent with a hypercall, which is definitely a solid enough barrier for these purposes.

Quoted 5 lines

Quoted 3 lines
i think Suresh might be referring to some of the fragilities Xen had with generic-ipi. But those AFAICT were due to the on-stack lifetime bug that Nick fixed via the kmalloc? v2.6.26-ish issue.
Right, that's all I could think of.

No. I am referring to Xen hypervisor code fix recently done by the Xen team in the Intel.

http://xenbits.xensource.com/xen-unstable.hg?rev/50170dc8649c

thanks,
suresh

Jeremy Fitzhardinge

2008-10-31 20:30 UTC #10 lore raw

Quoted 5 lines · Suresh Siddha

No. I am referring to Xen hypervisor code fix recently done by the Xen team in the Intel.
http://xenbits.xensource.com/xen-unstable.hg?rev/50170dc8649c

Ah, yes, OK then.

Ingo Molnar

2008-11-03 10:18 UTC #11 lore raw

Quoted 8 lines · Jeremy Fitzhardinge

Quoted 5 lines · Suresh Siddha
No. I am referring to Xen hypervisor code fix recently done by the Xen team in the Intel.
http://xenbits.xensource.com/xen-unstable.hg?rev/50170dc8649c
Ah, yes, OK then.

ok - so that makes it a v2.6.28 item i guess.

Ingo

Jeremy Fitzhardinge

2008-11-03 23:48 UTC #12 lore raw

Quoted 2 lines · Ingo Molnar

ok - so that makes it a v2.6.28 item i guess.

The case Suresh is talking about was a fix to Xen itself, rather than on the kernel side, so it doesn't need to be a .28 issue on Xen's account.

Ingo Molnar

2008-11-04 09:21 UTC #13 lore raw

Quoted 7 lines · Jeremy Fitzhardinge

Quoted 2 lines · Ingo Molnar
ok - so that makes it a v2.6.28 item i guess.
The case Suresh is talking about was a fix to Xen itself, rather than on the kernel side, so it doesn't need to be a .28 issue on Xen's account.

ok - but still the portion of the fix that strengthens barriers looks obvious to have and there's little downside that i can see.

Suresh, you might want to split the patch(es) in two: get the barrier strengthening changes into v2.6.28 (to fix the x2apic bug), while the aspects that _weaken_ barriers can wait for v2.6.29.

With that it would be a 100% safe change for v2.6.28-rc4.

Ingo

Suresh Siddha

author

2008-11-04 22:25 UTC #14 lore raw

Quoted 11 lines · Ingo Molnar

Quoted 7 lines · Jeremy Fitzhardinge
Quoted 2 lines · Ingo Molnar
ok - so that makes it a v2.6.28 item i guess.
The case Suresh is talking about was a fix to Xen itself, rather than on the kernel side, so it doesn't need to be a .28 issue on Xen's account.

Quoted 8 lines

ok - but still the portion of the fix that strengthens barriers looks obvious to have and there's little downside that i can see.
Suresh, you might want to split the patch(es) in two: get the barrier strengthening changes into v2.6.28 (to fix the x2apic bug), while the aspects that _weaken_ barriers can wait for v2.6.29.
With that it would be a 100% safe change for v2.6.28-rc4.

Ok. I just posted three patches (including the x86 specific change).

[patch 1/3] generic-ipi: add smp_mb() before sending the IPI [patch 2/3] x86: Add smp_mb() before sending INVALIDATE_TLB_VECTOR [patch 3/3] generic-ipi: fix the smp_mb() usage

First two patches are safe to go into v2.6.28. Third patch can wait for v2.6.29.

thanks,
suresh

Jens Axboe

2008-11-05 09:22 UTC #15 lore raw

Quoted 21 lines · Suresh Siddha

Quoted 11 lines · Ingo Molnar
Quoted 7 lines · Jeremy Fitzhardinge
Quoted 2 lines · Ingo Molnar
ok - so that makes it a v2.6.28 item i guess.
The case Suresh is talking about was a fix to Xen itself, rather than on the kernel side, so it doesn't need to be a .28 issue on Xen's account.
Quoted 8 lines
ok - but still the portion of the fix that strengthens barriers looks obvious to have and there's little downside that i can see.
Suresh, you might want to split the patch(es) in two: get the barrier strengthening changes into v2.6.28 (to fix the x2apic bug), while the aspects that _weaken_ barriers can wait for v2.6.29.
With that it would be a 100% safe change for v2.6.28-rc4.

Quoted 7 lines

Ok. I just posted three patches (including the x86 specific change).
[patch 1/3] generic-ipi: add smp_mb() before sending the IPI [patch 2/3] x86: Add smp_mb() before sending INVALIDATE_TLB_VECTOR [patch 3/3] generic-ipi: fix the smp_mb() usage
First two patches are safe to go into v2.6.28. Third patch can wait for v2.6.29.

I already have the combined 1+3 patch queued up...

Signature · 1 line

Jens Axboe