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