Re: [RFC PATCH 1/3] xl: replace vcpu-set --ignore-host with --ignore-warn

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

On Wed, Aug 07, 2013 at 11:50:24AM +0100, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [RFC PATCH 1/3] xl: replace vcpu-set --ignore-host with --ignore-warn"):
> > On Tue, Jul 23, 2013 at 8:17 PM, Konrad Rzeszutek Wilk
> > > Good point. In which case I think --no-warn and no short option
> > > makes the most sense.
> > 
> > TBH the no-warning option seems a bit unnecessary, and a part of me
> > thinks just making it possible to do without an override should be
> > enough.
> 
> I agree.
> 
> Also we need to keep accepting the previously-introduced option name,
> so that existing users don't break.  xl should have
> forward-compatibility.  But we can just ignore the option if the
> behaviour it requests becomes the default.

I think the following patch does what you guys were thinking of.

It neuters the --ignore-host, still prints out the warning (but
continues on) and in general allows the system admin to do whatever
they want. And adds a blurb in the manpage that the original commit
forgot to do (shame on me). Let me send it out as I have one more
patch for this.


>From 6170566eb78f86d73a9cc86ac36f7ec0849b1517 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 19 Jul 2013 10:15:53 -0400
Subject: [PATCH 1/2] xl: neuter vcpu-set --ignore-host.

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,
 - If a user wants to do it, let them do it,
 - The original author of the change did not realize the
   side-effect his patch caused this and had no intention of changing it.
 - The user can already boot a massively overcommitted guest by
   having a large 'vcpus=' value in the guest config and we allow
   that.

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.

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,
-- 
1.8.3.1