Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ryan Harper]

* Xen patchbot -unstable <patchbot-unstable@lists.xensource.com> [2005-10-03 13:16]:
> # HG changeset patch
> # User kaf24@firebug.cl.cam.ac.uk
> # Node ID 85f92475b9437fcd10bf1ae105f53b0abe963050
> # Parent  dd87869f877ca9c68c97f36b3870908fb279edb9
> Create new vcpu_op() hypercall. Replaces old boot_vcpu()
> hypercall and vcpu-related schedop commands.
> 
> Signed-off-by: Keir Fraser <keir@xensource.com>

Both do_boot_vcpu() and now VCPU_CREATE rely on domU kernel playing nice
and not making the hypercall more than has been indicated in the shared
table when we built the domain (nr_vcpus).  Wouldn't it be better to
have the domain creation hypercall specify the number of vcpus for a
domain (as well as a cpumap to indicate which physical cpus are to be
used) and alloc vcpu structures at that point leaving the vcpu_ops() to
get context and unpause the vcpu?

If I put together a patch that mode the above change, would that be
considered?  

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Keir Fraser]

On 3 Oct 2005, at 19:42, Ryan Harper wrote:

> Both do_boot_vcpu() and now VCPU_CREATE rely on domU kernel playing 
> nice
> and not making the hypercall more than has been indicated in the shared
> table when we built the domain (nr_vcpus).  Wouldn't it be better to
> have the domain creation hypercall specify the number of vcpus for a
> domain (as well as a cpumap to indicate which physical cpus are to be
> used) and alloc vcpu structures at that point leaving the vcpu_ops() to
> get context and unpause the vcpu?
>
> If I put together a patch that mode the above change, would that be
> considered?

I think there's not much in it -- allocating on demand vs. allocate at 
domain build time. I think the simplest patch would be for Xen to be 
passed a max_vcpuid or max_vcpus and apply the appropriate simple check 
at VCPUOP_create.

I'd take a patch that adds a new dom0 op to set that value 
(DOM0_SET_MAX_VCPUS or somesuch), applies the appropriate check inside 
Xen. Also that plumbs that hypercall into the Python wrapper, and adds 
a call to that command in xend (both during initial build and on 
save/restore).

  -- Keir

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ryan Harper]

* Keir Fraser <Keir.Fraser@cl.cam.ac.uk> [2005-10-03 15:56]:
> 
> On 3 Oct 2005, at 19:42, Ryan Harper wrote:
> 
> >Both do_boot_vcpu() and now VCPU_CREATE rely on domU kernel playing 
> >nice
> >and not making the hypercall more than has been indicated in the shared
> >table when we built the domain (nr_vcpus).  Wouldn't it be better to
> >have the domain creation hypercall specify the number of vcpus for a
> >domain (as well as a cpumap to indicate which physical cpus are to be
> >used) and alloc vcpu structures at that point leaving the vcpu_ops() to
> >get context and unpause the vcpu?
> >
> >If I put together a patch that mode the above change, would that be
> >considered?
> 
> I think there's not much in it -- allocating on demand vs. allocate at 
> domain build time. I think the simplest patch would be for Xen to be 
> passed a max_vcpuid or max_vcpus and apply the appropriate simple check 
> at VCPUOP_create.
> 
> I'd take a patch that adds a new dom0 op to set that value 
> (DOM0_SET_MAX_VCPUS or somesuch), applies the appropriate check inside 
> Xen. Also that plumbs that hypercall into the Python wrapper, and adds 
> a call to that command in xend (both during initial build and on 
> save/restore).

I was also thinking toward NUMA systems where I'd like to know which
physical cpus are being utilized for a given domain so when we allocate
memory for a domain we can try to localize those allocations to the cpus
that will be using it.  Currently domain memory allocation happens
before secondary processors are selected.

I'd rather see domain creation take both max_vcpus and cpu placement
bitmaps instead, but if that is not acceptable for 3.0, I'll go ahead
and work up the patch as described since it needs to be resolved for
3.0.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Keir Fraser]

On 3 Oct 2005, at 22:07, Ryan Harper wrote:

> I was also thinking toward NUMA systems where I'd like to know which
> physical cpus are being utilized for a given domain so when we allocate
> memory for a domain we can try to localize those allocations to the 
> cpus
> that will be using it.  Currently domain memory allocation happens
> before secondary processors are selected.
>
> I'd rather see domain creation take both max_vcpus and cpu placement
> bitmaps instead, but if that is not acceptable for 3.0, I'll go ahead
> and work up the patch as described since it needs to be resolved for
> 3.0.

I'd rather keep single-purpose dom0 ops than have createdomain become a 
'union of everything'.

There is already a dom0 op for pinning a vcpu. Right now it can't be 
used before the domain starts for other than vcpu0 since it requires 
VCPUOP_create to allocate the vcpu structure, but fixing that is 
unrelated to whether pin info is passed to DOM0_CREATEDOMAIN. As you 
say, it's also not critical to fix for 3.0.

  -- Keir

Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ryan Harper]

* Ryan Harper <ryanh@us.ibm.com> [2005-10-03 13:42]:
> * Xen patchbot -unstable <patchbot-unstable@lists.xensource.com> [2005-10-03 13:16]:
> > # HG changeset patch
> > # User kaf24@firebug.cl.cam.ac.uk
> > # Node ID 85f92475b9437fcd10bf1ae105f53b0abe963050
> > # Parent  dd87869f877ca9c68c97f36b3870908fb279edb9
> > Create new vcpu_op() hypercall. Replaces old boot_vcpu()
> > hypercall and vcpu-related schedop commands.
> > 
> > Signed-off-by: Keir Fraser <keir@xensource.com>

btw, this patch also regressed vcpu-{enable/disable} for dom0 and domU.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ryan Harper]

* Ryan Harper <ryanh@us.ibm.com> [2005-10-04 10:40]:
> * Ryan Harper <ryanh@us.ibm.com> [2005-10-03 13:42]:
> > * Xen patchbot -unstable <patchbot-unstable@lists.xensource.com> [2005-10-03 13:16]:
> > > # HG changeset patch
> > > # User kaf24@firebug.cl.cam.ac.uk
> > > # Node ID 85f92475b9437fcd10bf1ae105f53b0abe963050
> > > # Parent  dd87869f877ca9c68c97f36b3870908fb279edb9
> > > Create new vcpu_op() hypercall. Replaces old boot_vcpu()
> > > hypercall and vcpu-related schedop commands.
> > > 
> > > Signed-off-by: Keir Fraser <keir@xensource.com>
> 
> btw, this patch also regressed vcpu-{enable/disable} for dom0 and domU.

Actually this was just the first changeset where I noticed.  The break
is further back.  I'll reply in a bit with the changeset where this was
broken.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Keir Fraser]

On 4 Oct 2005, at 17:05, Ryan Harper wrote:

>> btw, this patch also regressed vcpu-{enable/disable} for dom0 and 
>> domU.
>
> Actually this was just the first changeset where I noticed.  The break
> is further back.  I'll reply in a bit with the changeset where this was
> broken.

I see problems on smp save/restore -- secondary cpus end up looping 
forever (I think waiting for their state to be CPU_UP_PREPARE in 
play_dead()). I'm sure there's a similar problem in vcpu-up/down logic. 
I don't think it can be very hard to fix. :-)

  -- Keir

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ryan Harper]

* Keir Fraser <Keir.Fraser@cl.cam.ac.uk> [2005-10-04 11:31]:
> 
> On 4 Oct 2005, at 17:05, Ryan Harper wrote:
> 
> >>btw, this patch also regressed vcpu-{enable/disable} for dom0 and 
> >>domU.
> >
> >Actually this was just the first changeset where I noticed.  The break
> >is further back.  I'll reply in a bit with the changeset where this was
> >broken.
> 
> I see problems on smp save/restore -- secondary cpus end up looping 
> forever (I think waiting for their state to be CPU_UP_PREPARE in 
> play_dead()). I'm sure there's a similar problem in vcpu-up/down logic. 
> I don't think it can be very hard to fix. :-)

Actually, hotplug works fine via sysfs:
(echo 0 > /sys/devices/system/cpu/cpu1/online), but the store watches for
hotplug don't seem to get triggered.  I can confirm the writes to the
store (with xs_tdb_dump), and with some debugging in the kernel I can
see that the watches never fire.  

Last changeset where vcpu-disable works is:

changeset:   7141:a39510ad5c591ee84592924e718c90d746f90097
user:        emellor@ewan
date:        Fri Sep 30 05:55:49 2005
summary:     Added cache-control headers to pages returned by HTTP server so that pages

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ewan Mellor]

On Tue, Oct 04, 2005 at 03:39:20PM -0500, Ryan Harper wrote:

> * Keir Fraser <Keir.Fraser@cl.cam.ac.uk> [2005-10-04 11:31]:
> > 
> > On 4 Oct 2005, at 17:05, Ryan Harper wrote:
> > 
> > >>btw, this patch also regressed vcpu-{enable/disable} for dom0 and 
> > >>domU.
> > >
> > >Actually this was just the first changeset where I noticed.  The break
> > >is further back.  I'll reply in a bit with the changeset where this was
> > >broken.
> > 
> > I see problems on smp save/restore -- secondary cpus end up looping 
> > forever (I think waiting for their state to be CPU_UP_PREPARE in 
> > play_dead()). I'm sure there's a similar problem in vcpu-up/down logic. 
> > I don't think it can be very hard to fix. :-)
> 
> Actually, hotplug works fine via sysfs:
> (echo 0 > /sys/devices/system/cpu/cpu1/online), but the store watches for
> hotplug don't seem to get triggered.  I can confirm the writes to the
> store (with xs_tdb_dump), and with some debugging in the kernel I can
> see that the watches never fire.  
> 
> Last changeset where vcpu-disable works is:
> 
> changeset:   7141:a39510ad5c591ee84592924e718c90d746f90097
> user:        emellor@ewan
> date:        Fri Sep 30 05:55:49 2005
> summary:     Added cache-control headers to pages returned by HTTP server so that pages

Or to put it another way, the changeset that broke it is 7142:Within the store, split the persistent information regarding a VM from the transient information
regarding a domain.

This suggests that the recent changes to the paths have broken something;
presumably whatever is watching is watching for the wrong thing now.  Do you
know which path is being changed, and which path was being changed in the
working changeset?

Ewan.

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ryan Harper]

* Ewan Mellor <ewan@xensource.com> [2005-10-04 16:11]:
> On Tue, Oct 04, 2005 at 03:39:20PM -0500, Ryan Harper wrote:
> 
> > * Keir Fraser <Keir.Fraser@cl.cam.ac.uk> [2005-10-04 11:31]:
> > > 
> > > On 4 Oct 2005, at 17:05, Ryan Harper wrote:
> > > 
> > > >>btw, this patch also regressed vcpu-{enable/disable} for dom0 and 
> > > >>domU.
> > > >
> > > >Actually this was just the first changeset where I noticed.  The break
> > > >is further back.  I'll reply in a bit with the changeset where this was
> > > >broken.
> > > 
> > > I see problems on smp save/restore -- secondary cpus end up looping 
> > > forever (I think waiting for their state to be CPU_UP_PREPARE in 
> > > play_dead()). I'm sure there's a similar problem in vcpu-up/down logic. 
> > > I don't think it can be very hard to fix. :-)
> > 
> > Actually, hotplug works fine via sysfs:
> > (echo 0 > /sys/devices/system/cpu/cpu1/online), but the store watches for
> > hotplug don't seem to get triggered.  I can confirm the writes to the
> > store (with xs_tdb_dump), and with some debugging in the kernel I can
> > see that the watches never fire.  
> > 
> > Last changeset where vcpu-disable works is:
> > 
> > changeset:   7141:a39510ad5c591ee84592924e718c90d746f90097
> > user:        emellor@ewan
> > date:        Fri Sep 30 05:55:49 2005
> > summary:     Added cache-control headers to pages returned by HTTP server so that pages
> 
> Or to put it another way, the changeset that broke it is 7142:Within the store, split the persistent information regarding a VM from the transient information
> regarding a domain.
> 
> This suggests that the recent changes to the paths have broken something;
> presumably whatever is watching is watching for the wrong thing now.  Do you
> know which path is being changed, and which path was being changed in the
> working changeset?

The vcpu hotplug watch is triggered on any writes to /cpu.
It will then examine the node value to see if the change was made to
/cpu/cpu#/availability, (e.g. /cpu/1/availability), if the full path of
the change is correct, it examines the change values which are either
'online' or 'offline' and invokes the vcpu_up/down handlers if the write
is a state change.

I only ever see one trigger and the value is 'cpu' but I can confirm
that tdb sees the full write ('/cpu/1/availabilty', 'offline').

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
ryanh@us.ibm.com

Re: Re: [Xen-changelog] Create new vcpu_op() hypercall. Replaces old boot_vcpu()

[Ewan Mellor]

On Tue, Oct 04, 2005 at 04:17:44PM -0500, Ryan Harper wrote:

> The vcpu hotplug watch is triggered on any writes to /cpu.
> It will then examine the node value to see if the change was made to
> /cpu/cpu#/availability, (e.g. /cpu/1/availability), if the full path of
> the change is correct, it examines the change values which are either
> 'online' or 'offline' and invokes the vcpu_up/down handlers if the write
> is a state change.
> 
> I only ever see one trigger and the value is 'cpu' but I can confirm
> that tdb sees the full write ('/cpu/1/availabilty', 'offline').

Thanks Ryan, that's been very helpful.  This is now bug #280.

Cheers,

Ewan.