From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for-xen-4.5] libxl: Allow copying smaller bitmap into a larger one Date: Wed, 26 Nov 2014 12:51:41 +0000 Message-ID: <1417006301.11944.47.camel@citrix.com> References: <1416518854-5284-1-git-send-email-boris.ostrovsky@oracle.com> <20141124104127.GF30053@zion.uk.xensource.com> <20141124104703.GH30053@zion.uk.xensource.com> <54735343.1020208@oracle.com> <20141125103940.GC28315@zion.uk.xensource.com> <20141125111522.GD28315@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141125111522.GD28315@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: Dario Faggioli , stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, Jim Fehlig , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On Tue, 2014-11-25 at 11:15 +0000, Wei Liu wrote: > This will result in assertion in libxl_set_vcpuaffinity()->libxl_bitmap_copy() > since the destination bitmap is created for maximum number of CPUs. FYI I'm also seeing this with libvirt (on ARM, but I don't think that matters) when the guest XML uses: 1 Results in: libvirtd: libxl_utils.c:612: libxl_bitmap_copy: Assertion `dptr->size == sptr->size' failed. Program received signal SIGABRT, Aborted. [Switching to Thread 0xb67f9420 (LWP 3881)] 0xb6ab7f96 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6 (gdb) bt #0 0xb6ab7f96 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6 #1 0xb6ac5f8a in raise () from /lib/arm-linux-gnueabihf/libc.so.6 #2 0xb6ac8428 in abort () from /lib/arm-linux-gnueabihf/libc.so.6 #3 0xb6ac101e in __assert_fail () from /lib/arm-linux-gnueabihf/libc.so.6 #4 0xb16caeb4 in libxl_bitmap_copy (ctx=, dptr=, sptr=) at libxl_utils.c:612 #5 0xb16af15c in libxl_set_vcpuaffinity (ctx=0x2, domid=3065494508, vcpuid=0, cpumap_hard=0xb67f8668, cpumap_soft=0x0) at libxl.c:5323 #6 0xb17195ae in libxlDomainSetVcpuAffinities () from /opt/libvirt/lib/libvirt/connection-driver/libvirt_driver_libxl.so #7 0xb1719cf2 in libxlDomainStart () from /opt/libvirt/lib/libvirt/connection-driver/libvirt_driver_libxl.so #8 0xb171b7c2 in libxlDomainCreateXML () from /opt/libvirt/lib/libvirt/connection-driver/libvirt_driver_libxl.so #9 0xb6d28630 in virDomainCreateXML () from /opt/libvirt/lib/libvirt.so.0 [...snip...] libxlDomainSetVcpuAffinities does: libxl_bitmap map; [...] map.size = cpumaplen; map.map = cpumap; if (libxl_set_vcpuaffinity(priv->ctx, def->id, vcpu, &map) != 0) { http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/libxl/libxl_domain.c;h=9c622910547ada174a3d787c76c8bb076c9a75c3;hb=HEAD#l1059 Applying this libxl patch fixes things. I don't think we've explicitly outlawed looking "inside" a libxl_bitmap in this way anywhere, so I think libvirtd is within its rights to do so, but it does highlight that we need to be mindful within libxl that people may not be using libxl_cpu_bitmap_alloc. > > We could allocate that bitmap of the same size as the source, however, it is > later passed to xc_vcpu_setaffinity() which expects it to be sized to the max > number of CPUs > > To fix this issue, introduce an internal function to allowing copying between > bitmaps of different sizes. Note that this function is only used in > libxl_set_vcpuaffinity at the moment. Though NUMA placement logic invoke > libxl_bitmap_copy as well there's no need to replace those invocations. NUMA > placement logic comes into effect when no vcpu / node pinning is provided, so > it always operates on bitmap of the same sizes (that is, size of maximum > number of cpus /nodes). > > Reported-by: Boris Ostrovsky > Signed-off-by: Wei Liu > Cc: Ian Campbell > Cc: Ian Jackson > Cc: Dario Faggioli > --- > This fixes a regression for 4.5. Can't think of obvious risk. > --- > tools/libxl/libxl.c | 4 ++-- > tools/libxl/libxl_internal.h | 11 +++++++++++ > tools/libxl/libxl_utils.c | 15 +++++++++++++++ > 3 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index de23fec..1e9da10 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -5320,7 +5320,7 @@ int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid, > if (rc) > goto out; > > - libxl_bitmap_copy(ctx, &hard, cpumap_hard); > + libxl__bitmap_copy_best_effort(gc, &hard, cpumap_hard); > flags = XEN_VCPUAFFINITY_HARD; > } > if (cpumap_soft) { > @@ -5328,7 +5328,7 @@ int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid, > if (rc) > goto out; > > - libxl_bitmap_copy(ctx, &soft, cpumap_soft); > + libxl__bitmap_copy_best_effort(gc, &soft, cpumap_soft); > flags |= XEN_VCPUAFFINITY_SOFT; > } > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 4361421..a38f695 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -3617,6 +3617,17 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc, > libxl_device_##type##_copy(CTX, DA_p, (dev)); \ > }) > > +/* This function copies X bytes from source to destination bitmap, > + * where X is the smaller of the two sizes. > + * > + * If destination's size is larger than source, the extra bytes are > + * untouched. > + * > + * XXX This is introduced to fix a regression for 4.5. It shall > + * be revisited in 4.6 time frame. > + */ > +void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr, > + const libxl_bitmap *sptr); > #endif > > /* > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c > index 58df4f3..3e1ba17 100644 > --- a/tools/libxl/libxl_utils.c > +++ b/tools/libxl/libxl_utils.c > @@ -614,6 +614,21 @@ void libxl_bitmap_copy(libxl_ctx *ctx, libxl_bitmap *dptr, > memcpy(dptr->map, sptr->map, sz * sizeof(*dptr->map)); > } > > +/* This function copies X bytes from source to destination bitmap, > + * where X is the smaller of the two sizes. > + * > + * If destination's size is larger than source, the extra bytes are > + * untouched. > + */ > +void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr, > + const libxl_bitmap *sptr) > +{ > + int sz; > + > + sz = dptr->size < sptr->size ? dptr->size : sptr->size; > + memcpy(dptr->map, sptr->map, sz * sizeof(*dptr->map)); > +} > + > void libxl_bitmap_copy_alloc(libxl_ctx *ctx, > libxl_bitmap *dptr, > const libxl_bitmap *sptr)