Skip to contentSkip to conversation

x86-x86_64 flush cache for CPU hotplug

PATCH In review Shaohua Li 4 participants 7 messages Jun 10, 2005 – Jun 13, 2005

lore
Share & tools
lore

Shaohua Li

author
#0 lore raw

PATCH 1/1 x86-x86_64 flush cache for CPU hotplug

Hi,
We should flush cache at CPU hotplug. An error has been observed data is corrupted after CPU hotplug in CPUs with bigger cache.
I guess IA64 requires similar change, Ashok?

Thanks,
Shaohua

Signed-off-by Shaohua.li<[email protected]>

---

 linux-2.6.12-rc6-mm1-root/arch/i386/kernel/process.c   |    1 +
 linux-2.6.12-rc6-mm1-root/arch/x86_64/kernel/process.c |    1 +
 2 files changed, 2 insertions(+)
arch/i386/kernel/process.c renamed from arch/i386/kernel/process.c~flush_cache_cpuhotplug +1 -0
diff -puN arch/i386/kernel/process.c~flush_cache_cpuhotplug arch/i386/kernel/process.c
@@ -155,6 +155,7 @@ static inline void play_dead(void)
 {
 	/* This must be done before dead CPU ack */
 	cpu_exit_clear();
+	wbinvd();
 	mb();
 	/* Ack it */
 	__get_cpu_var(cpu_state) = CPU_DEAD;
arch/x86_64/kernel/process.c renamed from arch/x86_64/kernel/process.c~flush_cache_cpuhotplug +1 -0
diff -puN arch/x86_64/kernel/process.c~flush_cache_cpuhotplug arch/x86_64/kernel/process.c
@@ -165,6 +165,7 @@ DECLARE_PER_CPU(int, cpu_state);
 static inline void play_dead(void)
 {
 	idle_task_exit();
+	wbinvd();
 	mb();
 	/* Ack it */
 	__get_cpu_var(cpu_state) = CPU_DEAD;

_

#1 lore raw
Quoted 4 lines · Shaohua Li

Hi,
We should flush cache at CPU hotplug. An error has been observed data is corrupted after CPU hotplug in CPUs with bigger cache.
I guess IA64 requires similar change, Ashok?

Yes, we need that for IA64 as well. i did put that in my notes tbd, but didnt turn around doing it yet.

Think we also need to define the same for asm-ia64/acpi.h FLUSH_CPU_CACHE i will try updating both in next.

If you have a targetted test that causes the corruption that would be great so we can add to our stress test senarios.

Cheers,
ashok

Quoted 11 lines

Thanks,
Shaohua

Signed-off-by Shaohua.li<[email protected]>

---

 linux-2.6.12-rc6-mm1-root/arch/i386/kernel/process.c   |    1 +
 linux-2.6.12-rc6-mm1-root/arch/x86_64/kernel/process.c |    1 +
 2 files changed, 2 insertions(+)
#2 lore raw
In reply to Shaohua Li
Quoted 5 lines · Shaohua Li

Hi,
We should flush cache at CPU hotplug. An error has been observed data is corrupted after CPU hotplug in CPUs with bigger cache.
I guess IA64 requires similar change, Ashok?

Good catch. Yes, we need it for IA64 as well. Strange i had put it in my TBD, and out of 2 items, this is one of them. I need to muck something for mca apart from this pending flush.

I think i will update the ACPI_FLUSH_CPU_CACHE as well when i push this change.

Quoted 7 lines
Signed-off-by Shaohua.li<[email protected]>

---

 linux-2.6.12-rc6-mm1-root/arch/i386/kernel/process.c   |    1 +
 linux-2.6.12-rc6-mm1-root/arch/x86_64/kernel/process.c |    1 +
 2 files changed, 2 insertions(+)
#3 lore raw
In reply to Shaohua Li
Quoted 3 lines · Shaohua Li

Hi,
We should flush cache at CPU hotplug. An error has been observed data is corrupted after CPU hotplug in CPUs with bigger cache.

Did you see that with actual hardware CPU hotplug? With software for testing only hotplug it should not make any difference.

/Andi

author
#4 lore raw
Quoted 7 lines · Andi Kleen
Quoted 3 lines · Shaohua Li

Hi,
We should flush cache at CPU hotplug. An error has been observed data is corrupted after CPU hotplug in CPUs with bigger cache.

Did you see that with actual hardware CPU hotplug? With software for testing only hotplug it should not make any difference.

Sure, this should only occur on physical CPU hotplug. I haven't hardware which supports physical CPU hotplug. What I'm testing is supend-to-ram. In this case, we offline all APs and BIOS possibly will put APs into some special states (maybe reboot APs). It's very like physical CPU hotplug.

Quoted 2 lines

If you have a targetted test that causes the corruption that would be great so we can add to our stress test senarios.

No, I haven't. IA64 doesn't support suspend/resume, so my test case can't apply to IA64.

Thanks,
Shaohua

#5 lore raw
In reply to Shaohua Li

Hello Shaohua,

Quoted 3 lines · Shaohua Li

We should flush cache at CPU hotplug. An error has been observed data is corrupted after CPU hotplug in CPUs with bigger cache.
I guess IA64 requires similar change, Ashok?

Interesting, which processor was this?

Quoted 11 lines
Signed-off-by Shaohua.li<[email protected]>

---

 linux-2.6.12-rc6-mm1-root/arch/i386/kernel/process.c   |    1 +
 linux-2.6.12-rc6-mm1-root/arch/x86_64/kernel/process.c |    1 +
 2 files changed, 2 insertions(+)

diff -puN arch/i386/kernel/process.c~flush_cache_cpuhotplug arch/i386/kernel/process.c

arch/i386/kernel/process.c static inline void play_dead(void) · line 159

Quoted 5 lines
arch/i386/kernel/process.c renamed from arch/i386/kernel/process.c~flush_cache_cpuhotplug +1 -0
@@ -155,6 +155,7 @@ static inline void play_dead(void)
 {
 	/* This must be done before dead CPU ack */
 	cpu_exit_clear();
+	wbinvd();
 	mb();

We shouldn't need that mb() there anymore then, ditto for the other location.

author
#6 lore raw
Quoted 9 lines · Zwane Mwaikambo

Hello Shaohua,

Quoted 3 lines · Shaohua Li

We should flush cache at CPU hotplug. An error has been observed data is corrupted after CPU hotplug in CPUs with bigger cache.
I guess IA64 requires similar change, Ashok?

Interesting, which processor was this?

It's a kind of Pentium M.

Quoted 5 lines
Quoted 5 lines
 linux-2.6.12-rc6-mm1-root/arch/i386/kernel/process.c   |    1 +
 linux-2.6.12-rc6-mm1-root/arch/x86_64/kernel/process.c |    1 +
 2 files changed, 2 insertions(+)
Quoted 12 lines

arch/i386/kernel/process.c static inline void play_dead(void) · line 159

Quoted 9 lines

diff -puN arch/i386/kernel/process.c~flush_cache_cpuhotplug arch/i386/kernel/process.c

arch/i386/kernel/process.c renamed from arch/i386/kernel/process.c~flush_cache_cpuhotplug +1 -0
@@ -155,6 +155,7 @@ static inline void play_dead(void)
 {
 	/* This must be done before dead CPU ack */
 	cpu_exit_clear();
+	wbinvd();
 	mb();

We shouldn't need that mb() there anymore then, ditto for the other location.

Indead. How about just leave it here for clearness?

Thanks,
Shaohua