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, JimSigned-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
