Xen devel - May 2013 - [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.

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

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

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.

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.

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
>

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

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.

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.

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

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.

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

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.

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

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.

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

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>

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

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.

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
>

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

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.