Xen devel - Jul 2013 - [RFC PATCH] Add the --ignore-warn parameter (v1).

Greetings,

During Xen 4.3 I found out that xl vpu-set was not behaving as it
should and sent out a fix for it. But since it was late in the cycle
we decided that the fix better go in, but we need to do it "right"
in Xen 4.4.

This patchset tries an approach that George thought would be OK
(the first patch that is it). I expanded the idea and made the rest
of the operations be in line with this.

Anyhow, please take a look at the patches and speak your mind.


 docs/man/xl.pod.1         |   21 +++++++++-
 tools/libxl/xl_cmdimpl.c  |  100 ++++++++++++++++++++++++++-------------------
 tools/libxl/xl_cmdtable.c |    8 ++-
 3 files changed, 83 insertions(+), 46 deletions(-)


Konrad Rzeszutek Wilk (3):
      xl: replace vcpu-set --ignore-host with --ignore-warn
      xl/create: warn if the ''vcpu'' parameter exceeds host
physical CPUs.
      xl/create: Sprinkle the check to silence warnings further.

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. 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 removes said option and adds the --ignore-warn option.
By default the user is allowed to set as many vCPUs as they would like.
We will print out a warning if the value is higher than the physical
CPU count. The --ignore-warn will silence said warning.

Furthermore mention this parameter in the man-page and add
the ''WARNING'' in the printf.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 docs/man/xl.pod.1         |   15 ++++++++++++++-
 tools/libxl/xl_cmdimpl.c  |   30 +++++++++++++-----------------
 tools/libxl/xl_cmdtable.c |    2 +-
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 5975d7b..e09d330 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-warn>
+
+Ignore the warning if the amount of physical CPUs is lesser
+than the amount of virtual CPUs that is being set. There are
+(depending on the type of workload and guest OS) performance
+drawback 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 4a8feaf..3d23fb4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4512,11 +4512,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, int ignore_warn)
 {
     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) {
@@ -4525,19 +4526,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 && !ignore_warn) {
+        fprintf(stderr, "WARNING: You are overcommmitting! You have
%d" \
+                " physical CPUs and want %d vCPUs! Continuing, use" \
+                " --ignore-warn to silence this.\n", host_cpu,
max_vcpus);
     }
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
         fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
@@ -4555,20 +4551,20 @@ static void vcpuset(uint32_t domid, const char*
nr_vcpus, int check_host)
 int main_vcpuset(int argc, char **argv)
 {
     static struct option opts[] = {
-        {"ignore-host", 0, 0, ''i''},
+        {"ignore-warn", 0, 0, ''i''},
         {0, 0, 0, 0}
     };
-    int opt, check_host = 1;
+    int opt, ignore_warn = 0;
 
     SWITCH_FOREACH_OPT(opt, "i", opts, "vcpu-set", 2) {
     case ''i'':
-        check_host = 0;
+        ignore_warn = 1;
         break;
     default:
         break;
     }
 
-    vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
+    vcpuset(find_domain(argv[optind]), argv[optind + 1], ignore_warn);
     return 0;
 }
 
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 326a660..8a98c6a 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-warn  Ignore warning if there are more vCPUs that
there are host CPUs",
     },
     { "vm-list",
       &main_vm_list, 0, 0,
-- 
1.7.7.6

It can be a performance disadvantage to allocate more vCPUs
for a guest that there are physical CPUs. If the guest config
has such setup warn the user. The warning can be silenced by
the usage of ''--ignore-warn'' (-i) parameter.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 docs/man/xl.pod.1         |    6 ++++++
 tools/libxl/xl_cmdimpl.c  |   21 ++++++++++++++++++---
 tools/libxl/xl_cmdtable.c |    6 ++++--
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index e09d330..2ebeb6c 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -147,6 +147,10 @@ It is possible to pass I<key=value> pairs on the
command line to provide
 options as if they were written in the configuration file; these override
 whatever is in the I<configfile>.
 
+=item B<-i>, B<--ignore-warn>
+
+Silence warnings.
+
 =back
 
 B<EXAMPLES>
@@ -496,7 +500,9 @@ Attach to domain''s VNC server, forking a vncviewer
process.
 
 Pass VNC password to vncviewer via stdin.
 
+=item B<-i>, B<--ignore-warn>
 
+Silence warnings.
 
 =back
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3d23fb4..b83676c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -132,6 +132,7 @@ struct domain_create {
     int vnc;
     int vncautopass;
     int console_autoconnect;
+    int ignore_warn;
     const char *config_file;
     const char *extra_config; /* extra config string */
     const char *restore_file;
@@ -657,8 +658,14 @@ static void parse_config_data(const char *config_source,
         b_info->sched_params.extratime = l;
 
     if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
+        unsigned int host_cpu = libxl_get_max_cpus(ctx);
         b_info->max_vcpus = l;
 
+        if (l > host_cpu && dom_info &&
!dom_info->ignore_warn)
+         fprintf(stderr, "WARNING: You are overcommmitting! You have %d
" \
+                 "physical CPUs and want %d vCPUs!\n", host_cpu,
+                 b_info->max_vcpus);
+
         if (libxl_cpu_bitmap_alloc(ctx, &b_info->avail_vcpus, l)) {
             fprintf(stderr, "Unable to allocate cpumap\n");
             exit(1);
@@ -3741,11 +3748,13 @@ int main_restore(int argc, char **argv)
     const char *config_file = NULL;
     struct domain_create dom_info;
     int paused = 0, debug = 0, daemonize = 1, monitor = 1,
-        console_autoconnect = 0, vnc = 0, vncautopass = 0;
+        console_autoconnect = 0, vnc = 0, vncautopass = 0,
+        ignore_warn = 0;
     int opt, rc;
     static struct option opts[] = {
         {"vncviewer", 0, 0, ''V''},
         {"vncviewer-autopass", 0, 0, ''A''},
+        {"ignore-warn", 0, 0, ''i''},
         COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
@@ -3773,6 +3782,8 @@ int main_restore(int argc, char **argv)
     case ''A'':
         vnc = vncautopass = 1;
         break;
+    case ''i'':
+        ignore_warn = 1;
     }
 
     if (argc-optind == 1) {
@@ -3796,7 +3807,7 @@ int main_restore(int argc, char **argv)
     dom_info.vnc = vnc;
     dom_info.vncautopass = vncautopass;
     dom_info.console_autoconnect = console_autoconnect;
-
+    dom_info.ignore_warn = ignore_warn;
     rc = create_domain(&dom_info);
     if (rc < 0)
         return -rc;
@@ -4139,7 +4150,7 @@ int main_create(int argc, char **argv)
     char extra_config[1024];
     struct domain_create dom_info;
     int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0,
-        quiet = 0, monitor = 1, vnc = 0, vncautopass = 0;
+        quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_warn = 0;
     int opt, rc;
     static struct option opts[] = {
         {"dryrun", 0, 0, ''n''},
@@ -4147,6 +4158,7 @@ int main_create(int argc, char **argv)
         {"defconfig", 1, 0, ''f''},
         {"vncviewer", 0, 0, ''V''},
         {"vncviewer-autopass", 0, 0, ''A''},
+        {"ignore-warn", 0, 0, ''i''},
         COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
@@ -4188,6 +4200,8 @@ int main_create(int argc, char **argv)
     case ''A'':
         vnc = vncautopass = 1;
         break;
+    case ''i'':
+        ignore_warn = 1;
     }
 
     extra_config[0] = ''\0'';
@@ -4216,6 +4230,7 @@ int main_create(int argc, char **argv)
     dom_info.vnc = vnc;
     dom_info.vncautopass = vncautopass;
     dom_info.console_autoconnect = console_autoconnect;
+    dom_info.ignore_warn = ignore_warn;
 
     rc = create_domain(&dom_info);
     if (rc < 0)
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 8a98c6a..6fc8932 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -33,7 +33,8 @@ struct cmd_spec cmd_table[] = {
       "-e                      Do not wait in the background for the death
of the domain.\n"
       "-V, --vncviewer         Connect to the VNC display after the domain
is created.\n"
       "-A, --vncviewer-autopass\n"
-      "                        Pass VNC password to viewer via
stdin."
+      "                        Pass VNC password to viewer via
stdin.\n"
+      "-i, --ignore-warn       Silence the warnings.",
     },
     { "config-update",
       &main_config_update, 1, 1,
@@ -172,7 +173,8 @@ struct cmd_spec cmd_table[] = {
       "-e                       Do not wait in the background for the
death of the domain.\n"
       "-d                       Enable debug messages.\n"
       "-V, --vncviewer          Connect to the VNC display after the
domain is created.\n"
-      "-A, --vncviewer-autopass Pass VNC password to viewer via
stdin."
+      "-A, --vncviewer-autopass Pass VNC password to viewer via
stdin.\n"
+      "-i, --ignore-warn        Silence warnings.",
     },
     { "migrate-receive",
       &main_migrate_receive, 0, 1,
-- 
1.7.7.6

We have now the parameter --ignore-warn which will silence
the warnings. Originally it was only for vCPUs but it might
as well be a blanket operation and silence all of the warnings.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl_cmdimpl.c |   49 +++++++++++++++++++++++++--------------------
 1 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b83676c..e91936f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -783,13 +783,14 @@ static void parse_config_data(const char *config_source,
     xlu_cfg_get_defbool(config, "nomigrate",
&b_info->disable_migrate, 0);
 
     if (!xlu_cfg_get_long(config, "tsc_mode", &l, 1)) {
-        const char *s = libxl_tsc_mode_to_string(l);
-        fprintf(stderr, "WARNING: specifying \"tsc_mode\" as an
integer is deprecated. "
-                "Please use the named parameter variant. %s%s%s\n",
-                s ? "e.g. tsc_mode=\"" : "",
-                s ? s : "",
-                s ? "\"" : "");
-
+        if (dom_info && !dom_info->ignore_warn) {
+                const char *s = libxl_tsc_mode_to_string(l);
+                fprintf(stderr, "WARNING: specifying
\"tsc_mode\" as an integer is deprecated. "
+                        "Please use the named parameter variant.
%s%s%s\n",
+                        s ? "e.g. tsc_mode=\"" : "",
+                        s ? s : "",
+                        s ? "\"" : "");
+        }
         if (l < LIBXL_TSC_MODE_DEFAULT ||
             l > LIBXL_TSC_MODE_NATIVE_PARAVIRT) {
             fprintf(stderr, "ERROR: invalid value %ld for
\"tsc_mode\"\n", l);
@@ -822,7 +823,8 @@ static void parse_config_data(const char *config_source,
 
     switch(b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
-        if (!xlu_cfg_get_string (config, "kernel", &buf, 0))
+        if (!xlu_cfg_get_string (config, "kernel", &buf, 0)
&& dom_info &&
+            !dom_info->ignore_warn)
             fprintf(stderr, "WARNING: ignoring \"kernel\"
directive for HVM guest. "
                     "Use \"firmware_override\" instead if you
really want a non-default firmware\n");
 
@@ -846,13 +848,14 @@ static void parse_config_data(const char *config_source,
         xlu_cfg_get_defbool(config, "vpt_align",
&b_info->u.hvm.vpt_align, 0);
 
         if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
-            const char *s = libxl_timer_mode_to_string(l);
-            fprintf(stderr, "WARNING: specifying \"timer_mode\"
as an integer is deprecated. "
-                    "Please use the named parameter variant.
%s%s%s\n",
-                    s ? "e.g. timer_mode=\"" : "",
-                    s ? s : "",
-                    s ? "\"" : "");
-
+            if (dom_info && !dom_info->ignore_warn) {
+                const char *s = libxl_timer_mode_to_string(l);
+                fprintf(stderr, "WARNING: specifying
\"timer_mode\" as an integer is deprecated. "
+                        "Please use the named parameter variant.
%s%s%s\n",
+                        s ? "e.g. timer_mode=\"" : "",
+                        s ? s : "",
+                        s ? "\"" : "");
+            }
             if (l < LIBXL_TIMER_MODE_DELAY_FOR_MISSED_TICKS ||
                 l > LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING) {
                 fprintf(stderr, "ERROR: invalid value %ld for
\"timer_mode\"\n", l);
@@ -905,10 +908,10 @@ static void parse_config_data(const char *config_source,
         case ESRCH: break; /* Option not present */
         case EINVAL:
             if (!xlu_cfg_get_string(config, "bootloader_args",
&buf, 0)) {
-
-                fprintf(stderr, "WARNING: Specifying
\"bootloader_args\""
-                        " as a string is deprecated. "
-                        "Please use a list of arguments.\n");
+                if (dom_info && !dom_info->ignore_warn)
+                    fprintf(stderr, "WARNING: Specifying
\"bootloader_args\""
+                            " as a string is deprecated. "
+                            "Please use a list of arguments.\n");
                 split_string_into_string_list(buf, " \t\n",
                                              
&b_info->u.pv.bootloader_args);
             }
@@ -1202,7 +1205,8 @@ skip_nic:
         }
     }
 
-    if (!xlu_cfg_get_list(config, "vif2", NULL, 0, 0)) {
+    if (!xlu_cfg_get_list(config, "vif2", NULL, 0, 0) &&
dom_info &&
+        !dom_info->ignore_warn) {
         fprintf(stderr, "WARNING: vif2: netchannel2 is deprecated and not
supported by xl\n");
     }
 
@@ -1386,7 +1390,8 @@ skip_vfb:
     }
 
     /* parse device model arguments, this works for pv, hvm and stubdom */
-    if (!xlu_cfg_get_string (config, "device_model", &buf, 0)) {
+    if (!xlu_cfg_get_string (config, "device_model", &buf, 0)
&& dom_info &&
+        !dom_info->ignore_warn) {
         fprintf(stderr,
                 "WARNING: ignoring device_model directive.\n"
                 "WARNING: Use \"device_model_override\" instead
if you"
@@ -1418,7 +1423,7 @@ skip_vfb:
                     "Unknown device_model_version \"%s\"
specified\n", buf);
             exit(1);
         }
-    } else if (b_info->device_model)
+    } else if (b_info->device_model && dom_info &&
!dom_info->ignore_warn)
         fprintf(stderr, "WARNING: device model override given without
specific DM version\n");
     xlu_cfg_get_defbool (config, "device_model_stubdomain_override",
                          &b_info->device_model_stubdomain, 0);
-- 
1.7.7.6

On 07/19/2013 04:48 PM, 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. 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 removes said option and adds the --ignore-warn option.
> By default the user is allowed to set as many vCPUs as they would like.
> We will print out a warning if the value is higher than the physical
> CPU count. The --ignore-warn will silence said warning.
I think this is a good change in general, but I don''t think the name is
quite right.  You''re not ignoring the warnings, you''re turning
them off.
  Maybe make the function argument "warn", and the option
"--no-warn"?
> +    host_cpu = libxl_get_max_cpus(ctx);
> +    if (max_vcpus > host_cpu && !ignore_warn) {
> +        fprintf(stderr, "WARNING: You are overcommmitting! You have
%d" \
> +                " physical CPUs and want %d vCPUs! Continuing,
use" \
> +                " --ignore-warn to silence this.\n", host_cpu,
max_vcpus);
>       }
This is relatively minor, but you might as well keep the same basic 
structure here, and only call libxl_get_max_cpus() if warn is set.

  -George

On Tue, Jul 23, 2013 at 12:29:36AM +0100, George Dunlap
wrote:
> On 07/19/2013 04:48 PM, 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. 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 removes said option and adds the --ignore-warn option.
> >By default the user is allowed to set as many vCPUs as they would like.
> >We will print out a warning if the value is higher than the physical
> >CPU count. The --ignore-warn will silence said warning.
> 
> I think this is a good change in general, but I don''t think the
name
> is quite right.  You''re not ignoring the warnings, you''re
turning
> them off.  Maybe make the function argument "warn", and the
option
> "--no-warn"?
--silence?

If we were to use the --no-warn any thoughts on the short
version of it ? ''-n'' is a bit odd. I like the
''-s''..
> 
> >+    host_cpu = libxl_get_max_cpus(ctx);
> >+    if (max_vcpus > host_cpu && !ignore_warn) {
> >+        fprintf(stderr, "WARNING: You are overcommmitting! You
have %d" \
> >+                " physical CPUs and want %d vCPUs! Continuing,
use" \
> >+                " --ignore-warn to silence this.\n",
host_cpu, max_vcpus);
> >      }
> 
> This is relatively minor, but you might as well keep the same basic
> structure here, and only call libxl_get_max_cpus() if warn is set.
<nods>
> 
>  -George
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

On 07/23/2013 03:01 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 23, 2013 at 12:29:36AM +0100, George Dunlap wrote:
>> On 07/19/2013 04:48 PM, 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. 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 removes said option and adds the --ignore-warn option.
>>> By default the user is allowed to set as many vCPUs as they would
like.
>>> We will print out a warning if the value is higher than the
physical
>>> CPU count. The --ignore-warn will silence said warning.
>>
>> I think this is a good change in general, but I don''t think
the name
>> is quite right.  You''re not ignoring the warnings,
you''re turning
>> them off.  Maybe make the function argument "warn", and the
option
>> "--no-warn"?
>
> --silence?
Well there is some --quiet, but I suppose that generally means don''t 
tell me *anything*, which is not what we want either.  I would think 
--silence would mean about the same thing.

  -George

On Tue, Jul 23, 2013 at 06:52:44PM +0100, George Dunlap
wrote:
> On 07/23/2013 03:01 PM, Konrad Rzeszutek Wilk wrote:
> >On Tue, Jul 23, 2013 at 12:29:36AM +0100, George Dunlap wrote:
> >>On 07/19/2013 04:48 PM, 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. 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 removes said option and adds the --ignore-warn
option.
> >>>By default the user is allowed to set as many vCPUs as they
would like.
> >>>We will print out a warning if the value is higher than the
physical
> >>>CPU count. The --ignore-warn will silence said warning.
> >>
> >>I think this is a good change in general, but I don''t
think the name
> >>is quite right.  You''re not ignoring the warnings,
you''re turning
> >>them off.  Maybe make the function argument "warn", and
the option
> >>"--no-warn"?
> >
> >--silence?
> 
> Well there is some --quiet, but I suppose that generally means
don''t
> tell me *anything*, which is not what we want either.  I would think
> --silence would mean about the same thing.
Good point. In which case I think --no-warn and no short option
makes the most sense.
> 
>  -George
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

On Tue, Jul 23, 2013 at 8:17 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Tue, Jul 23, 2013 at 06:52:44PM +0100, George Dunlap wrote:
>> On 07/23/2013 03:01 PM, Konrad Rzeszutek Wilk wrote:
>> >On Tue, Jul 23, 2013 at 12:29:36AM +0100, George Dunlap wrote:
>> >>On 07/19/2013 04:48 PM, 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. 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 removes said option and adds the --ignore-warn
option.
>> >>>By default the user is allowed to set as many vCPUs as they
would like.
>> >>>We will print out a warning if the value is higher than the
physical
>> >>>CPU count. The --ignore-warn will silence said warning.
>> >>
>> >>I think this is a good change in general, but I don''t
think the name
>> >>is quite right.  You''re not ignoring the warnings,
you''re turning
>> >>them off.  Maybe make the function argument "warn",
and the option
>> >>"--no-warn"?
>> >
>> >--silence?
>>
>> Well there is some --quiet, but I suppose that generally means
don''t
>> tell me *anything*, which is not what we want either.  I would think
>> --silence would mean about the same thing.
>
> 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.

 -George

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.

Ian.

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