[PATCH v2 0/4] fix freemem loop

[PATCH v2 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.


Changes on v2:
- update comment in libxl.h for libxl_wait_for_memory_target


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/libxl.h      |   14 +++++++++++++-
 tools/libxl/xl_cmdimpl.c |   29 ++++++-----------------------
 3 files changed, 42 insertions(+), 32 deletions(-)


Cheers,

Stefano

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

[Stefano Stabellini]

This reverts commit 2563bca11544361dc2afa5e20b5663e10a0715cb.

Reviewed-by: Mike Latimer <mlatimer@suse.com>
Tested-by: Mike Latimer <mlatimer@suse.com>
---
 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

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

[Wei Liu]

On Fri, Mar 06, 2015 at 05:19:14PM +0000, Stefano Stabellini wrote:
> This reverts commit 2563bca11544361dc2afa5e20b5663e10a0715cb.
> 

Missing S-o-B.

> Reviewed-by: Mike Latimer <mlatimer@suse.com>
> Tested-by: Mike Latimer <mlatimer@suse.com>
> ---
>  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

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

[Stefano Stabellini]

On Mon, 9 Mar 2015, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 05:19:14PM +0000, Stefano Stabellini wrote:
> > This reverts commit 2563bca11544361dc2afa5e20b5663e10a0715cb.
> > 
> 
> Missing S-o-B.

Do I really need to S-o-B a revert?

In any case:

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> > Reviewed-by: Mike Latimer <mlatimer@suse.com>
> > Tested-by: Mike Latimer <mlatimer@suse.com>
> > ---
> >  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 v2 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>
Reviewed-by: Mike Latimer <mlatimer@suse.com>
Tested-by: Mike Latimer <mlatimer@suse.com>
---
 tools/libxl/libxl.c      |   29 ++++++++++++++++++++++-------
 tools/libxl/libxl.h      |   14 +++++++++++++-
 tools/libxl/xl_cmdimpl.c |    4 ++--
 3 files changed, 37 insertions(+), 10 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/libxl.h b/tools/libxl/libxl.h
index e3d2ae8..3e63e42 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1043,7 +1043,19 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info,
 int libxl_get_free_memory(libxl_ctx *ctx, uint32_t *memkb);
 /* wait for a given amount of memory to be free in the system */
 int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t memory_kb, int wait_secs);
-/* wait for the memory target of a domain to be reached */
+/* 
+ * Wait for the memory target of a domain to be reached. Does not
+ * decrement wait_secs if the domain is making progress toward reaching
+ * the target. If the domain is not making progress, wait_secs is
+ * decremented. If the timeout expires before the target is reached, the
+ * function returns ERROR_FAIL.
+ *
+ * Older versions of this function (Xen 4.5 and older), decremented
+ * wait_secs even if the domain was making progress, resulting in far
+ * lower overall wait times. To make sure that your calling routine
+ * works with new and old implementations of the function, pass enough
+ * time for the guest to reach its target as an argument.
+ */
 int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs);
 
 int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
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

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

[Wei Liu]

On Fri, Mar 06, 2015 at 05:19:15PM +0000, Stefano Stabellini wrote:
[...]
>  int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t memory_kb, int wait_secs);
> -/* wait for the memory target of a domain to be reached */
> +/* 
> + * Wait for the memory target of a domain to be reached. Does not
> + * decrement wait_secs if the domain is making progress toward reaching
> + * the target. If the domain is not making progress, wait_secs is
> + * decremented. If the timeout expires before the target is reached, the
> + * function returns ERROR_FAIL.
> + *
> + * Older versions of this function (Xen 4.5 and older), decremented
> + * wait_secs even if the domain was making progress, resulting in far
> + * lower overall wait times. To make sure that your calling routine
> + * works with new and old implementations of the function, pass enough
> + * time for the guest to reach its target as an argument.
> + */

Need to have a #define LIBXL_HAVE_$FOO to mark the change in behaviour,
so that client can cope with this change.

Wei.

>  int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs);
>  
>  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
> 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

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

[Stefano Stabellini]

On Mon, 9 Mar 2015, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 05:19:15PM +0000, Stefano Stabellini wrote:
> [...]
> >  int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t memory_kb, int wait_secs);
> > -/* wait for the memory target of a domain to be reached */
> > +/* 
> > + * Wait for the memory target of a domain to be reached. Does not
> > + * decrement wait_secs if the domain is making progress toward reaching
> > + * the target. If the domain is not making progress, wait_secs is
> > + * decremented. If the timeout expires before the target is reached, the
> > + * function returns ERROR_FAIL.
> > + *
> > + * Older versions of this function (Xen 4.5 and older), decremented
> > + * wait_secs even if the domain was making progress, resulting in far
> > + * lower overall wait times. To make sure that your calling routine
> > + * works with new and old implementations of the function, pass enough
> > + * time for the guest to reach its target as an argument.
> > + */
> 
> Need to have a #define LIBXL_HAVE_$FOO to mark the change in behaviour,
> so that client can cope with this change.
> 

Given the very limited change in behaviour (in fact existing callers
should work as-is) I thought that we could do without one. Also see:
http://marc.info/?l=xen-devel&m=142563879603876&w=2.

> 
> >  int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs);
> >  
> >  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
> > 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
> 

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

[Ian Campbell]

On Mon, 2015-03-09 at 12:41 +0000, Stefano Stabellini wrote:
> Given the very limited change in behaviour (in fact existing callers
> should work as-is) I thought that we could do without one. Also see:
> http://marc.info/?l=xen-devel&m=142563879603876&w=2.

I was also basing that on the big comment above this section which
reads:

/*
 * WARNING
 * This memory management API is unstable even in Xen 4.2.
 * It has a numer of deficiencies and we intend to replace it.
 *
 * The semantics of these functions should not be relied on to be very
 * coherent or stable.  We will however endeavour to keep working
 * existing programs which use them in roughly the same way as libxl.
 */

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

[Wei Liu]

On Mon, Mar 09, 2015 at 12:41:25PM +0000, Stefano Stabellini wrote:
> On Mon, 9 Mar 2015, Wei Liu wrote:
> > On Fri, Mar 06, 2015 at 05:19:15PM +0000, Stefano Stabellini wrote:
> > [...]
> > >  int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t memory_kb, int wait_secs);
> > > -/* wait for the memory target of a domain to be reached */
> > > +/* 
> > > + * Wait for the memory target of a domain to be reached. Does not
> > > + * decrement wait_secs if the domain is making progress toward reaching
> > > + * the target. If the domain is not making progress, wait_secs is
> > > + * decremented. If the timeout expires before the target is reached, the
> > > + * function returns ERROR_FAIL.
> > > + *
> > > + * Older versions of this function (Xen 4.5 and older), decremented
> > > + * wait_secs even if the domain was making progress, resulting in far
> > > + * lower overall wait times. To make sure that your calling routine
> > > + * works with new and old implementations of the function, pass enough
> > > + * time for the guest to reach its target as an argument.
> > > + */
> > 
> > Need to have a #define LIBXL_HAVE_$FOO to mark the change in behaviour,
> > so that client can cope with this change.
> > 
> 
> Given the very limited change in behaviour (in fact existing callers
> should work as-is) I thought that we could do without one. Also see:
> http://marc.info/?l=xen-devel&m=142563879603876&w=2.
> 

Sorry, I missed that one. Adding the comment is enough.

Wei.

> > 
> > >  int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs);
> > >  
> > >  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
> > > 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 v2 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>
Reviewed-by: Mike Latimer <mlatimer@suse.com>
Tested-by: Mike Latimer <mlatimer@suse.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 v2 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>
Reviewed-by: Mike Latimer <mlatimer@suse.com>
Tested-by: Mike Latimer <mlatimer@suse.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 v2 0/4] fix freemem loop

[Ian Campbell]

On Fri, 2015-03-06 at 17:16 +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.  The patch series also reverts "libxl: Wait for
> ballooning if free memory is increasing", that is not actually
> implemented correctly.

Ackked + applied (with git am --whitespace=fix, please watch for
trailing spaces).