Re: [Xen-devel] [PATCH] libxl: fix dom0 autoballooning with Xen 4.8

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 02, 2017 at 11:15:41AM -0700, Jim Fehlig wrote:
On 02/02/2017 04:42 AM, Wei Liu wrote:
I saw this mail but didn't realise it required my input, sorry.

I suppose it didn't and I was shamelessly fishing for a review - so you have my
apologies :-). But the patch does fix an annoying, easily encountered bug which
I'm eager to see resolved in the next libvirt release.


On Wed, Feb 01, 2017 at 04:00:54PM -0700, Jim Fehlig wrote:
Jim Fehlig wrote:
xen.git commit 57f8b13c changed several of the libxl memory
get/set functions to take 64 bit parameters. The libvirt
libxl driver still uses uint32_t variables for these various
parameters, which is particularly problematic for the
libxl_set_memory_target() function.

When dom0 autoballooning is enabled, libvirt (like xl) determines
the memory needed to start a domain and the memory available. If
memory available is less than memory needed, dom0 is ballooned
down by passing a negative value to libxl_set_memory_target()
'target_memkb' parameter. Prior to xen.git commit 57f8b13c,
'target_memkb' was an int32_t. Subtracting a larger uint32 from
a smaller uint32 and assigning it to int32 resulted in a negative
number. After commit 57f8b13c, the same subtraction is widened
to a int64, resulting in a large positive number. The simple
fix taken by this patch is to assign the difference of the
uint32 values to a temporary int32 variable, which is then
passed to 'target_memkb' parameter of libxl_set_memory_target().



So theonly functional change is actually a type cast from u32 to i32,
just with the help of another variable.

Note that it is undesirable to change libvirt to use 64 bit
variables since it requires setting LIBXL_API_VERSION to 0x040800.
Currently libvirt supports LIBXL_API_VERSION >= 0x040400,
essentially Xen >= 4.4.

Any comments on this patch? xen-devel is already in cc, but I'll add Wei (one of
the Xen tools maintainers) and Juergen (author of commit 57f8b13c) for some
additional exposure :-).

Currently, "out-of-the-box" Xen >= 4.8 + libvirt setup (dom0 autoballooning
enabled) is broken without this fix.

Regards,
Jim


Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
---
 src/libxl/libxl_domain.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index a5314b0..ed73cd2 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -909,6 +909,7 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config)
 {
     uint32_t needed_mem;
     uint32_t free_mem;
+    int32_t target_mem;
     int tries = 3;
     int wait_secs = 10;

@@ -922,7 +923,8 @@ libxlDomainFreeMem(libxl_ctx *ctx, libxl_domain_config *d_config)
         if (free_mem >= needed_mem)
             return 0;

-        if (libxl_set_memory_target(ctx, 0, free_mem - needed_mem,
+        target_mem = free_mem - needed_mem;
+        if (libxl_set_memory_target(ctx, 0, target_mem,


So, clearly visible here, that this way it will always be negative.  And
the change makes sense if the parameter type would change.  Which is
exactly what you describe in the commit message.  So me trying to find
out the meaning ended as it should, so I think this is safe.

I'm reading the libxl code underneath, but I can't wrap my head around
the naming in there.  Anyway, after all the trying and history searching
it looks like this is desirable, so ACK from me.

This should do the trick.

Thanks. There are other ways to skin this cat and if folks prefer one of those I
can push a followup. But for now, so that we have a fix, I'll push this variant.

Regards,
Jim

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux