Hi,
Here is next version of libxl/xl memory management patches:
- xl: Allow user to configure xl mem-set behavior,
- libxl: Allow a guest to inform a host about its
maximum supported memory size,
- libxl: Some optimizations in libxl_set_memory_target(),
- xl: Improve xl documentation in regards to guest memory management.
Shortly I will send relevent Linux Kernel patches as a reference.
Daniel
Daniel Kiper
2013-Apr-29 11:09 UTC
[PATCH v4 1/4] xl: Allow user to configure xl mem-set behavior
Add mem_set_enforce_limit option to xl.conf file and equivalent xl
command line option. Those two things give a chance to align xl mem-set
behavior to xm mem-set behavior. Default xl mem-set behavior is not
changed.
v4 - suggestions/fixes:
- add limit check for mem_set_enforce_limit=0 case,
- fix some error messages.
v3 - suggestions/fixes:
- add xl command line option
(suggested by Ian Jackson).
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
tools/libxl/libxl.c | 25 ++++++++++++++++++-------
tools/libxl/xl.c | 4 ++++
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 16 ++++++++++------
tools/libxl/xl_cmdtable.c | 3 ++-
5 files changed, 35 insertions(+), 14 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 30302c7..9906224 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3517,7 +3517,7 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid,
uint32_t max_memkb)
}
if (max_memkb < memorykb) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "memory_static_max must be
greater than or or equal to memory_dynamic_max\n");
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "memory_xen_max must be
greater than or equal to memory_dynamic_max\n");
goto out;
}
rc = xc_domain_setmaxmem(ctx->xch, domid, max_memkb +
LIBXL_MAXMEM_CONSTANT);
@@ -3678,6 +3678,16 @@ retry_transaction:
goto out;
}
}
+
+ rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
+
+ if (rc != 1 || info.domain != domid) {
+ abort_transaction = 1;
+ goto out;
+ }
+
+ xcinfo2xlinfo(&info, &ptr);
+
memmax = libxl__xs_read(gc, t, libxl__sprintf(gc,
"%s/memory/static-max", dompath));
if (!memmax) {
@@ -3733,6 +3743,12 @@ retry_transaction:
abort_transaction = 1;
goto out;
}
+ } else if (new_target_memkb > ptr.max_memkb - LIBXL_MAXMEM_CONSTANT) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "memory_dynamic_max must be less than or equal to"
+ " memory_xen_max\n");
+ abort_transaction = 1;
+ goto out;
}
new_target_memkb -= videoram;
@@ -3749,12 +3765,7 @@ retry_transaction:
libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
dompath), "%"PRIu32, new_target_memkb);
- rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
- if (rc != 1 || info.domain != domid) {
- abort_transaction = 1;
- goto out;
- }
- xcinfo2xlinfo(&info, &ptr);
+
uuid = libxl__uuid2string(gc, ptr.uuid);
libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
"%"PRIu32, new_target_memkb / 1024);
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 3c141bf..2023230 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -39,6 +39,7 @@ xentoollog_logger_stdiostream *logger;
int dryrun_only;
int force_execution;
int autoballoon = -1;
+int mem_set_enforce_limit = 1;
char *blkdev_start;
int run_hotplug_scripts = 1;
char *lockfile;
@@ -107,6 +108,9 @@ static void parse_global_config(const char *configfile,
if (autoballoon == -1)
autoballoon = auto_autoballoon();
+ if (!xlu_cfg_get_long (config, "mem_set_enforce_limit", &l,
0))
+ mem_set_enforce_limit = l;
+
if (!xlu_cfg_get_long (config, "run_hotplug_scripts", &l, 0))
run_hotplug_scripts = l;
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 771b4af..2281535 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -144,6 +144,7 @@ int xl_child_pid(xlchildnum); /* returns 0 if child struct
is not in use */
/* global options */
extern int autoballoon;
+extern int mem_set_enforce_limit;
extern int run_hotplug_scripts;
extern int dryrun_only;
extern libxl_defbool claim_mode;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c1a969b..76799fc 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2475,7 +2475,7 @@ int main_memmax(int argc, char **argv)
return 0;
}
-static void set_memory_target(uint32_t domid, const char *mem)
+static void set_memory_target(uint32_t domid, const char *mem, int
enforce_limit)
{
long long int memorykb;
@@ -2485,23 +2485,27 @@ static void set_memory_target(uint32_t domid, const char
*mem)
exit(3);
}
- libxl_set_memory_target(ctx, domid, memorykb, 0, /* enforce */ 1);
+ libxl_set_memory_target(ctx, domid, memorykb, 0, enforce_limit);
}
int main_memset(int argc, char **argv)
{
uint32_t domid;
- int opt = 0;
+ int enforce_limit, opt = 0;
const char *mem;
- SWITCH_FOREACH_OPT(opt, "", NULL, "mem-set", 2) {
- /* No options */
+ enforce_limit = mem_set_enforce_limit;
+
+ SWITCH_FOREACH_OPT(opt, "e:", NULL, "mem-set", 2) {
+ case ''e'':
+ enforce_limit = atoi(optarg) ? 1 : 0;
+ break;
}
domid = find_domain(argv[optind]);
mem = argv[optind + 1];
- set_memory_target(domid, mem);
+ set_memory_target(domid, mem, enforce_limit);
return 0;
}
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 347302c..8918a57 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -196,7 +196,8 @@ struct cmd_spec cmd_table[] = {
{ "mem-set",
&main_memset, 0, 1,
"Set the current memory usage for a domain",
- "<Domain>
<MemMB[''b''[bytes]|''k''[KB]|''m''[MB]|''g''[GB]|''t''[TB]]>",
+ "[-e <0|1>] <Domain>
<MemMB[''b''[bytes]|''k''[KB]|''m''[MB]|''g''[GB]|''t''[TB]]>",
+ "-e <0|1> (Not)Enforce maximum memory allocation for a
domain"
},
{ "button-press",
&main_button_press, 0, 1,
--
1.7.10.4
Daniel Kiper
2013-Apr-29 11:09 UTC
[PATCH v4 2/4] libxl: Allow a guest to inform a host about its maximum supported memory size
Allow a guest to inform a host about its maximum
supported memory size. This gives a chance to ignore
limit established at boot by maxmem and usage of memory
hotplug infrustacture if the guest OS supports it.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
tools/libxl/libxl.c | 58 +++++++++++++++++++++++++++++++-------------
tools/libxl/libxl_create.c | 3 +++
2 files changed, 44 insertions(+), 17 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9906224..3427c13 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3640,7 +3640,7 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t
domid,
{
GC_INIT(ctx);
int rc = 1, abort_transaction = 0;
- uint32_t memorykb = 0, videoram = 0;
+ uint32_t guestkb, memorykb = 0, videoram = 0;
uint32_t current_target_memkb = 0, new_target_memkb = 0;
char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
char *dompath = libxl__xs_get_dompath(gc, domid);
@@ -3689,21 +3689,36 @@ retry_transaction:
xcinfo2xlinfo(&info, &ptr);
memmax = libxl__xs_read(gc, t, libxl__sprintf(gc,
- "%s/memory/static-max", dompath));
- if (!memmax) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
- "cannot get memory info from %s/memory/static-max\n",
- dompath);
- abort_transaction = 1;
- goto out;
- }
- memorykb = strtoul(memmax, &endptr, 10);
- if (*endptr != ''\0'') {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
- "invalid max memory %s from %s/memory/static-max\n",
- memmax, dompath);
- abort_transaction = 1;
- goto out;
+ "%s/memory/guest-max", dompath));
+
+ if (memmax) {
+ guestkb = strtoul(memmax, &endptr, 10);
+ if (*endptr != ''\0'')
+ guestkb = 0;
+ } else
+ guestkb = 0;
+
+ if (!guestkb) {
+ memmax = libxl__xs_read(gc, t, libxl__sprintf(gc,
+ "%s/memory/static-max", dompath));
+
+ if (!memmax) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "cannot get memory info from
%s/memory/static-max\n",
+ dompath);
+ abort_transaction = 1;
+ goto out;
+ }
+
+ memorykb = strtoul(memmax, &endptr, 10);
+
+ if (*endptr != ''\0'') {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "invalid max memory %s from
%s/memory/static-max\n",
+ memmax, dompath);
+ abort_transaction = 1;
+ goto out;
+ }
}
if (relative) {
@@ -3713,7 +3728,16 @@ retry_transaction:
new_target_memkb = current_target_memkb + target_memkb;
} else
new_target_memkb = target_memkb;
- if (new_target_memkb > memorykb) {
+
+ if (guestkb) {
+ if (new_target_memkb > guestkb) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "memory_dynamic_max must be less than or equal
to"
+ " memory_guest_max\n");
+ abort_transaction = 1;
+ goto out;
+ }
+ } else if (new_target_memkb > memorykb) {
LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
"memory_dynamic_max must be less than or equal to"
" memory_static_max\n");
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index cb9c822..f57cbe9 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -512,6 +512,9 @@ retry_transaction:
libxl__sprintf(gc,
"%s/device/suspend/event-channel", dom_path),
rwperm, ARRAY_SIZE(rwperm));
libxl__xs_mkdir(gc, t,
+ libxl__sprintf(gc, "%s/memory/guest-max",
dom_path),
+ rwperm, ARRAY_SIZE(rwperm));
+ libxl__xs_mkdir(gc, t,
libxl__sprintf(gc, "%s/data", dom_path),
rwperm, ARRAY_SIZE(rwperm));
if (info->type == LIBXL_DOMAIN_TYPE_HVM)
--
1.7.10.4
Daniel Kiper
2013-Apr-29 11:09 UTC
[PATCH v4 3/4] libxl: Some optimizations in libxl_set_memory_target()
Some optimizations in libxl_set_memory_target().
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
tools/libxl/libxl.c | 48 ++++++++++++++++++++++--------------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3427c13..683b700 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3640,21 +3640,18 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t
domid,
{
GC_INIT(ctx);
int rc = 1, abort_transaction = 0;
- uint32_t guestkb, memorykb = 0, videoram = 0;
- uint32_t current_target_memkb = 0, new_target_memkb = 0;
- char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
- char *dompath = libxl__xs_get_dompath(gc, domid);
+ uint32_t current_target_memkb, guestkb, memorykb = 0, new_target_memkb;
+ char *dompath = libxl__xs_get_dompath(gc, domid), *endptr, *s;
xc_domaininfo_t info;
libxl_dominfo ptr;
- char *uuid;
xs_transaction_t t;
retry_transaction:
t = xs_transaction_start(ctx->xsh);
- target = libxl__xs_read(gc, t, libxl__sprintf(gc,
+ s = libxl__xs_read(gc, t, libxl__sprintf(gc,
"%s/memory/target", dompath));
- if (!target && !domid) {
+ if (!s && !domid) {
xs_transaction_end(ctx->xsh, t, 1);
rc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb);
if (rc < 0) {
@@ -3662,18 +3659,18 @@ retry_transaction:
goto out;
}
goto retry_transaction;
- } else if (!target) {
+ } else if (!s) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
"cannot get target memory info from
%s/memory/target\n",
dompath);
abort_transaction = 1;
goto out;
} else {
- current_target_memkb = strtoul(target, &endptr, 10);
+ current_target_memkb = strtoul(s, &endptr, 10);
if (*endptr != ''\0'') {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
"invalid memory target %s from
%s/memory/target\n",
- target, dompath);
+ s, dompath);
abort_transaction = 1;
goto out;
}
@@ -3688,21 +3685,21 @@ retry_transaction:
xcinfo2xlinfo(&info, &ptr);
- memmax = libxl__xs_read(gc, t, libxl__sprintf(gc,
+ s = libxl__xs_read(gc, t, libxl__sprintf(gc,
"%s/memory/guest-max", dompath));
- if (memmax) {
- guestkb = strtoul(memmax, &endptr, 10);
+ if (s) {
+ guestkb = strtoul(s, &endptr, 10);
if (*endptr != ''\0'')
guestkb = 0;
} else
guestkb = 0;
if (!guestkb) {
- memmax = libxl__xs_read(gc, t, libxl__sprintf(gc,
+ s = libxl__xs_read(gc, t, libxl__sprintf(gc,
"%s/memory/static-max", dompath));
- if (!memmax) {
+ if (!s) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
"cannot get memory info from
%s/memory/static-max\n",
dompath);
@@ -3710,12 +3707,12 @@ retry_transaction:
goto out;
}
- memorykb = strtoul(memmax, &endptr, 10);
+ memorykb = strtoul(s, &endptr, 10);
if (*endptr != ''\0'') {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
"invalid max memory %s from
%s/memory/static-max\n",
- memmax, dompath);
+ s, dompath);
abort_transaction = 1;
goto out;
}
@@ -3752,18 +3749,14 @@ retry_transaction:
abort_transaction = 1;
goto out;
}
- videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc,
- "%s/memory/videoram", dompath));
- videoram = videoram_s ? atoi(videoram_s) : 0;
if (enforce) {
- memorykb = new_target_memkb;
- rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
+ rc = xc_domain_setmaxmem(ctx->xch, domid, new_target_memkb +
LIBXL_MAXMEM_CONSTANT);
if (rc != 0) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
"xc_domain_setmaxmem domid=%d memkb=%d failed "
- "rc=%d\n", domid, memorykb +
LIBXL_MAXMEM_CONSTANT, rc);
+ "rc=%d\n", domid, new_target_memkb +
LIBXL_MAXMEM_CONSTANT, rc);
abort_transaction = 1;
goto out;
}
@@ -3775,7 +3768,10 @@ retry_transaction:
goto out;
}
- new_target_memkb -= videoram;
+ s = libxl__xs_read(gc, t, libxl__sprintf(gc,
+ "%s/memory/videoram", dompath));
+ new_target_memkb -= s ? atoi(s) : 0;
+
rc = xc_domain_set_pod_target(ctx->xch, domid,
new_target_memkb / 4, NULL, NULL, NULL);
if (rc != 0) {
@@ -3790,8 +3786,8 @@ retry_transaction:
libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
dompath), "%"PRIu32, new_target_memkb);
- uuid = libxl__uuid2string(gc, ptr.uuid);
- libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
+ s = libxl__uuid2string(gc, ptr.uuid);
+ libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", s),
"%"PRIu32, new_target_memkb / 1024);
out:
--
1.7.10.4
Daniel Kiper
2013-Apr-29 11:09 UTC
[PATCH v4 4/4] xl: Improve xl documentation in regards to guest memory management
Improve xl documentation in regards to guest memory management.
Some improvements are suggested by Ian Jackson and Konrad Rzeszutek Wilk.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
docs/man/xl.conf.pod.5 | 15 +++++
docs/misc/libxl_memory.txt | 124 +++++++++++++++++++++----------------
docs/misc/xenstore-paths.markdown | 22 ++++---
tools/examples/xl.conf | 4 ++
4 files changed, 105 insertions(+), 60 deletions(-)
diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 1229c8a..658b1eb 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -62,6 +62,21 @@ of memory given to domain 0 by default.
Default: C<"auto">
+=item B<mem_set_enforce_limit=BOOLEAN>
+
+If enabled, then C<xl mem-set> by default will set a memory allocation
+target and enforce the maximum memory allocation for each domain.
+If disabled, then C<xl mem-set> by default will set only a memory
allocation
+target for each domain.
+
+The memory allocation target is a suggested amount of memory which
+should be allocated by a guest. It is nice when the guest follows this
+value but it may have its own idea about memory requirments. However,
+it could not allocate more memory than a ceiling known as the maximum
+memory allocation.
+
+Default: C<1>
+
=item B<run_hotplug_scripts=BOOLEAN>
If disabled hotplug scripts will be called from udev, as it used to
diff --git a/docs/misc/libxl_memory.txt b/docs/misc/libxl_memory.txt
index 253476d..b89214a 100644
--- a/docs/misc/libxl_memory.txt
+++ b/docs/misc/libxl_memory.txt
@@ -1,70 +1,90 @@
/* === Domain memory breakdown: HVM guests =================================-
- + +----------+ +
- | | shadow | |
- | +----------+ |
- overhead | | extra | |
- | | external | |
- | +----------+ + |
- | | extra | | |
- | | internal | | |
- + +----------+ + | | footprint
- | | video | | | |
- | +----------+ + + | | xen |
- | | | | | | actual | maximum |
- | | | | | | target | |
- | | guest | | | build | | |
- | | | | | start | | |
- static | | | | | | | |
- maximum | +----------+ | + + + +
- | | | |
- | | | |
- | | balloon | | build
- | | | | maximum
- | | | |
- + +----------+ +
-
-
+
+ + +----------+ +
+ | | shadow | |
+ | +----------+ |
+ overhead | | extra | |
+ | | external | |
+ | +----------+ + |
+ | | extra | | |
+ | | internal | | |
+ + +----------+ + + | |
footprint
+ | | video | | | | |
+ | +----------+ | + + | | xen |
+ | | | | | | | actual | maximum |
+ | | | | | | | target | |
+ | | guest | | | | build | | |
+ | | | | | | start | | |
+ | | | | | | | | |
+ | +----------+ | | + + + +
+ | | | | |
+ | | | | |
+ guest | | | | | build
+ maximum | | | | | maximum
+ | | balloon | | |
+ | | | | |
+ | | | | static |
+ | | | | maximum |
+ | | | | |
+ | +----------+ + +
+ | | |
+ | | memory |
+ | | hotplug |
+ | | |
+ + +----------+
+
+
extra internal = LIBXL_MAXMEM_CONSTANT
extra external = LIBXL_HVM_EXTRA_MEMORY
shadow = libxl_domain_build_info.shadow_memkb
static maximum = libxl_domain_build_info.max_memkb
video = libxl_domain_build_info.video_memkb
build start = libxl_domain_build_info.target_memkb
+ guest maximum = declared by a guest its maximum supported memory size
libxl_domain_setmaxmem -> xen maximum
- libxl_set_memory_target -> actual target
-
-
+ libxl_set_memory_target -> actual target & optionaly xen maximum
+
+
=== Domain memory breakdown: PV guests =================================-
-
- + +----------+ +
- overhead | | extra | |
- | | external | |
- | +----------+ + |
- | | extra | | |
- | | internal | | |
- + +----------+ + + + | | footprint
- | | | | | | | xen |
- | | | | | | actual | maximum |
- | | guest | | | build | target | |
- | | | | | start | | |
- static | | | | | | | |
- maximum | +----------+ | + + + +
- | | | |
- | | | |
- | | balloon | | build
- | | | | maximum
- | | | |
- + +----------+ +
-
+
+
+ + +----------+ +
+ overhead | | extra | |
+ | | external | |
+ | +----------+ + |
+ | | extra | | |
+ | | internal | | |
+ + +----------+ + + + + | |
footprint
+ | | | | | | | | xen |
+ | | | | | | | actual | maximum |
+ | | guest | | | | build | target | |
+ | | | | | | start | | |
+ | | | | | | | | |
+ | +----------+ | | + + + +
+ | | | | |
+ | | | | |
+ | | | | | build
+ guest | | | | | maximum
+ maximum | | balloon | | |
+ | | | | |
+ | | | | static |
+ | | | | maximum |
+ | | | | |
+ | +----------+ + +
+ | | |
+ | | memory |
+ | | hotplug |
+ | | |
+ + +----------+
+
extra internal = LIBXL_MAXMEM_CONSTANT
extra external = LIBXL_PV_EXTRA_MEMORY
static maximum = libxl_domain_build_info.max_memkb
build start = libxl_domain_build_info.target_memkb
+ guest maximum = declared by a guest its maximum supported memory size
libxl_domain_setmaxmem -> xen maximum
- libxl_set_memory_target -> actual target
+ libxl_set_memory_target -> actual target & optionaly xen maximum
========================================================================= */
diff --git a/docs/misc/xenstore-paths.markdown
b/docs/misc/xenstore-paths.markdown
index 535830e..1ac2159 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -124,18 +124,24 @@ This protocol is not currently well documented.
#### ~/memory/static-max = MEMKB []
-Specifies a static maximum amount memory which this domain should
-expect to be given. In the absence of in-guest memory hotplug support
-this set on domain boot and is usually the maximum amount of RAM which
-a guest can make use of. See [docs/misc/libxl_memory.txt][LIBXLMEM]
-for a description of how memory is accounted for in toolstacks using
-the libxl library.
+Specifies a static maximum amount of memory which this domain
+should expect to be given. It is usually the maximum amount
+of RAM which a guest can make use of. This value is set on the
+domain boot. If ~/memory/guest-max is set then static maximum
+is ignored by libxl. See [docs/misc/libxl_memory.txt][LIBXLMEM]
+for a description of how the memory is accounted for in toolstacks
+using the libxl library.
+
+#### ~/memory/guest-max = ""|MEMKB [w]
+
+Declared by a guest its maximum supported memory size. Valid if numeric.
#### ~/memory/target = MEMKB []
The current balloon target for the domain. The balloon driver within
-the guest is expected to make every effort to every effort use no more
-than this amount of RAM.
+the guest is expected to make every effort to use no more than this
+amount of RAM. Guests whose actual allocation exceeds the target may
+experience memory allocation failures.
#### ~/memory/videoram = MEMKB [HVM,INTERNAL]
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 9402c3f..9dbb1be 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -5,6 +5,10 @@
# starts with all the host''s memory.
#autoballoon="auto"
+# xl mem-set by default will set a memory allocation target and enforce
+# the maximum memory allocation for each domain
+#mem_set_enforce_limit=1
+
# full path of the lockfile used by xl during domain creation
#lockfile="/var/lock/xl"
--
1.7.10.4
Ian Campbell
2013-Apr-29 11:16 UTC
Re: [PATCH v4 3/4] libxl: Some optimizations in libxl_set_memory_target()
On Mon, 2013-04-29 at 12:09 +0100, Daniel Kiper wrote:> Some optimizations in libxl_set_memory_target().Please describe them in the commit message. At first glance it just looks like you are renaming some variables from "memmax"/"target"/"uuid" to "s", why? Just to reduce the number of local variables? They seem more meaningful and easier to read with their current names and if you are "optimising" the use of local variable then that seems like a false optimisation to me -- any modern compiler can surely work out that the lifetimes of these various things do not overlap and do the sensible thing WRT register allocation. At the very least this renaming is hiding what you are actually doing.> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> > --- > tools/libxl/libxl.c | 48 ++++++++++++++++++++++-------------------------- > 1 file changed, 22 insertions(+), 26 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 3427c13..683b700 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -3640,21 +3640,18 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, > { > GC_INIT(ctx); > int rc = 1, abort_transaction = 0; > - uint32_t guestkb, memorykb = 0, videoram = 0; > - uint32_t current_target_memkb = 0, new_target_memkb = 0; > - char *memmax, *endptr, *videoram_s = NULL, *target = NULL; > - char *dompath = libxl__xs_get_dompath(gc, domid); > + uint32_t current_target_memkb, guestkb, memorykb = 0, new_target_memkb; > + char *dompath = libxl__xs_get_dompath(gc, domid), *endptr, *s; > xc_domaininfo_t info; > libxl_dominfo ptr; > - char *uuid; > xs_transaction_t t; > > retry_transaction: > t = xs_transaction_start(ctx->xsh); > > - target = libxl__xs_read(gc, t, libxl__sprintf(gc, > + s = libxl__xs_read(gc, t, libxl__sprintf(gc, > "%s/memory/target", dompath)); > - if (!target && !domid) { > + if (!s && !domid) { > xs_transaction_end(ctx->xsh, t, 1); > rc = libxl__fill_dom0_memory_info(gc, ¤t_target_memkb); > if (rc < 0) { > @@ -3662,18 +3659,18 @@ retry_transaction: > goto out; > } > goto retry_transaction; > - } else if (!target) { > + } else if (!s) { > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > "cannot get target memory info from %s/memory/target\n", > dompath); > abort_transaction = 1; > goto out; > } else { > - current_target_memkb = strtoul(target, &endptr, 10); > + current_target_memkb = strtoul(s, &endptr, 10); > if (*endptr != ''\0'') { > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > "invalid memory target %s from %s/memory/target\n", > - target, dompath); > + s, dompath); > abort_transaction = 1; > goto out; > } > @@ -3688,21 +3685,21 @@ retry_transaction: > > xcinfo2xlinfo(&info, &ptr); > > - memmax = libxl__xs_read(gc, t, libxl__sprintf(gc, > + s = libxl__xs_read(gc, t, libxl__sprintf(gc, > "%s/memory/guest-max", dompath)); > > - if (memmax) { > - guestkb = strtoul(memmax, &endptr, 10); > + if (s) { > + guestkb = strtoul(s, &endptr, 10); > if (*endptr != ''\0'') > guestkb = 0; > } else > guestkb = 0; > > if (!guestkb) { > - memmax = libxl__xs_read(gc, t, libxl__sprintf(gc, > + s = libxl__xs_read(gc, t, libxl__sprintf(gc, > "%s/memory/static-max", dompath)); > > - if (!memmax) { > + if (!s) { > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > "cannot get memory info from %s/memory/static-max\n", > dompath); > @@ -3710,12 +3707,12 @@ retry_transaction: > goto out; > } > > - memorykb = strtoul(memmax, &endptr, 10); > + memorykb = strtoul(s, &endptr, 10); > > if (*endptr != ''\0'') { > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > "invalid max memory %s from %s/memory/static-max\n", > - memmax, dompath); > + s, dompath); > abort_transaction = 1; > goto out; > } > @@ -3752,18 +3749,14 @@ retry_transaction: > abort_transaction = 1; > goto out; > } > - videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc, > - "%s/memory/videoram", dompath)); > - videoram = videoram_s ? atoi(videoram_s) : 0; > > if (enforce) { > - memorykb = new_target_memkb; > - rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb + > + rc = xc_domain_setmaxmem(ctx->xch, domid, new_target_memkb + > LIBXL_MAXMEM_CONSTANT); > if (rc != 0) { > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > "xc_domain_setmaxmem domid=%d memkb=%d failed " > - "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc); > + "rc=%d\n", domid, new_target_memkb + LIBXL_MAXMEM_CONSTANT, rc); > abort_transaction = 1; > goto out; > } > @@ -3775,7 +3768,10 @@ retry_transaction: > goto out; > } > > - new_target_memkb -= videoram; > + s = libxl__xs_read(gc, t, libxl__sprintf(gc, > + "%s/memory/videoram", dompath)); > + new_target_memkb -= s ? atoi(s) : 0; > + > rc = xc_domain_set_pod_target(ctx->xch, domid, > new_target_memkb / 4, NULL, NULL, NULL); > if (rc != 0) { > @@ -3790,8 +3786,8 @@ retry_transaction: > libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target", > dompath), "%"PRIu32, new_target_memkb); > > - uuid = libxl__uuid2string(gc, ptr.uuid); > - libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid), > + s = libxl__uuid2string(gc, ptr.uuid); > + libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", s), > "%"PRIu32, new_target_memkb / 1024); > > out:
Ian Campbell
2013-Apr-29 14:32 UTC
Re: [PATCH v4 4/4] xl: Improve xl documentation in regards to guest memory management
On Mon, 2013-04-29 at 12:09 +0100, Daniel Kiper wrote:> Improve xl documentation in regards to guest memory management.Thank you for remembering docs! Could I ask that where you are documenting new options, xenstore paths or xl commands etc you do it in the patch which makes the code change. I''m thinking specifically of the addition of mem_set_enforce_limit and the new xenstore paths. I''m not sure if the libxl_memory.txt changes corresponds to a code change or not.> Some improvements are suggested by Ian Jackson and Konrad Rzeszutek Wilk. > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> > --- > docs/man/xl.conf.pod.5 | 15 +++++ > docs/misc/libxl_memory.txt | 124 +++++++++++++++++++++---------------- > docs/misc/xenstore-paths.markdown | 22 ++++--- > tools/examples/xl.conf | 4 ++ > 4 files changed, 105 insertions(+), 60 deletions(-) > > diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 > index 1229c8a..658b1eb 100644 > --- a/docs/man/xl.conf.pod.5 > +++ b/docs/man/xl.conf.pod.5 > @@ -62,6 +62,21 @@ of memory given to domain 0 by default. > > Default: C<"auto"> > > +=item B<mem_set_enforce_limit=BOOLEAN> > + > +If enabled, then C<xl mem-set> by default will set a memory allocation > +target and enforce the maximum memory allocation for each domain. > +If disabled, then C<xl mem-set> by default will set only a memory allocation > +target for each domain. > + > +The memory allocation target is a suggested amount of memory which > +should be allocated by a guest. It is nice when the guest follows this > +value but it may have its own idea about memory requirments. However,requirements.> +it could not allocate more memory than a ceiling known as the maximum > +memory allocation. > + > +Default: C<1> > + > =item B<run_hotplug_scripts=BOOLEAN> > > If disabled hotplug scripts will be called from udev, as it used to > diff --git a/docs/misc/libxl_memory.txt b/docs/misc/libxl_memory.txt > index 253476d..b89214a 100644 > --- a/docs/misc/libxl_memory.txt > +++ b/docs/misc/libxl_memory.txt > @@ -1,70 +1,90 @@ > /* === Domain memory breakdown: HVM guests =================================> - > - + +----------+ + > - | | shadow | | > - | +----------+ | > - overhead | | extra | | > - | | external | | > - | +----------+ + | > - | | extra | | | > - | | internal | | | > - + +----------+ + | | footprint > - | | video | | | | > - | +----------+ + + | | xen | > - | | | | | | actual | maximum | > - | | | | | | target | | > - | | guest | | | build | | | > - | | | | | start | | | > - static | | | | | | | | > - maximum | +----------+ | + + + + > - | | | | > - | | | | > - | | balloon | | build > - | | | | maximum > - | | | | > - + +----------+ + > - > - > + > + + +----------+ + > + | | shadow | | > + | +----------+ | > + overhead | | extra | | > + | | external | | > + | +----------+ + | > + | | extra | | | > + | | internal | | | > + + +----------+ + + | | footprint > + | | video | | | | | > + | +----------+ | + + | | xen | > + | | | | | | | actual | maximum | > + | | | | | | | target | | > + | | guest | | | | build | | | > + | | | | | | start | | | > + | | | | | | | | | > + | +----------+ | | + + + + > + | | | | | > + | | | | | > + guest | | | | | build > + maximum | | | | | maximum > + | | balloon | | | > + | | | | | > + | | | | static | > + | | | | maximum | > + | | | | | > + | +----------+ + + > + | | | > + | | memory | > + | | hotplug | > + | | | > + + +----------+Hard to read a diff of a picture, do I understand correctly that you have just added guest-maximum and moved static maximum over to the other side? How about, on the left (aligned as appropriate with the existing boxes etc): overhead + | + + | | | static | | maximum| | | guest | + maximum| | | memory | | hotplug| | | + + ?> + > + > extra internal = LIBXL_MAXMEM_CONSTANT > extra external = LIBXL_HVM_EXTRA_MEMORY > shadow = libxl_domain_build_info.shadow_memkb > static maximum = libxl_domain_build_info.max_memkb > video = libxl_domain_build_info.video_memkb > build start = libxl_domain_build_info.target_memkb > + guest maximum = declared by a guest its maximum supported memory size^comma here I think? Or "as"> libxl_domain_setmaxmem -> xen maximum > - libxl_set_memory_target -> actual target > - > - > + libxl_set_memory_target -> actual target & optionaly xen maximumoptionally> + > + > === Domain memory breakdown: PV guests =================================> - > - > - + +----------+ + > - overhead | | extra | | > - | | external | | > - | +----------+ + | > - | | extra | | | > - | | internal | | | > - + +----------+ + + + | | footprint > - | | | | | | | xen | > - | | | | | | actual | maximum | > - | | guest | | | build | target | | > - | | | | | start | | | > - static | | | | | | | | > - maximum | +----------+ | + + + + > - | | | | > - | | | | > - | | balloon | | build > - | | | | maximum > - | | | | > - + +----------+ + > - > + > + > + + +----------+ + > + overhead | | extra | | > + | | external | | > + | +----------+ + | > + | | extra | | | > + | | internal | | | > + + +----------+ + + + + | | footprint > + | | | | | | | | xen | > + | | | | | | | actual | maximum | > + | | guest | | | | build | target | | > + | | | | | | start | | | > + | | | | | | | | | > + | +----------+ | | + + + + > + | | | | | > + | | | | | > + | | | | | build > + guest | | | | | maximum > + maximum | | balloon | | | > + | | | | | > + | | | | static | > + | | | | maximum | > + | | | | | > + | +----------+ + + > + | | | > + | | memory | > + | | hotplug | > + | | | > + + +----------+ > + > > extra internal = LIBXL_MAXMEM_CONSTANT > extra external = LIBXL_PV_EXTRA_MEMORY > static maximum = libxl_domain_build_info.max_memkb > build start = libxl_domain_build_info.target_memkb > + guest maximum = declared by a guest its maximum supported memory sizeAnother comma I think.> libxl_domain_setmaxmem -> xen maximum > - libxl_set_memory_target -> actual target > + libxl_set_memory_target -> actual target & optionaly xen maximumoptionally again.> > > ========================================================================= */ > diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown > index 535830e..1ac2159 100644 > --- a/docs/misc/xenstore-paths.markdown > +++ b/docs/misc/xenstore-paths.markdown > @@ -124,18 +124,24 @@ This protocol is not currently well documented. > > #### ~/memory/static-max = MEMKB [] > > -Specifies a static maximum amount memory which this domain should > -expect to be given. In the absence of in-guest memory hotplug support > -this set on domain boot and is usually the maximum amount of RAM which > -a guest can make use of. See [docs/misc/libxl_memory.txt][LIBXLMEM] > -for a description of how memory is accounted for in toolstacks using > -the libxl library. > +Specifies a static maximum amount of memory which this domain > +should expect to be given. It is usually the maximum amount > +of RAM which a guest can make use of. This value is set on the > +domain boot. If ~/memory/guest-max is set then static maximum > +is ignored by libxl. See [docs/misc/libxl_memory.txt][LIBXLMEM] > +for a description of how the memory is accounted for in toolstacks > +using the libxl library.I''d retain the "and in the absence of in-guest..." stuff here but go on to add a sentence lile "If in-guest memory hotplug is supported, then it will write guest-max (see below). The key point is to describe the behaviour with and without hotplug.> + > +#### ~/memory/guest-max = ""|MEMKB [w] > + > +Declared by a guest its maximum supported memory size. Valid if numeric.I think "Valid if numeric" is covered by the specification of MEMKB.> #### ~/memory/target = MEMKB [] > > The current balloon target for the domain. The balloon driver within > -the guest is expected to make every effort to every effort use no more > -than this amount of RAM. > +the guest is expected to make every effort to use no more than this > +amount of RAM. Guests whose actual allocation exceeds the target may > +experience memory allocation failures. > > #### ~/memory/videoram = MEMKB [HVM,INTERNAL] > > diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf > index 9402c3f..9dbb1be 100644 > --- a/tools/examples/xl.conf > +++ b/tools/examples/xl.conf > @@ -5,6 +5,10 @@ > # starts with all the host''s memory. > #autoballoon="auto" > > +# xl mem-set by default will set a memory allocation target and enforce > +# the maximum memory allocation for each domain > +#mem_set_enforce_limit=1 > + > # full path of the lockfile used by xl during domain creation > #lockfile="/var/lock/xl" > > -- > 1.7.10.4 >
Daniel Kiper
2013-Apr-30 12:23 UTC
Re: [PATCH v4 3/4] libxl: Some optimizations in libxl_set_memory_target()
On Mon, Apr 29, 2013 at 12:16:43PM +0100, Ian Campbell wrote:> On Mon, 2013-04-29 at 12:09 +0100, Daniel Kiper wrote: > > Some optimizations in libxl_set_memory_target(). > > Please describe them in the commit message.OK.> At first glance it just looks like you are renaming some variables from > "memmax"/"target"/"uuid" to "s", why? Just to reduce the number of local > variables? > > They seem more meaningful and easier to read with their current names > and if you are "optimising" the use of local variable then that seems > like a false optimisation to me -- any modern compiler can surely work > out that the lifetimes of these various things do not overlap and do the > sensible thing WRT register allocation. > > At the very least this renaming is hiding what you are actually doing.Here are four things: - reduction of number of variables which you found, - removal of unneeded intialization; probably it was needed some time ago but now it is not, - removal of memorykb = new_target_memkb assignment with relevant changes in other places, - merge of video memory size calculation which is currently in two places. I do not insist on first thing but I think that other optimizations are worth to do. If you wish I could do all of them in separate patches. Daniel
Daniel Kiper
2013-Apr-30 12:34 UTC
Re: [PATCH v4 4/4] xl: Improve xl documentation in regards to guest memory management
On Mon, Apr 29, 2013 at 03:32:40PM +0100, Ian Campbell wrote:> On Mon, 2013-04-29 at 12:09 +0100, Daniel Kiper wrote: > > Improve xl documentation in regards to guest memory management. > > Thank you for remembering docs!During Document Day it is a must... ;-)))> Could I ask that where you are documenting new options, xenstore paths > or xl commands etc you do it in the patch which makes the code change. > I''m thinking specifically of the addition of mem_set_enforce_limit andSure...> the new xenstore paths. I''m not sure if the libxl_memory.txt changes > corresponds to a code change or not.Yes, it does.> > Some improvements are suggested by Ian Jackson and Konrad Rzeszutek Wilk. > > > > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> > > --- > > docs/man/xl.conf.pod.5 | 15 +++++ > > docs/misc/libxl_memory.txt | 124 +++++++++++++++++++++---------------- > > docs/misc/xenstore-paths.markdown | 22 ++++--- > > tools/examples/xl.conf | 4 ++ > > 4 files changed, 105 insertions(+), 60 deletions(-) > > > > diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5 > > index 1229c8a..658b1eb 100644 > > --- a/docs/man/xl.conf.pod.5 > > +++ b/docs/man/xl.conf.pod.5 > > @@ -62,6 +62,21 @@ of memory given to domain 0 by default. > > > > Default: C<"auto"> > > > > +=item B<mem_set_enforce_limit=BOOLEAN> > > + > > +If enabled, then C<xl mem-set> by default will set a memory allocation > > +target and enforce the maximum memory allocation for each domain. > > +If disabled, then C<xl mem-set> by default will set only a memory allocation > > +target for each domain. > > + > > +The memory allocation target is a suggested amount of memory which > > +should be allocated by a guest. It is nice when the guest follows this > > +value but it may have its own idea about memory requirments. However, > > requirements. > > > +it could not allocate more memory than a ceiling known as the maximum > > +memory allocation. > > + > > +Default: C<1> > > + > > =item B<run_hotplug_scripts=BOOLEAN> > > > > If disabled hotplug scripts will be called from udev, as it used to > > diff --git a/docs/misc/libxl_memory.txt b/docs/misc/libxl_memory.txt > > index 253476d..b89214a 100644 > > --- a/docs/misc/libxl_memory.txt > > +++ b/docs/misc/libxl_memory.txt > > @@ -1,70 +1,90 @@ > > /* === Domain memory breakdown: HVM guests =================================> > - > > - + +----------+ + > > - | | shadow | | > > - | +----------+ | > > - overhead | | extra | | > > - | | external | | > > - | +----------+ + | > > - | | extra | | | > > - | | internal | | | > > - + +----------+ + | | footprint > > - | | video | | | | > > - | +----------+ + + | | xen | > > - | | | | | | actual | maximum | > > - | | | | | | target | | > > - | | guest | | | build | | | > > - | | | | | start | | | > > - static | | | | | | | | > > - maximum | +----------+ | + + + + > > - | | | | > > - | | | | > > - | | balloon | | build > > - | | | | maximum > > - | | | | > > - + +----------+ + > > - > > - > > + > > + + +----------+ + > > + | | shadow | | > > + | +----------+ | > > + overhead | | extra | | > > + | | external | | > > + | +----------+ + | > > + | | extra | | | > > + | | internal | | | > > + + +----------+ + + | | footprint > > + | | video | | | | | > > + | +----------+ | + + | | xen | > > + | | | | | | | actual | maximum | > > + | | | | | | | target | | > > + | | guest | | | | build | | | > > + | | | | | | start | | | > > + | | | | | | | | | > > + | +----------+ | | + + + + > > + | | | | | > > + | | | | | > > + guest | | | | | build > > + maximum | | | | | maximum > > + | | balloon | | | > > + | | | | | > > + | | | | static | > > + | | | | maximum | > > + | | | | | > > + | +----------+ + + > > + | | | > > + | | memory | > > + | | hotplug | > > + | | | > > + + +----------+ > > Hard to read a diff of a picture, do I understand correctly that you > have just added guest-maximum and moved static maximum over to the other > side?Right.> How about, on the left (aligned as appropriate with the existing boxes > etc): > > overhead + > | > + + > | | > | static | > | maximum| > | | > guest | + > maximum| | > | memory | > | hotplug| > | | > + + > ?OK. [...]> > ========================================================================= */ > > diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown > > index 535830e..1ac2159 100644 > > --- a/docs/misc/xenstore-paths.markdown > > +++ b/docs/misc/xenstore-paths.markdown > > @@ -124,18 +124,24 @@ This protocol is not currently well documented. > > > > #### ~/memory/static-max = MEMKB [] > > > > -Specifies a static maximum amount memory which this domain should > > -expect to be given. In the absence of in-guest memory hotplug support > > -this set on domain boot and is usually the maximum amount of RAM which > > -a guest can make use of. See [docs/misc/libxl_memory.txt][LIBXLMEM] > > -for a description of how memory is accounted for in toolstacks using > > -the libxl library. > > +Specifies a static maximum amount of memory which this domain > > +should expect to be given. It is usually the maximum amount > > +of RAM which a guest can make use of. This value is set on the > > +domain boot. If ~/memory/guest-max is set then static maximum > > +is ignored by libxl. See [docs/misc/libxl_memory.txt][LIBXLMEM] > > +for a description of how the memory is accounted for in toolstacks > > +using the libxl library. > > I''d retain the "and in the absence of in-guest..." stuff here but go on > to add a sentence lile "If in-guest memory hotplug is supported, then it > will write guest-max (see below). > > The key point is to describe the behaviour with and without hotplug.I think that every new polite guest, with or without memory hotplug, should inform about its maximum supported memory size. It means that we should not differentiate both cases here. Daniel
Ian Campbell
2013-Apr-30 14:15 UTC
Re: [PATCH v4 3/4] libxl: Some optimizations in libxl_set_memory_target()
On Tue, 2013-04-30 at 13:23 +0100, Daniel Kiper wrote:> On Mon, Apr 29, 2013 at 12:16:43PM +0100, Ian Campbell wrote: > > On Mon, 2013-04-29 at 12:09 +0100, Daniel Kiper wrote: > > > Some optimizations in libxl_set_memory_target(). > > > > Please describe them in the commit message. > > OK. > > > At first glance it just looks like you are renaming some variables from > > "memmax"/"target"/"uuid" to "s", why? Just to reduce the number of local > > variables? > > > > They seem more meaningful and easier to read with their current names > > and if you are "optimising" the use of local variable then that seems > > like a false optimisation to me -- any modern compiler can surely work > > out that the lifetimes of these various things do not overlap and do the > > sensible thing WRT register allocation. > > > > At the very least this renaming is hiding what you are actually doing. > > Here are four things: > - reduction of number of variables which you found, > - removal of unneeded intialization; probably it was > needed some time ago but now it is not, > - removal of memorykb = new_target_memkb assignment > with relevant changes in other places, > - merge of video memory size calculation which > is currently in two places. > > I do not insist on first thing but I think that > other optimizations are worth to do. If you wish > I could do all of them in separate patches.IMHO we should be optimising this code for readability, not for number of variables or even performance (this is hardly a hot path). With that in mind, #1 -- no, #2 ok, #3 might be ok if it improves readability (but hard to tell with it mixed in with the other fixes), #4 is probably ok on the basis of not repeating the logic. So separate patches are probably the way to go. Ian.
Ian Campbell
2013-Apr-30 14:17 UTC
Re: [PATCH v4 4/4] xl: Improve xl documentation in regards to guest memory management
On Tue, 2013-04-30 at 13:34 +0100, Daniel Kiper wrote:> > > ========================================================================= */ > > > diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown > > > index 535830e..1ac2159 100644 > > > --- a/docs/misc/xenstore-paths.markdown > > > +++ b/docs/misc/xenstore-paths.markdown > > > @@ -124,18 +124,24 @@ This protocol is not currently well documented. > > > > > > #### ~/memory/static-max = MEMKB [] > > > > > > -Specifies a static maximum amount memory which this domain should > > > -expect to be given. In the absence of in-guest memory hotplug support > > > -this set on domain boot and is usually the maximum amount of RAM which > > > -a guest can make use of. See [docs/misc/libxl_memory.txt][LIBXLMEM] > > > -for a description of how memory is accounted for in toolstacks using > > > -the libxl library. > > > +Specifies a static maximum amount of memory which this domain > > > +should expect to be given. It is usually the maximum amount > > > +of RAM which a guest can make use of. This value is set on the > > > +domain boot. If ~/memory/guest-max is set then static maximum > > > +is ignored by libxl. See [docs/misc/libxl_memory.txt][LIBXLMEM] > > > +for a description of how the memory is accounted for in toolstacks > > > +using the libxl library. > > > > I''d retain the "and in the absence of in-guest..." stuff here but go on > > to add a sentence lile "If in-guest memory hotplug is supported, then it > > will write guest-max (see below). > > > > The key point is to describe the behaviour with and without hotplug. > > I think that every new polite guest, with or without memory hotplug, > should inform about its maximum supported memory size. It means > that we should not differentiate both cases here.We can say things like "guests should do foo" but we need to account for existing guests which don''t and for future guests which still don''t, e.g. at the moment all non-Linux guests. Ian.