From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin Kletzander Subject: Re: [libvirt] [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 Date: Fri, 3 Feb 2017 20:18:57 +0100 Message-ID: <20170203191857.GC13358@wheatley> References: <20170119175005.10220-1-jfehlig@suse.com> <589268A6.8030302@suse.com> <20170202114245.a4qy2t6wdae7gyoq@citrix.com> <37e46230-109a-0c91-34fb-d8483468d288@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1499933614305485387==" Return-path: In-Reply-To: <37e46230-109a-0c91-34fb-d8483468d288@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jim Fehlig Cc: libvir-list@redhat.com, Juergen Gross , Wei Liu , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============1499933614305485387== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="rQ2U398070+RC21q" Content-Disposition: inline --rQ2U398070+RC21q Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline 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 >>>> --- >>>> 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@redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list --rQ2U398070+RC21q Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEiXAnXDYdKAaCyvS1CB/CnyQXht0FAliU16EACgkQCB/CnyQX ht2gDQ//bzuf1ZeVrwBNXywcKyTzmFSwXTsevKfRkCk7CqVOn/VqlJiI+od0mk/n 5n8qCloVgBWDWKpVmCLEjPfGu8un4FWbBhxKj+cSjiUcokDaIB5tU4/5BR4QQt0f FBsj+DaK+cJHwkmlJusdDwD49YoMb4y0geDHYD2KQmACURKvpyDtLxOrwrkk0noi hE1WhhB5koqhUKZYS16BmQSpCldCnsewh+YW5Eag0fAg9UKi+ghhtJw918yMioSK u7BrG1Ra1UidlzPcstAxcGe1KqnK4f8ezZVVLliWNmoqP8m/9IiwoqC4VeQyhuEx J4hRd9ViINUGJfLudP92DO0cSUQrSkX2/lHJBqxnlv+l5AOm+/x2yAPoDLfqE2eN Vcr3aQqtrzt0W9r7qLmh9z3TU2HFJabPta7ewcjhn+nbjNYkXv9Si/DqdBGdnq81 zAFkJ9hzEZ/CmmcQI0zRtRSFbxf+Qdui5nzM98JG4v8ZB1zQIPtziOcrIfH/jV1g sZAZuTdAeDPAzC/qZ0i/FCer+TTDlt2TiPpiIxPC8b7jC1apODrEWD8sKNiJkvGS 1N0aKTZMdo0n0oUQ4NdeZj7rixL7H6MOk7Hgb4H+ZEQIYI/eq+qnEYmLUSDDJO5n NT/W/Pkvva1aWGZTz1oXBvRxief0vaM+Z+xm7xB3GbAZw36rBcs= =7sVX -----END PGP SIGNATURE----- --rQ2U398070+RC21q-- --===============1499933614305485387== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============1499933614305485387==--