Re: [Xen-devel] [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited hosts.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-05-08 at 15:29 +0100, Konrad Rzeszutek Wilk wrote:
> On Wed, May 08, 2013 at 11:46:39AM +0100, Ian Campbell wrote:
> > On Wed, 2013-05-08 at 11:32 +0100, George Dunlap wrote:
> > > On Tue, May 7, 2013 at 9:40 PM, Konrad Rzeszutek Wilk
> > > <konrad.wilk@xxxxxxxxxx> wrote:
> > > > The libxl_cpu_bitmap_alloc(..) function, if provided with a zero
> > > > value for max CPUs will call xc_get_max_cpus() which will retrieve
> > > > the number of physical CPUs the host has. This is usually
> > > > OK if the guest's maxvcpus <= host pcpus. But if the value
> > > > is different, then the bitmap for VCPUs is limited by the
> > > > number of CPUs the host has.
> > > >
> > > > This is incorrect as what we want is to hotplug in the guest
> > > > the amount of CPUs that the user specified on the command line
> > > > and not be limited by the amount of physical CPUs.
> > > >
> > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > 
> > > Given the cost/benefits of this, I'm inclined to say this should wait
> > > until the 4.4 window opens.  Having more guest vcpus than host pcpus
> > > is not an urgent need, and there is a chance (however small) of this
> > > exposing some other kind of bug.
> > 
> > Not disputing this.
> > 
> > I imagine the desire is to add vcpus to a guest after migrating to a
> > larger host. If so then this should be in the commit log because you are
> > right that as the commit message currently stands the immediate response
> > is "why on earth..."
> 
> > 
> > My only other concern would be that the existing code has a buffer
> > overrun under these circumstances. I've not checked this either.
> 
> No over-runs, but this is a regression compared to Xend.

>From your updated description it seems like if you migrated to a larger
host you could indeed plug more VCPUS, up to the limit on that host, is
that right?

If that's the case then it sounds to me as if the xl behaviour is
actually an improvement over xend's, except perhaps for some slightly
niche test scenarios.

> Here is a bit of expanded git commit
> 
> 
> From 42eef8cc5624bab907b6064fb183a5ae21b99df0 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date: Tue, 7 May 2013 16:32:20 -0400
> Subject: [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited 
> hosts.
> 
> The libxl_cpu_bitmap_alloc(..) function, if provided with a zero
> value for max CPUs will call xc_get_max_cpus() which will retrieve
> the number of physical CPUs the host has. This is usually
> OK if the guest's maxvcpus <= host pcpus. But if the value
> is different, then the bitmap for VCPUs is limited by the
> number of CPUs the host has.
> 
> This is incorrect as what we want is to hotplug in the guest
> the amount of CPUs that the user specified on the command line
> and not be limited by the amount of physical CPUs.

IOW what I'm saying above is that this is statement is not axiomatically
true, I think what is missing is the why anyone would want to do this.

> 
> This means that a guest config like this:
> 
> vcpus=8
> maxvcpus=32
> 
> and on a 4 PCPU machine doing
> 
> xl vcpu-set <guest name> 16
> 
> won't work. This is b/c the the size of the bitmap is one byte
> so it can only hold up to 8 VCPUs. Hence anything above that
> is going to be ignored.
> 
> Note that all of the libxl_cpu_bitmap_[test|set] silently ignore
> any test or sets above its size:
> 
>      if (bit >= bitmap->size * 8)
>          return 0;
> 
> so we were never notified off this bug.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  tools/libxl/xl_cmdimpl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c1a969b..ef7f81b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4499,7 +4499,7 @@ static void vcpuset(uint32_t domid, const char* 
> nr_vcpus)
>          return;
>      }
>  
> -    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
> +    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
>          fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
>          return;
>      }



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel