* [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 @ 2017-01-19 17:50 Jim Fehlig 2017-02-01 23:00 ` Jim Fehlig 0 siblings, 1 reply; 5+ messages in thread From: Jim Fehlig @ 2017-01-19 17:50 UTC (permalink / raw) To: libvir-list; +Cc: Jim Fehlig, xen-devel 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(). 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. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- 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, /* relative */ 1, 0) < 0) goto error; -- 2.9.2 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 2017-01-19 17:50 [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 Jim Fehlig @ 2017-02-01 23:00 ` Jim Fehlig 2017-02-02 11:42 ` Wei Liu 0 siblings, 1 reply; 5+ messages in thread From: Jim Fehlig @ 2017-02-01 23:00 UTC (permalink / raw) To: libvir-list; +Cc: Juergen Gross, Wei Liu, xen-devel 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(). > > 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@suse.com> > --- > 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, > /* relative */ 1, 0) < 0) > goto error; > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 2017-02-01 23:00 ` Jim Fehlig @ 2017-02-02 11:42 ` Wei Liu 2017-02-02 18:15 ` Jim Fehlig 0 siblings, 1 reply; 5+ messages in thread From: Wei Liu @ 2017-02-02 11:42 UTC (permalink / raw) To: Jim Fehlig; +Cc: libvir-list, Juergen Gross, Wei Liu, xen-devel I saw this mail but didn't realise it required my input, sorry. 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(). > > > > 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@suse.com> > > --- > > 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, This should do the trick. Wei. > > /* relative */ 1, 0) < 0) > > goto error; > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 2017-02-02 11:42 ` Wei Liu @ 2017-02-02 18:15 ` Jim Fehlig 2017-02-03 19:18 ` [libvirt] " Martin Kletzander 0 siblings, 1 reply; 5+ messages in thread From: Jim Fehlig @ 2017-02-02 18:15 UTC (permalink / raw) To: Wei Liu; +Cc: libvir-list, Juergen Gross, xen-devel 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(). >>> >>> 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@suse.com> >>> --- >>> 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, > > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [libvirt] [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 2017-02-02 18:15 ` Jim Fehlig @ 2017-02-03 19:18 ` Martin Kletzander 0 siblings, 0 replies; 5+ messages in thread From: Martin Kletzander @ 2017-02-03 19:18 UTC (permalink / raw) To: Jim Fehlig; +Cc: libvir-list, Juergen Gross, Wei Liu, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 4020 bytes --] 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@suse.com> >>>> --- >>>> 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 [-- Attachment #1.2: Digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 127 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-03 19:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-19 17:50 [PATCH] libxl: fix dom0 autoballooning with Xen 4.8 Jim Fehlig 2017-02-01 23:00 ` Jim Fehlig 2017-02-02 11:42 ` Wei Liu 2017-02-02 18:15 ` Jim Fehlig 2017-02-03 19:18 ` [libvirt] " Martin Kletzander
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).