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

[Martin Kletzander]

On Fri, Aug 07, 2015 at 09:14:29AM -0400, John Ferlan wrote:
>
>
>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 at redhat.com>
>>>> ---
>>>>  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.
>

So, GetIOThreadsLive does GetAffinity, similarly to how the others did
it some time back.  But GetIOThreadsConfig cannot work with autoCpuset
anyhow.  That's initialized when the domain is being started.  There
is no numad hint when the domain is shut off (or in its permanent
definition).

>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?
>

Well, no.  virDomainDefNeedsPlacementAdvice() just tells us whether we
need to run numad to get an advice, at all (like if _any_ part of the
configuration uses automatic placement).  But in these getters we need
to return it only if it was used for pinning the threads.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150807/4a928345/attachment-0001.sig>