[PATCH 0/4] fix freemem loop

[PATCH 0/4] fix freemem loop

[Stefano Stabellini]

Hi all,

this patch series fixes the freemem loop on machines with very large
amount of memory, where the current wait time is not enough.

In order to be able to handle arbitrarly large amount of ram, we
implement in libxl_wait_for_memory_target a policy of waiting until dom0
is making progress.  The patch series also reverts "libxl: Wait for
ballooning if free memory is increasing", that is not actually
implemented correctly.


Stefano Stabellini (4):
      Revert "libxl: Wait for ballooning if free memory is increasing"
      libxl_wait_for_memory_target: wait as long as dom0 is making progress
      freemem: remove call to libxl_wait_for_free_memory
      libxl_wait_for_memory_target: wait for 2 sec at a time

 tools/libxl/libxl.c      |   31 +++++++++++++++++++++++--------
 tools/libxl/xl_cmdimpl.c |   29 ++++++-----------------------
 2 files changed, 29 insertions(+), 31 deletions(-)

Cheers,

Stefano

[PATCH 1/4] Revert "libxl: Wait for ballooning if free memory is increasing"

[Stefano Stabellini]

This reverts commit 2563bca11544361dc2afa5e20b5663e10a0715cb.
---
 tools/libxl/xl_cmdimpl.c |   17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index e41f633..f4c4122 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2198,9 +2198,8 @@ static int preserve_domain(uint32_t *r_domid, libxl_event *event,
 
 static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
 {
-    int rc, retries;
-    const int MAX_RETRIES = 3;
-    uint32_t need_memkb, free_memkb, free_memkb_prev = 0;
+    int rc, retries = 3;
+    uint32_t need_memkb, free_memkb;
 
     if (!autoballoon)
         return 0;
@@ -2209,7 +2208,6 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
     if (rc < 0)
         return rc;
 
-    retries = MAX_RETRIES;
     do {
         rc = libxl_get_free_memory(ctx, &free_memkb);
         if (rc < 0)
@@ -2234,16 +2232,7 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
         if (rc < 0)
             return rc;
 
-        /*
-         * If the amount of free mem has increased on this iteration (i.e.
-         * some progress has been made) then reset the retry counter.
-         */
-        if (free_memkb > free_memkb_prev) {
-            retries = MAX_RETRIES;
-            free_memkb_prev = free_memkb;
-        } else {
-            retries--;
-        }
+        retries--;
     } while (retries > 0);
 
     return ERROR_NOMEM;
-- 
1.7.10.4

[PATCH 2/4] libxl_wait_for_memory_target: wait as long as dom0 is making progress

[Stefano Stabellini]

Decrement wait_secs only if dom0 is making no progress toward reaching
the balloon target, otherwise loop again for free.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Tested-by: Mike Latimer <mlatimer@suse.com>
---
 tools/libxl/libxl.c      |   29 ++++++++++++++++++++++-------
 tools/libxl/xl_cmdimpl.c |    4 ++--
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 088786e..648a227 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4959,26 +4959,41 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
 {
     int rc = 0;
     uint32_t target_memkb = 0;
+    uint64_t current_memkb, prev_memkb;
     libxl_dominfo info;
 
+    rc = libxl_get_memory_target(ctx, domid, &target_memkb);
+    if (rc < 0)
+        return rc;
+
     libxl_dominfo_init(&info);
+    prev_memkb = UINT64_MAX;
 
     do {
-        wait_secs--;
         sleep(1);
 
-        rc = libxl_get_memory_target(ctx, domid, &target_memkb);
-        if (rc < 0)
-            goto out;
-
         libxl_dominfo_dispose(&info);
         libxl_dominfo_init(&info);
         rc = libxl_domain_info(ctx, &info, domid);
         if (rc < 0)
             goto out;
-    } while (wait_secs > 0 && (info.current_memkb + info.outstanding_memkb) > target_memkb);
 
-    if ((info.current_memkb + info.outstanding_memkb) <= target_memkb)
+        current_memkb = info.current_memkb + info.outstanding_memkb;
+
+        if (current_memkb > prev_memkb)
+        {
+            rc = ERROR_FAIL;
+            goto out;
+        }
+        else if (current_memkb == prev_memkb)
+            wait_secs--;
+        /* if current_memkb < prev_memkb loop for free as progress has
+         * been made */
+
+        prev_memkb = current_memkb;
+    } while (wait_secs > 0 && current_memkb > target_memkb);
+
+    if (current_memkb <= target_memkb)
         rc = 0;
     else
         rc = ERROR_FAIL;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f4c4122..2dc7574 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2226,8 +2226,8 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
         else if (rc != ERROR_NOMEM)
             return rc;
 
-        /* the memory target has been reached but the free memory is still
-         * not enough: loop over again */
+        /* wait until dom0 reaches its target, as long as we are making
+         * progress */
         rc = libxl_wait_for_memory_target(ctx, 0, 1);
         if (rc < 0)
             return rc;
-- 
1.7.10.4

[PATCH 3/4] freemem: remove call to libxl_wait_for_free_memory

[Stefano Stabellini]

Now that libxl_wait_for_memory_target is capable of waiting until dom0
reaches its target, we can remove the other wait function call:
libxl_wait_for_free_memory. No need to wait twice. Once dom0 has met its
target, simply loop again and recalculate free_memkb.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2dc7574..2a19cca 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2220,15 +2220,9 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
         if (rc < 0)
             return rc;
 
-        rc = libxl_wait_for_free_memory(ctx, domid, need_memkb, 10);
-        if (!rc)
-            return 0;
-        else if (rc != ERROR_NOMEM)
-            return rc;
-
         /* wait until dom0 reaches its target, as long as we are making
          * progress */
-        rc = libxl_wait_for_memory_target(ctx, 0, 1);
+        rc = libxl_wait_for_memory_target(ctx, 0, 10);
         if (rc < 0)
             return rc;
 
-- 
1.7.10.4

[PATCH 4/4] libxl_wait_for_memory_target: wait for 2 sec at a time

[Stefano Stabellini]

Use a 2 sec sleep time in the loop to allow the guest to release a
decent amount of memory in an iteration (empirical tests show ballooning
speed to be 512MB/sec or recent boxes).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 648a227..94b4d59 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4970,7 +4970,7 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
     prev_memkb = UINT64_MAX;
 
     do {
-        sleep(1);
+        sleep(2);
 
         libxl_dominfo_dispose(&info);
         libxl_dominfo_init(&info);
@@ -4986,7 +4986,7 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
             goto out;
         }
         else if (current_memkb == prev_memkb)
-            wait_secs--;
+            wait_secs -= 2;
         /* if current_memkb < prev_memkb loop for free as progress has
          * been made */
 
-- 
1.7.10.4

Re: [PATCH 0/4] fix freemem loop

[Mike Latimer]

On Tuesday, March 03, 2015 11:08:38 AM Stefano Stabellini wrote:
> Hi all,
> 
> this patch series fixes the freemem loop on machines with very large
> amount of memory, where the current wait time is not enough.
> 
> In order to be able to handle arbitrarly large amount of ram, we
> implement in libxl_wait_for_memory_target a policy of waiting until dom0
> is making progress.  The patch series also reverts "libxl: Wait for
> ballooning if free memory is increasing", that is not actually
> implemented correctly.
>
> Stefano Stabellini (4):
>       Revert "libxl: Wait for ballooning if free memory is increasing"
>       libxl_wait_for_memory_target: wait as long as dom0 is making progress
>       freemem: remove call to libxl_wait_for_free_memory
>       libxl_wait_for_memory_target: wait for 2 sec at a time
> 
>  tools/libxl/libxl.c      |   31 +++++++++++++++++++++++--------
>  tools/libxl/xl_cmdimpl.c |   29 ++++++-----------------------
>  2 files changed, 29 insertions(+), 31 deletions(-)

I just tested this whole series and it works well in my environment (64G - 
512G guests). dom0 now balloons down just the required amount. Also, domU 
startup works the first time, as it correctly waits until memory is freed. 
(Using dom0_mem is still a preferred option, as the ballooning delay can be 
significant.)

Thanks for all the help and patience as we've worked through this. Ack to the 
whole series:

Acked-by: Mike Latimer <mlatimer@suse.com>

-Mike

Re: [PATCH 0/4] fix freemem loop

[Mike Latimer]

On Tuesday, March 03, 2015 02:54:50 PM Mike Latimer wrote:
> Thanks for all the help and patience as we've worked through this. Ack to
> the whole series:
> 
> Acked-by: Mike Latimer <mlatimer@suse.com>

I guess the more correct response is:

  Reviewed-by: Mike Latimer <mlatimer@suse.com>
  Tested-by: Mike Latimer <mlatimer@suse.com>

-Mike  

Re: [PATCH 0/4] fix freemem loop

[Ian Campbell]

On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> Hi all,
> 
> this patch series fixes the freemem loop on machines with very large
> amount of memory, where the current wait time is not enough.
> 
> In order to be able to handle arbitrarly large amount of ram, we
> implement in libxl_wait_for_memory_target a policy of waiting until dom0
> is making progress.

What is the impact of the libxl API change made here on other callers,
in particular libvirt?

If it is expected that existing callers should continue to work as
before (perhaps with a redundant call etc) then please state this in the
relevant commit message(s).

>   The patch series also reverts "libxl: Wait for
> ballooning if free memory is increasing", that is not actually
> implemented correctly.
> 
> 
> Stefano Stabellini (4):
>       Revert "libxl: Wait for ballooning if free memory is increasing"
>       libxl_wait_for_memory_target: wait as long as dom0 is making progress
>       freemem: remove call to libxl_wait_for_free_memory
>       libxl_wait_for_memory_target: wait for 2 sec at a time
> 
>  tools/libxl/libxl.c      |   31 +++++++++++++++++++++++--------
>  tools/libxl/xl_cmdimpl.c |   29 ++++++-----------------------
>  2 files changed, 29 insertions(+), 31 deletions(-)
> 
> Cheers,
> 
> Stefano

Re: [PATCH 0/4] fix freemem loop

[Mike Latimer]

On Thursday, March 05, 2015 05:49:35 PM Ian Campbell wrote:
> On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > Hi all,
> > 
> > this patch series fixes the freemem loop on machines with very large
> > amount of memory, where the current wait time is not enough.
> > 
> > In order to be able to handle arbitrarly large amount of ram, we
> > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > is making progress.
> 
> What is the impact of the libxl API change made here on other callers,
> in particular libvirt?

This change will have one interesting effect on libvirt. Currently, 
libxlDomainFreeMem loops 3 times, then returns 0 (if no errors are 
encountered). This means that domain creation starts before dom0 finishes 
ballooning (unlike xl's previous behavior, which would fail).

With this change, domain creation through virsh will wait (in 
libxl_wait_for_memory_target) until dom0 finishes ballooning. This should 
result in an increase in the speed of the domain starting, as there will not 
be memory contention between the two processes.

The libvirt side calls libxl_wait_for_memory_target with a 1 second timeout - 
which doesn't leave a huge amount of room for slow memory allocation. This 
timeout, as well as the logic in general, should be changed to match the new 
xl behavior (IMO). I expect this to really only matter when dealing with large 
domains.

-Mike

Re: [PATCH 0/4] fix freemem loop

[Ian Campbell]

On Thu, 2015-03-05 at 21:08 -0700, Mike Latimer wrote:
> On Thursday, March 05, 2015 05:49:35 PM Ian Campbell wrote:
> > On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > > Hi all,
> > > 
> > > this patch series fixes the freemem loop on machines with very large
> > > amount of memory, where the current wait time is not enough.
> > > 
> > > In order to be able to handle arbitrarly large amount of ram, we
> > > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > > is making progress.
> > 
> > What is the impact of the libxl API change made here on other callers,
> > in particular libvirt?

I should have CCd Jim when I asked the question. Now done.

> This change will have one interesting effect on libvirt. Currently, 
> libxlDomainFreeMem loops 3 times, then returns 0 (if no errors are 
> encountered). This means that domain creation starts before dom0 finishes 
> ballooning (unlike xl's previous behavior, which would fail).
> 
> With this change, domain creation through virsh will wait (in 
> libxl_wait_for_memory_target) until dom0 finishes ballooning. This should 
> result in an increase in the speed of the domain starting, as there will not 
> be memory contention between the two processes.
> 
> The libvirt side calls libxl_wait_for_memory_target with a 1 second timeout - 
> which doesn't leave a huge amount of room for slow memory allocation. This 
> timeout, as well as the logic in general, should be changed to match the new 
> xl behavior (IMO). I expect this to really only matter when dealing with large 
> domains.

For libvirt we also need to consider what happens when a libvirt which
is modified along these lines uses an older libxl (I believe back to 4.2
is supported).

Ian.

Re: [PATCH 0/4] fix freemem loop

[Stefano Stabellini]

On Thu, 5 Mar 2015, Ian Campbell wrote:
> On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > Hi all,
> > 
> > this patch series fixes the freemem loop on machines with very large
> > amount of memory, where the current wait time is not enough.
> > 
> > In order to be able to handle arbitrarly large amount of ram, we
> > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > is making progress.
> 
> What is the impact of the libxl API change made here on other callers,
> in particular libvirt?
> 
> If it is expected that existing callers should continue to work as
> before (perhaps with a redundant call etc) then please state this in the
> relevant commit message(s).

The change to libxl_wait_for_memory_target will cause the function to
wait as long as dom0 is ballooning down by any amount of memory in a 2
seconds time frame. Therefore callers might end up waiting for longer
than before, specifically longer than the amount of seconds they pass to
libxl_wait_for_memory_target.

I'll add this message to the relevant commit.


> >   The patch series also reverts "libxl: Wait for
> > ballooning if free memory is increasing", that is not actually
> > implemented correctly.
> > 
> > 
> > Stefano Stabellini (4):
> >       Revert "libxl: Wait for ballooning if free memory is increasing"
> >       libxl_wait_for_memory_target: wait as long as dom0 is making progress
> >       freemem: remove call to libxl_wait_for_free_memory
> >       libxl_wait_for_memory_target: wait for 2 sec at a time
> > 
> >  tools/libxl/libxl.c      |   31 +++++++++++++++++++++++--------
> >  tools/libxl/xl_cmdimpl.c |   29 ++++++-----------------------
> >  2 files changed, 29 insertions(+), 31 deletions(-)
> > 
> > Cheers,
> > 
> > Stefano
> 
> 

Re: [PATCH 0/4] fix freemem loop

[Stefano Stabellini]

On Fri, 6 Mar 2015, Ian Campbell wrote:
> On Thu, 2015-03-05 at 21:08 -0700, Mike Latimer wrote:
> > On Thursday, March 05, 2015 05:49:35 PM Ian Campbell wrote:
> > > On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > > > Hi all,
> > > > 
> > > > this patch series fixes the freemem loop on machines with very large
> > > > amount of memory, where the current wait time is not enough.
> > > > 
> > > > In order to be able to handle arbitrarly large amount of ram, we
> > > > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > > > is making progress.
> > > 
> > > What is the impact of the libxl API change made here on other callers,
> > > in particular libvirt?
> 
> I should have CCd Jim when I asked the question. Now done.
> 
> > This change will have one interesting effect on libvirt. Currently, 
> > libxlDomainFreeMem loops 3 times, then returns 0 (if no errors are 
> > encountered). This means that domain creation starts before dom0 finishes 
> > ballooning (unlike xl's previous behavior, which would fail).
> > 
> > With this change, domain creation through virsh will wait (in 
> > libxl_wait_for_memory_target) until dom0 finishes ballooning. This should 
> > result in an increase in the speed of the domain starting, as there will not 
> > be memory contention between the two processes.
> > 
> > The libvirt side calls libxl_wait_for_memory_target with a 1 second timeout - 
> > which doesn't leave a huge amount of room for slow memory allocation. This 
> > timeout, as well as the logic in general, should be changed to match the new 
> > xl behavior (IMO). I expect this to really only matter when dealing with large 
> > domains.
> 
> For libvirt we also need to consider what happens when a libvirt which
> is modified along these lines uses an older libxl (I believe back to 4.2
> is supported).

In the libvirt case even if libvirt calls libxl_wait_for_memory_target
with just 1 second timeout, it is likely going to end up waiting longer
than that due to the change in behaviour of the function.

What I am saying is that even if we don't modify libvirt, we are going to
get a more reliable and more stable behaviour than what we have now.

That said, I agree that it would be good to improve the freemem loop in
libvirt in a similar way to what we are doing with xl.

Re: [PATCH 0/4] fix freemem loop

[Ian Campbell]

On Fri, 2015-03-06 at 09:48 +0000, Stefano Stabellini wrote:
> On Thu, 5 Mar 2015, Ian Campbell wrote:
> > On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > > Hi all,
> > > 
> > > this patch series fixes the freemem loop on machines with very large
> > > amount of memory, where the current wait time is not enough.
> > > 
> > > In order to be able to handle arbitrarly large amount of ram, we
> > > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > > is making progress.
> > 
> > What is the impact of the libxl API change made here on other callers,
> > in particular libvirt?
> > 
> > If it is expected that existing callers should continue to work as
> > before (perhaps with a redundant call etc) then please state this in the
> > relevant commit message(s).
> 
> The change to libxl_wait_for_memory_target will cause the function to
> wait as long as dom0 is ballooning down by any amount of memory in a 2
> seconds time frame. Therefore callers might end up waiting for longer
> than before, specifically longer than the amount of seconds they pass to
> libxl_wait_for_memory_target.
> 
> I'll add this message to the relevant commit.

Thanks. It might be worth waiting for Jim to say what the impact on new
libvirt (i.e. updated to take advantage of the fixed interface) built
against old libxl would be -- since we may want to include a #define
LIBXL_HAVE_SOMETHING in the resend.

> 
> 
> > >   The patch series also reverts "libxl: Wait for
> > > ballooning if free memory is increasing", that is not actually
> > > implemented correctly.
> > > 
> > > 
> > > Stefano Stabellini (4):
> > >       Revert "libxl: Wait for ballooning if free memory is increasing"
> > >       libxl_wait_for_memory_target: wait as long as dom0 is making progress
> > >       freemem: remove call to libxl_wait_for_free_memory
> > >       libxl_wait_for_memory_target: wait for 2 sec at a time
> > > 
> > >  tools/libxl/libxl.c      |   31 +++++++++++++++++++++++--------
> > >  tools/libxl/xl_cmdimpl.c |   29 ++++++-----------------------
> > >  2 files changed, 29 insertions(+), 31 deletions(-)
> > > 
> > > Cheers,
> > > 
> > > Stefano
> > 
> > 

Re: [PATCH 0/4] fix freemem loop

[Ian Campbell]

On Fri, 2015-03-06 at 09:52 +0000, Stefano Stabellini wrote:
> On Fri, 6 Mar 2015, Ian Campbell wrote:
> > On Thu, 2015-03-05 at 21:08 -0700, Mike Latimer wrote:
> > > On Thursday, March 05, 2015 05:49:35 PM Ian Campbell wrote:
> > > > On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > > > > Hi all,
> > > > > 
> > > > > this patch series fixes the freemem loop on machines with very large
> > > > > amount of memory, where the current wait time is not enough.
> > > > > 
> > > > > In order to be able to handle arbitrarly large amount of ram, we
> > > > > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > > > > is making progress.
> > > > 
> > > > What is the impact of the libxl API change made here on other callers,
> > > > in particular libvirt?
> > 
> > I should have CCd Jim when I asked the question. Now done.
> > 
> > > This change will have one interesting effect on libvirt. Currently, 
> > > libxlDomainFreeMem loops 3 times, then returns 0 (if no errors are 
> > > encountered). This means that domain creation starts before dom0 finishes 
> > > ballooning (unlike xl's previous behavior, which would fail).
> > > 
> > > With this change, domain creation through virsh will wait (in 
> > > libxl_wait_for_memory_target) until dom0 finishes ballooning. This should 
> > > result in an increase in the speed of the domain starting, as there will not 
> > > be memory contention between the two processes.
> > > 
> > > The libvirt side calls libxl_wait_for_memory_target with a 1 second timeout - 
> > > which doesn't leave a huge amount of room for slow memory allocation. This 
> > > timeout, as well as the logic in general, should be changed to match the new 
> > > xl behavior (IMO). I expect this to really only matter when dealing with large 
> > > domains.
> > 
> > For libvirt we also need to consider what happens when a libvirt which
> > is modified along these lines uses an older libxl (I believe back to 4.2
> > is supported).
> 
> In the libvirt case even if libvirt calls libxl_wait_for_memory_target
> with just 1 second timeout, it is likely going to end up waiting longer
> than that due to the change in behaviour of the function.
> 
> What I am saying is that even if we don't modify libvirt, we are going to
> get a more reliable and more stable behaviour than what we have now.
> 
> That said, I agree that it would be good to improve the freemem loop in
> libvirt in a similar way to what we are doing with xl.

This is missing my point.

We need to consider the case of a modified libvirt with the old, broken,
libxl too.

You've only considered an unmodified libvirt running with a fixed libxl.

Ian.

Re: [PATCH 0/4] fix freemem loop

[Stefano Stabellini]

On Fri, 6 Mar 2015, Ian Campbell wrote:
> On Fri, 2015-03-06 at 09:52 +0000, Stefano Stabellini wrote:
> > On Fri, 6 Mar 2015, Ian Campbell wrote:
> > > On Thu, 2015-03-05 at 21:08 -0700, Mike Latimer wrote:
> > > > On Thursday, March 05, 2015 05:49:35 PM Ian Campbell wrote:
> > > > > On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > this patch series fixes the freemem loop on machines with very large
> > > > > > amount of memory, where the current wait time is not enough.
> > > > > > 
> > > > > > In order to be able to handle arbitrarly large amount of ram, we
> > > > > > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > > > > > is making progress.
> > > > > 
> > > > > What is the impact of the libxl API change made here on other callers,
> > > > > in particular libvirt?
> > > 
> > > I should have CCd Jim when I asked the question. Now done.
> > > 
> > > > This change will have one interesting effect on libvirt. Currently, 
> > > > libxlDomainFreeMem loops 3 times, then returns 0 (if no errors are 
> > > > encountered). This means that domain creation starts before dom0 finishes 
> > > > ballooning (unlike xl's previous behavior, which would fail).
> > > > 
> > > > With this change, domain creation through virsh will wait (in 
> > > > libxl_wait_for_memory_target) until dom0 finishes ballooning. This should 
> > > > result in an increase in the speed of the domain starting, as there will not 
> > > > be memory contention between the two processes.
> > > > 
> > > > The libvirt side calls libxl_wait_for_memory_target with a 1 second timeout - 
> > > > which doesn't leave a huge amount of room for slow memory allocation. This 
> > > > timeout, as well as the logic in general, should be changed to match the new 
> > > > xl behavior (IMO). I expect this to really only matter when dealing with large 
> > > > domains.
> > > 
> > > For libvirt we also need to consider what happens when a libvirt which
> > > is modified along these lines uses an older libxl (I believe back to 4.2
> > > is supported).
> > 
> > In the libvirt case even if libvirt calls libxl_wait_for_memory_target
> > with just 1 second timeout, it is likely going to end up waiting longer
> > than that due to the change in behaviour of the function.
> > 
> > What I am saying is that even if we don't modify libvirt, we are going to
> > get a more reliable and more stable behaviour than what we have now.
> > 
> > That said, I agree that it would be good to improve the freemem loop in
> > libvirt in a similar way to what we are doing with xl.
> 
> This is missing my point.
> 
> We need to consider the case of a modified libvirt with the old, broken,
> libxl too.

For that case, we might want to make sure that the new timeout passed to
libxl_wait_for_memory_target is not lower than the overall libvirt
timeout today (libxl_wait_for_free_memory+libxl_wait_for_memory_target).

Re: [PATCH 0/4] fix freemem loop

[Ian Campbell]

On Fri, 2015-03-06 at 10:22 +0000, Stefano Stabellini wrote:
> On Fri, 6 Mar 2015, Ian Campbell wrote:
> > On Fri, 2015-03-06 at 09:52 +0000, Stefano Stabellini wrote:
> > > On Fri, 6 Mar 2015, Ian Campbell wrote:
> > > > On Thu, 2015-03-05 at 21:08 -0700, Mike Latimer wrote:
> > > > > On Thursday, March 05, 2015 05:49:35 PM Ian Campbell wrote:
> > > > > > On Tue, 2015-03-03 at 11:08 +0000, Stefano Stabellini wrote:
> > > > > > > Hi all,
> > > > > > > 
> > > > > > > this patch series fixes the freemem loop on machines with very large
> > > > > > > amount of memory, where the current wait time is not enough.
> > > > > > > 
> > > > > > > In order to be able to handle arbitrarly large amount of ram, we
> > > > > > > implement in libxl_wait_for_memory_target a policy of waiting until dom0
> > > > > > > is making progress.
> > > > > > 
> > > > > > What is the impact of the libxl API change made here on other callers,
> > > > > > in particular libvirt?
> > > > 
> > > > I should have CCd Jim when I asked the question. Now done.
> > > > 
> > > > > This change will have one interesting effect on libvirt. Currently, 
> > > > > libxlDomainFreeMem loops 3 times, then returns 0 (if no errors are 
> > > > > encountered). This means that domain creation starts before dom0 finishes 
> > > > > ballooning (unlike xl's previous behavior, which would fail).
> > > > > 
> > > > > With this change, domain creation through virsh will wait (in 
> > > > > libxl_wait_for_memory_target) until dom0 finishes ballooning. This should 
> > > > > result in an increase in the speed of the domain starting, as there will not 
> > > > > be memory contention between the two processes.
> > > > > 
> > > > > The libvirt side calls libxl_wait_for_memory_target with a 1 second timeout - 
> > > > > which doesn't leave a huge amount of room for slow memory allocation. This 
> > > > > timeout, as well as the logic in general, should be changed to match the new 
> > > > > xl behavior (IMO). I expect this to really only matter when dealing with large 
> > > > > domains.
> > > > 
> > > > For libvirt we also need to consider what happens when a libvirt which
> > > > is modified along these lines uses an older libxl (I believe back to 4.2
> > > > is supported).
> > > 
> > > In the libvirt case even if libvirt calls libxl_wait_for_memory_target
> > > with just 1 second timeout, it is likely going to end up waiting longer
> > > than that due to the change in behaviour of the function.
> > > 
> > > What I am saying is that even if we don't modify libvirt, we are going to
> > > get a more reliable and more stable behaviour than what we have now.
> > > 
> > > That said, I agree that it would be good to improve the freemem loop in
> > > libvirt in a similar way to what we are doing with xl.
> > 
> > This is missing my point.
> > 
> > We need to consider the case of a modified libvirt with the old, broken,
> > libxl too.
> 
> For that case, we might want to make sure that the new timeout passed to
> libxl_wait_for_memory_target is not lower than the overall libvirt
> timeout today (libxl_wait_for_free_memory+libxl_wait_for_memory_target).

OK, sounds like we may not need a #define then.

But perhaps you could update the comment above the prototypes of these
functions in libxl.h which explains how to use the interface such that
it works well with new libxl and at least tolerably with old libxl and
(if it is different) how to use it so it works super-well with new libxl
if you don't care about old libxl.

Ian.

Re: [PATCH 0/4] fix freemem loop

[Jim Fehlig]

Stefano Stabellini wrote:
> On Fri, 6 Mar 2015, Ian Campbell wrote:
>   
>> This is missing my point.
>>
>> We need to consider the case of a modified libvirt with the old, broken,
>> libxl too.
>>     
>
> For that case, we might want to make sure that the new timeout passed to
> libxl_wait_for_memory_target is not lower than the overall libvirt
> timeout today (libxl_wait_for_free_memory+libxl_wait_for_memory_target).
>   

Currently in libvirt,
libxl_wait_for_free_memory+libxl_wait_for_memory_target = 11sec.  But
IMO it will be fine to replace that with
'libxl_wait_for_memory_target(ctx, 0, 10)', as you did for xl in 3/4.

A modified libvirt with unmodified libxl would behave the same as a
modified xl with unmodified libxl.

Regards,
Jim

Re: [PATCH 2/4] libxl_wait_for_memory_target: wait as long as dom0 is making progress

[Jim Fehlig]

Stefano Stabellini wrote:
> Decrement wait_secs only if dom0 is making no progress toward reaching
> the balloon target, otherwise loop again for free.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Tested-by: Mike Latimer <mlatimer@suse.com>
> ---
>  tools/libxl/libxl.c      |   29 ++++++++++++++++++++++-------
>  tools/libxl/xl_cmdimpl.c |    4 ++--
>  2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 088786e..648a227 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4959,26 +4959,41 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
>   

Would be nice to have a comment clarifying the semantics of wait_secs,
otherwise callers might assume it is the wait time to reach the memory
target vs the wait time if no ballooning progress is being made.

Regards,
Jim

>  {
>      int rc = 0;
>      uint32_t target_memkb = 0;
> +    uint64_t current_memkb, prev_memkb;
>      libxl_dominfo info;
>  
> +    rc = libxl_get_memory_target(ctx, domid, &target_memkb);
> +    if (rc < 0)
> +        return rc;
> +
>      libxl_dominfo_init(&info);
> +    prev_memkb = UINT64_MAX;
>  
>      do {
> -        wait_secs--;
>          sleep(1);
>  
> -        rc = libxl_get_memory_target(ctx, domid, &target_memkb);
> -        if (rc < 0)
> -            goto out;
> -
>          libxl_dominfo_dispose(&info);
>          libxl_dominfo_init(&info);
>          rc = libxl_domain_info(ctx, &info, domid);
>          if (rc < 0)
>              goto out;
> -    } while (wait_secs > 0 && (info.current_memkb + info.outstanding_memkb) > target_memkb);
>  
> -    if ((info.current_memkb + info.outstanding_memkb) <= target_memkb)
> +        current_memkb = info.current_memkb + info.outstanding_memkb;
> +
> +        if (current_memkb > prev_memkb)
> +        {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +        else if (current_memkb == prev_memkb)
> +            wait_secs--;
> +        /* if current_memkb < prev_memkb loop for free as progress has
> +         * been made */
> +
> +        prev_memkb = current_memkb;
> +    } while (wait_secs > 0 && current_memkb > target_memkb);
> +
> +    if (current_memkb <= target_memkb)
>          rc = 0;
>      else
>          rc = ERROR_FAIL;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index f4c4122..2dc7574 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2226,8 +2226,8 @@ static int freemem(uint32_t domid, libxl_domain_build_info *b_info)
>          else if (rc != ERROR_NOMEM)
>              return rc;
>  
> -        /* the memory target has been reached but the free memory is still
> -         * not enough: loop over again */
> +        /* wait until dom0 reaches its target, as long as we are making
> +         * progress */
>          rc = libxl_wait_for_memory_target(ctx, 0, 1);
>          if (rc < 0)
>              return rc;
>