[PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()

[PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()

[Wei Liu]

From: Vitaly Kuznetsov <vkuznets@redhat.com>

Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
introduced target_memkb adjustment for HVM PoD domains on create. The
adjustment is however being reset on reload_domain_config() (e.g. when
we reboot the guest). For example:

I'm trying to create HVM PoD domain with the following settings:
memory=1024
maxmem=4096
When the domain boots for the first time I see the following in Xen:

(XEN)     nr_pages=262145 ... max_pages=262400 (looks good)

When I reboot the domain I see the following:
(XEN)     nr_pages=261889 ... max_pages=262144  (oops)

After second reboot:
(XEN)     nr_pages=261633 ... max_pages=261888 (even less)

And eventually the domain crashes with "p2m_pod_demand_populate: Dom4 out of
PoD memory! (tot=261377 ents=787200 dom4)".

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/libxl/libxl.c          |  8 ++++----
 tools/libxl/libxl_dom.c      | 10 ++--------
 tools/libxl/libxl_internal.h | 15 +++++++++++++++
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b9d855b..5aaf02d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7229,12 +7229,12 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
             LOG(ERROR, "fail to get memory target for domain %d", domid);
             goto out;
         }
-        /* Target memory in xenstore is different from what user has
-         * asked for. The difference is video_memkb. See
-         * libxl_set_memory_target.
+
+        /* libxl__get_targetmem_difference() calculates the difference from
+         * what is in xenstore to what we have in the domain build info.
          */
         d_config->b_info.target_memkb = target_memkb +
-            d_config->b_info.video_memkb;
+            libxl__get_targetmem_difference(gc, &d_config->b_info);
 
         d_config->b_info.max_memkb = max_memkb;
     }
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 9b20cf5..20fd458 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -490,7 +490,6 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
     xs_transaction_t t;
     char **ents;
     int i, rc;
-    int64_t mem_target_fudge;
 
     if (info->num_vnuma_nodes && !info->num_vcpu_soft_affinity) {
         rc = set_vnuma_affinity(gc, domid, info);
@@ -523,17 +522,12 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
         }
     }
 
-    mem_target_fudge =
-        (info->type == LIBXL_DOMAIN_TYPE_HVM &&
-         info->max_memkb > info->target_memkb)
-        ? LIBXL_MAXMEM_CONSTANT : 0;
-
     ents = libxl__calloc(gc, 12 + (info->max_vcpus * 2) + 2, sizeof(char *));
     ents[0] = "memory/static-max";
     ents[1] = GCSPRINTF("%"PRId64, info->max_memkb);
     ents[2] = "memory/target";
-    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb - info->video_memkb
-                        - mem_target_fudge);
+    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb -
+                        libxl__get_targetmem_difference(gc, info));
     ents[4] = "memory/videoram";
     ents[5] = GCSPRINTF("%"PRId64, info->video_memkb);
     ents[6] = "domid";
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fac5751..59df53d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4107,6 +4107,21 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
     libxl_uuid_copy(CTX, &dst->uuid, &src->uuid);
 }
 
+/* Target memory in xenstore is different from what user has
+ * asked for. The difference is video_memkb + (possible) fudge.
+ * See libxl_set_memory_target.
+ */
+static inline
+uint64_t libxl__get_targetmem_difference(libxl__gc *gc,
+                                         const libxl_domain_build_info *info)
+{
+    int64_t mem_target_fudge = (info->type == LIBXL_DOMAIN_TYPE_HVM &&
+                                info->max_memkb > info->target_memkb)
+                                ? LIBXL_MAXMEM_CONSTANT : 0;
+
+    return info->video_memkb + mem_target_fudge;
+}
+
 /* Macros used to compare device identifier. Returns true if the two
  * devices have same identifier. */
 #define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid)
-- 
2.1.4


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

Re: [PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()

[George Dunlap]

On Tue, May 31, 2016 at 6:42 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
> introduced target_memkb adjustment for HVM PoD domains on create. The
> adjustment is however being reset on reload_domain_config() (e.g. when
> we reboot the guest).

It was determined that all of the following...

>  For example:
>
> I'm trying to create HVM PoD domain with the following settings:
> memory=1024
> maxmem=4096
> When the domain boots for the first time I see the following in Xen:
>
> (XEN)     nr_pages=262145 ... max_pages=262400 (looks good)
>
> When I reboot the domain I see the following:
> (XEN)     nr_pages=261889 ... max_pages=262144  (oops)
>
> After second reboot:
> (XEN)     nr_pages=261633 ... max_pages=261888 (even less)
>
> And eventually the domain crashes with "p2m_pod_demand_populate: Dom4 out of
> PoD memory! (tot=261377 ents=787200 dom4)".

has actually nothing to do with this patch; and that actually,
contrary to the description above, the problem is that the adjustment
is *not* being reset.

A better description would be something like this:

---
Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
introduced target_memkb adjustment for HVM PoD domains on create,
wherein
the value it wrote to target is always 1MiB lower than the actual
target_memkb.  Unfortunately, on reboot, it is this value which is
read *unmodified* to feed into the next domain creation; from which
1MiB is subtracted *again*.  This means that any guest which reboots
with memory < maxmem will have its memory target decreased by 1MiB on
every boot.

This patch makes it so that when reading target on reboot, we adjust
the value we read *up* by 1MiB, so that the domain will be build with
the appropriate amount of memory and the target will remain the same
after reboot.

This is still not quite a complete fix, as the 1MiB offset is only
subtracted when creating or rebooting; it is not subtracted when 'xl
set-memory' is called.  But it will prevent any situations where
memory is continually increased or decreased.  A better fix will have
to wait until after the release.
---

 -George

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

Re: [PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()

[Vitaly Kuznetsov]

George Dunlap <George.Dunlap@eu.citrix.com> writes:

> On Tue, May 31, 2016 at 6:42 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>>
>> Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
>> introduced target_memkb adjustment for HVM PoD domains on create. The
>> adjustment is however being reset on reload_domain_config() (e.g. when
>> we reboot the guest).
>
> It was determined that all of the following...
>
>>  For example:
>>
>> I'm trying to create HVM PoD domain with the following settings:
>> memory=1024
>> maxmem=4096
>> When the domain boots for the first time I see the following in Xen:
>>
>> (XEN)     nr_pages=262145 ... max_pages=262400 (looks good)
>>
>> When I reboot the domain I see the following:
>> (XEN)     nr_pages=261889 ... max_pages=262144  (oops)
>>
>> After second reboot:
>> (XEN)     nr_pages=261633 ... max_pages=261888 (even less)
>>
>> And eventually the domain crashes with "p2m_pod_demand_populate: Dom4 out of
>> PoD memory! (tot=261377 ents=787200 dom4)".
>
> has actually nothing to do with this patch; and that actually,
> contrary to the description above, the problem is that the adjustment
> is *not* being reset.
>
> A better description would be something like this:
>
> ---
> Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
> introduced target_memkb adjustment for HVM PoD domains on create,
> wherein
> the value it wrote to target is always 1MiB lower than the actual
> target_memkb.  Unfortunately, on reboot, it is this value which is
> read *unmodified* to feed into the next domain creation; from which
> 1MiB is subtracted *again*.  This means that any guest which reboots
> with memory < maxmem will have its memory target decreased by 1MiB on
> every boot.
>
> This patch makes it so that when reading target on reboot, we adjust
> the value we read *up* by 1MiB, so that the domain will be build with
> the appropriate amount of memory and the target will remain the same
> after reboot.
>
> This is still not quite a complete fix, as the 1MiB offset is only
> subtracted when creating or rebooting; it is not subtracted when 'xl
> set-memory' is called.  But it will prevent any situations where
> memory is continually increased or decreased.  A better fix will have
> to wait until after the release.
> ---

Yes, thanks, this description is better. Wei, please let me know if you
want me to re-send the patch with this description or if it's possible
to make the adjustment upon commit.

-- 
  Vitaly

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

Re: [PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()

[Wei Liu]

On Wed, Jun 01, 2016 at 10:29:36AM +0200, Vitaly Kuznetsov wrote:
> George Dunlap <George.Dunlap@eu.citrix.com> writes:
> 
> > On Tue, May 31, 2016 at 6:42 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> >>
> >> Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
> >> introduced target_memkb adjustment for HVM PoD domains on create. The
> >> adjustment is however being reset on reload_domain_config() (e.g. when
> >> we reboot the guest).
> >
> > It was determined that all of the following...
> >
> >>  For example:
> >>
> >> I'm trying to create HVM PoD domain with the following settings:
> >> memory=1024
> >> maxmem=4096
> >> When the domain boots for the first time I see the following in Xen:
> >>
> >> (XEN)     nr_pages=262145 ... max_pages=262400 (looks good)
> >>
> >> When I reboot the domain I see the following:
> >> (XEN)     nr_pages=261889 ... max_pages=262144  (oops)
> >>
> >> After second reboot:
> >> (XEN)     nr_pages=261633 ... max_pages=261888 (even less)
> >>
> >> And eventually the domain crashes with "p2m_pod_demand_populate: Dom4 out of
> >> PoD memory! (tot=261377 ents=787200 dom4)".
> >
> > has actually nothing to do with this patch; and that actually,
> > contrary to the description above, the problem is that the adjustment
> > is *not* being reset.
> >
> > A better description would be something like this:
> >
> > ---
> > Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
> > introduced target_memkb adjustment for HVM PoD domains on create,
> > wherein
> > the value it wrote to target is always 1MiB lower than the actual
> > target_memkb.  Unfortunately, on reboot, it is this value which is
> > read *unmodified* to feed into the next domain creation; from which
> > 1MiB is subtracted *again*.  This means that any guest which reboots
> > with memory < maxmem will have its memory target decreased by 1MiB on
> > every boot.
> >
> > This patch makes it so that when reading target on reboot, we adjust
> > the value we read *up* by 1MiB, so that the domain will be build with
> > the appropriate amount of memory and the target will remain the same
> > after reboot.
> >
> > This is still not quite a complete fix, as the 1MiB offset is only
> > subtracted when creating or rebooting; it is not subtracted when 'xl
> > set-memory' is called.  But it will prevent any situations where
> > memory is continually increased or decreased.  A better fix will have
> > to wait until after the release.
> > ---
> 
> Yes, thanks, this description is better. Wei, please let me know if you
> want me to re-send the patch with this description or if it's possible
> to make the adjustment upon commit.
> 

No need to resend. I will wait for Ian to review as well and take care
of everything.

Wei.

> -- 
>   Vitaly

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

Re: [PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()

[Ian Jackson]

Wei Liu writes ("[Xen-devel] [PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()"):
> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
> introduced target_memkb adjustment for HVM PoD domains on create. The
> adjustment is however being reset on reload_domain_config() (e.g. when
> we reboot the guest). For example:

With George's revised commit message this patch makes sense for 4.7.

I would like to see this function retain the name "fudge" though:

> -    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb - info->video_memkb
> -                        - mem_target_fudge);
> +    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb -
> +                        libxl__get_targetmem_difference(gc, info));

ie:

  +    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb -
  +                        libxl__get_targetmem_fudge(gc, info));

This makes it clear that there is still a problem here (and it will
help things like "git grep -G").

With that name changed, and George's commit message, you may put my
ack on this.


George writes:

> better fix will have to wait until after the release.
 
Realistically speaking, that is quite optimistic.  Empirically, we
keep squelching into the memory accounting swamp, but we have as yet
failed to drain it.  Instead our code keeps accreting bodges in this
area, and the swamp keeps growing...

Ian.

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

Re: [PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()

[Wei Liu]

On Wed, Jun 01, 2016 at 04:48:47PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[Xen-devel] [PATCH for-4.7] libxl: keep PoD target adjustment by memory fudge after reload_domain_config()"):
> > From: Vitaly Kuznetsov <vkuznets@redhat.com>
> > 
> > Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
> > introduced target_memkb adjustment for HVM PoD domains on create. The
> > adjustment is however being reset on reload_domain_config() (e.g. when
> > we reboot the guest). For example:
> 
> With George's revised commit message this patch makes sense for 4.7.
> 
> I would like to see this function retain the name "fudge" though:
> 
> > -    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb - info->video_memkb
> > -                        - mem_target_fudge);
> > +    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb -
> > +                        libxl__get_targetmem_difference(gc, info));
> 
> ie:
> 
>   +    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb -
>   +                        libxl__get_targetmem_fudge(gc, info));
> 
> This makes it clear that there is still a problem here (and it will
> help things like "git grep -G").
> 
> With that name changed, and George's commit message, you may put my
> ack on this.
> 
> 

Thanks everyone.

Updated patch here and I will push it shortly.

---8<---
From b9d63c2da58471a767852ab68111d245ee8d195f Mon Sep 17 00:00:00 2001
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Wed, 3 Feb 2016 16:53:03 +0100
Subject: [PATCH] libxl: keep PoD target adjustment by memory fudge after
 reload_domain_config()

Commit 56fb5fd623 ("libxl: adjust PoD target by memory fudge, too")
introduced target_memkb adjustment for HVM PoD domains on create,
wherein the value it wrote to target is always 1MiB lower than the
actual target_memkb.  Unfortunately, on reboot, it is this value which
is read *unmodified* to feed into the next domain creation; from which
1MiB is subtracted *again*.  This means that any guest which reboots
with memory < maxmem will have its memory target decreased by 1MiB on
every boot.

This patch makes it so that when reading target on reboot, we adjust the
value we read *up* by 1MiB, so that the domain will be build with the
appropriate amount of memory and the target will remain the same after
reboot.

This is still not quite a complete fix, as the 1MiB offset is only
subtracted when creating or rebooting; it is not subtracted when 'xl
set-memory' is called.  But it will prevent any situations where memory
is continually increased or decreased.  A better fix will have to wait
until after the release.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl.c          |  8 ++++----
 tools/libxl/libxl_dom.c      | 10 ++--------
 tools/libxl/libxl_internal.h | 15 +++++++++++++++
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b9d855b..306984b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7229,12 +7229,12 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
             LOG(ERROR, "fail to get memory target for domain %d", domid);
             goto out;
         }
-        /* Target memory in xenstore is different from what user has
-         * asked for. The difference is video_memkb. See
-         * libxl_set_memory_target.
+
+        /* libxl__get_targetmem_fudge() calculates the difference from
+         * what is in xenstore to what we have in the domain build info.
          */
         d_config->b_info.target_memkb = target_memkb +
-            d_config->b_info.video_memkb;
+            libxl__get_targetmem_fudge(gc, &d_config->b_info);
 
         d_config->b_info.max_memkb = max_memkb;
     }
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 9b20cf5..805774f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -490,7 +490,6 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
     xs_transaction_t t;
     char **ents;
     int i, rc;
-    int64_t mem_target_fudge;
 
     if (info->num_vnuma_nodes && !info->num_vcpu_soft_affinity) {
         rc = set_vnuma_affinity(gc, domid, info);
@@ -523,17 +522,12 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
         }
     }
 
-    mem_target_fudge =
-        (info->type == LIBXL_DOMAIN_TYPE_HVM &&
-         info->max_memkb > info->target_memkb)
-        ? LIBXL_MAXMEM_CONSTANT : 0;
-
     ents = libxl__calloc(gc, 12 + (info->max_vcpus * 2) + 2, sizeof(char *));
     ents[0] = "memory/static-max";
     ents[1] = GCSPRINTF("%"PRId64, info->max_memkb);
     ents[2] = "memory/target";
-    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb - info->video_memkb
-                        - mem_target_fudge);
+    ents[3] = GCSPRINTF("%"PRId64, info->target_memkb -
+                        libxl__get_targetmem_fudge(gc, info));
     ents[4] = "memory/videoram";
     ents[5] = GCSPRINTF("%"PRId64, info->video_memkb);
     ents[6] = "domid";
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fac5751..3bdeb3c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4107,6 +4107,21 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
     libxl_uuid_copy(CTX, &dst->uuid, &src->uuid);
 }
 
+/* Target memory in xenstore is different from what user has
+ * asked for. The difference is video_memkb + (possible) fudge.
+ * See libxl_set_memory_target.
+ */
+static inline
+uint64_t libxl__get_targetmem_fudge(libxl__gc *gc,
+                                    const libxl_domain_build_info *info)
+{
+    int64_t mem_target_fudge = (info->type == LIBXL_DOMAIN_TYPE_HVM &&
+                                info->max_memkb > info->target_memkb)
+                                ? LIBXL_MAXMEM_CONSTANT : 0;
+
+    return info->video_memkb + mem_target_fudge;
+}
+
 /* Macros used to compare device identifier. Returns true if the two
  * devices have same identifier. */
 #define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid)
-- 
2.1.4


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