Re: [PATCH] xenpaging: libxl support

[Olaf Hering <olaf@aepfle.de>]

On Wed, Sep 21, Ian Campbell wrote:

> > +    ("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.

Yes, that could be changed. Right now xenpaging opens the file in the
current directory. But since all config values could be passed via
xenstore, it may contain the full path to the paging file as well.

> > +    ("xenpaging_debug",           bool,       False, "enable debug output in pager"),
> 
> Perhaps a generic "extra_arguments" type field (string) would be more
> useful?

Yes, that would work too.

> > +    ("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?

It defines how many pages are seen as "hot" by xenpaging, they wont be
paged-out again until that many other pages were paged-in.
See tools/xenpaging/policy_default.c:policy_notify_paged_in().
For example, a value of less than 1024 for a fully paged-out guest will
make it nearly unusuable. But a guest with a smaller xenpaging value can
also have a smaller mru size.


> > +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?

Yes, thats a typo. I did just copy&paste.

> It looks like xp_xenstore_record_pid could be shared with the dm one
> with only a little refactoring/abstraction.

Yes, but could share the same struct libxl__*_starting.

> > +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().

I will update that part.

> > +    /* 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 will update that part.

> > +    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.

I will add a --debug option.

> In the case of the policy mru size perhaps that should be in xenstore
> such that it can be changed on the fly?

Changing it on the fly needs some refactoring of the code, but it should
be doable.

> > 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?

Yes, depends were it ends up. I will move it elsewhere.

> Did I miss a call to libxl_xenpaging_info_destroy() somewhere or does
> this leak?

I will check.

Thanks for the comments.

Olaf