Re: [libvirt] [PATCH 3/3] qemu: Return true pining info when using numad

On 08/07/2015 06:44 AM, Martin Kletzander wrote: > On Tue, Aug 04, 2015 at 09:27:13PM -0400, John Ferlan wrote: >> $SUBJ >> >> s/pining/pinning >> >> Or perhaps - "qemu: Use numad information when getting pin information"" >> > > OK, I won't mention anything pine-cone-related then ;) > >> On 07/26/2015 12:57 PM, Martin Kletzander wrote: >>> Pinning information returned for emulatorpin and vcpupin calls is being >>> returned from our data without querying cgroups for some time. However, >>> not all the data were utilized. When automatic placement is used the >>> information is not returned for the calls mentioned above. Since the >>> numad hint in private data is properly saved/restored, we can safely use >>> it to return true information. >>> >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1162947 >>> >>> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_driver.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >> >> Should qemuDomainGetIOThreadsConfig be adjusted as well? In the for >> loop that's fetching/filling in the iothreadid there's a filling of the >> cpumask as well. >> > > From the name I think that rather qemuDomainGetIOThreadsLive() should > be adjusted, not Config(), unless I misunderstood what they do. > The Live function takes whatever bitmap was used to set using GetAffinity. It can be initially set from autoCpuset in hotplug or qemuProcessStart (return from qemuSetupCgroupForIOThreads). So I think Live is fine. The "Config" though would need adjustment.

For the Emulator/Vcpu code LIVE/CONFIG is a bit different since it combines live/config... >> Patches seem reasonable otherwise, although patch2 could have a wee bit >> more information in the commit log to explain what's being done... > > OK, I'll add some info in there. > >> Beyond that does that value matter if placement_mode != >> VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO? or if >> !virDomainDefNeedsPlacementAdvice (from qemuProcessStart)? Was checking >> where it was set and if it's set to something reasonable... >> > > No it doesn't. Is there a problem with that? I haven't found any. > IIRC - when I was looking at how/where autoCpuset was initialized - it was only done so in qemuProcessStart, but I think my thought was around whether there was a chance if placement_mode could be AUTO and autoCpuset not be properly initialized. In hindsight and after more looking it doesn't seem possible; however, I do note that the NeedsPlacementAdvice can return true for more than one reason. So now I'm wondering - should any of those checks that only check placement_mode == AUTO - should they change to using virDomainDefNeedsPlacementAdvice?