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