Xen devel - Nov 2009 - [PATCH] Don''t assume the vcpu_id is continous in alloc_vcpu

Currently in alloc_vcpu, it assumes the vcpu is allocated with vcpu_id is
continous.
When cpu hot-added, this assumption is broken because the hot-added CPU may be
brougt online by dom0 in arbitrary order. This patch try to link the new vcpu to
the end of the link.

Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>


diff -r 68b6bf9b5c8b xen/common/domain.c
--- a/xen/common/domain.c	Thu Nov 12 04:08:25 2009 +0800
+++ b/xen/common/domain.c	Thu Nov 12 04:08:43 2009 +0800
@@ -176,7 +176,13 @@ struct vcpu *alloc_vcpu(
 
     d->vcpu[vcpu_id] = v;
     if ( vcpu_id != 0 )
-        d->vcpu[v->vcpu_id-1]->next_in_list = v;
+    {
+        struct vcpu *tmp = d->vcpu[0];
+
+        while (tmp->next_in_list)
+            tmp = tmp->next_in_list;
+        tmp->next_in_list = v;
+    }
 
     /* Must be called after making new vcpu visible to for_each_vcpu(). */
     vcpu_check_shutdown(v);


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

On 12/11/2009 11:08, "Jiang, Yunhong" <yunhong.jiang@intel.com>
wrote:
> Currently in alloc_vcpu, it assumes the vcpu is allocated with vcpu_id is
> continous.
> When cpu hot-added, this assumption is broken because the hot-added CPU may
be
> brougt online by dom0 in arbitrary order. This patch try to link the new
vcpu
> to the end of the link.
> 
> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
Is this something to do with allocating vcpus for the idle domain?

If so, I suggest we just allocate vcpu_ids sequentially on-demand. I can
work up a patch for you to test if you like.

 -- Keir



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

On 12/11/2009 11:31, "Keir Fraser" <keir.fraser@eu.citrix.com>
wrote:
>> Currently in alloc_vcpu, it assumes the vcpu is allocated with vcpu_id
is
>> continous.
>> When cpu hot-added, this assumption is broken because the hot-added CPU
may
>> be
>> brougt online by dom0 in arbitrary order. This patch try to link the
new vcpu
>> to the end of the link.
>> 
>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
> 
> Is this something to do with allocating vcpus for the idle domain?
> 
> If so, I suggest we just allocate vcpu_ids sequentially on-demand. I can
> work up a patch for you to test if you like.
I was wrong, that was a bad idea since idle_vcpu[] is also the idle
domain''s
vcpu pointer array. Actually I also have been able to revert c/s 20045 since
I was mistaken about that being needed. And I''ve applied your patch as
c/s
20433, but I rewrote it to keep vcpus in ascending order of vcpuid (not sure
it''s necessary really, but it''s nice to do).

 -- Keir



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

Keir Fraser wrote:
> On 12/11/2009 11:31, "Keir Fraser"
<keir.fraser@eu.citrix.com> wrote:
> 
>>> Currently in alloc_vcpu, it assumes the vcpu is allocated with
>>> vcpu_id is continous. When cpu hot-added, this assumption is broken
>>> because the hot-added CPU may be brougt online by dom0 in arbitrary
>>> order. This patch try to link the new vcpu to the end of the link. 
>>> 
>>> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>
>> 
>> Is this something to do with allocating vcpus for the idle domain?
>> 
>> If so, I suggest we just allocate vcpu_ids sequentially on-demand. I
>> can work up a patch for you to test if you like.
> 
> I was wrong, that was a bad idea since idle_vcpu[] is also the
> idle domain''s
> vcpu pointer array. Actually I also have been able to revert
> c/s 20045 since
> I was mistaken about that being needed. And I''ve applied your
> patch as c/s
> 20433, but I rewrote it to keep vcpus in ascending order of
> vcpuid (not sure
> it''s necessary really, but it''s nice to do).
Thanks for your changes very much.
I just noticed I missed get a lock for this change.
Originally I thought the XEN_DOMCTL_max_vcpus will be synced, but seems no. So
alloc_vcpu() may have race condition. (for idle_vcpu or dom0, it should be safe
already)

I''m also wondering if we need take lock at XEN_DOMCTL_max_vcpus,
because following checking may failed if two hypercall at most same time. Or, we
need depends on user space tools to keep this synced?
       if ( (d->max_vcpus > 0) && (max > d->max_vcpus) )
            goto maxvcpu_out;

Thanks
Yunhong Jiang
> 
> -- Keir
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 12/11/2009 15:27, "Jiang, Yunhong" <yunhong.jiang@intel.com>
wrote:
> Thanks for your changes very much.
> I just noticed I missed get a lock for this change.
> Originally I thought the XEN_DOMCTL_max_vcpus will be synced, but seems no.
So
> alloc_vcpu() may have race condition. (for idle_vcpu or dom0, it should be
> safe already)
All domctl operations are serialised on domctl_lock. I think that suffices?

 -- Keir



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

Aha, yes. I just checked the beginning of the do_domctl, sorry for wrong alerm.

--jyh

Keir Fraser wrote:
> On 12/11/2009 15:27, "Jiang, Yunhong"
<yunhong.jiang@intel.com> wrote:
> 
>> Thanks for your changes very much.
>> I just noticed I missed get a lock for this change.
>> Originally I thought the XEN_DOMCTL_max_vcpus will be synced, but
>> seems no. So alloc_vcpu() may have race condition. (for idle_vcpu or
>> dom0, it should be safe already)
> 
> All domctl operations are serialised on domctl_lock. I think
> that suffices?
> 
> -- Keir
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel