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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, May 08, 2013 at 04:04:53PM +0100, Ian Campbell wrote:
> 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@oracle.com> 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@oracle.com>
> > > > 
> > > > 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.

Well, overcommit comes in mind. Say you migrate to a 4PCPU box and you
have 12VCPUs, then you decide to go down to 4, then back to 16 before
migrating it to some other box. Can't do.

> 
> > Here is a bit of expanded git commit
> > 
> > 
> > From 42eef8cc5624bab907b6064fb183a5ae21b99df0 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 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.

See above.
> 
> > 
> > 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@oracle.com>
> > ---
> >  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;
> >      }
> 
>