qemu-xen: fix cpu hotplug
The current xenstore watch path for a vcpu-set event is wrong and is
also wrong the code to parse it.
This patch fixes both of them: a xenstore vcpu hotplug command is of the
following form:
path: /local/domain/DOMID/cpu/VCPU_NUMBER/availability
values: "online" or "offline"
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
diff --git a/xenstore.c b/xenstore.c
index 6d24613..2c325ad 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -680,9 +680,12 @@ void xenstore_parse_domain_config(int hvm_domid)
}
/* Set a watch for vcpu-set */
- if (pasprintf(&buf, "/local/domain/%u/cpu", domid) != -1) {
- xs_watch(xsh, buf, "vcpu-set");
- fprintf(logfile, "Watching %s\n", buf);
+ for (i = 0; i < vcpus; i++) {
+ if (pasprintf(&buf,
"/local/domain/%u/cpu/%u/availability",
+ domid, i) != -1) {
+ xs_watch(xsh, buf, "vcpu-set");
+ fprintf(logfile, "Watching %s\n", buf);
+ }
}
/* no need for ifdef CONFIG_STUBDOM, since in the qemu case
@@ -970,15 +973,14 @@ void xenstore_record_dm_state(const char *state)
static void xenstore_process_vcpu_set_event(char **vec)
{
char *act = NULL;
- char *vcpustr, *node = vec[XS_WATCH_PATH];
- unsigned int vcpu, len;
+ char *node = vec[XS_WATCH_PATH];
+ unsigned int vcpu, len, domid;
- vcpustr = strstr(node, "cpu/");
- if (!vcpustr) {
- fprintf(stderr, "vcpu-set: watch node error.\n");
+ if (sscanf(node, "/local/domain/%u/cpu/%u/availability",
+ &domid, &vcpu) <= 0) {
+ fprintf(stderr, "vcpu-set: watch node error, path=%s\n",
node);
return;
}
- sscanf(vcpustr, "cpu/%u", &vcpu);
act = xs_read(xsh, XBT_NULL, node, &len);
if (!act) {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Stefano Stabellini writes ("[Xen-devel] [PATCH] qemu-xen: fix cpu
hotplug"):> qemu-xen: fix cpu hotplug
>
> The current xenstore watch path for a vcpu-set event is wrong and is
> also wrong the code to parse it. This patch fixes both of them:
Thanks. So it seems you''re saying it''s completely broken in
xen-unstable.
I have CC''d a bunch of Intel folks who were doing some other
work on cpu hotplug. They were dealing with a race when multiple CPUs
were added at once.
So I think there must be some confusion. Perhaps xl and xend have
different ideas about what the xenstore syntax is for these operations ?
Jinsong Liu et al: would you care to comment ? Particularly about this:
> A xenstore vcpu hotplug command is of the following form:
> path: /local/domain/DOMID/cpu/VCPU_NUMBER/availability
> values: "online" or "offline"
That doesn''t seem to match up with what''s in xend, which seems
to
write "cpu_avail" in the vm tree.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson wrote:> Stefano Stabellini writes ("[Xen-devel] [PATCH] qemu-xen: fix cpu > hotplug"): >> qemu-xen: fix cpu hotplug >> >> The current xenstore watch path for a vcpu-set event is wrong and is >> also wrong the code to parse it. This patch fixes both of them: > > Thanks. So it seems you''re saying it''s completely broken in > xen-unstable.Stabellini, I read your attached patch, it''s OK. In fact, we firstly implemented xenstore watch by same scheme of your patch, watching each cpu node status: /local/domain/xx/cpu/yy/availability=offline (online) However, we finally didn''t use this scheme. We watch ''common'' node instead: /local/domain/xx/cpu in this way, only 1 watch point need. Considering vcpu number may become more and more in the future (say, more than 128), it''s more simple and reasonable. (Watches can be set at points in the hierarchy and an individual watch will be triggered when anything at or below that point in the hierachy changes)> > I have CC''d a bunch of Intel folks who were doing some other > work on cpu hotplug. They were dealing with a race when multiple CPUs > were added at once. > > So I think there must be some confusion. Perhaps xl and xend have > different ideas about what the xenstore syntax is for these > operations ? > > Jinsong Liu et al: would you care to comment ? Particularly about > this: > >> A xenstore vcpu hotplug command is of the following form: >> path: /local/domain/DOMID/cpu/VCPU_NUMBER/availability >> values: "online" or "offline"Yes, I think there must be some confusion. Currently ''xm vcpu-set'' command works fine with both PV and HVM vcpu hotplug. Stabellini/Jackson, would your please tell me what xl recently happened for vcpu hotplug? is there any different ideas xl and xend about xenstore syntax? (Each time ''xm vcpu-set'' executed, xend will write all xenstore cpu node status)> > That doesn''t seem to match up with what''s in xend, which seems to > write "cpu_avail" in the vm tree. > > Thanks, > Ian._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Friday 03 September 2010 08:28:53 Liu, Jinsong wrote:> Ian Jackson wrote: > > Stefano Stabellini writes ("[Xen-devel] [PATCH] qemu-xen: fix cpu > > > > hotplug"): > >> qemu-xen: fix cpu hotplug > >> > >> The current xenstore watch path for a vcpu-set event is wrong and is > >> also wrong the code to parse it. This patch fixes both of them: > > > > Thanks. So it seems you''re saying it''s completely broken in > > xen-unstable. > > Stabellini, I read your attached patch, it''s OK. > In fact, we firstly implemented xenstore watch by same scheme of your > patch, watching each cpu node status: > /local/domain/xx/cpu/yy/availability=offline (online) > > However, we finally didn''t use this scheme. We watch ''common'' node instead: > /local/domain/xx/cpu > in this way, only 1 watch point need. > Considering vcpu number may become more and more in the future (say, more > than 128), it''s more simple and reasonable. (Watches can be set at points > in the hierarchy and an individual watch will be triggered when anything at > or below that point in the hierachy changes)Does this scheme allow to say how many cores per cpu exist ? When you run a Windows guest with a license for one cpu socket, then you can use 4 cores. But if one cpu is equal to one socket, then you can''t use SMP for the Windows guest. Christoph> > > I have CC''d a bunch of Intel folks who were doing some other > > work on cpu hotplug. They were dealing with a race when multiple CPUs > > were added at once. > > > > So I think there must be some confusion. Perhaps xl and xend have > > different ideas about what the xenstore syntax is for these > > operations ? > > > > Jinsong Liu et al: would you care to comment ? Particularly about > > > > this: > >> A xenstore vcpu hotplug command is of the following form: > >> path: /local/domain/DOMID/cpu/VCPU_NUMBER/availability > >> values: "online" or "offline" > > Yes, I think there must be some confusion. > Currently ''xm vcpu-set'' command works fine with both PV and HVM vcpu > hotplug. > > Stabellini/Jackson, would your please tell me what xl recently happened for > vcpu hotplug? is there any different ideas xl and xend about xenstore > syntax? > (Each time ''xm vcpu-set'' executed, xend will write all xenstore cpu node > status) > > > That doesn''t seem to match up with what''s in xend, which seems to > > write "cpu_avail" in the vm tree. > > > > Thanks, > > Ian.-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger wrote:> On Friday 03 September 2010 08:28:53 Liu, Jinsong wrote: >> Ian Jackson wrote: >>> Stefano Stabellini writes ("[Xen-devel] [PATCH] qemu-xen: fix cpu >>> >>> hotplug"): >>>> qemu-xen: fix cpu hotplug >>>> >>>> The current xenstore watch path for a vcpu-set event is wrong and >>>> is also wrong the code to parse it. This patch fixes both of them: >>> >>> Thanks. So it seems you''re saying it''s completely broken in >>> xen-unstable. >> >> Stabellini, I read your attached patch, it''s OK. >> In fact, we firstly implemented xenstore watch by same scheme of your >> patch, watching each cpu node status: >> /local/domain/xx/cpu/yy/availability=offline (online) >> >> However, we finally didn''t use this scheme. We watch ''common'' node >> instead: /local/domain/xx/cpu in this way, only 1 watch point need. >> Considering vcpu number may become more and more in the future (say, >> more than 128), it''s more simple and reasonable. (Watches can be set >> at points in the hierarchy and an individual watch will be triggered >> when anything at or below that point in the hierachy changes) > > Does this scheme allow to say how many cores per cpu exist ? > > When you run a Windows guest with a license for one cpu socket, > then you can use 4 cores. But if one cpu is equal to one socket, > then you can''t use SMP for the Windows guest. > > Christoph >Seems this is another story? Jinsong> >> >>> I have CC''d a bunch of Intel folks who were doing some other >>> work on cpu hotplug. They were dealing with a race when multiple >>> CPUs were added at once. >>> >>> So I think there must be some confusion. Perhaps xl and xend have >>> different ideas about what the xenstore syntax is for these >>> operations ? >>> >>> Jinsong Liu et al: would you care to comment ? Particularly about >>> >>> this: >>>> A xenstore vcpu hotplug command is of the following form: >>>> path: /local/domain/DOMID/cpu/VCPU_NUMBER/availability >>>> values: "online" or "offline" >> >> Yes, I think there must be some confusion. >> Currently ''xm vcpu-set'' command works fine with both PV and HVM vcpu >> hotplug. >> >> Stabellini/Jackson, would your please tell me what xl recently >> happened for vcpu hotplug? is there any different ideas xl and xend >> about xenstore syntax? (Each time ''xm vcpu-set'' executed, xend will >> write all xenstore cpu node status) >> >>> That doesn''t seem to match up with what''s in xend, which seems to >>> write "cpu_avail" in the vm tree. >>> >>> Thanks, >>> Ian._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Sep-03 10:10 UTC
RE: [Xen-devel] [PATCH] qemu-xen: fix cpu hotplug
On Fri, 3 Sep 2010, Liu, Jinsong wrote:> Christoph Egger wrote: > > On Friday 03 September 2010 08:28:53 Liu, Jinsong wrote: > >> Ian Jackson wrote: > >>> Stefano Stabellini writes ("[Xen-devel] [PATCH] qemu-xen: fix cpu > >>> > >>> hotplug"): > >>>> qemu-xen: fix cpu hotplug > >>>> > >>>> The current xenstore watch path for a vcpu-set event is wrong and > >>>> is also wrong the code to parse it. This patch fixes both of them: > >>> > >>> Thanks. So it seems you''re saying it''s completely broken in > >>> xen-unstable. > >> > >> Stabellini, I read your attached patch, it''s OK. > >> In fact, we firstly implemented xenstore watch by same scheme of your > >> patch, watching each cpu node status: > >> /local/domain/xx/cpu/yy/availability=offline (online) > >> > >> However, we finally didn''t use this scheme. We watch ''common'' node > >> instead: /local/domain/xx/cpu in this way, only 1 watch point need. > >> Considering vcpu number may become more and more in the future (say, > >> more than 128), it''s more simple and reasonable. (Watches can be set > >> at points in the hierarchy and an individual watch will be triggered > >> when anything at or below that point in the hierachy changes) > > > > Does this scheme allow to say how many cores per cpu exist ? > > > > When you run a Windows guest with a license for one cpu socket, > > then you can use 4 cores. But if one cpu is equal to one socket, > > then you can''t use SMP for the Windows guest. > > > > Christoph > > > > Seems this is another story? >Yes, the scheme we are talking about regards the communication of vcpu online and offline to qemu and nothing else. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Liu, Jinsong writes ("RE: [Xen-devel] [PATCH] qemu-xen: fix cpu
hotplug"):> Stabellini, I read your attached patch, it''s OK.
So we should apply Stefano''s patch ? I''m not sure
I''m convinced.
> In fact, we firstly implemented xenstore watch by same scheme of
> your patch, watching each cpu node status:
> /local/domain/xx/cpu/yy/availability=offline (online)
>
> However, we finally didn''t use this scheme. We watch
''common'' node
> instead: /local/domain/xx/cpu in this way, only 1 watch point need.
> Considering vcpu number may become more and more in the future (say,
> more than 128), it''s more simple and reasonable. (Watches can be
> set at points in the hierarchy and an individual watch will be
> triggered when anything at or below that point in the hierachy
> changes)
Here you seem to be saying that the scheme that you implemented (in
xend, I take it) is not the same as the one in Stefano''s patch.
> Yes, I think there must be some confusion.
> Currently ''xm vcpu-set'' command works fine with both PV
and HVM vcpu hotplug.
Right, good.
> Stabellini/Jackson, would your please tell me what xl recently
> happened for vcpu hotplug?
Well, we accepted patches to try to implement "xl vcpu-set" to make it
work like "xm vcpu-set" but they are apparently wrong.
> is there any different ideas xl and xend about xenstore syntax?
xl should use the same syntax in xenstore as xend currently does. If
xm (and therefore xend) and qemu currently work properly then changing
qemu will break xend. Instead, libxl should be changed to use the
same scheme as xend.
> (Each time ''xm vcpu-set'' executed, xend will write all
xenstore cpu
> node status)
Right.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian, I have discussed with Stefano about current xl/xend situation: 1). current issue seems casued by xl side; 2). we don''t need apply Stefano''s patch, at least now; Stefano, you will check xl side, right? Thanks, Jinsong Ian Jackson wrote:> Liu, Jinsong writes ("RE: [Xen-devel] [PATCH] qemu-xen: fix cpu > hotplug"): >> Stabellini, I read your attached patch, it''s OK. > > So we should apply Stefano''s patch ? I''m not sure I''m convinced. > >> In fact, we firstly implemented xenstore watch by same scheme of >> your patch, watching each cpu node status: >> /local/domain/xx/cpu/yy/availability=offline (online) >> >> However, we finally didn''t use this scheme. We watch ''common'' node >> instead: /local/domain/xx/cpu in this way, only 1 watch point need. >> Considering vcpu number may become more and more in the future (say, >> more than 128), it''s more simple and reasonable. (Watches can be >> set at points in the hierarchy and an individual watch will be >> triggered when anything at or below that point in the hierachy >> changes) > > Here you seem to be saying that the scheme that you implemented (in > xend, I take it) is not the same as the one in Stefano''s patch. > >> Yes, I think there must be some confusion. >> Currently ''xm vcpu-set'' command works fine with both PV and HVM vcpu >> hotplug. > > Right, good. > >> Stabellini/Jackson, would your please tell me what xl recently >> happened for vcpu hotplug? > > Well, we accepted patches to try to implement "xl vcpu-set" to make it > work like "xm vcpu-set" but they are apparently wrong. > >> is there any different ideas xl and xend about xenstore syntax? > > xl should use the same syntax in xenstore as xend currently does. If > xm (and therefore xend) and qemu currently work properly then changing > qemu will break xend. Instead, libxl should be changed to use the > same scheme as xend. > >> (Each time ''xm vcpu-set'' executed, xend will write all xenstore cpu >> node status) > > Right. > > Thanks, > Ian._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Sep-06 11:00 UTC
RE: [Xen-devel] [PATCH] qemu-xen: fix cpu hotplug
On Fri, 3 Sep 2010, Liu, Jinsong wrote:> Ian, > > I have discussed with Stefano about current xl/xend situation: > 1). current issue seems casued by xl side; > 2). we don''t need apply Stefano''s patch, at least now; > > Stefano, you will check xl side, right? >Yes. However I tried to reproduce the issue again but it seems to be gone now, maybe it was an error with my setup. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel