Konrad Rzeszutek Wilk
2013-Jul-12 18:08 UTC
[PATCH] xl.conf defaults changes for Xen 4.4. (v1).
Hey George and Ian, During the Xen 4.3 release we briefly chatted about enabling and providing an seatbelt parameter for some of the functionality that libxl provides. In particular these were: - xl vcpu-set - it has an host check such that the vCPU cannot be set higher than the pCPU. But a user can set such values in the guest config and launch an over-subscribed guest without any warnings. (Perhaps we should add a warning for that as well? Or don''t launch the guest?) - claim_mode - The functionality here is not just specific to tmem enabled guests. Any generic guest can benefit from this and it allows the user to get up-to-date memory information on how much free memory there really is - and how much is being consumed for the guest creation. By default it is disabled. This turns it on by default. Please see the patches and enjoy reviewing them. I''ve asbestos jumpsuit ready in case this discussion gets heated. docs/man/xl.conf.pod.5 | 26 +++++++++++++++++++++++++- tools/examples/xl.conf | 8 +++++++- tools/libxl/xl.c | 4 ++++ tools/libxl/xl.h | 1 + tools/libxl/xl_cmdimpl.c | 6 ++++++ 5 files changed, 43 insertions(+), 2 deletions(-) Konrad Rzeszutek Wilk (2): claim: By default enable it. expert_mode: Add a new configuration option for expert users.
During the Xen 4.3 release we discussed that this feature could be turned on by default - as it benefits all of the guests - not just tmem related. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- docs/man/xl.conf.pod.5 | 2 +- tools/examples/xl.conf | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 index 1229c8a..125f786 100644 --- a/docs/man/xl.conf.pod.5 +++ b/docs/man/xl.conf.pod.5 @@ -139,7 +139,7 @@ Xen hypervisor argument and as well on the Linux kernel command line. Note that the claim call is not attempted if C<superpages> option is used in the guest config (see xl.cfg(5)). -Default: C<0> +Default: C<1> =over 4 diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf index 9c037a6..a80b8e7 100644 --- a/tools/examples/xl.conf +++ b/tools/examples/xl.conf @@ -32,4 +32,4 @@ # feedback whether the guest can be launched due to memory exhaustion # (which can take a long time to find out if launching huge guests). # see xl.conf(5) for details. -#claim_mode=0 +claim_mode=1 -- 1.7.7.6
Konrad Rzeszutek Wilk
2013-Jul-12 18:08 UTC
[PATCH 2/2] expert_mode: Add a new configuration option for expert users.
This could also be called ''seatbelt'' option. libxl has a variety of checks where it will fail out an operation unless the user has provided an --force (or --ignore) parameter. Currently one such check is for the ''vcpu-set'' command which will error out if the count of virtual cpus is greater than the physical cpus. This parameter will ignore such checks and allow the user to do the operations without the need for override flags. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- docs/man/xl.conf.pod.5 | 24 ++++++++++++++++++++++++ tools/examples/xl.conf | 6 ++++++ tools/libxl/xl.c | 4 ++++ tools/libxl/xl.h | 1 + tools/libxl/xl_cmdimpl.c | 6 ++++++ 5 files changed, 41 insertions(+), 0 deletions(-) diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 index 125f786..ed67472 100644 --- a/docs/man/xl.conf.pod.5 +++ b/docs/man/xl.conf.pod.5 @@ -158,6 +158,30 @@ massively huge guests). =back +=item B<expert_mode=BOOLEAN> + +Do not act on host performed checks that might lead to performance +degradations. Currently checks are made for following operations: + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the + physical count the operation will error out. + + +Default: C<1> + +=over 4 + +=item C<0> + +The checks are active and the operation will fail if the checks +are triggered. + +=item C<1> + +The checks are active but will be ignored and the operations +will commence. + +=back + =back =head1 SEE ALSO diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf index a80b8e7..6f27d67 100644 --- a/tools/examples/xl.conf +++ b/tools/examples/xl.conf @@ -33,3 +33,9 @@ # (which can take a long time to find out if launching huge guests). # see xl.conf(5) for details. claim_mode=1 + +# The user knows what to do. Currently enabling this option will mean that: +# vcpu-set won''t check the physical CPU count - which means the guest can +# over-subscribe (more vCPUS than pCPUS). +# see xl.conf(5) for details. +expert_mode=1 diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c index 1ce820c..4063b7c 100644 --- a/tools/libxl/xl.c +++ b/tools/libxl/xl.c @@ -47,6 +47,7 @@ char *default_bridge = NULL; char *default_gatewaydev = NULL; enum output_format default_output_format = OUTPUT_FORMAT_JSON; int claim_mode = 0; +int expert_mode = 0; static xentoollog_level minmsglevel = XTL_PROGRESS; @@ -173,6 +174,9 @@ static void parse_global_config(const char *configfile, if (!xlu_cfg_get_long (config, "claim_mode", &l, 0)) claim_mode = l; + if (!xlu_cfg_get_long (config, "expert_mode", &l, 0)) + expert_mode = l; + xlu_cfg_destroy(config); } diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h index 5ad3e17..51b7008 100644 --- a/tools/libxl/xl.h +++ b/tools/libxl/xl.h @@ -147,6 +147,7 @@ extern int autoballoon; extern int run_hotplug_scripts; extern int dryrun_only; extern int claim_mode; +extern int expert_mode; extern char *lockfile; extern char *default_vifscript; extern char *default_bridge; diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 8a478ba..a55c66a 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -4560,6 +4560,12 @@ int main_vcpuset(int argc, char **argv) break; } + /* + * No seatbelts for the user. + */ + if (expert_mode) + check_host = 0; + vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host); return 0; } -- 1.7.7.6
On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:> During the Xen 4.3 release we discussed that this feature could be > turned on by default - as it benefits all of the guests - not just > tmem related. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > docs/man/xl.conf.pod.5 | 2 +- > tools/examples/xl.conf | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 > index 1229c8a..125f786 100644 > --- a/docs/man/xl.conf.pod.5 > +++ b/docs/man/xl.conf.pod.5 > @@ -139,7 +139,7 @@ Xen hypervisor argument and as well on the Linux kernel command line. > Note that the claim call is not attempted if C<superpages> option is > used in the guest config (see xl.cfg(5)). > > -Default: C<0> > +Default: C<1> > > =over 4 > > diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf > index 9c037a6..a80b8e7 100644 > --- a/tools/examples/xl.conf > +++ b/tools/examples/xl.conf > @@ -32,4 +32,4 @@ > # feedback whether the guest can be launched due to memory exhaustion > # (which can take a long time to find out if launching huge guests). > # see xl.conf(5) for details. > -#claim_mode=0 > +claim_mode=1We should change the default in the C code and maker this a commented out claim_mode=1. Ian.
Ian Campbell
2013-Jul-16 09:04 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote:> This could also be called ''seatbelt'' option. > > libxl has a variety of checks where it will fail out an operation > unless the user has provided an --force (or --ignore) parameter. > Currently one such check is for the ''vcpu-set'' command which > will error out if the count of virtual cpus is greater than the > physical cpus. This parameter will ignore such checks and allow > the user to do the operations without the need for override flags.Does this overlap somewhat with various commands which individually take a -f(orce) option?> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > docs/man/xl.conf.pod.5 | 24 ++++++++++++++++++++++++ > tools/examples/xl.conf | 6 ++++++ > tools/libxl/xl.c | 4 ++++ > tools/libxl/xl.h | 1 + > tools/libxl/xl_cmdimpl.c | 6 ++++++ > 5 files changed, 41 insertions(+), 0 deletions(-) > > diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 > index 125f786..ed67472 100644 > --- a/docs/man/xl.conf.pod.5 > +++ b/docs/man/xl.conf.pod.5 > @@ -158,6 +158,30 @@ massively huge guests). > > =back > > +=item B<expert_mode=BOOLEAN> > + > +Do not act on host performed checks that might lead to performance > +degradations. Currently checks are made for following operations: > + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the > + physical count the operation will error out. > + > + > +Default: C<1> > + > +=over 4 > + > +=item C<0> > + > +The checks are active and the operation will fail if the checks > +are triggered. > + > +=item C<1> > + > +The checks are active but will be ignored and the operations > +will commence. > + > +=back > + > =back > > =head1 SEE ALSO > diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf > index a80b8e7..6f27d67 100644 > --- a/tools/examples/xl.conf > +++ b/tools/examples/xl.conf > @@ -33,3 +33,9 @@ > # (which can take a long time to find out if launching huge guests). > # see xl.conf(5) for details. > claim_mode=1 > + > +# The user knows what to do. Currently enabling this option will mean that: > +# vcpu-set won''t check the physical CPU count - which means the guest can > +# over-subscribe (more vCPUS than pCPUS). > +# see xl.conf(5) for details. > +expert_mode=1 > diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c > index 1ce820c..4063b7c 100644 > --- a/tools/libxl/xl.c > +++ b/tools/libxl/xl.c > @@ -47,6 +47,7 @@ char *default_bridge = NULL; > char *default_gatewaydev = NULL; > enum output_format default_output_format = OUTPUT_FORMAT_JSON; > int claim_mode = 0; > +int expert_mode = 0; > > static xentoollog_level minmsglevel = XTL_PROGRESS; > > @@ -173,6 +174,9 @@ static void parse_global_config(const char *configfile, > if (!xlu_cfg_get_long (config, "claim_mode", &l, 0)) > claim_mode = l; > > + if (!xlu_cfg_get_long (config, "expert_mode", &l, 0)) > + expert_mode = l; > + > xlu_cfg_destroy(config); > } > > diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h > index 5ad3e17..51b7008 100644 > --- a/tools/libxl/xl.h > +++ b/tools/libxl/xl.h > @@ -147,6 +147,7 @@ extern int autoballoon; > extern int run_hotplug_scripts; > extern int dryrun_only; > extern int claim_mode; > +extern int expert_mode; > extern char *lockfile; > extern char *default_vifscript; > extern char *default_bridge; > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 8a478ba..a55c66a 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -4560,6 +4560,12 @@ int main_vcpuset(int argc, char **argv) > break; > } > > + /* > + * No seatbelts for the user. > + */ > + if (expert_mode) > + check_host = 0; > + > vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host); > return 0; > }
Konrad Rzeszutek Wilk
2013-Jul-16 15:21 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
On Tue, Jul 16, 2013 at 10:04:15AM +0100, Ian Campbell wrote:> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote: > > This could also be called ''seatbelt'' option. > > > > libxl has a variety of checks where it will fail out an operation > > unless the user has provided an --force (or --ignore) parameter. > > Currently one such check is for the ''vcpu-set'' command which > > will error out if the count of virtual cpus is greater than the > > physical cpus. This parameter will ignore such checks and allow > > the user to do the operations without the need for override flags. > > Does this overlap somewhat with various commands which individually > take a -f(orce) option?Not exactly. This particular issue was more of the variety of - we add some checks that earlier version of toolstacks did not have. And as such we fail to do the operation _unless_ a force is given. But all of this kind of falls under a seatbelt function - if the user knows what he or she is doing could be bad, well - if they know what they are doing then they can go ahead.
Ian Jackson
2013-Jul-17 10:56 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."):> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote: > > This could also be called ''seatbelt'' option. > > > > libxl has a variety of checks where it will fail out an operation > > unless the user has provided an --force (or --ignore) parameter. > > Currently one such check is for the ''vcpu-set'' command which > > will error out if the count of virtual cpus is greater than the > > physical cpus. This parameter will ignore such checks and allow > > the user to do the operations without the need for override flags. > > Does this overlap somewhat with various commands which individually > take a -f(orce) option?Clearly it should disable all of those -f''s too.> > +=item B<expert_mode=BOOLEAN> > > + > > +Do not act on host performed checks that might lead to performance > > +degradations. Currently checks are made for following operations: > > + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the > > + physical count the operation will error out.I don''t think this is a very coherent specification. Surely it should override "all -f options" or something similar. I still don''t see why you would want such a thing. Ian.
George Dunlap
2013-Jul-17 11:03 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
On 17/07/13 11:56, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."): >> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote: >>> This could also be called ''seatbelt'' option. >>> >>> libxl has a variety of checks where it will fail out an operation >>> unless the user has provided an --force (or --ignore) parameter. >>> Currently one such check is for the ''vcpu-set'' command which >>> will error out if the count of virtual cpus is greater than the >>> physical cpus. This parameter will ignore such checks and allow >>> the user to do the operations without the need for override flags. >> Does this overlap somewhat with various commands which individually >> take a -f(orce) option? > Clearly it should disable all of those -f''s too. > >>> +=item B<expert_mode=BOOLEAN> >>> + >>> +Do not act on host performed checks that might lead to performance >>> +degradations. Currently checks are made for following operations: >>> + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the >>> + physical count the operation will error out. > I don''t think this is a very coherent specification. Surely it should > override "all -f options" or something similar.I think that makes sense for some but not all. For example, for "xl shutdown", -f means "send an ACPI event if no PV drivers are detected". I don''t think we''d want to change that behavior with such a switch. (Arguably -f was the wrong option to use for that in the first place, but it''s a bit late for that now.) -George.
Ian Campbell
2013-Jul-17 11:12 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
On Wed, 2013-07-17 at 12:03 +0100, George Dunlap wrote:> On 17/07/13 11:56, Ian Jackson wrote: > > Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."): > >> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote: > >>> This could also be called ''seatbelt'' option. > >>> > >>> libxl has a variety of checks where it will fail out an operation > >>> unless the user has provided an --force (or --ignore) parameter. > >>> Currently one such check is for the ''vcpu-set'' command which > >>> will error out if the count of virtual cpus is greater than the > >>> physical cpus. This parameter will ignore such checks and allow > >>> the user to do the operations without the need for override flags. > >> Does this overlap somewhat with various commands which individually > >> take a -f(orce) option? > > Clearly it should disable all of those -f''s too. > > > >>> +=item B<expert_mode=BOOLEAN> > >>> + > >>> +Do not act on host performed checks that might lead to performance > >>> +degradations. Currently checks are made for following operations: > >>> + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the > >>> + physical count the operation will error out. > > I don''t think this is a very coherent specification. Surely it should > > override "all -f options" or something similar. > > I think that makes sense for some but not all. For example, for "xl > shutdown", -f means "send an ACPI event if no PV drivers are detected". > I don''t think we''d want to change that behavior with such a switch. > (Arguably -f was the wrong option to use for that in the first place, > but it''s a bit late for that now.)That''s -F (for fallback not force), deliberately avoiding -f for the reasons you might think. If "xl shutdown -f" were to do anything it would probably be to act as "xl destroy" (nb I don''t think we should actually do that...) Ian.
George Dunlap
2013-Jul-17 11:13 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
On 17/07/13 12:12, Ian Campbell wrote:> On Wed, 2013-07-17 at 12:03 +0100, George Dunlap wrote: >> On 17/07/13 11:56, Ian Jackson wrote: >>> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."): >>>> On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote: >>>>> This could also be called ''seatbelt'' option. >>>>> >>>>> libxl has a variety of checks where it will fail out an operation >>>>> unless the user has provided an --force (or --ignore) parameter. >>>>> Currently one such check is for the ''vcpu-set'' command which >>>>> will error out if the count of virtual cpus is greater than the >>>>> physical cpus. This parameter will ignore such checks and allow >>>>> the user to do the operations without the need for override flags. >>>> Does this overlap somewhat with various commands which individually >>>> take a -f(orce) option? >>> Clearly it should disable all of those -f''s too. >>> >>>>> +=item B<expert_mode=BOOLEAN> >>>>> + >>>>> +Do not act on host performed checks that might lead to performance >>>>> +degradations. Currently checks are made for following operations: >>>>> + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the >>>>> + physical count the operation will error out. >>> I don''t think this is a very coherent specification. Surely it should >>> override "all -f options" or something similar. >> I think that makes sense for some but not all. For example, for "xl >> shutdown", -f means "send an ACPI event if no PV drivers are detected". >> I don''t think we''d want to change that behavior with such a switch. >> (Arguably -f was the wrong option to use for that in the first place, >> but it''s a bit late for that now.) > That''s -F (for fallback not force), deliberately avoiding -f for the > reasons you might think. > > If "xl shutdown -f" were to do anything it would probably be to act as > "xl destroy" (nb I don''t think we should actually do that...)Ah, right. That makes sense. -G
Konrad Rzeszutek Wilk
2013-Jul-17 17:17 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
On Wed, Jul 17, 2013 at 11:56:24AM +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."): > > On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote: > > > This could also be called ''seatbelt'' option. > > > > > > libxl has a variety of checks where it will fail out an operation > > > unless the user has provided an --force (or --ignore) parameter. > > > Currently one such check is for the ''vcpu-set'' command which > > > will error out if the count of virtual cpus is greater than the > > > physical cpus. This parameter will ignore such checks and allow > > > the user to do the operations without the need for override flags. > > > > Does this overlap somewhat with various commands which individually > > take a -f(orce) option? > > Clearly it should disable all of those -f''s too. > > > > +=item B<expert_mode=BOOLEAN> > > > + > > > +Do not act on host performed checks that might lead to performance > > > +degradations. Currently checks are made for following operations: > > > + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the > > > + physical count the operation will error out. > > I don''t think this is a very coherent specification. Surely it should > override "all -f options" or something similar.Sure, I hadn''t looked at the other ones.> > I still don''t see why you would want such a thing.We discussed it during Xen 4.3 release that certain changes, like this one are inconsistent (for example you can launch an guest with more vCPUs than pCPU, but you if you lower the amount of vCPUs you can''t increase it until you use -f flag). But you could consider such inconsistent behavior to be a failsafe mechanism so that the user does not do something silly. But if they are an expert... well
George Dunlap
2013-Jul-18 11:08 UTC
Re: [PATCH 2/2] expert_mode: Add a new configuration option for expert users.
On Wed, Jul 17, 2013 at 6:17 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:> On Wed, Jul 17, 2013 at 11:56:24AM +0100, Ian Jackson wrote: >> Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] expert_mode: Add a new configuration option for expert users."): >> > On Fri, 2013-07-12 at 14:08 -0400, Konrad Rzeszutek Wilk wrote: >> > > This could also be called ''seatbelt'' option. >> > > >> > > libxl has a variety of checks where it will fail out an operation >> > > unless the user has provided an --force (or --ignore) parameter. >> > > Currently one such check is for the ''vcpu-set'' command which >> > > will error out if the count of virtual cpus is greater than the >> > > physical cpus. This parameter will ignore such checks and allow >> > > the user to do the operations without the need for override flags. >> > >> > Does this overlap somewhat with various commands which individually >> > take a -f(orce) option? >> >> Clearly it should disable all of those -f''s too. >> >> > > +=item B<expert_mode=BOOLEAN> >> > > + >> > > +Do not act on host performed checks that might lead to performance >> > > +degradations. Currently checks are made for following operations: >> > > + - C<vcpu-set> - if the number of VCPUs set for a guest is higher than the >> > > + physical count the operation will error out. >> >> I don''t think this is a very coherent specification. Surely it should >> override "all -f options" or something similar. > > Sure, I hadn''t looked at the other ones. >> >> I still don''t see why you would want such a thing. > > We discussed it during Xen 4.3 release that certain changes, like this one > are inconsistent (for example you can launch an guest with more vCPUs > than pCPU, but you if you lower the amount of vCPUs you can''t increase > it until you use -f flag). > > But you could consider such inconsistent behavior to be a failsafe > mechanism so that the user does not do something silly. But if they > are an expert... wellIs it really such a burden for "experts" to type ''-f'' occasionally when doing strange configurations? The problem with this in general is one of unpredictability. You may want to disable this check by default, but do you really want to disable *all* checks? That doesn''t sound like a good idea... but then if not, which checks *are* disabled? It''s not clear from the name, and it''s likely that we''ll want to add or remove things, which means having safety checks suddenly disappear... it''s a UI nightmare. I suppose I could accept a "force_all" option. You''d have to be basically daft to use it outside a testing environment, but it would certainly be simple to define. I still mostly think typing ''-f'' is a better option. -George