On Thu, 2011-09-15 at 16:20 +0100, Olaf Hering wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@xxxxxxxxx>
> # Date 1316099907 -7200
> # Node ID c9a9779fd9581666c3276f95020ad12e8ceff930
> # Parent 7fb21fac6d7d0a20a7af9aa2caa6fb4145ee3e23
> xenpaging: libxl support
>
> Add support to libxl for starting xenpaging, I send it out for review.
> It mimics what the code to start qemu-dm does.
>
> The patch adds four new config options:
> xenpaging=<int> , the number of pages to page-out
> xenpaging_dir=<string>, the working directory where the pagefile is stored
> xenpaging_debug=<int>, enable or disable debug output
> xenpaging_policy_mru_size=<int>, tweak the number of most-recently-used pages
>
> There are still a few pieces missing like 'xl list -l'.
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
>
> diff -r 7fb21fac6d7d -r c9a9779fd958 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -229,6 +229,7 @@ typedef struct {
> libxl_domain_create_info c_info;
> libxl_domain_build_info b_info;
> libxl_device_model_info dm_info;
> + libxl_xenpaging_info xp_info;
>
> int num_disks, num_vifs, num_pcidevs, num_vfbs, num_vkbs;
>
> diff -r 7fb21fac6d7d -r c9a9779fd958 tools/libxl/libxl.idl
> --- a/tools/libxl/libxl.idl
> +++ b/tools/libxl/libxl.idl
> @@ -190,6 +190,17 @@ by libxl_domain_build/restore. If either
> then the user is responsible for calling
> libxl_file_reference_unmap.""")
>
> +libxl_xenpaging_info = Struct("xenpaging_info",[
I'm not convinced this needs to be it's own struct (rather than being
part of e.g. build_info or create_info) but I don't feel particularly
strongly one way or the other.
> + ("xenpaging", integer, False, "number of pages"),
> + ("xenpaging_workdir", string, False, "directory to store
> guest page file"),
Really a directory or actually a file? xenpaging_path would probably be
a nicer name.
> + ("xenpaging_debug", bool, False, "enable debug output in
> pager"),
Perhaps a generic "extra_arguments" type field (string) would be more
useful?
> + ("xenpaging_policy_mru_size", integer, False, "number of paged-in
> pages to keep in memory"),
How is the distinct from / related to the xenpaging integer?
> + ],
> + comment=
> +"""xenpaging information.
> +
> +Something review is missing""")
> +
> libxl_device_model_info = Struct("device_model_info",[
> ("domid", libxl_domid),
> ("uuid", libxl_uuid, False, "this is use only with stubdom,
> and must be different from the domain uuid"),
> diff -r 7fb21fac6d7d -r c9a9779fd958 tools/libxl/libxl_create.c
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -429,6 +429,109 @@ retry_transaction:
> return rc;
> }
>
> +static void xp_xenstore_record_pid(void *for_spawn, pid_t innerchild)
> +{
> + libxl__device_model_starting *starting = for_spawn;
Do you mean libxl__xenpaging_starting here?
It looks like xp_xenstore_record_pid could be shared with the dm one
with only a little refactoring/abstraction.
> +static int libxl__create_xenpaging(libxl__gc *gc, char *dom_name, uint32_t
> domid, libxl_xenpaging_info *xp_info)
> +{
> [...]
> + /* Set working directory if not specified in config file */
> + if (!xp_info->xenpaging_workdir)
> + xp_info->xenpaging_workdir = "/var/lib/xen/xenpaging";
Should come from libxl_paths, see e.g. libxl_sbindir_path().
> + /* Spawn the child */
> + rc = libxl__spawn_spawn(gc, NULL, "xenpaging", xp_xenstore_record_pid,
> &buf_starting);
No libxl__spawn_starting argument. Do you handle failure to exec etc?
I wonder if we should make that argument to spawn_spawn mandatory?
> + if (rc < 0)
> + goto out_close;
> + if (!rc) { /* inner child */
> + setsid();
> + /* Enable debug output in the child */
> + if (xp_info->xenpaging_debug)
> + setenv("XENPAGING_DEBUG", "1", 1);
> + /* Adjust most-recently-used value in the child */
> + if (xp_info->xenpaging_policy_mru_size)
> + setenv("XENPAGING_POLICY_MRU_SIZE", libxl__sprintf(gc, "%d",
> xp_info->xenpaging_policy_mru_size), 1);
I think the xenpaging binary should have a proper command line interface
rather than passing them via the environment.
In the case of the policy mru size perhaps that should be in xenstore
such that it can be changed on the fly?
> diff -r 7fb21fac6d7d -r c9a9779fd958 tools/libxl/libxl_internal.h
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -251,6 +251,11 @@ typedef struct {
> libxl__spawn_starting *for_spawn;
> } libxl__device_model_starting;
>
> +typedef struct {
> + char *dom_path; /* from libxl_malloc, only for xp_xenstore_record_pid */
> + int domid;
> +} libxl__xenpaging_starting;
> +
> /* from xl_create */
> _hidden int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info
> *info, uint32_t *domid);
> _hidden int libxl__domain_build(libxl__gc *gc,
> diff -r 7fb21fac6d7d -r c9a9779fd958 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -290,6 +290,7 @@ static void dolog(const char *file, int
>
> static void printf_info(int domid,
> libxl_domain_config *d_config,
> + libxl_xenpaging_info *xp_info,
xp_info is d_config->xpo_info or something isn't it?
I'm not sure why dm_info is called out specially though. I think perhaps
because of stubdomains there can be more than one so we need to pass
round the relevant one for the context. Or maybe it's just a historical
wart. In either case it doesn't really apply to the xp_info.
> libxl_device_model_info *dm_info)
> {
> int i;
> @@ -519,6 +520,7 @@ static void parse_config_data(const char
> const char *configfile_data,
> int configfile_len,
> libxl_domain_config *d_config,
> + libxl_xenpaging_info *xp_info,
Same comment as above.
Did I miss a call to libxl_xenpaging_info_destroy() somewhere or does
this leak?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home