[XEN,for-4.13,5/6] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
diff mbox series

Message ID 20191004151707.24844-6-ian.jackson@eu.citrix.com
State New
Headers show
Series
  • Drop/deprecate libxl_get_required_*_memory
Related show

Commit Message

Ian Jackson Oct. 4, 2019, 3:17 p.m. UTC
Defaulting is supposed to be done by libxl.  So these calculations
should be here in libxl.  libxl__domain_config_setdefault has all the
necessary information including the values of max_memkb and max_vcpus.

The overall functional effect depends on the caller:

For xl, no change.  The code moves from xl to libxl.

For callers who set one or both shadow_memkb and iommu_memkb (whether
from libxl_get_required_shadow_memory or otherwise) before calling
libxl_domain_need_memory (any version): the new code will leave their
setting(s) unchanged.

For callers who do not call libxl_domain_need_memory at all, and who
fail to set one of these memory values: now they are both are properly
set.  The shadow and iommu memory to be properly accounted for as
intended.

For callers which call libxl_domain_need_memory and request the
current API (4.13) or which track libxl, the default values are also
now right and everything works as intended.

For callers which call libxl_domain_need_memory, and request an old
pre-4.13 libxl API, and which leave one of these memkb settings unset,
we take special measures to preserve the old behaviour.

This means that they don't get the additional iommu memory and are at
risk of the domain running out of memory as a result of f89f555827a6
"remove late (on-demand) construction of IOMMU page tables".  But this
is no worse than the state just after f89f555827a6, which already
broke such callers in that way.  This is perhaps justifiable because
of the API stability warning next to libxl_domain_need_memory.

An alternative would be to drop the special-casing of these callers.
That would cause a discrepancy between libxl_domain_need_memory and
libxl_domain_create: the former would not include the iommu memory and
the latter would.  That seems worse, but it's debateable.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_create.c   | 40 ++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_mem.c      |  4 ++++
 tools/xl/xl_parse.c          | 15 ++-------------
 4 files changed, 49 insertions(+), 13 deletions(-)

Comments

Anthony PERARD Oct. 4, 2019, 5:19 p.m. UTC | #1
On Fri, Oct 04, 2019 at 04:17:06PM +0100, Ian Jackson wrote:
> @@ -862,6 +864,30 @@ static void domcreate_destruction_cb(libxl__egc *egc,
>                                       libxl__domain_destroy_state *dds,
>                                       int rc);
>  
> +static _Bool ok_to_default_memkb_in_create(libxl__gc *gc)

Is there a reason to use _Bool instead of `bool'? It would be the first
_Bool in libxl.

> +{
> +    /*
> +     * This is a fudge.  We are trying to find whether the caller
> +     * calls the old version of libxl_domain_need_memory.  If they do
> +     * then, because it only gets the b_info, and because it can't
> +     * update the b_info (because it's const), it will base its
> +     * calculations on defaulting shadow_memkb and iommu_memkb to 0
> +     * In that case we probably shouldn't default them differently
> +     * during libxl_domain_create.
> +     *
> +     * The result is that the behaviour with old callers is the same
> +     * as in 4.13: no additional memory is allocated for shadow and
> +     * iommu (unless the caller set shadow_memkb, eg from a call to
> +     * libxl_get_required_shadow_memory).
> +     */
> +    return CTX->libxl_domain_need_memory_0x041200_called &&
> +          !CTX->libxl_domain_need_memory_called;

I think the logic here is inverted.

With this patch applied, we have:
xl
    calls libxl_domain_need_memory()
        libxl__domain_config_setdefault()
            because shadow_memkb is default and that function return 0
            then shadow_memkb is still default
    create_domain()
        libxl__domain_config_setdefault()
            same thing, shadow_memkb is untouch, so still default
            libxl__domain_build_info_setdefault()
                shadow_memkb is default
                    -> shadow_memkb = 0

This is different what is expected by the commit message.
> For xl, no change.  The code moves from xl to libxl.

Instead, if xl calls libxl_domain_need_memory_0x041200():
    create_domain()
        libxl__domain_config_setdefault()
            if shadow_memkb is default; ok_to_default_memkb_in_create is 1
                so shadow_memkb is set to a value different that the one
                calculated by need_memory_0x04100.

Or did I miss something?
Ian Jackson Oct. 4, 2019, 5:23 p.m. UTC | #2
Anthony PERARD writes ("Re: [XEN PATCH for-4.13 5/6] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl"):
> On Fri, Oct 04, 2019 at 04:17:06PM +0100, Ian Jackson wrote:
> > @@ -862,6 +864,30 @@ static void domcreate_destruction_cb(libxl__egc *egc,
> >                                       libxl__domain_destroy_state *dds,
> >                                       int rc);
> >  
> > +static _Bool ok_to_default_memkb_in_create(libxl__gc *gc)
> 
> Is there a reason to use _Bool instead of `bool'? It would be the first
> _Bool in libxl.

I had the wrong head on.  (I've been writing code recently where
`bool' wasn't available.)

> > +    return CTX->libxl_domain_need_memory_0x041200_called &&
> > +          !CTX->libxl_domain_need_memory_called;
> 
> I think the logic here is inverted.

You are right.

Thanks,
Ian.

Patch
diff mbox series

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 457beb6462..3c5033b05b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -250,6 +250,7 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
     switch (b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
         if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
+            /* Normally defaulted in libxl__domain_create_info_setdefault */
             b_info->shadow_memkb = 0;
         if (b_info->u.hvm.mmio_hole_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->u.hvm.mmio_hole_memkb = 0;
@@ -395,6 +396,7 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
         if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->video_memkb = 0;
         if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
+            /* Normally defaulted in libxl__domain_create_info_setdefault */
             b_info->shadow_memkb = 0;
         if (b_info->u.pv.slack_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->u.pv.slack_memkb = 0;
@@ -862,6 +864,30 @@  static void domcreate_destruction_cb(libxl__egc *egc,
                                      libxl__domain_destroy_state *dds,
                                      int rc);
 
+static _Bool ok_to_default_memkb_in_create(libxl__gc *gc)
+{
+    /*
+     * This is a fudge.  We are trying to find whether the caller
+     * calls the old version of libxl_domain_need_memory.  If they do
+     * then, because it only gets the b_info, and because it can't
+     * update the b_info (because it's const), it will base its
+     * calculations on defaulting shadow_memkb and iommu_memkb to 0
+     * In that case we probably shouldn't default them differently
+     * during libxl_domain_create.
+     *
+     * The result is that the behaviour with old callers is the same
+     * as in 4.13: no additional memory is allocated for shadow and
+     * iommu (unless the caller set shadow_memkb, eg from a call to
+     * libxl_get_required_shadow_memory).
+     */
+    return CTX->libxl_domain_need_memory_0x041200_called &&
+          !CTX->libxl_domain_need_memory_called;
+    /*
+     * Treat mixed callers as new callers.  Presumably they know what
+     * they are doing.
+     */
+}
+
 int libxl__domain_config_setdefault(libxl__gc *gc,
                                     libxl_domain_config *d_config,
                                     uint32_t domid)
@@ -974,6 +1000,20 @@  int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    if (d_config->b_info.shadow_memkb == LIBXL_MEMKB_DEFAULT
+        && ok_to_default_memkb_in_create(gc))
+        d_config->b_info.shadow_memkb =
+            libxl_get_required_shadow_memory(d_config->b_info.max_memkb,
+                                             d_config->b_info.max_vcpus);
+
+    /* No IOMMU reservation is needed if passthrough mode is not 'sync_pt' */
+    if (d_config->b_info.iommu_memkb == LIBXL_MEMKB_DEFAULT
+        && ok_to_default_memkb_in_create(gc))
+        d_config->b_info.iommu_memkb =
+            (d_config->c_info.passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
+            ? libxl_get_required_iommu_memory(d_config->b_info.max_memkb)
+            : 0;
+
     ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
     if (ret) {
         LOGD(ERROR, domid, "Unable to set domain build info defaults");
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 01de5576d9..3e306ad4f6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -637,6 +637,9 @@  struct libxl__ctx {
     LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
 
     libxl_version_info version_info;
+
+    _Bool libxl_domain_need_memory_0x041200_called,
+          libxl_domain_need_memory_called;
 };
 
 /*
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 26cf136ac2..0537846ee2 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -485,6 +485,8 @@  int libxl_domain_need_memory(libxl_ctx *ctx,
     GC_INIT(ctx);
     int rc;
 
+    ctx->libxl_domain_need_memory_called = 1;
+
     rc = libxl__domain_config_setdefault(gc,
                                          d_config,
                                          domid_for_logging);
@@ -508,6 +510,8 @@  int libxl_domain_need_memory_0x041200(libxl_ctx *ctx,
     GC_INIT(ctx);
     int rc;
 
+    ctx->libxl_domain_need_memory_0x041200_called = 1;
+
     libxl_domain_build_info b_info[1];
     libxl_domain_build_info_init(b_info);
     libxl_domain_build_info_copy(ctx, b_info, b_info_in);
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 03a2c54dd2..79871c22d0 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1572,19 +1572,8 @@  void parse_config_data(const char *config_source,
         exit(-ERROR_FAIL);
     }
 
-    /* libxl_get_required_shadow_memory() and
-     * libxl_get_required_iommu_memory() must be called after final values
-     * (default or specified) for vcpus and memory are set, because the
-     * calculations depend on those values. */
-    b_info->shadow_memkb = !xlu_cfg_get_long(config, "shadow_memory", &l, 0)
-        ? l * 1024
-        : libxl_get_required_shadow_memory(b_info->max_memkb,
-                                           b_info->max_vcpus);
-
-    /* No IOMMU reservation is needed if passthrough mode is not 'sync_pt' */
-    b_info->iommu_memkb = (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
-        ? libxl_get_required_iommu_memory(b_info->max_memkb)
-        : 0;
+    if (!xlu_cfg_get_long(config, "shadow_memory", &l, 0))
+        b_info->shadow_memkb = l * 1024;
 
     xlu_cfg_get_defbool(config, "nomigrate", &b_info->disable_migrate, 0);