Running VMs couldn't use newly hot plugged host CPUs even if the VMs had no CPU pinning defined AND the cpuset controller was disabled in the libvirt qemu configuration.

This was because in this case the process affinity was set by libvirt to all currently present host CPUs in order to avoid situations, where libvirtd was deliberately running on a CPU subset and thus the spawned VMs would be involuntarily restricted to the CPU subset inherited by libvirtd. That however prevents new host CPUs to be utilized when they show up. With this change we will NOT set the VM's affinity mask if it matches the online host CPU mask. There's still the chance that for some reason the deliberately chosen libvirtd affinity matches the online host CPU mask by accident. In this case the behavior remains as it was before (CPUs offline while setting the affinity will not be used if they show up later on). Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com&gt; Tested-by: Matthew Rosato <mjrosato(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_process.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b67aee..76f9210 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2202,6 +2202,7 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm) int ret = -1; virBitmapPtr cpumap = NULL; virBitmapPtr cpumapToSet = NULL; + virBitmapPtr hostcpumap = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; if (!vm->pid) { @@ -2223,6 +2224,16 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm) * the spawned QEMU instance to all pCPUs if no map is given in * its config file */ int hostcpus; + hostcpumap = virHostCPUGetOnlineBitmap(); + cpumap = virProcessGetAffinity(vm->pid);

+ + if (hostcpumap && cpumap && virBitmapEqual(hostcpumap, cpumap)) { + /* we're using all available CPUs, no reason to set + * mask. If libvirtd is running without explicit + * affinity, we can use hotplugged CPUs for this VM */ + ret = 0; + goto cleanup; + } /* setaffinity fails if you set bits for CPUs which * aren't present, so we have to limit ourselves */ @@ -2248,6 +2259,7 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm) cleanup: virBitmapFree(cpumap); + virBitmapFree(hostcpumap); return ret; }

On 11/04/2016 04:01 AM, Viktor Mihajlovski wrote: > Running VMs couldn't use newly hot plugged host CPUs even if the VMs > had no CPU pinning defined AND the cpuset controller was disabled in > the libvirt qemu configuration. Add blank lines between paragraphs - just makes it easier to read. > This was because in this case the process affinity was set by libvirt > to all currently present host CPUs in order to avoid situations, where > libvirtd was deliberately running on a CPU subset and thus the spawned > VMs would be involuntarily restricted to the CPU subset inherited by > libvirtd. > That however prevents new host CPUs to be utilized when they show up. > With this change we will NOT set the VM's affinity mask if it > matches the online host CPU mask. > > There's still the chance that for some reason the deliberately chosen > libvirtd affinity matches the online host CPU mask by accident. In this > case the behavior remains as it was before (CPUs offline while setting > the affinity will not be used if they show up later on). > > Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com&gt; > Tested-by: Matthew Rosato <mjrosato(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_process.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 1b67aee..76f9210 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -2202,6 +2202,7 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm) > int ret = -1; > virBitmapPtr cpumap = NULL; > virBitmapPtr cpumapToSet = NULL; > + virBitmapPtr hostcpumap = NULL; > qemuDomainObjPrivatePtr priv = vm->privateData; > > if (!vm->pid) { > @@ -2223,6 +2224,16 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm) > * the spawned QEMU instance to all pCPUs if no map is given in > * its config file */ > int hostcpus; > + hostcpumap = virHostCPUGetOnlineBitmap(); > + cpumap = virProcessGetAffinity(vm->pid); Wouldn't this set 'cpumap' to something that shortly would be overwritten by virBitmapNew if we don't jump to cleanup in this patch?

Beyond that - I'll let someone with more detailed knowledge of SetAffinity nuances decide whether avoiding the call is proper.

> + > + if (hostcpumap && cpumap && virBitmapEqual(hostcpumap, cpumap)) { > + /* we're using all available CPUs, no reason to set > + * mask. If libvirtd is running without explicit > + * affinity, we can use hotplugged CPUs for this VM */ > + ret = 0; > + goto cleanup; > + } >

> /* setaffinity fails if you set bits for CPUs which > * aren't present, so we have to limit ourselves */ > @@ -2248,6 +2259,7 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm) > > cleanup: > virBitmapFree(cpumap); > + virBitmapFree(hostcpumap); > return ret; > } > > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 24.11.2016 14:26, Martin Kletzander wrote: > On Wed, Nov 23, 2016 at 12:03:22PM -0500, John Ferlan wrote: >> >> >> On 11/04/2016 04:01 AM, Viktor Mihajlovski wrote: >>> Running VMs couldn't use newly hot plugged host CPUs even if >>> the VMs had no CPU pinning defined AND the cpuset controller >>> was disabled in the libvirt qemu configuration. >> >> Add blank lines between paragraphs - just makes it easier to >> read. >> >>> This was because in this case the process affinity was set by >>> libvirt to all currently present host CPUs in order to avoid >>> situations, where libvirtd was deliberately running on a CPU >>> subset and thus the spawned VMs would be involuntarily >>> restricted to the CPU subset inherited by libvirtd. That >>> however prevents new host CPUs to be utilized when they show >>> up. With this change we will NOT set the VM's affinity mask if >>> it matches the online host CPU mask. >>> >>> There's still the chance that for some reason the deliberately >>> chosen libvirtd affinity matches the online host CPU mask by >>> accident. In this case the behavior remains as it was before >>> (CPUs offline while setting the affinity will not be used if >>> they show up later on). >>> >>> Signed-off-by: Viktor Mihajlovski >>> <mihajlov(a)linux.vnet.ibm.com&gt; Tested-by: Matthew Rosato >>> <mjrosato(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_process.c | 12 >>> ++++++++++++ 1 file changed, 12 insertions(+) >>> >>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >>> index 1b67aee..76f9210 100644 --- a/src/qemu/qemu_process.c +++ >>> b/src/qemu/qemu_process.c @@ -2202,6 +2202,7 @@ >>> qemuProcessInitCpuAffinity(virDomainObjPtr vm) int ret = -1; >>> virBitmapPtr cpumap = NULL; virBitmapPtr cpumapToSet = NULL; + >>> virBitmapPtr hostcpumap = NULL; qemuDomainObjPrivatePtr priv = >>> vm->privateData; >>> >>> if (!vm->pid) { @@ -2223,6 +2224,16 @@ >>> qemuProcessInitCpuAffinity(virDomainObjPtr vm) * the spawned >>> QEMU instance to all pCPUs if no map is given in * its config >>> file */ int hostcpus; + hostcpumap = >>> virHostCPUGetOnlineBitmap(); + cpumap = >>> virProcessGetAffinity(vm->pid); >> >> Wouldn't this set 'cpumap' to something that shortly would be >> overwritten by virBitmapNew if we don't jump to cleanup in this >> patch? >> > > Sure, that would need cleaning. > >> Beyond that - I'll let someone with more detailed knowledge of >> SetAffinity nuances decide whether avoiding the call is proper. >> > > Well, that's a long standing kernel "bug" (depending on what point > you look at it from) and this is one way of fixing lot of the > issues. We still have no (nice) way how to fix more than half of > the problems without kernel's helping hand, but at least we can > have this working. > >> >>> + + if (hostcpumap && cpumap && >>> virBitmapEqual(hostcpumap, cpumap)) { + /* we're >>> using all available CPUs, no reason to set + * >>> mask. If libvirtd is running without explicit + >>> * affinity, we can use hotplugged CPUs for this VM */ + >>> ret = 0; + goto cleanup; + } >>> > > However, I think we want something else here. Firstly, this will > report error for guests on anything else than Linux as > virHostCPUGetOnlineBitmap() is implemented only there, other > platforms just report error. Also the code, as it is now, sets the > affinity to all CPUs on the system (even those that are offline) > and it seems to work (for some definition of "work", i.e. we don't > get an error for sched_setaffinity()). I can't seem to wrap my > head around why that wouldn't work then. Well, it doesn't work :-). You can try out the following (on a 4 cpu system). --- $ sudo -i bash $ taskset -p $$ pid 25380's current affinity mask: f $ echo 0 > /sys/devices/system/cpu/cpu1/online taskset -p $$ pid 25380's current affinity mask: d # we can see that the affinty mask reflects the offline CPU $ bash $ taskset -p $$ pid 25956's current affinity mask: d $ taskset -p -c 0-3 $$ pid 25956's current affinity list: 0,2,3 pid 25956's new affinity list: 0,2,3 # the above command attempts to set the affinity to all CPUs # including the offline one $ taskset -p $$ pid 25956's current affinity mask: d # so, it seems to have worked, but echo 1 > /sys/devices/system/cpu/cpu1/online $ taskset -p $$ pid 25956's current affinity mask: d # the process won't get CPU #1 back! $ exit $ taskset -p $$ pid 25380's current affinity mask: f # the parent can use the hot plugged CPU because it didn't # and so would have the child --- If you substitute the "outer" bash with libvirtd and the "inner" bash with QEMU, the mess becomes obvious as well as the remedy.

I'll send out a patch with the attempt of better wording...

> >>> /* setaffinity fails if you set bits for CPUs which * aren't >>> present, so we have to limit ourselves */ @@ -2248,6 +2259,7 @@ >>> qemuProcessInitCpuAffinity(virDomainObjPtr vm) >>> >>> cleanup: virBitmapFree(cpumap); + >>> virBitmapFree(hostcpumap); return ret; } >>> >>> >> >> -- libvir-list mailing list libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294