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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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.

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.

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;
     }
-- 
1.7.3.4

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

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