|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] xl: neuter vcpu-set --ignore-host.
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@xxxxxxxxxx>
> > ---
> > 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,
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |