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

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

[Konrad Rzeszutek Wilk]

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>
---
 tools/libxl/xl_cmdimpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.8.1.4

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

[George Dunlap]

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.

 -George

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

[Ian Campbell]

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.

Ian.

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

[Konrad Rzeszutek Wilk]

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.

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.

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

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

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

[Ian Campbell]

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.

> 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.

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

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited hosts."):
> 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.

Another reason someone might want to do this is testing, of course.
If you want to stress out your guest pv spinlocks, that's one way to
do it.

Ian.

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

[Konrad Rzeszutek Wilk]

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

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

[Ian Campbell]

On Wed, 2013-05-08 at 17:35 +0100, Konrad Rzeszutek Wilk wrote:
> 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.

You could do it *after* the migration back to a 16 way box n stead of
before though, which is most likely when you would actually want to do
it...

Ian.

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

[Konrad Rzeszutek Wilk]

On Wed, May 08, 2013 at 05:49:34PM +0100, Ian Campbell wrote:
> On Wed, 2013-05-08 at 17:35 +0100, Konrad Rzeszutek Wilk wrote:
> > 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.
> 
> You could do it *after* the migration back to a 16 way box n stead of
> before though, which is most likely when you would actually want to do
> it...

I am kind of lost. Are we arguing for this being a bug or whether there is
justification for putting in Xen 4.3?

If you want to delay it to Xen 4.4 that is OK with me - I can carry the
patch in my tree and post it then.

> 
> Ian.
> 

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

[Ian Campbell]

On Wed, 2013-05-08 at 18:05 +0100, Konrad Rzeszutek Wilk wrote:
> On Wed, May 08, 2013 at 05:49:34PM +0100, Ian Campbell wrote:
> > On Wed, 2013-05-08 at 17:35 +0100, Konrad Rzeszutek Wilk wrote:
> > > 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.
> > 
> > You could do it *after* the migration back to a 16 way box n stead of
> > before though, which is most likely when you would actually want to do
> > it...
> 
> I am kind of lost. Are we arguing for this being a bug or whether there is
> justification for putting in Xen 4.3?

The former needs deciding before the latter.

I'm not convinced that the current xl behaviour of refusing to
overcommit VCPUs on a host isn't the right one for the majority of use
cases. Obviously the silently refusing bit is a bug which should be
fixed.

I don't buy that this is a "regression compared to Xend". It's certainly
a difference from how xend behaved but it seems on the whole to be a
positive one (i.e. xend was wrong).

Can you explain the use case for wanting to do this? I don't think the
migration one you give above is very convincing since a normal user
wouldn't want to overcommit on the source host, they would want to
migrate and then increase the number of vcpus, without ever
overcommitting, and therefore without the terrible performance of
overcommitting.

A compromise might be a non-default option to allow users to force
overcommit but to otherwise deny it.

Ian.

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

[Konrad Rzeszutek Wilk]

> > > > 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.
> > > 
> > > You could do it *after* the migration back to a 16 way box n stead of
> > > before though, which is most likely when you would actually want to do
> > > it...
> > 
> > I am kind of lost. Are we arguing for this being a bug or whether there is
> > justification for putting in Xen 4.3?
> 
> The former needs deciding before the latter.
> 
> I'm not convinced that the current xl behaviour of refusing to
> overcommit VCPUs on a host isn't the right one for the majority of use
> cases. Obviously the silently refusing bit is a bug which should be
> fixed.
> 
> I don't buy that this is a "regression compared to Xend". It's certainly
> a difference from how xend behaved but it seems on the whole to be a
> positive one (i.e. xend was wrong).

CC-ing Juergen here as he added this in.
> 
> Can you explain the use case for wanting to do this? I don't think the
> migration one you give above is very convincing since a normal user
> wouldn't want to overcommit on the source host, they would want to
> migrate and then increase the number of vcpus, without ever
> overcommitting, and therefore without the terrible performance of
> overcommitting.

It seems clear to me if a user wants to over-commit, then we should
allow the user to do so. If we provide a command to set X vCPUs it
should work as described - without the extra checks (unless
that is enabled by some other option).

That is currently not the case and the documentation does not mention
why or why the choice to limit the amount was implemented  - and looking
back at the changeset that introduced this: c/s 22918 (CC Juergen here)
it looks as the original behavior was to do it as Xend does.

If we want a behavior where we don't allow to overcommit (which sounds
bogus - that is part allure of virtualization) then we should also
tweak other 'xl' commands. For example you can do a similar scenario in
which you launch say sixteen 1VCPU guests and you get the same performance
characteristics as if you launch one guest with 16VCPUs on a 4PCPU machine.
Or perhaps worst.

Or say you launch a guest with sixteen VCPUs without trouble right now, but
then if you decrease to one and then want to go back to sixteen it refuses
to do so (without telling you why).

But if tells you why (say by adding a warning that says "You don't want to
overcommit") then the user will ask two questions (I would :-)):
 a) Why didn't xl create complain then initially?
 b) Why can't I overcommit? I want to and this 'xl' is refusing me to do it!
 c) It worked with xend!

> 
> A compromise might be a non-default option to allow users to force
> overcommit but to otherwise deny it.

That can be done. But I would turn it around. Allow it by default and
provide another option (perhaps a default global in /etc/xen/xl.conf)
that will disable overcommiting as much as possible.

And also why don't we want overcommiting?
> 
> Ian.
> 

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

[Ian Campbell]

On Wed, 2013-05-08 at 23:39 +0100, Konrad Rzeszutek Wilk wrote:
> > > > > 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.
> > > > 
> > > > You could do it *after* the migration back to a 16 way box n stead of
> > > > before though, which is most likely when you would actually want to do
> > > > it...
> > > 
> > > I am kind of lost. Are we arguing for this being a bug or whether there is
> > > justification for putting in Xen 4.3?
> > 
> > The former needs deciding before the latter.
> > 
> > I'm not convinced that the current xl behaviour of refusing to
> > overcommit VCPUs on a host isn't the right one for the majority of use
> > cases. Obviously the silently refusing bit is a bug which should be
> > fixed.
> > 
> > I don't buy that this is a "regression compared to Xend". It's certainly
> > a difference from how xend behaved but it seems on the whole to be a
> > positive one (i.e. xend was wrong).
> 
> CC-ing Juergen here as he added this in.
> > 
> > Can you explain the use case for wanting to do this? I don't think the
> > migration one you give above is very convincing since a normal user
> > wouldn't want to overcommit on the source host, they would want to
> > migrate and then increase the number of vcpus, without ever
> > overcommitting, and therefore without the terrible performance of
> > overcommitting.
> 
> It seems clear to me if a user wants to over-commit, then we should
> allow the user to do so. If we provide a command to set X vCPUs it
> should work as described - without the extra checks (unless
> that is enabled by some other option).
> 
> That is currently not the case and the documentation does not mention
> why or why the choice to limit the amount was implemented  - and looking
> back at the changeset that introduced this: c/s 22918 (CC Juergen here)
> it looks as the original behavior was to do it as Xend does.

I'm afraid that doesn't make it correct.

> If we want a behavior where we don't allow to overcommit (which sounds
> bogus - that is part allure of virtualization)

Note that I'm not against overcommitting the PCPUS on the host in
general, just against the idea that giving a single VM more VCPUs than
PCPUs. Giving a single VM more VCPUs than the host has PCPUs has only
negative effects.

>  then we should also
> tweak other 'xl' commands. For example you can do a similar scenario in
> which you launch say sixteen 1VCPU guests and you get the same performance
> characteristics as if you launch one guest with 16VCPUs on a 4PCPU machine.
> Or perhaps worst.

The latter case is much worse and that's the only one I'm arguing we
should avoid.

In the 16VMsx1VCPUs sharing 4PCPUs case those 16 VCPUs can be idling etc
and so you can quite possibly pack them onto 4 PCPUs. Or if they are
busy then they get timesliced in a fair way etc. All good and proper and
yes this is part of the allure of virtualisation.

If you have 1VMx16VCPUs sharing 4CPUs then this is not the same
situation. That guest can only ever see 4VCPUs running simultaneously,
so at most it can only ever use 400% of a CPU, all the other 12VCPUs do
is divide that 400% more thinly, cause scheduling contention, locking
overhead (especially internally to the guest) and other negative
impacts. The VM is contending with itself for no benefit.

If you have a scenario where running a 16VCPU VM on a 4PCPU system is
beneficial to the guest (or perhaps the host) then please tell us what
it is, because that would indeed be a useful justification for allowing
this behaviour by default.

The only scenario which has been mentioned so far in this thread is
deliberately provoking the kinds of bad behaviours I've mentioned above,
for testing and development purposes (i.e. exposing guest locking
contention), which is the main reason I'm willing to consider an
override to allow this, but this isn't an argument for allowing it by
default.

> Or say you launch a guest with sixteen VCPUs without trouble right now, but
> then if you decrease to one and then want to go back to sixteen it refuses
> to do so (without telling you why).

I think it is a bug that xl lets you silently create a 16VCPU guest on a
4PCPU host without at least warning you or better requiring you to say
"yes I mean it".

> But if tells you why (say by adding a warning that says "You don't want to
> overcommit") then the user will ask two questions (I would :-)):
>  a) Why didn't xl create complain then initially?
>  b) Why can't I overcommit? I want to and this 'xl' is refusing me to do it!
>  c) It worked with xend!

Please stop making argument c). The purpose of xl's xm compatibility is
to offer an upgrade path, it is not to copy every possible misbehaviour
which xend had.

Yes, this is your mother's classic "if xend jumped off a bridge"
argument.

> > A compromise might be a non-default option to allow users to force
> > overcommit but to otherwise deny it.
> 
> That can be done. But I would turn it around. Allow it by default and
> provide another option (perhaps a default global in /etc/xen/xl.conf)
> that will disable overcommiting as much as possible.
> 
> And also why don't we want overcommiting?

Because the sort of overcommit we are talking about here has only
negative effects on the guest.

Ian.

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

[George Dunlap]

On 08/05/13 23:39, Konrad Rzeszutek Wilk wrote:
>>>>> 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.
>>>> You could do it *after* the migration back to a 16 way box n stead of
>>>> before though, which is most likely when you would actually want to do
>>>> it...
>>> I am kind of lost. Are we arguing for this being a bug or whether there is
>>> justification for putting in Xen 4.3?
>> The former needs deciding before the latter.
>>
>> I'm not convinced that the current xl behaviour of refusing to
>> overcommit VCPUs on a host isn't the right one for the majority of use
>> cases. Obviously the silently refusing bit is a bug which should be
>> fixed.
>>
>> I don't buy that this is a "regression compared to Xend". It's certainly
>> a difference from how xend behaved but it seems on the whole to be a
>> positive one (i.e. xend was wrong).
> CC-ing Juergen here as he added this in.
>> Can you explain the use case for wanting to do this? I don't think the
>> migration one you give above is very convincing since a normal user
>> wouldn't want to overcommit on the source host, they would want to
>> migrate and then increase the number of vcpus, without ever
>> overcommitting, and therefore without the terrible performance of
>> overcommitting.
> It seems clear to me if a user wants to over-commit, then we should
> allow the user to do so. If we provide a command to set X vCPUs it
> should work as described - without the extra checks (unless
> that is enabled by some other option).

I tend to agree with this; I hate it when some piece of software tells 
me, "I'm not going to let you do this for your own good." When that 
happens to me, it's almost always the case that I *do* have a good 
reason for doing so that the author of the software didn't think about.  
Our users are sysadmins; we should allow them to shoot themselves in the 
foot if they want to.

So I think the right thing to do long-term is to make it possible to do 
in xl.  Having a "seatbelt" restriction by default that can be 
overridden would be OK with me, but I think a warning message when vcpus 
 > pcpus would suffice.  (We do occasionally see people show up who 
don't realize that in the vast majority of cases, vcpus > pcpus is a 
stupid way to run things.)

Having a "seatbelt" which is off by default is pretty useless; as anyone 
that knows to turn it on almost surely doesn't need it.

My argument was that for the 4.3 release, the potential use cases of 
vcpus > pcpus are basically of 0 value as far as I'm concerned; it's not 
worth introducing a risk of regression, no matter how small, in order to 
allow the "vcpu overcommit" scenario to work.

But as IanJ says:
1. You can already do "vcpu overcommit" by setting things in the config file
2. This fixes a bug when you are running vcpu-set

Fixing a bug *is* worth the tiny amount of risk this represents; so 
probably is making the interface consistent.

  -George

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

[Konrad Rzeszutek Wilk]

On Thu, May 09, 2013 at 10:09:18AM +0100, George Dunlap wrote:
> On 08/05/13 23:39, Konrad Rzeszutek Wilk wrote:
> >>>>>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.
> >>>>You could do it *after* the migration back to a 16 way box n stead of
> >>>>before though, which is most likely when you would actually want to do
> >>>>it...
> >>>I am kind of lost. Are we arguing for this being a bug or whether there is
> >>>justification for putting in Xen 4.3?
> >>The former needs deciding before the latter.
> >>
> >>I'm not convinced that the current xl behaviour of refusing to
> >>overcommit VCPUs on a host isn't the right one for the majority of use
> >>cases. Obviously the silently refusing bit is a bug which should be
> >>fixed.
> >>
> >>I don't buy that this is a "regression compared to Xend". It's certainly
> >>a difference from how xend behaved but it seems on the whole to be a
> >>positive one (i.e. xend was wrong).
> >CC-ing Juergen here as he added this in.
> >>Can you explain the use case for wanting to do this? I don't think the
> >>migration one you give above is very convincing since a normal user
> >>wouldn't want to overcommit on the source host, they would want to
> >>migrate and then increase the number of vcpus, without ever
> >>overcommitting, and therefore without the terrible performance of
> >>overcommitting.
> >It seems clear to me if a user wants to over-commit, then we should
> >allow the user to do so. If we provide a command to set X vCPUs it
> >should work as described - without the extra checks (unless
> >that is enabled by some other option).
> 
> I tend to agree with this; I hate it when some piece of software
> tells me, "I'm not going to let you do this for your own good." When
> that happens to me, it's almost always the case that I *do* have a
> good reason for doing so that the author of the software didn't
> think about.  Our users are sysadmins; we should allow them to shoot
> themselves in the foot if they want to.
> 
> So I think the right thing to do long-term is to make it possible to
> do in xl.  Having a "seatbelt" restriction by default that can be
> overridden would be OK with me, but I think a warning message when
> vcpus > pcpus would suffice.  (We do occasionally see people show up
> who don't realize that in the vast majority of cases, vcpus > pcpus
> is a stupid way to run things.)
> 
> Having a "seatbelt" which is off by default is pretty useless; as
> anyone that knows to turn it on almost surely doesn't need it.
> 
> My argument was that for the 4.3 release, the potential use cases of
> vcpus > pcpus are basically of 0 value as far as I'm concerned; it's
> not worth introducing a risk of regression, no matter how small, in
> order to allow the "vcpu overcommit" scenario to work.
> 
> But as IanJ says:
> 1. You can already do "vcpu overcommit" by setting things in the config file
> 2. This fixes a bug when you are running vcpu-set
> 
> Fixing a bug *is* worth the tiny amount of risk this represents; so
> probably is making the interface consistent.

OK, I am bit lost here. You and Ian seem to have different viewpoints of
what is the right way, or perhaps more likely is that I am misreading
it. The interface consistent in my reading sounds like you are agreeing
that the "I am not going to let you do this for your own good" is silly.

I think what you and Ian would like me to do:

For Xen 4.3:
 - Add a warning to the vcpu-set that the user might be doing something
   silly if they are trying to set 'xl vcpu-set vcpus' where vcpus >
   pcpus. Something along: "You are overcommiting. You have x PCPUs and
   you want y vCPUS. Aborting - if you would like to do this please use
   --no-seatbelt'.
 - If user is trying to do vcpu-set on more than pcpus abort the request
   unless the '--no-seatbelt' option is provided.

For Xen 4.4:
 - Remove those two things above and make vcpu-set work irregardless of
   the pcpus value? No that does not sound right.

<goes off to get some coffee>

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

[George Dunlap]

On 09/05/13 14:13, Konrad Rzeszutek Wilk wrote:
> On Thu, May 09, 2013 at 10:09:18AM +0100, George Dunlap wrote:
>> On 08/05/13 23:39, Konrad Rzeszutek Wilk wrote:
>>>>>>> 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.
>>>>>> You could do it *after* the migration back to a 16 way box n stead of
>>>>>> before though, which is most likely when you would actually want to do
>>>>>> it...
>>>>> I am kind of lost. Are we arguing for this being a bug or whether there is
>>>>> justification for putting in Xen 4.3?
>>>> The former needs deciding before the latter.
>>>>
>>>> I'm not convinced that the current xl behaviour of refusing to
>>>> overcommit VCPUs on a host isn't the right one for the majority of use
>>>> cases. Obviously the silently refusing bit is a bug which should be
>>>> fixed.
>>>>
>>>> I don't buy that this is a "regression compared to Xend". It's certainly
>>>> a difference from how xend behaved but it seems on the whole to be a
>>>> positive one (i.e. xend was wrong).
>>> CC-ing Juergen here as he added this in.
>>>> Can you explain the use case for wanting to do this? I don't think the
>>>> migration one you give above is very convincing since a normal user
>>>> wouldn't want to overcommit on the source host, they would want to
>>>> migrate and then increase the number of vcpus, without ever
>>>> overcommitting, and therefore without the terrible performance of
>>>> overcommitting.
>>> It seems clear to me if a user wants to over-commit, then we should
>>> allow the user to do so. If we provide a command to set X vCPUs it
>>> should work as described - without the extra checks (unless
>>> that is enabled by some other option).
>> I tend to agree with this; I hate it when some piece of software
>> tells me, "I'm not going to let you do this for your own good." When
>> that happens to me, it's almost always the case that I *do* have a
>> good reason for doing so that the author of the software didn't
>> think about.  Our users are sysadmins; we should allow them to shoot
>> themselves in the foot if they want to.
>>
>> So I think the right thing to do long-term is to make it possible to
>> do in xl.  Having a "seatbelt" restriction by default that can be
>> overridden would be OK with me, but I think a warning message when
>> vcpus > pcpus would suffice.  (We do occasionally see people show up
>> who don't realize that in the vast majority of cases, vcpus > pcpus
>> is a stupid way to run things.)
>>
>> Having a "seatbelt" which is off by default is pretty useless; as
>> anyone that knows to turn it on almost surely doesn't need it.
>>
>> My argument was that for the 4.3 release, the potential use cases of
>> vcpus > pcpus are basically of 0 value as far as I'm concerned; it's
>> not worth introducing a risk of regression, no matter how small, in
>> order to allow the "vcpu overcommit" scenario to work.
>>
>> But as IanJ says:
>> 1. You can already do "vcpu overcommit" by setting things in the config file
>> 2. This fixes a bug when you are running vcpu-set
>>
>> Fixing a bug *is* worth the tiny amount of risk this represents; so
>> probably is making the interface consistent.
> OK, I am bit lost here. You and Ian seem to have different viewpoints of
> what is the right way, or perhaps more likely is that I am misreading
> it. The interface consistent in my reading sounds like you are agreeing
> that the "I am not going to let you do this for your own good" is silly.

Yes, I think IanC and I are disagreeing about ideal xl behavior.  We 
both agree that "vcpus > pcpus" is a bad configuration.  I think ideally 
we should support it (because administrators should be allowed to shoot 
themselves in the foot) and Ian seems to be making the case that we 
shouldn't support it.

But I think IanC and I also both agree that, "This patch enables vcpus > 
pcpus" is not a good argument to accept any patch at this point in the 
release cycle.

> I think what you and Ian would like me to do:
>
> For Xen 4.3:
>   - Add a warning to the vcpu-set that the user might be doing something
>     silly if they are trying to set 'xl vcpu-set vcpus' where vcpus >
>     pcpus. Something along: "You are overcommiting. You have x PCPUs and
>     you want y vCPUS. Aborting - if you would like to do this please use
>     --no-seatbelt'.
>   - If user is trying to do vcpu-set on more than pcpus abort the request
>     unless the '--no-seatbelt' option is provided.
>
> For Xen 4.4:
>   - Remove those two things above and make vcpu-set work irregardless of
>     the pcpus value? No that does not sound right.

I think for 4.3, if this patch is the quickest way to get rid of the bug 
of xl crashing, we should just accept it.

Then for 4.4, we can discuss warnings or "seatbelt" options.

  -George

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

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited hosts."):
> Yes, I think IanC and I are disagreeing about ideal xl behavior.  We 
> both agree that "vcpus > pcpus" is a bad configuration.  I think ideally 
> we should support it (because administrators should be allowed to shoot 
> themselves in the foot) and Ian seems to be making the case that we 
> shouldn't support it.

I'm with George on this one.

> > I think what you and Ian would like me to do:
> >
> > For Xen 4.3:
> >   - Add a warning to the vcpu-set that the user might be doing something
> >     silly if they are trying to set 'xl vcpu-set vcpus' where vcpus >
> >     pcpus. Something along: "You are overcommiting. You have x PCPUs and
> >     you want y vCPUS. Aborting - if you would like to do this please use
> >     --no-seatbelt'.
> >   - If user is trying to do vcpu-set on more than pcpus abort the request
> >     unless the '--no-seatbelt' option is provided.
> >
> > For Xen 4.4:
> >   - Remove those two things above and make vcpu-set work irregardless of
> >     the pcpus value? No that does not sound right.
> 
> I think for 4.3, if this patch is the quickest way to get rid of the bug 
> of xl crashing, we should just accept it.

Yes, absolutely.  I think the patch should go into 4.3.

Ian.

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

[Juergen Gross]

On 09.05.2013 00:39, Konrad Rzeszutek Wilk wrote:
>>>>> 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.
>>>>
>>>> You could do it *after* the migration back to a 16 way box n stead of
>>>> before though, which is most likely when you would actually want to do
>>>> it...
>>>
>>> I am kind of lost. Are we arguing for this being a bug or whether there is
>>> justification for putting in Xen 4.3?
>>
>> The former needs deciding before the latter.
>>
>> I'm not convinced that the current xl behaviour of refusing to
>> overcommit VCPUs on a host isn't the right one for the majority of use
>> cases. Obviously the silently refusing bit is a bug which should be
>> fixed.
>>
>> I don't buy that this is a "regression compared to Xend". It's certainly
>> a difference from how xend behaved but it seems on the whole to be a
>> positive one (i.e. xend was wrong).
>
> CC-ing Juergen here as he added this in.

I surely didn't mean to disable overcommitting. So I'm fine with your patch
to do the cpumap allocation with maxvcpus.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

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

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited hosts."):
> 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.

I hadn't spotted that.  Arguably "return 0" should be "abort()".

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

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

[Konrad Rzeszutek Wilk]

On Wed, May 08, 2013 at 05:03:53PM +0100, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited hosts."):
> > 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.
> 
> I hadn't spotted that.  Arguably "return 0" should be "abort()".

I am really hesistant about doing this in Xen 4.3. There are a bunch
of places where this is used and I fear this will cause tons of
asserts and more bugs to crep up which will make George MAD!!!


I can certainly prep a patch for Xen 4.4 and slowly fix the fallout.
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Is that for adding an assert or to the patch?
> 
> Thanks,
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

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

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited hosts."):
> On Wed, May 08, 2013 at 05:03:53PM +0100, Ian Jackson wrote:
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Is that for adding an assert or to the patch?

The patch.  Naturally I'm not acking my own assert (which is a bad
idea at this stage for all the reasons you give).

Ian.

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

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH] libxl: Make 'xl vcpu-set' work properly on overcommited hosts."):
> 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.

I would say that this is a bugfix which should go into 4.3.  This bug
isn't limiting vcpus to <= pcpus because you can create the guest with
more.  AIUI what happens right now is that if you try to vcpu-set with
vcpus>pcups xl will crash (or do something else undefined).  This is
only not a security hole because xl isn't exposed to untrusted users.

Ian.