Xen devel - Apr 2010 - [Question] vcpu-set before or after xen_pause_requested

Keir,

I have a question.
Recently we want to support ''xm vcpu-set'' command for HVM vcpu
hotplug.
At xenstore.c --> xenstore_parse_domain_config(), we add xs_watch for
xendstore cpu node;
At xenstore.c --> xenstore_process_event(), we add
xenstore_process_vcpu_set_event() to handle vcpu-set event.

Now the question is,
At xenstore.c --> xenstore_process_event(), where is right place to add
xenstore_process_vcpu_set_event():
1. add xenstore_process_vcpu_set_event() before xen_pause_requested check? or
2. add xenstore_process_vcpu_set_event() after xen_pause_requested check?
That means, may vcpu-set take effect between xm save/resume command? seems now
pci hotplug and usb hotplug do so.

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

This might be a question for Ian Jackson, since it is qemu related?

 -- Keir

On 15/04/2010 09:58, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> Keir,
> 
> I have a question.
> Recently we want to support ''xm vcpu-set'' command for HVM
vcpu hotplug.
> At xenstore.c --> xenstore_parse_domain_config(), we add xs_watch for
> xendstore cpu node;
> At xenstore.c --> xenstore_process_event(), we add
> xenstore_process_vcpu_set_event() to handle vcpu-set event.
> 
> Now the question is,
> At xenstore.c --> xenstore_process_event(), where is right place to add
> xenstore_process_vcpu_set_event():
> 1. add xenstore_process_vcpu_set_event() before xen_pause_requested check?
or
> 2. add xenstore_process_vcpu_set_event() after xen_pause_requested check?
> That means, may vcpu-set take effect between xm save/resume command? seems
now
> pci hotplug and usb hotplug do so.
> 
> Thanks,
> Jinsong


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

Liu, Jinsong writes ("[Xen-devel] [Question] vcpu-set before or after
xen_pause_requested"):
> At xenstore.c --> xenstore_process_event(), where is right place to add
xenstore_process_vcpu_set_event():
> 1. add xenstore_process_vcpu_set_event() before xen_pause_requested check?
or
> 2. add xenstore_process_vcpu_set_event() after xen_pause_requested check?
> That means, may vcpu-set take effect between xm save/resume command? seems
now pci hotplug and usb hotplug do so.
Is your vcpu-set done by writing to the xenstore key
   /local/domain/0/device-model/<domain>/command
?  If so it should be added as a new branch to the ifs in to
xenstore_process_dm_command_event, and you should make sure that when
your command is processed you correctly write a "processed" value to
that same xenstore key, just as usb-add does.  Because only one
command can be processed at once there is no possibility that a domain
which is supposed to be pausing could simultaneously be told to do a
hotplug (or vcpu-set).

Please also add corresponding code to xl and libxl to do the vcpu-set
operation.  We''re hoping make xl and libxl the primary mechanism in
4.1.

If your vcpu-set is not done by writing to /command, how does it work
and what is the protocol ?  Or haven''t you designed the protocol yet?
If you haven''t I''d suggest using the existing /command
mechanism.

The logic surrounding pci-{rem,ins} seems to be wrong to me because it
does not write a "processed" value.  So don''t use that as an
example.

Ian.

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

Ian Jackson wrote:
> 
> Is your vcpu-set done by writing to the xenstore key
>    /local/domain/0/device-model/<domain>/command
No. Currently it has ''xm vcpu-set'' command for PV domain, but
not available for HVM domain.
We keep same (HVM) xm command --> xend server --> xenstore path as PV
domain. At /local/domain/(domid)/cpu, we setup vcpu watch and handle at qemu
side.
> 
> Please also add corresponding code to xl and libxl to do the vcpu-set
> operation.  We''re hoping make xl and libxl the primary mechanism
in
> 4.1.
OK, we will add xl and libxl at next patch. Attached are our patches for
''xm vcpu-set'' command for HVM domain.


Ian,

We implement patches to enable ''xm vcpu-set'' command for HVM
domain, as attached.
Our question is, at qemu-xm-vcpu-set-2.patch, is it safe to do
''xenstore_process_vcpu_set_event()'' before
''xen_pause_requested'' check?
Or, is it safe for ''xm vcpu-set'' command runing between
''xm save'' and ''xm resume''?

Thanks,
Jinsong


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

Ian Jackson wrote:
> 
> Is your vcpu-set done by writing to the xenstore key
>    /local/domain/0/device-model/<domain>/command
No. Currently it has ''xm vcpu-set'' command for PV domain, but
not available for HVM domain.
We keep same (HVM) xm command --> xend server --> xenstore path as PV
domain. At /local/domain/(domid)/cpu, we setup vcpu watch and handle at qemu
side.
> 
> Please also add corresponding code to xl and libxl to do the vcpu-set
> operation.  We''re hoping make xl and libxl the primary mechanism
in
> 4.1.
OK, we will add xl and libxl at next patch. Attached are our patches for
''xm vcpu-set'' command for HVM domain.


Ian,

We implement patches to enable ''xm vcpu-set'' command for HVM
domain, as attached.
Our question is, at qemu-xm-vcpu-set-2.patch, is it safe to do
''xenstore_process_vcpu_set_event()'' before
''xen_pause_requested'' check?
Or, is it safe for ''xm vcpu-set'' command runing between
''xm save'' and ''xm resume''?

Thanks,
Jinsong


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

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested"):
...
> We keep same (HVM) xm command --> xend server --> xenstore path as
> PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and
> handle at qemu side.
I''ve looked at your patch 2 and it''s not correct because there
is no
acknowledgement back to the utility which changes xenstore.  You have
to close the loop, if for no other reason than if there are two
xenstore changes in a row which the receiving qemu-dm only gets around
to dealing with after the second, it will see only the second value.

How does the PV vcpu protocol deal with this ?  Doesn''t a PV guest
find out about VCPU changes from Xen ?

Ian.

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

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): ...
>> We keep same (HVM) xm command --> xend server --> xenstore path
as
>> PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and
>> handle at qemu side.
> 
> I''ve looked at your patch 2 and it''s not correct because
there is no
> acknowledgement back to the utility which changes xenstore.  You have
> to close the loop, if for no other reason than if there are two
> xenstore changes in a row which the receiving qemu-dm only gets around
> to dealing with after the second, it will see only the second value.
I think no problem here. In our test of the patch, 16 xenstore changes in row
(vcpu0~15), and it trigger 16 event.
> 
> How does the PV vcpu protocol deal with this ?  Doesn''t a PV guest
> find out about VCPU changes from Xen ?
''xm vcpu-set'' command works for PV now. Nixon and Campbell
implement vcpu-set PV dirver at drivers/xen/cpu_hotplug.c. CC them :)
> 
Ian, 

I noticed that qemu watch xenstore nodes and handle event in a close-loop way, 
like, usb-add/usb-del watch
''/local/domain/0/device-model/domid/command'' node and response
xenstore with ''usb-added'' / ''usb-deleted''.
It''s one way to communicate between xenstore and qemu.

However, is it the only way to communicate between qemu/xenstore, or between
PV/xenstore?
I check ''xm vcpu-set'' command path, it now works for PV domain
in an open-loop way:
1). PV register a xenbus_watch
        static struct xenbus_watch cpu_watch = {
                .node = "cpu",
                .callback = handle_vcpu_hotplug_event};

        (void)register_xenbus_watch(&cpu_watch);
2). when xend write xenstore cpu node or its sub-level nodes, it trigger
callback function handle_vcpu_hotplug_event(), then xenbus_scanf() /
xenbus_read() ...

Before we implement our current patch, in fact we have 2 choices:
    A). keep same HVM ''xm command --> xend --> xenstore''
path with PV, qemu watch ''/local/domain/domid/cpu'', trigger
callback function in open-loop way (also similar as PV). The advantage is we
have unify ''xm command --> xend --> xenstore'' path for
both PV and HVM.
    B). qemu work in close-loop way, like ''usb-add'' and
''usb-del'' command. The disadvantage is, we need add an hvm
path at ''xm command --> xend --> xenstore'', so there are
2 path for ''xm vcpu-set'' command, 1 for PV and 1 for HVM.
It''s not beautiful. BTW, since vcpu-set need watch different xenstore
node with ''usb-add''''usb-del'', we cannot
re-use ''usb'' path or function like signalDeviceModel(). It
need add more code to ''xm command --> xend -->
xendstore''.

Current our patch use scheme A). However, I think scheme B) also can work fine.
It depend on you and let me know your decision :)


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

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested"):
> I think no problem here. In our test of the patch, 16 xenstore
> changes in row (vcpu0~15), and it trigger 16 event.
Firstly, testing is no way to eliminate the possibility of races.
That must be done by analysis.

Secondly, yes, you will in the current implementation get 16 watch
triggers for 16 changes (although that''s not guaranteed to remain the
case).  But if you don''t do xs_read in time for one of them you will
miss one of the 16 different values.
> I noticed that qemu watch xenstore nodes and handle event in a
> close-loop way, like, usb-add/usb-del watch
> ''/local/domain/0/device-model/domid/command'' node and
response
> xenstore with ''usb-added'' /
''usb-deleted''.  It''s one way to
> communicate between xenstore and qemu.
Yes, that is how it should be done.
> However, is it the only way to communicate between qemu/xenstore, or
between PV/xenstore?
> I check ''xm vcpu-set'' command path, it now works for PV
domain in an open-loop way:
> 1). PV register a xenbus_watch
>         static struct xenbus_watch cpu_watch = {
>                 .node = "cpu",
>                 .callback = handle_vcpu_hotplug_event};
> 
>         (void)register_xenbus_watch(&cpu_watch);
> 2). when xend write xenstore cpu node or its sub-level nodes, it trigger
callback function handle_vcpu_hotplug_event(), then xenbus_scanf() /
xenbus_read() ...
This is broken.  If for any reason multiple vcpu-set actions happen in
quick succession, before the PV guest is scheduled, the
xenbus_scanf/read will see only the last one.

The protocol should be fixed before we implement any more of it.

Ian.

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

If so, things become some complicated.
I list sub-tasks to implement ''xm vcpu-set'':
1. setup a reliable communication protocol between xenstore and HVM qemu/ PV
driver, similar with what usb-add/usb-del use;
2. update old ''xm command --> xend --> xenstore'' path
according to new protocal, and keep an unify ''xm command --> xend
--> xnestore'' path for both HVM and PV;
3. update old PV driver to close-loop way;
4. update our HVM patch to close-loop way;

Is it OK, or something else ignored?

Thanks,
Jinsong


Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> I think no problem here. In our test of the patch, 16 xenstore
>> changes in row (vcpu0~15), and it trigger 16 event.
> 
> Firstly, testing is no way to eliminate the possibility of races.
> That must be done by analysis.
> 
> Secondly, yes, you will in the current implementation get 16 watch
> triggers for 16 changes (although that''s not guaranteed to remain
the
> case).  But if you don''t do xs_read in time for one of them you
will
> miss one of the 16 different values.
> 
>> I noticed that qemu watch xenstore nodes and handle event in a
>> close-loop way, like, usb-add/usb-del watch
>> ''/local/domain/0/device-model/domid/command'' node and
response
>> xenstore with ''usb-added'' /
''usb-deleted''.  It''s one way to
>> communicate between xenstore and qemu.
> 
> Yes, that is how it should be done.
> 
>> However, is it the only way to communicate between qemu/xenstore, or
>> between PV/xenstore? 
>> I check ''xm vcpu-set'' command path, it now works for
PV domain in an
>> open-loop way: 1). PV register a xenbus_watch
>>         static struct xenbus_watch cpu_watch = {
>>                 .node = "cpu",
>>                 .callback = handle_vcpu_hotplug_event};
>> 
>>         (void)register_xenbus_watch(&cpu_watch);
>> 2). when xend write xenstore cpu node or its sub-level nodes, it
>> trigger callback function handle_vcpu_hotplug_event(), then
>> xenbus_scanf() / xenbus_read() ...  
> 
> This is broken.  If for any reason multiple vcpu-set actions happen in
> quick succession, before the PV guest is scheduled, the
> xenbus_scanf/read will see only the last one.
> 
> The protocol should be fixed before we implement any more of it.
> 
> Ian.

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

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested"):
> Is it OK, or something else ignored?
That list seems about right.  Sorry about that.  (It would have been
better to get it right the first time!)

One question: what about old PV guests ?  Do you care about them or
can we just decree that they won''t support the feature ?

Ian.

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

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> Is it OK, or something else ignored?
> 
> That list seems about right.  Sorry about that.  (It would have been
> better to get it right the first time!)
> 
> One question: what about old PV guests ?  Do you care about them or
> can we just decree that they won''t support the feature ?
> 
Yes, that''s a problem.

How about this solution:
For trigger PV dirver, it''s easy to keep same event/node with old,
though some change to ''xm command --> xend --> xenstore''
path.
For response state, maybe we can error/warning log to xend.log when timeout,
like ''Timed out waiting for vcpu-set action, please update your PV
driver to latest version''?.
In this way, old PV guest can still work in old way (no bad than before) with
warning message, suggesting user to update their PV driver.

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

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested"):
> How about this solution:
...
> In this way, old PV guest can still work in old way (no bad than
> before) with warning message, suggesting user to update their PV
> driver.
But, in new guests which are malfunctioning, things will not be
handled properly: the timeout will trigger and it will seem to have
worked.  (Consider a guest which is paused or is starved of CPU
because the host is very busy.)  There is also no good value for the
timeout.

I would suggest using a different xenstore key name for the new
protocol.  That way you don''t get the feature with old drivers but at
least you don''t make malfunctions worse with the new ones.

Ian.

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

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> How about this solution:
> ...
>> In this way, old PV guest can still work in old way (no bad than
>> before) with warning message, suggesting user to update their PV
>> driver.
> 
> But, in new guests which are malfunctioning, things will not be
> handled properly: the timeout will trigger and it will seem to have
> worked.  (Consider a guest which is paused or is starved of CPU
> because the host is very busy.)  There is also no good value for the
> timeout.
> 
> I would suggest using a different xenstore key name for the new
> protocol.  That way you don''t get the feature with old drivers but
at
> least you don''t make malfunctions worse with the new ones.
> 
Ian,

I''m not quite clear your idea. Per my understanding, a new xenstore key
name cannot avoid the issue of old key, like timeout value, malfunction guest,
...
Currently I have no enough resource to solve all problems, so how about you
solve old issue that PV driver has, and then I will update my HVM vcpu-set patch
based on the new protocol?

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

Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested"):
> I''m not quite clear your idea. Per my understanding, a new
xenstore
> key name cannot avoid the issue of old key, like timeout value,
> malfunction guest, ...
How does the current arrangement deal with an attempt to "xm vcpu-set"
a PV guest which doesn''t support that operation ?

Ian.

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

Ian Jackson wrote:
> Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
> after xen_pause_requested"): 
>> I''m not quite clear your idea. Per my understanding, a new
xenstore
>> key name cannot avoid the issue of old key, like timeout value,
>> malfunction guest, ...
> 
> How does the current arrangement deal with an attempt to "xm
vcpu-set"
> a PV guest which doesn''t support that operation ?
> 
Currently if PV guest doesn''t support vcpu-hotplug, it then has no
xenbus_watch to related xenstore nodes.
then if attempt ''xm vcpu-set'', 
1). xend will still write to xenstore nodes with value
"online/offline"
/local/domain/domid/cpu/0/availability
/local/domain/domid/cpu/1/availability
......
/local/domain/domid/cpu/maxvcpus-1/availability
2). PV guest has not any info about xenstore change, and will not be influenced
by ''xm vcpu-set'' command;

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

I''m curios do we really care about we lost the intermediate value. 
For example, considering user do vcpu-set 2 (i.e set vcpu number to 2), vcpu-set
4, and vcpu-set 2, and guest didn''t take any action at all, would it be
really so important? Or, if it is 2->4->3, should the xm vcpu-set itself
really need care 4 vcpu is bringup in some time?

Especially consider guest (i.e. OS) will take several steps to really utilize
the new added vcpu. For example, when CPU hot-added, Linux kernel will send a
uevent to user space, and ueventd will try to bring up the CPU after get the
event. In a system without properly setup the uevent script, the xm vcpu-set
command will not cause changes to both guest kernel and xen hypervisor at all.
In old kernel, even hot-remove will go-through uevent also. Also, the uevent
script may combine several uevent still, depends on how the script is
implemented.

I think the close loop should happen in caller to the vcpu-set, if the caller
really cares about the result of the command, it should check xen''s
vcpu status to check if the command has take effect or not.

Any idea?

Thanks
Yunhong Jiang
>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Ian Jackson
>Sent: Monday, April 19, 2010 11:48 PM
>To: Liu, Jinsong
>Cc: xen-devel; Keir Fraser; Zhai, Edwin; Jiang, Yunhong
>Subject: RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested
>
>Liu, Jinsong writes ("RE: [Xen-devel] [Question] vcpu-set before or
after
>xen_pause_requested"):
>...
>> We keep same (HVM) xm command --> xend server --> xenstore path
as
>> PV domain. At /local/domain/(domid)/cpu, we setup vcpu watch and
>> handle at qemu side.
>
>I''ve looked at your patch 2 and it''s not correct because
there is no
>acknowledgement back to the utility which changes xenstore.  You have
>to close the loop, if for no other reason than if there are two
>xenstore changes in a row which the receiving qemu-dm only gets around
>to dealing with after the second, it will see only the second value.
>
>How does the PV vcpu protocol deal with this ?  Doesn''t a PV guest
>find out about VCPU changes from Xen ?
>
>Ian.
>
>_______________________________________________
>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

Jiang, Yunhong writes ("RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested"):
> For example, considering user do vcpu-set 2 (i.e set vcpu number to
> 2), vcpu-set 4, and vcpu-set 2, and guest didn''t take any action
at
> all, would it be really so important? Or, if it is 2->4->3, should
> the xm vcpu-set itself really need care 4 vcpu is bringup in some
> time?
This is a good point.  If you are sure that intermediate values are
not important, and that changes are idempotent, then the open loop
approach is quite fine.

Ian.

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

Ian, thanks for your reply. Yes, I think the intermediate values is not
important in general. If it is really important, the caller of "xm 
vcpu-set" should take care of it, i.e. do xm vcpu-set, wait and check the
vcpu''s status (maybe from xen hypervisor) with timeout mechanism.

BTW, from the followed changeset in dom0''s drivers/xen/cpu_hotplug.c ,
it means the xm vcpu-set may happen even before guest is fully up, at that time,
it is difficult for xend to achieve the close loop.

commit d745562cc40bff60f0597004d8128fa0225cc267
Author: Ian Campbell <Ian.Campbell@citrix.com>
Date:   Thu Apr 2 13:24:28 2009 +0100

    xen: honour VCPU availability on boot

    If a VM is booted with offline VCPUs then unplug them during boot.
Determining
    the availability of a VCPU requires access to XenStore which is not
available
    at the point smp_prepare_cpus() is called, therefore we bring up all VCPUS
    initially and unplug the offline ones as soon as XenStore becomes available.

    Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Thanks
--jyh
>-----Original Message-----
>From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
>Sent: Wednesday, April 28, 2010 6:31 PM
>To: Jiang, Yunhong
>Cc: Liu, Jinsong; xen-devel; Keir Fraser; Zhai, Edwin
>Subject: RE: [Xen-devel] [Question] vcpu-set before or after
xen_pause_requested
>
>Jiang, Yunhong writes ("RE: [Xen-devel] [Question] vcpu-set before or
after
>xen_pause_requested"):
>> For example, considering user do vcpu-set 2 (i.e set vcpu number to
>> 2), vcpu-set 4, and vcpu-set 2, and guest didn''t take any
action at
>> all, would it be really so important? Or, if it is 2->4->3,
should
>> the xm vcpu-set itself really need care 4 vcpu is bringup in some
>> time?
>
>This is a good point.  If you are sure that intermediate values are
>not important, and that changes are idempotent, then the open loop
>approach is quite fine.
>
>Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel