[PATCH v2 0/3] tools/libxc: use superpages

[PATCH v2 0/3] tools/libxc: use superpages

[Olaf Hering]

Using superpages on the receiving dom0 will avoid performance regressions.

Olaf

v2:
 split into individual commits

Olaf Hering (3):
  tools/libxc: move SUPERPAGE macros to common header
  tools/libxc: add API for bitmap access for restore
  tools/libxc: use superpages during restore of HVM guest

 tools/libxc/xc_dom_x86.c            |   5 -
 tools/libxc/xc_private.h            |   5 +
 tools/libxc/xc_sr_common.c          |  41 ++++++++
 tools/libxc/xc_sr_common.h          |  82 +++++++++++++++-
 tools/libxc/xc_sr_restore.c         | 136 +--------------------------
 tools/libxc/xc_sr_restore_x86_hvm.c | 180 ++++++++++++++++++++++++++++++++++++
 tools/libxc/xc_sr_restore_x86_pv.c  |  72 ++++++++++++++-
 7 files changed, 381 insertions(+), 140 deletions(-)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 1/3] tools/libxc: move SUPERPAGE macros to common header

[Olaf Hering]

The macros SUPERPAGE_2MB_SHIFT and SUPERPAGE_1GB_SHIFT will be used by
other code in libxc. Move the macros to a header file.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_dom_x86.c | 5 -----
 tools/libxc/xc_private.h | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index cb68efcbd3..5aff5cad58 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -43,11 +43,6 @@
 
 #define SUPERPAGE_BATCH_SIZE 512
 
-#define SUPERPAGE_2MB_SHIFT   9
-#define SUPERPAGE_2MB_NR_PFNS (1UL << SUPERPAGE_2MB_SHIFT)
-#define SUPERPAGE_1GB_SHIFT   18
-#define SUPERPAGE_1GB_NR_PFNS (1UL << SUPERPAGE_1GB_SHIFT)
-
 #define X86_CR0_PE 0x01
 #define X86_CR0_ET 0x10
 
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 1c27b0fded..d581f850b0 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -66,6 +66,11 @@ struct iovec {
 #define DECLARE_FLASK_OP struct xen_flask_op op
 #define DECLARE_PLATFORM_OP struct xen_platform_op platform_op
 
+#define SUPERPAGE_2MB_SHIFT   9
+#define SUPERPAGE_2MB_NR_PFNS (1UL << SUPERPAGE_2MB_SHIFT)
+#define SUPERPAGE_1GB_SHIFT   18
+#define SUPERPAGE_1GB_NR_PFNS (1UL << SUPERPAGE_1GB_SHIFT)
+
 #undef PAGE_SHIFT
 #undef PAGE_SIZE
 #undef PAGE_MASK

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/3] tools/libxc: move SUPERPAGE macros to common header

[Wei Liu]

On Thu, Aug 17, 2017 at 07:01:31PM +0200, Olaf Hering wrote:
> The macros SUPERPAGE_2MB_SHIFT and SUPERPAGE_1GB_SHIFT will be used by
> other code in libxc. Move the macros to a header file.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/3] tools/libxc: add API for bitmap access for restore

[Olaf Hering]

Extend API for managing bitmaps. Each bitmap is now represented by a
generic struct xc_sr_bitmap.
Switch the existing populated_pfns to this API.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_sr_common.c  | 41 +++++++++++++++++++++++++++
 tools/libxc/xc_sr_common.h  | 67 +++++++++++++++++++++++++++++++++++++++++++--
 tools/libxc/xc_sr_restore.c | 66 ++------------------------------------------
 3 files changed, 109 insertions(+), 65 deletions(-)

diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index 79b9c3e940..4d221ca90c 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -155,6 +155,47 @@ static void __attribute__((unused)) build_assertions(void)
     BUILD_BUG_ON(sizeof(struct xc_sr_rec_hvm_params)        != 8);
 }
 
+/*
+ * Expand the tracking structures as needed.
+ * To avoid realloc()ing too excessively, the size increased to the nearest power
+ * of two large enough to contain the required number of bits.
+ */
+bool _xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits)
+{
+    if (bits > bm->bits)
+    {
+        size_t new_max;
+        size_t old_sz, new_sz;
+        void *p;
+
+        /* Round up to the nearest power of two larger than bit, less 1. */
+        new_max = bits;
+        new_max |= new_max >> 1;
+        new_max |= new_max >> 2;
+        new_max |= new_max >> 4;
+        new_max |= new_max >> 8;
+        new_max |= new_max >> 16;
+#ifdef __x86_64__
+        new_max |= new_max >> 32;
+#endif
+
+        old_sz = bitmap_size(bm->bits + 1);
+        new_sz = bitmap_size(new_max + 1);
+        p = realloc(bm->p, new_sz);
+        if (!p)
+            return false;
+
+        if (bm->p)
+            memset(p + old_sz, 0, new_sz - old_sz);
+        else
+            memset(p, 0, new_sz);
+
+        bm->p = p;
+        bm->bits = new_max;
+    }
+    return true;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index a83f22af4e..5d78f461af 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -172,6 +172,12 @@ struct xc_sr_x86_pv_restore_vcpu
     size_t basicsz, extdsz, xsavesz, msrsz;
 };
 
+struct xc_sr_bitmap
+{
+    void *p;
+    unsigned long bits;
+};
+
 struct xc_sr_context
 {
     xc_interface *xch;
@@ -255,8 +261,7 @@ struct xc_sr_context
             domid_t      xenstore_domid,  console_domid;
 
             /* Bitmap of currently populated PFNs during restore. */
-            unsigned long *populated_pfns;
-            xen_pfn_t max_populated_pfn;
+            struct xc_sr_bitmap populated_pfns;
 
             /* Sender has invoked verify mode on the stream. */
             bool verify;
@@ -343,6 +348,64 @@ extern struct xc_sr_save_ops save_ops_x86_hvm;
 extern struct xc_sr_restore_ops restore_ops_x86_pv;
 extern struct xc_sr_restore_ops restore_ops_x86_hvm;
 
+extern bool _xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits);
+
+static inline bool xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits)
+{
+    if (bits > bm->bits)
+        return _xc_sr_bitmap_resize(bm, bits);
+    return true;
+}
+
+static inline void xc_sr_bitmap_free(struct xc_sr_bitmap *bm)
+{
+    free(bm->p);
+}
+
+static inline bool xc_sr_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
+{
+    if (!xc_sr_bitmap_resize(bm, bit))
+        return false;
+
+    set_bit(bit, bm->p);
+    return true;
+}
+
+static inline bool xc_sr_test_bit(unsigned long bit, struct xc_sr_bitmap *bm)
+{
+    if (bit > bm->bits)
+        return false;
+    return !!test_bit(bit, bm->p);
+}
+
+static inline int xc_sr_test_and_clear_bit(unsigned long bit, struct xc_sr_bitmap *bm)
+{
+    return test_and_clear_bit(bit, bm->p);
+}
+
+static inline int xc_sr_test_and_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
+{
+    return test_and_set_bit(bit, bm->p);
+}
+
+static inline bool pfn_is_populated(struct xc_sr_context *ctx, xen_pfn_t pfn)
+{
+    return xc_sr_test_bit(pfn, &ctx->restore.populated_pfns);
+}
+
+static inline int pfn_set_populated(struct xc_sr_context *ctx, xen_pfn_t pfn)
+{
+    xc_interface *xch = ctx->xch;
+
+    if ( !xc_sr_set_bit(pfn, &ctx->restore.populated_pfns) )
+    {
+        ERROR("Failed to realloc populated_pfns bitmap");
+        errno = ENOMEM;
+        return -1;
+    }
+    return 0;
+}
+
 struct xc_sr_record
 {
     uint32_t type;
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index a016678332..d53948e1a6 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -68,64 +68,6 @@ static int read_headers(struct xc_sr_context *ctx)
     return 0;
 }
 
-/*
- * Is a pfn populated?
- */
-static bool pfn_is_populated(const struct xc_sr_context *ctx, xen_pfn_t pfn)
-{
-    if ( pfn > ctx->restore.max_populated_pfn )
-        return false;
-    return test_bit(pfn, ctx->restore.populated_pfns);
-}
-
-/*
- * Set a pfn as populated, expanding the tracking structures if needed. To
- * avoid realloc()ing too excessively, the size increased to the nearest power
- * of two large enough to contain the required pfn.
- */
-static int pfn_set_populated(struct xc_sr_context *ctx, xen_pfn_t pfn)
-{
-    xc_interface *xch = ctx->xch;
-
-    if ( pfn > ctx->restore.max_populated_pfn )
-    {
-        xen_pfn_t new_max;
-        size_t old_sz, new_sz;
-        unsigned long *p;
-
-        /* Round up to the nearest power of two larger than pfn, less 1. */
-        new_max = pfn;
-        new_max |= new_max >> 1;
-        new_max |= new_max >> 2;
-        new_max |= new_max >> 4;
-        new_max |= new_max >> 8;
-        new_max |= new_max >> 16;
-#ifdef __x86_64__
-        new_max |= new_max >> 32;
-#endif
-
-        old_sz = bitmap_size(ctx->restore.max_populated_pfn + 1);
-        new_sz = bitmap_size(new_max + 1);
-        p = realloc(ctx->restore.populated_pfns, new_sz);
-        if ( !p )
-        {
-            ERROR("Failed to realloc populated bitmap");
-            errno = ENOMEM;
-            return -1;
-        }
-
-        memset((uint8_t *)p + old_sz, 0x00, new_sz - old_sz);
-
-        ctx->restore.populated_pfns    = p;
-        ctx->restore.max_populated_pfn = new_max;
-    }
-
-    assert(!test_bit(pfn, ctx->restore.populated_pfns));
-    set_bit(pfn, ctx->restore.populated_pfns);
-
-    return 0;
-}
-
 /*
  * Given a set of pfns, obtain memory from Xen to fill the physmap for the
  * unpopulated subset.  If types is NULL, no page type checking is performed
@@ -684,10 +626,8 @@ static int setup(struct xc_sr_context *ctx)
     if ( rc )
         goto err;
 
-    ctx->restore.max_populated_pfn = (32 * 1024 / 4) - 1;
-    ctx->restore.populated_pfns = bitmap_alloc(
-        ctx->restore.max_populated_pfn + 1);
-    if ( !ctx->restore.populated_pfns )
+    rc = !xc_sr_bitmap_resize(&ctx->restore.populated_pfns, 32 * 1024 / 4);
+    if ( rc )
     {
         ERROR("Unable to allocate memory for populated_pfns bitmap");
         rc = -1;
@@ -722,7 +662,7 @@ static void cleanup(struct xc_sr_context *ctx)
         xc_hypercall_buffer_free_pages(xch, dirty_bitmap,
                                    NRPAGES(bitmap_size(ctx->restore.p2m_size)));
     free(ctx->restore.buffered_records);
-    free(ctx->restore.populated_pfns);
+    xc_sr_bitmap_free(&ctx->restore.populated_pfns);
     if ( ctx->restore.ops.cleanup(ctx) )
         PERROR("Failed to clean up");
 }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] tools/libxc: add API for bitmap access for restore

[Wei Liu]

On Thu, Aug 17, 2017 at 07:01:32PM +0200, Olaf Hering wrote:
> Extend API for managing bitmaps. Each bitmap is now represented by a
> generic struct xc_sr_bitmap.
> Switch the existing populated_pfns to this API.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> +
[...]
> +static inline void xc_sr_bitmap_free(struct xc_sr_bitmap *bm)
> +{
> +    free(bm->p);

Also set bm->p to NULL to make it idempotent please.

> +}
> +
> +static inline bool xc_sr_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> +{
> +    if (!xc_sr_bitmap_resize(bm, bit))
> +        return false;
> +
> +    set_bit(bit, bm->p);
> +    return true;
> +}
> +
> +static inline bool xc_sr_test_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> +{
> +    if (bit > bm->bits)
> +        return false;
> +    return !!test_bit(bit, bm->p);
> +}
> +
> +static inline int xc_sr_test_and_clear_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> +{
> +    return test_and_clear_bit(bit, bm->p);
> +}
> +
> +static inline int xc_sr_test_and_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> +{
> +    return test_and_set_bit(bit, bm->p);
> +}
> +
> +static inline bool pfn_is_populated(struct xc_sr_context *ctx, xen_pfn_t pfn)
> +{
> +    return xc_sr_test_bit(pfn, &ctx->restore.populated_pfns);
> +}
> +
> +static inline int pfn_set_populated(struct xc_sr_context *ctx, xen_pfn_t pfn)
> +{
> +    xc_interface *xch = ctx->xch;
> +

This is not used, right?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] tools/libxc: add API for bitmap access for restore

[Wei Liu]

On Tue, Aug 22, 2017 at 03:34:37PM +0100, Wei Liu wrote:
> > +static inline int pfn_set_populated(struct xc_sr_context *ctx, xen_pfn_t pfn)
> > +{
> > +    xc_interface *xch = ctx->xch;
> > +
> 
> This is not used, right?

Oh this is acutally used. Please ignore this comment.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] tools/libxc: add API for bitmap access for restore

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 714 bytes --]

On Thu, Aug 17, Olaf Hering wrote:

> Extend API for managing bitmaps. Each bitmap is now represented by a
> generic struct xc_sr_bitmap.

> +static inline bool xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits)
> +static inline void xc_sr_bitmap_free(struct xc_sr_bitmap *bm)

> +static inline bool xc_sr_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> +static inline bool xc_sr_test_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> +static inline int xc_sr_test_and_clear_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> +static inline int xc_sr_test_and_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)

Any objection to remove the trailing '_bit' from these four function names?

Olaf

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/3] tools/libxc: add API for bitmap access for restore

[Wei Liu]

On Thu, Aug 24, 2017 at 08:36:16AM +0200, Olaf Hering wrote:
> On Thu, Aug 17, Olaf Hering wrote:
> 
> > Extend API for managing bitmaps. Each bitmap is now represented by a
> > generic struct xc_sr_bitmap.
> 
> > +static inline bool xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bits)
> > +static inline void xc_sr_bitmap_free(struct xc_sr_bitmap *bm)
> 
> > +static inline bool xc_sr_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> > +static inline bool xc_sr_test_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> > +static inline int xc_sr_test_and_clear_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> > +static inline int xc_sr_test_and_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
> 
> Any objection to remove the trailing '_bit' from these four function names?

No preference.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Olaf Hering]

During creating of a HVM domU meminit_hvm() tries to map superpages.
After save/restore or migration this mapping is lost, everything is
allocated in single pages. This causes a performance degradition after
migration.

Add neccessary code to preallocate a superpage for the chunk of pfns
that is received. In case a pfn was not populated on the sending side it
must be freed on the receiving side to avoid over-allocation.

The existing code for x86_pv is moved unmodified into its own file.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libxc/xc_sr_common.h          |  15 +++
 tools/libxc/xc_sr_restore.c         |  70 +-------------
 tools/libxc/xc_sr_restore_x86_hvm.c | 180 ++++++++++++++++++++++++++++++++++++
 tools/libxc/xc_sr_restore_x86_pv.c  |  72 ++++++++++++++-
 4 files changed, 267 insertions(+), 70 deletions(-)

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index 5d78f461af..26c45fdd6d 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -139,6 +139,16 @@ struct xc_sr_restore_ops
      */
     int (*setup)(struct xc_sr_context *ctx);
 
+    /**
+     * Populate PFNs
+     *
+     * Given a set of pfns, obtain memory from Xen to fill the physmap for the
+     * unpopulated subset.
+     */
+    int (*populate_pfns)(struct xc_sr_context *ctx, unsigned count,
+                         const xen_pfn_t *original_pfns, const uint32_t *types);
+
+
     /**
      * Process an individual record from the stream.  The caller shall take
      * care of processing common records (e.g. END, PAGE_DATA).
@@ -336,6 +346,11 @@ struct xc_sr_context
                     /* HVM context blob. */
                     void *context;
                     size_t contextsz;
+
+                    /* Bitmap of currently allocated PFNs during restore. */
+                    struct xc_sr_bitmap attempted_1g;
+                    struct xc_sr_bitmap attempted_2m;
+                    struct xc_sr_bitmap allocated_pfns;
                 } restore;
             };
         } x86_hvm;
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index d53948e1a6..1f9fe25b8f 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -68,74 +68,6 @@ static int read_headers(struct xc_sr_context *ctx)
     return 0;
 }
 
-/*
- * Given a set of pfns, obtain memory from Xen to fill the physmap for the
- * unpopulated subset.  If types is NULL, no page type checking is performed
- * and all unpopulated pfns are populated.
- */
-int populate_pfns(struct xc_sr_context *ctx, unsigned count,
-                  const xen_pfn_t *original_pfns, const uint32_t *types)
-{
-    xc_interface *xch = ctx->xch;
-    xen_pfn_t *mfns = malloc(count * sizeof(*mfns)),
-        *pfns = malloc(count * sizeof(*pfns));
-    unsigned i, nr_pfns = 0;
-    int rc = -1;
-
-    if ( !mfns || !pfns )
-    {
-        ERROR("Failed to allocate %zu bytes for populating the physmap",
-              2 * count * sizeof(*mfns));
-        goto err;
-    }
-
-    for ( i = 0; i < count; ++i )
-    {
-        if ( (!types || (types &&
-                         (types[i] != XEN_DOMCTL_PFINFO_XTAB &&
-                          types[i] != XEN_DOMCTL_PFINFO_BROKEN))) &&
-             !pfn_is_populated(ctx, original_pfns[i]) )
-        {
-            rc = pfn_set_populated(ctx, original_pfns[i]);
-            if ( rc )
-                goto err;
-            pfns[nr_pfns] = mfns[nr_pfns] = original_pfns[i];
-            ++nr_pfns;
-        }
-    }
-
-    if ( nr_pfns )
-    {
-        rc = xc_domain_populate_physmap_exact(
-            xch, ctx->domid, nr_pfns, 0, 0, mfns);
-        if ( rc )
-        {
-            PERROR("Failed to populate physmap");
-            goto err;
-        }
-
-        for ( i = 0; i < nr_pfns; ++i )
-        {
-            if ( mfns[i] == INVALID_MFN )
-            {
-                ERROR("Populate physmap failed for pfn %u", i);
-                rc = -1;
-                goto err;
-            }
-
-            ctx->restore.ops.set_gfn(ctx, pfns[i], mfns[i]);
-        }
-    }
-
-    rc = 0;
-
- err:
-    free(pfns);
-    free(mfns);
-
-    return rc;
-}
-
 /*
  * Given a list of pfns, their types, and a block of page data from the
  * stream, populate and record their types, map the relevant subset and copy
@@ -161,7 +93,7 @@ static int process_page_data(struct xc_sr_context *ctx, unsigned count,
         goto err;
     }
 
-    rc = populate_pfns(ctx, count, pfns, types);
+    rc = ctx->restore.ops.populate_pfns(ctx, count, pfns, types);
     if ( rc )
     {
         ERROR("Failed to populate pfns for batch of %u pages", count);
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 1dca85354a..60454148db 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -135,6 +135,8 @@ static int x86_hvm_localise_page(struct xc_sr_context *ctx,
 static int x86_hvm_setup(struct xc_sr_context *ctx)
 {
     xc_interface *xch = ctx->xch;
+    struct xc_sr_bitmap *bm;
+    unsigned long bits;
 
     if ( ctx->restore.guest_type != DHDR_TYPE_X86_HVM )
     {
@@ -149,7 +151,30 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
         return -1;
     }
 
+    bm = &ctx->x86_hvm.restore.attempted_1g;
+    bits = (ctx->restore.p2m_size >> SUPERPAGE_1GB_SHIFT) + 1;
+    if ( xc_sr_bitmap_resize(bm, bits) == false )
+        goto out;
+
+    bm = &ctx->x86_hvm.restore.attempted_2m;
+    bits = (ctx->restore.p2m_size >> SUPERPAGE_2MB_SHIFT) + 1;
+    if ( xc_sr_bitmap_resize(bm, bits) == false )
+        goto out;
+
+    bm = &ctx->x86_hvm.restore.allocated_pfns;
+    bits = ctx->restore.p2m_size + 1;
+    if ( xc_sr_bitmap_resize(bm, bits) == false )
+        goto out;
+
+    /* No superpage in 1st 2MB due to VGA hole */
+    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_1g);
+    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_2m);
+
     return 0;
+
+out:
+    ERROR("Unable to allocate memory for pfn bitmaps");
+    return -1;
 }
 
 /*
@@ -224,10 +249,164 @@ static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
 static int x86_hvm_cleanup(struct xc_sr_context *ctx)
 {
     free(ctx->x86_hvm.restore.context);
+    xc_sr_bitmap_free(&ctx->x86_hvm.restore.attempted_1g);
+    xc_sr_bitmap_free(&ctx->x86_hvm.restore.attempted_2m);
+    xc_sr_bitmap_free(&ctx->x86_hvm.restore.allocated_pfns);
 
     return 0;
 }
 
+/*
+ * Set a pfn as allocated, expanding the tracking structures if needed.
+ */
+static int pfn_set_allocated(struct xc_sr_context *ctx, xen_pfn_t pfn)
+{
+    xc_interface *xch = ctx->xch;
+
+    if ( !xc_sr_set_bit(pfn, &ctx->x86_hvm.restore.allocated_pfns) )
+    {
+        ERROR("Failed to realloc allocated_pfns bitmap");
+        errno = ENOMEM;
+        return -1;
+    }
+    return 0;
+}
+
+/*
+ * Attempt to allocate a superpage where the pfn resides.
+ */
+static int x86_hvm_allocate_pfn(struct xc_sr_context *ctx, xen_pfn_t pfn)
+{
+    xc_interface *xch = ctx->xch;
+    bool success = false;
+    int rc = -1, done;
+    unsigned int order;
+    unsigned long i;
+    unsigned long stat_1g = 0, stat_2m = 0, stat_4k = 0;
+    unsigned long idx_1g, idx_2m;
+    unsigned long count;
+    xen_pfn_t base_pfn = 0, extnt;
+
+    if (xc_sr_test_bit(pfn, &ctx->x86_hvm.restore.allocated_pfns))
+        return 0;
+
+    idx_1g = pfn >> SUPERPAGE_1GB_SHIFT;
+    idx_2m = pfn >> SUPERPAGE_2MB_SHIFT;
+    if (!xc_sr_bitmap_resize(&ctx->x86_hvm.restore.attempted_1g, idx_1g))
+    {
+        PERROR("Failed to realloc attempted_1g");
+        return -1;
+    }
+    if (!xc_sr_bitmap_resize(&ctx->x86_hvm.restore.attempted_2m, idx_2m))
+    {
+        PERROR("Failed to realloc attempted_2m");
+        return -1;
+    }
+    DPRINTF("idx_1g %lu idx_2m %lu\n", idx_1g, idx_2m);
+    if (!xc_sr_test_and_set_bit(idx_1g, &ctx->x86_hvm.restore.attempted_1g)) {
+        order = SUPERPAGE_1GB_SHIFT;
+        count = 1UL << order;
+        base_pfn = (pfn >> order) << order;
+        extnt = base_pfn;
+        done = xc_domain_populate_physmap(xch, ctx->domid, 1, order, 0, &extnt);
+        DPRINTF("1G base_pfn %" PRI_xen_pfn " done %d\n", base_pfn, done);
+        if (done > 0) {
+            struct xc_sr_bitmap *bm = &ctx->x86_hvm.restore.attempted_2m;
+            success = true;
+            stat_1g = done;
+            for (i = 0; i < (count >> SUPERPAGE_2MB_SHIFT); i++)
+                xc_sr_set_bit((base_pfn >> SUPERPAGE_2MB_SHIFT) + i, bm);
+        }
+    }
+
+    if (!xc_sr_test_and_set_bit(idx_2m, &ctx->x86_hvm.restore.attempted_2m)) {
+        order = SUPERPAGE_2MB_SHIFT;
+        count = 1UL << order;
+        base_pfn = (pfn >> order) << order;
+        extnt = base_pfn;
+        done = xc_domain_populate_physmap(xch, ctx->domid, 1, order, 0, &extnt);
+        DPRINTF("2M base_pfn %" PRI_xen_pfn " done %d\n", base_pfn, done);
+        if (done > 0) {
+            success = true;
+            stat_2m = done;
+        }
+    }
+    if (success == false) {
+        count = 1;
+        extnt = base_pfn = pfn;
+        done = xc_domain_populate_physmap(xch, ctx->domid, count, 0, 0, &extnt);
+        if (done > 0) {
+            success = true;
+            stat_4k = count;
+        }
+    }
+    DPRINTF("count %lu 1G %lu 2M %lu 4k %lu\n", count, stat_1g, stat_2m, stat_4k);
+    if (success == true) {
+        do {
+            count--;
+            rc = pfn_set_allocated(ctx, base_pfn + count);
+            if (rc)
+                break;
+        } while (count);
+    }
+    return rc;
+}
+
+static int x86_hvm_populate_pfns(struct xc_sr_context *ctx, unsigned count,
+                                 const xen_pfn_t *original_pfns,
+                                 const uint32_t *types)
+{
+    xc_interface *xch = ctx->xch;
+    xen_pfn_t min_pfn = original_pfns[0], max_pfn = original_pfns[0];
+    unsigned i;
+    int rc = -1;
+
+    for ( i = 0; i < count; ++i )
+    {
+        if (original_pfns[i] < min_pfn)
+            min_pfn = original_pfns[i];
+        if (original_pfns[i] > max_pfn)
+            max_pfn = original_pfns[i];
+        if ( (types[i] != XEN_DOMCTL_PFINFO_XTAB &&
+              types[i] != XEN_DOMCTL_PFINFO_BROKEN) &&
+             !pfn_is_populated(ctx, original_pfns[i]) )
+        {
+            rc = x86_hvm_allocate_pfn(ctx, original_pfns[i]);
+            if ( rc )
+                goto err;
+            rc = pfn_set_populated(ctx, original_pfns[i]);
+            if ( rc )
+                goto err;
+        }
+    }
+
+    while (min_pfn < max_pfn)
+    {
+        if (!xc_sr_bitmap_resize(&ctx->x86_hvm.restore.allocated_pfns, min_pfn))
+        {
+            PERROR("Failed to realloc allocated_pfns %" PRI_xen_pfn, min_pfn);
+            goto err;
+        }
+        if (!pfn_is_populated(ctx, min_pfn) &&
+            xc_sr_test_and_clear_bit(min_pfn, &ctx->x86_hvm.restore.allocated_pfns)) {
+            xen_pfn_t pfn = min_pfn;
+            rc = xc_domain_decrease_reservation_exact(xch, ctx->domid, 1, 0, &pfn);
+            if ( rc )
+            {
+                PERROR("Failed to release pfn %" PRI_xen_pfn, min_pfn);
+                goto err;
+            }
+        }
+        min_pfn++;
+    }
+
+    rc = 0;
+
+ err:
+    return rc;
+}
+
+
 struct xc_sr_restore_ops restore_ops_x86_hvm =
 {
     .pfn_is_valid    = x86_hvm_pfn_is_valid,
@@ -236,6 +415,7 @@ struct xc_sr_restore_ops restore_ops_x86_hvm =
     .set_page_type   = x86_hvm_set_page_type,
     .localise_page   = x86_hvm_localise_page,
     .setup           = x86_hvm_setup,
+    .populate_pfns   = x86_hvm_populate_pfns,
     .process_record  = x86_hvm_process_record,
     .stream_complete = x86_hvm_stream_complete,
     .cleanup         = x86_hvm_cleanup,
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index 50e25c162c..87957559bc 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -936,6 +936,75 @@ static void x86_pv_set_gfn(struct xc_sr_context *ctx, xen_pfn_t pfn,
         ((uint32_t *)ctx->x86_pv.p2m)[pfn] = mfn;
 }
 
+/*
+ * Given a set of pfns, obtain memory from Xen to fill the physmap for the
+ * unpopulated subset.  If types is NULL, no page type checking is performed
+ * and all unpopulated pfns are populated.
+ */
+static int x86_pv_populate_pfns(struct xc_sr_context *ctx, unsigned count,
+                                const xen_pfn_t *original_pfns,
+                                const uint32_t *types)
+{
+    xc_interface *xch = ctx->xch;
+    xen_pfn_t *mfns = malloc(count * sizeof(*mfns)),
+        *pfns = malloc(count * sizeof(*pfns));
+    unsigned i, nr_pfns = 0;
+    int rc = -1;
+
+    if ( !mfns || !pfns )
+    {
+        ERROR("Failed to allocate %zu bytes for populating the physmap",
+              2 * count * sizeof(*mfns));
+        goto err;
+    }
+
+    for ( i = 0; i < count; ++i )
+    {
+        if ( (!types || (types &&
+                         (types[i] != XEN_DOMCTL_PFINFO_XTAB &&
+                          types[i] != XEN_DOMCTL_PFINFO_BROKEN))) &&
+             !pfn_is_populated(ctx, original_pfns[i]) )
+        {
+            rc = pfn_set_populated(ctx, original_pfns[i]);
+            if ( rc )
+                goto err;
+            pfns[nr_pfns] = mfns[nr_pfns] = original_pfns[i];
+            ++nr_pfns;
+        }
+    }
+
+    if ( nr_pfns )
+    {
+        rc = xc_domain_populate_physmap_exact(
+            xch, ctx->domid, nr_pfns, 0, 0, mfns);
+        if ( rc )
+        {
+            PERROR("Failed to populate physmap");
+            goto err;
+        }
+
+        for ( i = 0; i < nr_pfns; ++i )
+        {
+            if ( mfns[i] == INVALID_MFN )
+            {
+                ERROR("Populate physmap failed for pfn %u", i);
+                rc = -1;
+                goto err;
+            }
+
+            ctx->restore.ops.set_gfn(ctx, pfns[i], mfns[i]);
+        }
+    }
+
+    rc = 0;
+
+ err:
+    free(pfns);
+    free(mfns);
+
+    return rc;
+}
+
 /*
  * restore_ops function.  Convert pfns back to mfns in pagetables.  Possibly
  * needs to populate new frames if a PTE is found referring to a frame which
@@ -980,7 +1049,7 @@ static int x86_pv_localise_page(struct xc_sr_context *ctx,
         }
     }
 
-    if ( to_populate && populate_pfns(ctx, to_populate, pfns, NULL) )
+    if ( to_populate && x86_pv_populate_pfns(ctx, to_populate, pfns, NULL) )
         return -1;
 
     for ( i = 0; i < (PAGE_SIZE / sizeof(uint64_t)); ++i )
@@ -1160,6 +1229,7 @@ struct xc_sr_restore_ops restore_ops_x86_pv =
     .set_gfn         = x86_pv_set_gfn,
     .localise_page   = x86_pv_localise_page,
     .setup           = x86_pv_setup,
+    .populate_pfns   = x86_pv_populate_pfns,
     .process_record  = x86_pv_process_record,
     .stream_complete = x86_pv_stream_complete,
     .cleanup         = x86_pv_cleanup,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Wei Liu]

On Thu, Aug 17, 2017 at 07:01:33PM +0200, Olaf Hering wrote:
[...]
> diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
> index 1dca85354a..60454148db 100644
> --- a/tools/libxc/xc_sr_restore_x86_hvm.c
> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
> @@ -135,6 +135,8 @@ static int x86_hvm_localise_page(struct xc_sr_context *ctx,
>  static int x86_hvm_setup(struct xc_sr_context *ctx)
>  {
>      xc_interface *xch = ctx->xch;
> +    struct xc_sr_bitmap *bm;
> +    unsigned long bits;
>  
>      if ( ctx->restore.guest_type != DHDR_TYPE_X86_HVM )
>      {
> @@ -149,7 +151,30 @@ static int x86_hvm_setup(struct xc_sr_context *ctx)
>          return -1;
>      }
>  
> +    bm = &ctx->x86_hvm.restore.attempted_1g;
> +    bits = (ctx->restore.p2m_size >> SUPERPAGE_1GB_SHIFT) + 1;
> +    if ( xc_sr_bitmap_resize(bm, bits) == false )
> +        goto out;
> +
> +    bm = &ctx->x86_hvm.restore.attempted_2m;
> +    bits = (ctx->restore.p2m_size >> SUPERPAGE_2MB_SHIFT) + 1;
> +    if ( xc_sr_bitmap_resize(bm, bits) == false )
> +        goto out;
> +
> +    bm = &ctx->x86_hvm.restore.allocated_pfns;
> +    bits = ctx->restore.p2m_size + 1;
> +    if ( xc_sr_bitmap_resize(bm, bits) == false )
> +        goto out;
> +
> +    /* No superpage in 1st 2MB due to VGA hole */
> +    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_1g);
> +    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_2m);
> +

I don't quite get this. What about other holes such as MMIO?

>      return 0;
> +
> +out:
> +    ERROR("Unable to allocate memory for pfn bitmaps");
> +    return -1;
>  }
>  
>  /*
> @@ -224,10 +249,164 @@ static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
>  static int x86_hvm_cleanup(struct xc_sr_context *ctx)
>  {
>      free(ctx->x86_hvm.restore.context);
> +    xc_sr_bitmap_free(&ctx->x86_hvm.restore.attempted_1g);
> +    xc_sr_bitmap_free(&ctx->x86_hvm.restore.attempted_2m);
> +    xc_sr_bitmap_free(&ctx->x86_hvm.restore.allocated_pfns);
>  
>      return 0;
>  }
>  
> +/*
> + * Set a pfn as allocated, expanding the tracking structures if needed.
> + */
> +static int pfn_set_allocated(struct xc_sr_context *ctx, xen_pfn_t pfn)
> +{
> +    xc_interface *xch = ctx->xch;
> +
> +    if ( !xc_sr_set_bit(pfn, &ctx->x86_hvm.restore.allocated_pfns) )
> +    {
> +        ERROR("Failed to realloc allocated_pfns bitmap");
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +/*
> + * Attempt to allocate a superpage where the pfn resides.
> + */
> +static int x86_hvm_allocate_pfn(struct xc_sr_context *ctx, xen_pfn_t pfn)
> +{
> +    xc_interface *xch = ctx->xch;
> +    bool success = false;
> +    int rc = -1, done;
> +    unsigned int order;
> +    unsigned long i;
> +    unsigned long stat_1g = 0, stat_2m = 0, stat_4k = 0;
> +    unsigned long idx_1g, idx_2m;
> +    unsigned long count;
> +    xen_pfn_t base_pfn = 0, extnt;
> +
> +    if (xc_sr_test_bit(pfn, &ctx->x86_hvm.restore.allocated_pfns))

Style is wrong here and in some other places.

> +        return 0;
> +
> +    idx_1g = pfn >> SUPERPAGE_1GB_SHIFT;
> +    idx_2m = pfn >> SUPERPAGE_2MB_SHIFT;
> +    if (!xc_sr_bitmap_resize(&ctx->x86_hvm.restore.attempted_1g, idx_1g))
> +    {
> +        PERROR("Failed to realloc attempted_1g");
> +        return -1;
> +    }
> +    if (!xc_sr_bitmap_resize(&ctx->x86_hvm.restore.attempted_2m, idx_2m))
> +    {
> +        PERROR("Failed to realloc attempted_2m");
> +        return -1;
> +    }
> +    DPRINTF("idx_1g %lu idx_2m %lu\n", idx_1g, idx_2m);
> +    if (!xc_sr_test_and_set_bit(idx_1g, &ctx->x86_hvm.restore.attempted_1g)) {
> +        order = SUPERPAGE_1GB_SHIFT;
> +        count = 1UL << order;
> +        base_pfn = (pfn >> order) << order;
> +        extnt = base_pfn;
> +        done = xc_domain_populate_physmap(xch, ctx->domid, 1, order, 0, &extnt);
> +        DPRINTF("1G base_pfn %" PRI_xen_pfn " done %d\n", base_pfn, done);
> +        if (done > 0) {
> +            struct xc_sr_bitmap *bm = &ctx->x86_hvm.restore.attempted_2m;
> +            success = true;
> +            stat_1g = done;
> +            for (i = 0; i < (count >> SUPERPAGE_2MB_SHIFT); i++)
> +                xc_sr_set_bit((base_pfn >> SUPERPAGE_2MB_SHIFT) + i, bm);
> +        }
> +    }
> +
> +    if (!xc_sr_test_and_set_bit(idx_2m, &ctx->x86_hvm.restore.attempted_2m)) {
> +        order = SUPERPAGE_2MB_SHIFT;
> +        count = 1UL << order;
> +        base_pfn = (pfn >> order) << order;
> +        extnt = base_pfn;
> +        done = xc_domain_populate_physmap(xch, ctx->domid, 1, order, 0, &extnt);
> +        DPRINTF("2M base_pfn %" PRI_xen_pfn " done %d\n", base_pfn, done);
> +        if (done > 0) {
> +            success = true;
> +            stat_2m = done;
> +        }
> +    }
> +    if (success == false) {
> +        count = 1;
> +        extnt = base_pfn = pfn;
> +        done = xc_domain_populate_physmap(xch, ctx->domid, count, 0, 0, &extnt);
> +        if (done > 0) {
> +            success = true;
> +            stat_4k = count;
> +        }
> +    }
> +    DPRINTF("count %lu 1G %lu 2M %lu 4k %lu\n", count, stat_1g, stat_2m, stat_4k);
> +    if (success == true) {
> +        do {
> +            count--;
> +            rc = pfn_set_allocated(ctx, base_pfn + count);
> +            if (rc)
> +                break;
> +        } while (count);
> +    }
> +    return rc;
> +}
> +
> +static int x86_hvm_populate_pfns(struct xc_sr_context *ctx, unsigned count,
> +                                 const xen_pfn_t *original_pfns,
> +                                 const uint32_t *types)
> +{
> +    xc_interface *xch = ctx->xch;
> +    xen_pfn_t min_pfn = original_pfns[0], max_pfn = original_pfns[0];
> +    unsigned i;
> +    int rc = -1;
> +
> +    for ( i = 0; i < count; ++i )
> +    {
> +        if (original_pfns[i] < min_pfn)
> +            min_pfn = original_pfns[i];
> +        if (original_pfns[i] > max_pfn)
> +            max_pfn = original_pfns[i];
> +        if ( (types[i] != XEN_DOMCTL_PFINFO_XTAB &&
> +              types[i] != XEN_DOMCTL_PFINFO_BROKEN) &&
> +             !pfn_is_populated(ctx, original_pfns[i]) )
> +        {
> +            rc = x86_hvm_allocate_pfn(ctx, original_pfns[i]);
> +            if ( rc )
> +                goto err;
> +            rc = pfn_set_populated(ctx, original_pfns[i]);
> +            if ( rc )
> +                goto err;
> +        }
> +    }

One potential issue I can see with your algorithm is, if the stream of
page info contains pages from different super pages, the risk of going
over memory limit is high (hence failing the migration).

Is my concern unfounded?

> +
> +    while (min_pfn < max_pfn)
> +    {
> +        if (!xc_sr_bitmap_resize(&ctx->x86_hvm.restore.allocated_pfns, min_pfn))
> +        {
> +            PERROR("Failed to realloc allocated_pfns %" PRI_xen_pfn, min_pfn);
> +            goto err;
> +        }
> +        if (!pfn_is_populated(ctx, min_pfn) &&
> +            xc_sr_test_and_clear_bit(min_pfn, &ctx->x86_hvm.restore.allocated_pfns)) {
> +            xen_pfn_t pfn = min_pfn;
> +            rc = xc_domain_decrease_reservation_exact(xch, ctx->domid, 1, 0, &pfn);
> +            if ( rc )
> +            {
> +                PERROR("Failed to release pfn %" PRI_xen_pfn, min_pfn);
> +                goto err;
> +            }
> +        }
> +        min_pfn++;
> +    }
> +
> +    rc = 0;
> +
> + err:
> +    return rc;
> +}
> +
> +
>  struct xc_sr_restore_ops restore_ops_x86_hvm =
>  {
>      .pfn_is_valid    = x86_hvm_pfn_is_valid,
> @@ -236,6 +415,7 @@ struct xc_sr_restore_ops restore_ops_x86_hvm =
>      .set_page_type   = x86_hvm_set_page_type,
>      .localise_page   = x86_hvm_localise_page,
>      .setup           = x86_hvm_setup,
> +    .populate_pfns   = x86_hvm_populate_pfns,
>      .process_record  = x86_hvm_process_record,
>      .stream_complete = x86_hvm_stream_complete,
>      .cleanup         = x86_hvm_cleanup,
> diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
> index 50e25c162c..87957559bc 100644
> --- a/tools/libxc/xc_sr_restore_x86_pv.c
> +++ b/tools/libxc/xc_sr_restore_x86_pv.c
> @@ -936,6 +936,75 @@ static void x86_pv_set_gfn(struct xc_sr_context *ctx, xen_pfn_t pfn,
>          ((uint32_t *)ctx->x86_pv.p2m)[pfn] = mfn;
>  }
>  
> +/*
> + * Given a set of pfns, obtain memory from Xen to fill the physmap for the
> + * unpopulated subset.  If types is NULL, no page type checking is performed
> + * and all unpopulated pfns are populated.
> + */
> +static int x86_pv_populate_pfns(struct xc_sr_context *ctx, unsigned count,
> +                                const xen_pfn_t *original_pfns,
> +                                const uint32_t *types)
> +{
> +    xc_interface *xch = ctx->xch;
> +    xen_pfn_t *mfns = malloc(count * sizeof(*mfns)),
> +        *pfns = malloc(count * sizeof(*pfns));
> +    unsigned i, nr_pfns = 0;
> +    int rc = -1;
> +
> +    if ( !mfns || !pfns )
> +    {
> +        ERROR("Failed to allocate %zu bytes for populating the physmap",
> +              2 * count * sizeof(*mfns));
> +        goto err;
> +    }
> +
> +    for ( i = 0; i < count; ++i )
> +    {
> +        if ( (!types || (types &&
> +                         (types[i] != XEN_DOMCTL_PFINFO_XTAB &&
> +                          types[i] != XEN_DOMCTL_PFINFO_BROKEN))) &&
> +             !pfn_is_populated(ctx, original_pfns[i]) )
> +        {
> +            rc = pfn_set_populated(ctx, original_pfns[i]);
> +            if ( rc )
> +                goto err;
> +            pfns[nr_pfns] = mfns[nr_pfns] = original_pfns[i];
> +            ++nr_pfns;
> +        }
> +    }
> +
> +    if ( nr_pfns )
> +    {
> +        rc = xc_domain_populate_physmap_exact(
> +            xch, ctx->domid, nr_pfns, 0, 0, mfns);
> +        if ( rc )
> +        {
> +            PERROR("Failed to populate physmap");
> +            goto err;
> +        }
> +
> +        for ( i = 0; i < nr_pfns; ++i )
> +        {
> +            if ( mfns[i] == INVALID_MFN )
> +            {
> +                ERROR("Populate physmap failed for pfn %u", i);
> +                rc = -1;
> +                goto err;
> +            }
> +
> +            ctx->restore.ops.set_gfn(ctx, pfns[i], mfns[i]);
> +        }
> +    }
> +
> +    rc = 0;
> +
> + err:
> +    free(pfns);
> +    free(mfns);
> +
> +    return rc;
> +}
> +

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 1280 bytes --]

On Tue, Aug 22, Wei Liu wrote:

> On Thu, Aug 17, 2017 at 07:01:33PM +0200, Olaf Hering wrote:
> > +    /* No superpage in 1st 2MB due to VGA hole */
> > +    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_1g);
> > +    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_2m);
> I don't quite get this. What about other holes such as MMIO?

This just copies what meminit_hvm does. Is there a way to know where the
MMIO hole is? Maybe I just missed the MMIO part. In the worst case I
think a super page is allocated, which is later split into single pages.

> One potential issue I can see with your algorithm is, if the stream of
> page info contains pages from different super pages, the risk of going
> over memory limit is high (hence failing the migration).
> 
> Is my concern unfounded?

In my testing I have seen the case of over-allocation. Thats why I
implemented the freeing of unpopulated parts. It would be nice to know
how many pages are actually coming. I think this info is not available.

On the other side, the first iteration sends the pfns linear. This is
when the allocation actually happens. So the over-allocation will only
trigger near the end, if a 1G range is allocated but only a few pages
will be stored into this range.

Olaf

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 707 bytes --]

On Tue, Aug 22, Olaf Hering wrote:

> In my testing I have seen the case of over-allocation. Thats why I
> implemented the freeing of unpopulated parts. It would be nice to know
> how many pages are actually coming. I think this info is not available.

If the receiving dom0 recognizes an over-allocation it must know how
much memory a domU is supposed to have. Perhaps there is a way to
retreive this info.

An interesting case is ballooning during migration. Is the new amount of
pages per domU actually transfered to the receiving domU? If the domU is
ballooned up the other side may see the incoming domU as over-allocated.
If it is ballooned down pages may be missing. Was this ever considered?


Olaf

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Wei Liu]

On Wed, Aug 23, 2017 at 10:05:37AM +0200, Olaf Hering wrote:
> On Tue, Aug 22, Olaf Hering wrote:
> 
> > In my testing I have seen the case of over-allocation. Thats why I
> > implemented the freeing of unpopulated parts. It would be nice to know
> > how many pages are actually coming. I think this info is not available.
> 
> If the receiving dom0 recognizes an over-allocation it must know how
> much memory a domU is supposed to have. Perhaps there is a way to
> retreive this info.
> 

Dom0 probably gets an error from Xen about failing to allocate memory,
but I'm not sure it can tell whether it is due to DomU trying to use
more than it should or Xen is oom.

> An interesting case is ballooning during migration. Is the new amount of
> pages per domU actually transfered to the receiving domU? If the domU is
> ballooned up the other side may see the incoming domU as over-allocated.
> If it is ballooned down pages may be missing. Was this ever considered?
> 

No, I don't think that's covered.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Wei Liu]

On Tue, Aug 22, 2017 at 05:53:25PM +0200, Olaf Hering wrote:
> On Tue, Aug 22, Wei Liu wrote:
> 
> > On Thu, Aug 17, 2017 at 07:01:33PM +0200, Olaf Hering wrote:
> > > +    /* No superpage in 1st 2MB due to VGA hole */
> > > +    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_1g);
> > > +    xc_sr_set_bit(0, &ctx->x86_hvm.restore.attempted_2m);
> > I don't quite get this. What about other holes such as MMIO?
> 
> This just copies what meminit_hvm does. Is there a way to know where the
> MMIO hole is? Maybe I just missed the MMIO part. In the worst case I
> think a super page is allocated, which is later split into single pages.

That's a bit different from migration. IIRC hvmloader is responsible for
shuffling pages around. I don't think that is applicable to migration.
> 
> > One potential issue I can see with your algorithm is, if the stream of
> > page info contains pages from different super pages, the risk of going
> > over memory limit is high (hence failing the migration).
> > 
> > Is my concern unfounded?
> 
> In my testing I have seen the case of over-allocation. Thats why I
> implemented the freeing of unpopulated parts. It would be nice to know
> how many pages are actually coming. I think this info is not available.
> 

Not sure I follow. What do you mean by "how many pages are actually
coming"?

> On the other side, the first iteration sends the pfns linear. This is
> when the allocation actually happens. So the over-allocation will only
> trigger near the end, if a 1G range is allocated but only a few pages
> will be stored into this range.

This could be making too many assumptions on the data stream.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 1101 bytes --]

On Wed, Aug 23, Wei Liu wrote:

> On Tue, Aug 22, 2017 at 05:53:25PM +0200, Olaf Hering wrote:
> > In my testing I have seen the case of over-allocation. Thats why I
> > implemented the freeing of unpopulated parts. It would be nice to know
> > how many pages are actually coming. I think this info is not available.
> Not sure I follow. What do you mean by "how many pages are actually
> coming"?

This meant the expected number of pages to populate.

The value of p2m_size does not represent the actual number of pages
assigned to a domU. This info is stored in getdomaininfo.max_pages,
which is currently not used by restore. I will see if using this value
will avoid triggering the Over-allocation check.

> > On the other side, the first iteration sends the pfns linear. This is
> > when the allocation actually happens. So the over-allocation will only
> > trigger near the end, if a 1G range is allocated but only a few pages
> > will be stored into this range.
> This could be making too many assumptions on the data stream.

With the usage of max_pages some assumptions can be avoided.

Olaf

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] tools/libxc: use superpages during restore of HVM guest

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 4561 bytes --]

On Wed, Aug 23, Olaf Hering wrote:

> The value of p2m_size does not represent the actual number of pages
> assigned to a domU. This info is stored in getdomaininfo.max_pages,
> which is currently not used by restore. I will see if using this value
> will avoid triggering the Over-allocation check.

This untested change ontop of this series (done with git diff -w -b
base..HEAD) does some accounting to avoid Over-allocation:

diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index 26c45fdd6d..e0321ea224 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -234,6 +234,8 @@ struct xc_sr_context
 
             int send_back_fd;
             unsigned long p2m_size;
+            unsigned long max_pages;
+            unsigned long tot_pages;
             xc_hypercall_buffer_t dirty_bitmap_hbuf;
 
             /* From Image Header. */
@@ -375,6 +377,7 @@ static inline bool xc_sr_bitmap_resize(struct xc_sr_bitmap *bm, unsigned long bi
 static inline void xc_sr_bitmap_free(struct xc_sr_bitmap *bm)
 {
     free(bm->p);
+    bm->p = NULL;
 }
 
 static inline bool xc_sr_set_bit(unsigned long bit, struct xc_sr_bitmap *bm)
diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 1f9fe25b8f..eff24d3805 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -758,6 +758,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
         return -1;
     }
 
+    /* See xc_domain_getinfo */
+    ctx.restore.max_pages = ctx.dominfo.max_memkb >> (PAGE_SHIFT-10);
+    ctx.restore.tot_pages = ctx.dominfo.nr_pages;
     ctx.restore.p2m_size = nr_pfns;
 
     if ( ctx.dominfo.hvm )
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 60454148db..f2932dafb7 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -278,7 +278,8 @@ static int pfn_set_allocated(struct xc_sr_context *ctx, xen_pfn_t pfn)
 static int x86_hvm_allocate_pfn(struct xc_sr_context *ctx, xen_pfn_t pfn)
 {
     xc_interface *xch = ctx->xch;
-    bool success = false;
+    struct xc_sr_bitmap *bm;
+    bool success = false, do_sp;
     int rc = -1, done;
     unsigned int order;
     unsigned long i;
@@ -303,15 +304,18 @@ static int x86_hvm_allocate_pfn(struct xc_sr_context *ctx, xen_pfn_t pfn)
         return -1;
     }
     DPRINTF("idx_1g %lu idx_2m %lu\n", idx_1g, idx_2m);
-    if (!xc_sr_test_and_set_bit(idx_1g, &ctx->x86_hvm.restore.attempted_1g)) {
+
+    bm = &ctx->x86_hvm.restore.attempted_1g;
     order = SUPERPAGE_1GB_SHIFT;
     count = 1UL << order;
+    do_sp = ctx->restore.tot_pages + count <= ctx->restore.max_pages;
+    if ( do_sp && !xc_sr_test_and_set_bit(idx_1g, bm) ) {
         base_pfn = (pfn >> order) << order;
         extnt = base_pfn;
         done = xc_domain_populate_physmap(xch, ctx->domid, 1, order, 0, &extnt);
         DPRINTF("1G base_pfn %" PRI_xen_pfn " done %d\n", base_pfn, done);
         if ( done > 0 ) {
-            struct xc_sr_bitmap *bm = &ctx->x86_hvm.restore.attempted_2m;
+            bm = &ctx->x86_hvm.restore.attempted_2m;
             success = true;
             stat_1g = done;
             for ( i = 0; i < (count >> SUPERPAGE_2MB_SHIFT); i++ )
@@ -319,9 +323,11 @@ static int x86_hvm_allocate_pfn(struct xc_sr_context *ctx, xen_pfn_t pfn)
         }
     }
 
-    if (!xc_sr_test_and_set_bit(idx_2m, &ctx->x86_hvm.restore.attempted_2m)) {
+    bm = &ctx->x86_hvm.restore.attempted_2m;
     order = SUPERPAGE_2MB_SHIFT;
     count = 1UL << order;
+    do_sp = ctx->restore.tot_pages + count <= ctx->restore.max_pages;
+    if ( do_sp && !xc_sr_test_and_set_bit(idx_2m, bm) ) {
         base_pfn = (pfn >> order) << order;
         extnt = base_pfn;
         done = xc_domain_populate_physmap(xch, ctx->domid, 1, order, 0, &extnt);
@@ -344,6 +350,7 @@ static int x86_hvm_allocate_pfn(struct xc_sr_context *ctx, xen_pfn_t pfn)
     if ( success == true ) {
         do {
             count--;
+            ctx->restore.tot_pages++;
             rc = pfn_set_allocated(ctx, base_pfn + count);
             if ( rc )
                 break;
@@ -396,6 +403,7 @@ static int x86_hvm_populate_pfns(struct xc_sr_context *ctx, unsigned count,
                 PERROR("Failed to release pfn %" PRI_xen_pfn, min_pfn);
                 goto err;
             }
+            ctx->restore.tot_pages--;
         }
         min_pfn++;
     }

Olaf

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel