Re: [PATCH v1 2/2] libxl: vcpu-set - allow to decrease vcpu count on overcommitted guests (v2)

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

On Thu, Jun 05, 2014 at 12:02:57PM +0100, Ian Campbell wrote:
> On Wed, 2014-06-04 at 09:33 -0400, Konrad Rzeszutek Wilk wrote:
> > We have a check to warn the user if they are overcommitting.
> > But the check only checks the hosts CPU amount and does
> > not take into account the case when the user is trying to fix
> > the overcommit. That is - they want to limit the amount of
> > online VCPUs.
> > 
> > This fix allows the user to offline vCPUs without any
> > warnings when they are running an overcommitted guest.
> > 
> > Also while at it, remove crud code.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Contrary to $SUBJECT this is an xl patch not a libxl one. Also there is
> a spurious "(v2)" in the subject.
> 
> > [v2: Remove crud code as spotted by Boris]
> > ---
> >  tools/libxl/xl_cmdimpl.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 5195914..5b27bd8 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -4754,15 +4754,21 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> >       * by the host's amount of pCPUs.
> >       */
> >      if (check_host) {
> > +        libxl_dominfo dominfo;
> > +
> >          unsigned int host_cpu = libxl_get_max_cpus(ctx);
> > -        if (max_vcpus > host_cpu) {
> > -            fprintf(stderr, "You are overcommmitting! You have %d physical " \
> > -                    " CPUs and want %d vCPUs! Aborting, use --ignore-host to " \
> > -                    " continue\n", host_cpu, max_vcpus);
> > -            return;
> > +
> > +        if (libxl_domain_info(ctx, &dominfo, domid) != 0)
> > +            dominfo.vcpu_online = host_cpu;
> > +
> > +        if (max_vcpus > dominfo.vcpu_online) {
> > +            if ((max_vcpus > host_cpu)) {
> 
> I think this is 
>         if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
> 
> and if not then the second one has a spurious set of ()s.
> 
> > +                fprintf(stderr, "You are overcommmitting! You have %d physical" \
> 
> You've carried over the typo here (unless you intended to overcommit on
> the number of m's ;-)). Might as well fix while you are here..

Mmmmm.. You are riggggghhhhhhhhhhttttttttt.
> 
> > +                        " CPUs and want %d vCPUs! Aborting, use --ignore-host to" \
> > +                        " continue\n", host_cpu, max_vcpus);
> > +                return;
> > +            }
> >          }
> > -        /* NB: This also limits how many are set in the bitmap */
> > -        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
> 
> Where did this go?

No need for it actually. As we already do the action if 'max_vcpus >
host_cpu' - which is that we return. So in essence that code will set max_vcpus
to max_vcpus.

> 
> >      }
> >      if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
> >          fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
> 
>