Re: [PATCH 1/2] xl: neuter vcpu-set --ignore-host.

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

On Thu, Sep 26, 2013 at 10:06:31AM +0100, Ian Campbell wrote:
> On Wed, 2013-09-25 at 16:40 -0400, Konrad Rzeszutek Wilk wrote:
> > When Xen 4.3 was released we had a discussion whether we should
> > allow the vcpu-set command to allow the user to set more than
> > physical CPUs for a guest (it didn't). The author brought up:
> >  - Xend used to do it,
> 
> IMHO xend is buggy here. If it were being maintained I encourage a patch
> to file this particular sharp edge off.
> 
> >  - If a user wants to do it, let them do it,
> 
> We do, we have an option for those who know what they are doing to use
> in the tiny minority of cases where they need to do this.
> 
> >  - The original author of the change did not realize the
> >    side-effect his patch caused this and had no intention of changing it.
> 
> a happy accident then.
> 
> >  - The user can already boot a massively overcommitted guest by
> >    having a large 'vcpus=' value in the guest config and we allow
> >    that.
> 
> IMHO this is an xl bug, I'd be happy to see a patch to fix this and
> require and override here too.

I think I posted one some time ago, but I don't recall anybody
commenting on it. Will repost it.
> 
> > 
> > Since we were close to the release we added --ignore-host parameter
> > as a mechanism for a user to still set more vCPUs that the physical
> > machine as a stop-gate.
> > 
> > This patch keeps said option but neuters the check so that we
> > can overcommit. In other words - by default the user is
> > allowed to set as many vCPUs as they would like.
> 
> and why would a naive user want to do this? non-naive users can use the
> option if this is what they really want, and are probably grateful for
> the catch if they didn't intend to overcommit, which is almost always
> even for expert users.
> 
> This change need far better rationalisation than "because xend did it"
> and "because we can". IMHO.

I am going to defer to George here. His viewpoint (I am going to
probably mangle it up) was that - if the user wants to do, let him/her
do it without us putting obstacles.

And I think Ian Jackson was ambivalent here and was deferring to George.

George?
> 
> > 
> > Furthermore mention this parameter change in the man-page.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  docs/man/xl.pod.1         | 15 ++++++++++++++-
> >  tools/libxl/xl_cmdimpl.c  | 28 ++++++++++++----------------
> >  tools/libxl/xl_cmdtable.c |  2 +-
> >  3 files changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> > index 5975d7b..1199d01 100644
> > --- a/docs/man/xl.pod.1
> > +++ b/docs/man/xl.pod.1
> > @@ -597,7 +597,7 @@ This command is only available for HVM domains.
> >  Moves a domain out of the paused state.  This will allow a previously
> >  paused domain to now be eligible for scheduling by the Xen hypervisor.
> >  
> > -=item B<vcpu-set> I<domain-id> I<vcpu-count>
> > +=item B<vcpu-set> I<OPTION> I<domain-id> I<vcpu-count>
> >  
> >  Enables the I<vcpu-count> virtual CPUs for the domain in question.
> >  Like mem-set, this command can only allocate up to the maximum virtual
> > @@ -614,6 +614,19 @@ quietly ignored.
> >  Some guests may need to actually bring the newly added CPU online
> >  after B<vcpu-set>, go to B<SEE ALSO> section for information.
> >  
> > +B<OPTION>
> > +
> > +=over 4
> > +
> > +=item B<-i>, B<--ignore-host>
> > +
> > +Deprecated. Used to allow the user to increase the current number of
> > +active VCPUs, if it was greater than physical number of CPUs.
> > +This seatbelt option was introduced due to being (depending on the type
> > +of workload and guest OS) performance drawbacks of CPU overcommitting.
> > +
> > +=back
> > +
> >  =item B<vcpu-list> [I<domain-id>]
> >  
> >  Lists VCPU information for a specific domain.  If no domain is
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 3d7eaad..ecab9a6 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -4536,11 +4536,12 @@ int main_vcpupin(int argc, char **argv)
> >      return 0;
> >  }
> >  
> > -static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> > +static void vcpuset(uint32_t domid, const char* nr_vcpus)
> >  {
> >      char *endptr;
> >      unsigned int max_vcpus, i;
> >      libxl_bitmap cpumap;
> > +    unsigned int host_cpu;
> >  
> >      max_vcpus = strtoul(nr_vcpus, &endptr, 10);
> >      if (nr_vcpus == endptr) {
> > @@ -4549,19 +4550,14 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> >      }
> >  
> >      /*
> > -     * Maximum amount of vCPUS the guest is allowed to set is limited
> > -     * by the host's amount of pCPUs.
> > +     * Warn if maximum amount of vCPUS the guest wants is higher than
> > +     * the host's amount of pCPUs.
> >       */
> > -    if (check_host) {
> > -        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;
> > -        }
> > -        /* NB: This also limits how many are set in the bitmap */
> > -        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
> > +    host_cpu = libxl_get_max_cpus(ctx);
> > +    if (max_vcpus > host_cpu) {
> > +        fprintf(stderr, "WARNING: You are overcommmitting! You have %d" \
> > +                " physical CPUs and want %d vCPUs! Continuing..\n",
> > +                host_cpu, max_vcpus);
> >      }
> >      if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
> >          fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
> > @@ -4582,17 +4578,17 @@ int main_vcpuset(int argc, char **argv)
> >          {"ignore-host", 0, 0, 'i'},
> >          {0, 0, 0, 0}
> >      };
> > -    int opt, check_host = 1;
> > +    int opt;
> >  
> >      SWITCH_FOREACH_OPT(opt, "i", opts, "vcpu-set", 2) {
> >      case 'i':
> > -        check_host = 0;
> > +        /* deprecated. */;
> >          break;
> >      default:
> >          break;
> >      }
> >  
> > -    vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
> > +    vcpuset(find_domain(argv[optind]), argv[optind + 1]);
> >      return 0;
> >  }
> >  
> > diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> > index 326a660..2ed9715 100644
> > --- a/tools/libxl/xl_cmdtable.c
> > +++ b/tools/libxl/xl_cmdtable.c
> > @@ -219,7 +219,7 @@ struct cmd_spec cmd_table[] = {
> >        &main_vcpuset, 0, 1,
> >        "Set the number of active VCPUs allowed for the domain",
> >        "[option] <Domain> <vCPUs>",
> > -      "-i, --ignore-host  Don't limit the vCPU based on the host CPU count",
> > +      "-i, --ignore-host  Don't limit the vCPU based on the host CPU count (deprecated)",
> >      },
> >      { "vm-list",
> >        &main_vm_list, 0, 0,
> 
>