Xen devel - Jun 2010 - [PATCH] Fix cpu hotplug bug of mtrr update inconsistency

Fix cpu hotplug bug of mtrr update inconsistency

c/s 20021 changes some mtrr update logic.
A bug is, when a cpu hot-add and then mtrr update, another cpu hot-add may break
cpu_online_map
consistency and result in deadlock. 
This patch is used to fix the bug. It move ''mtrr_ap_init'' back
to bp-ap sync stage protected by
CPU_STATE_CALLOUT and CPU_STATE_CALLIN, and then keep consistency.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>


diff -r 96917cf25bf3 xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c	Fri May 28 10:54:07 2010 +0100
+++ b/xen/arch/x86/smpboot.c	Tue Jun 01 13:58:44 2010 +0800
@@ -202,6 +202,8 @@ void smp_callin(void)
     /* Save our processor parameters. */
     smp_store_cpu_info(cpu);
 
+    mtrr_ap_init();
+
     if ( (rc = hvm_cpu_up()) != 0 )
     {
         extern void (*dead_idle) (void);
@@ -374,7 +376,6 @@ void start_secondary(void *unused)
 
     /* We can take interrupts now: we''re officially "up". */
     local_irq_enable();
-    mtrr_ap_init();
 
     microcode_resume_cpu(cpu);
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 01/06/2010 09:22, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> Fix cpu hotplug bug of mtrr update inconsistency
> 
> c/s 20021 changes some mtrr update logic.
> A bug is, when a cpu hot-add and then mtrr update, another cpu hot-add may
> break cpu_online_map
> consistency and result in deadlock.
> This patch is used to fix the bug. It move ''mtrr_ap_init''
back to bp-ap sync
> stage protected by
> CPU_STATE_CALLOUT and CPU_STATE_CALLIN, and then keep consistency.
This is an old bug, rather than introduced by my changes, right?

I suggest we leave the call where it is, and fix set_mtrr() to not race CPUs
coming online. It is called elsewhere other than mtrr_ap_init() after all.
Also if you call mtrr_ap_init() before being in cpu_online_map, you then
race further MTRR changes:
 - CPU X calls mtrr_ap_init()
 - CPU Y calls set_mtrr() to actually change an MTRR.
 - CPU X adds itself to cpu_online_map
 - Aiee, CPU X is missing Y''s update

I''ll make a patch.

By the way, our MTRR subsystem is really pants! :-/

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>Sent: Tuesday, June 01, 2010 5:19 PM
>To: Liu, Jinsong; xen-devel@lists.xensource.com
>Subject: [Xen-devel] Re: [PATCH] Fix cpu hotplug bug of mtrr update
inconsistency
>
>On 01/06/2010 09:22, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
>
>> Fix cpu hotplug bug of mtrr update inconsistency
>>
>> c/s 20021 changes some mtrr update logic.
>> A bug is, when a cpu hot-add and then mtrr update, another cpu hot-add
may
>> break cpu_online_map
>> consistency and result in deadlock.
>> This patch is used to fix the bug. It move
''mtrr_ap_init'' back to bp-ap sync
>> stage protected by
>> CPU_STATE_CALLOUT and CPU_STATE_CALLIN, and then keep consistency.
>
>This is an old bug, rather than introduced by my changes, right?
Yes, not caused by the new changes.
>
>I suggest we leave the call where it is, and fix set_mtrr() to not race CPUs
>coming online. It is called elsewhere other than mtrr_ap_init() after all.
>Also if you call mtrr_ap_init() before being in cpu_online_map, you then
>race further MTRR changes:
There is a cpu_hotplug lock held when mtrr_add/del_page, but that lock is NULL
now.
One argument is, because the mtrr_add/del_page and the cpu_o*ling request all
from dom0, if dom0 should sync all vCPUs before mtrr_a/d_page, it should be safe
that there is no race. But not sure about dom0''s operation.

--jyh
> - CPU X calls mtrr_ap_init()
> - CPU Y calls set_mtrr() to actually change an MTRR.
> - CPU X adds itself to cpu_online_map
> - Aiee, CPU X is missing Y''s update
>
>I''ll make a patch.
>
>By the way, our MTRR subsystem is really pants! :-/
>
> -- Keir
>
>
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 01/06/2010 10:18, "Keir Fraser" <keir.fraser@eu.citrix.com>
wrote:
> I suggest we leave the call where it is, and fix set_mtrr() to not race
CPUs
> coming online. It is called elsewhere other than mtrr_ap_init() after all.
> Also if you call mtrr_ap_init() before being in cpu_online_map, you then
> race further MTRR changes:
>  - CPU X calls mtrr_ap_init()
>  - CPU Y calls set_mtrr() to actually change an MTRR.
>  - CPU X adds itself to cpu_online_map
>  - Aiee, CPU X is missing Y''s update
> 
> I''ll make a patch.
xen-unstable:21499.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 01/06/2010 10:18, "Keir Fraser"
<keir.fraser@eu.citrix.com> wrote:
> 
>> I suggest we leave the call where it is, and fix set_mtrr() to not
>> race CPUs coming online. It is called elsewhere other than
>> mtrr_ap_init() after all. Also if you call mtrr_ap_init() before
>> being in cpu_online_map, you then race further MTRR changes: 
>>  - CPU X calls mtrr_ap_init()
>>  - CPU Y calls set_mtrr() to actually change an MTRR.
>>  - CPU X adds itself to cpu_online_map
>>  - Aiee, CPU X is missing Y''s update
>> 
>> I''ll make a patch.
> 
> xen-unstable:21499.
> 
>  -- Keir
Yeah, good method :)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel