[libvirt] [PATCH] RFC: Clarify the relationship between <vcpu>, <vcpupin>, and <emulatorpin>

[Osier Yang]

On 2012年10月15日 14:22, Hu Tao wrote:
> On Fri, Oct 12, 2012 at 01:27:27PM +0800, Osier Yang wrote:
>> These 3 elements conflicts with each other in either the doc
>> or the underlying codes are. This is to propse a solution.
>>
>> Before writing any codes, I want to see if the principle is
>> correct, any advise is welcomed.
>>
>> Current problems:
>>
>> Problem 1:
>>
>> The doc shouldn't simply say "These settings are superseded
>> by CPU tuning. " for element<vcpu>. As except the tuning,<vcpu>
>> allows to specify the current, maxmum vcpu number. Apart from that,
>> <vcpu>  also allows to specify the placement as "auto", which binds
>> the domain process to the advisory nodeset from numad.
>>
>> Problem 2:
>>
>> Doc for<vcpu>  says its "cpuset" specify the physical CPUs
>> that the vcpus can be pinned. But it's not the truth, as
>> actually it only pin domain process to the specified physical
>> CPUs. So either it's a document bug, or code bug.
>>
>> Problem 3:
>>
>> Doc for<vcpupin>  says it supersed "cpuset" of<vcpu>, it's
>> not quite correct, as each<vcpupin>  specify the pinning policy
>> only for one vcpu. How about the ones which doesn't have
>> <vcpupin>  specified? it says the vcpu will be pinned to all
>> available physical CPUs, but what's the meaning of attribute
>> "cpuset" of<vcpu>  then?
>>
>> Problem 4:
>>
>> Doc for<emulatorpin>  says it pin the emulator threads (domain
>> process in other context, perhaps another follow up patch to
>> cleanup the inconsistency is needed) to the physical CPUs
>> specified its attribute "cpuset". Which conflicts with
>> <vcpu>'s "cpuset". And actually in the underlying codes,
>> it set the affinity for domain process twice if both
>> "cpuset" for<vcpu>  and<emulatorpin>  are specified,
>> and<emulatorpin>'s pinning will override<vcpu>'s.
>>
>> Problem 5:
>>
>> When "placement" of<vcpu>  is "auto" (I.e. uses numad to
>> get the advisory nodeset to which the domain process is
>> pinned to), it will also be overridden by<emulatorpin>,
>>
>> This patch is trying to sort out the conflicts or bugs by:
>>
>> 1) Don't say<vcpu>  is superseded by<cputune>
>
> You mean in the documentation of XML format?

Yes.

>
> Acutally the VCPUs placement settings of<vcpu>  will be overrided
> by those of<cputune>. So I think it's better to keep the words in
> doc to make users aware of this.

The problem is <vcpu> not only defines the vcpu affinities. And
in the new design (see following), "cpuset" of <vcpu> defines
the **default** placement for both domain process (emulator threads
in emulatorpin context) and vcpu threads. For vcpus which doesn't
have <vcpupin> specified, they still inherit the default placement.
Also domain process will be pinned to the default placement if
<emulatorpin> is not specified.

>
>>
>> 2) Keep the semanteme for "cpuset" of<vcpu>  (I.e. Still says it
>>     specify the physical CPUs the virtual CPUs). But modifying it
>>     to mention it also set the pinning policy for domain process,
>>     and the CPU placement of domain process specified by "cpuset"
>>     of<vcpu>  will be ingored if<emulatorpin>  specified, and
>>     similary, the CPU placement of vcpu thread will be ignored
>>     if it has<vcpupin>  specified, for vcpu which doesn't have
>>     <vcpupin>  specified, it inherits "cpuset" of<vcpu>.
>
> OK.
>
>>
>> 3) Don't say<vcpu>  is supersed by<vcpupin>. If neither<vcpupin>
>>     nor "cpuset" of<vcpu>  is specified, the vcpu will be pinned
>>     to all available pCPUs.
>
> OK.
>
>>
>> 4) If neither<emulatorpin>  nor "cpuset" of<vcpu>  is specified,
>>     the domain process (emulator threads in the context) will be
>>     pinned to all available pCPUs.
>
> OK.
>
>>
>> 5) If "placement" of<vcpu>  is "auto",<emulatorpin>  is not allowed.
>
> Conflicts with 2). Why not just override the emulator part? for vcpu
> threads the "placement" is still "auto".

In the final patch, <emulatorpin> is ignored if vcpu placement
is "auto" when parsing.

>
>>
>> 6) hotplugged vcpus will also inherit "cpuset" of<vcpu>
>
> OK.
>
>>
>> Codes changes above document changes will cause:
>>
>> 1) Inherit def->cpumask for each vcpu which doesn't have<vcpupin>
>>     specified, during parsing.
>
> OK.
>
>>
>> 2) ping the vcpu which doesn't have<vcpupin>  specified to def->cpumask
>
> s/ping/pin/
>
>>     either by cgroup for sched_setaffinity(2), which is actually done
>>     by 1).
>
> pin vcpu according to this_vcpu->cpumask, since the cpumask either
> inherits from def->cpumask(<vcpu>), or is set by<vcpupin>.

Yeah, but this is already done by either cgroup or sched_setaffinity
when domain starting. So no new codes is needed.

>
>>
>> 3) Error out if "placement" == "auto", and<emulatorpin>  is specified.
>>     Otherwise,<emulatorpin>  is honored, and "cpuset" of<cpuset>  is
>>     ignored.
>
> You mean "cpuset" of<vcpu>  here?

Right, a typo.

>
> But I still don't understand why "placement" = "auto" and<emulatorpin>
> can not both exist, but the latter overides the former.

I think we have agreement on the princinple: we must use either
"placement == auto" or <emulatorpin> to set the affinity for
domain process, but not the both, right?

Based on the agreement, there are two ways, one is to ignore one
of them inside driver, another is to ignore when parsing conf.
The later one is better as other drivers could support "placement"
and "emulatorpin" in future, doing the ignoring work inside
each driver is duplicate work, and we need a general rule
about the relationship between them for doc.

In case of we don't have agreement we can't use both of them
to set the affinites. The reason is:

numad is likely to manage the affinity for domain process
dynamically in future, it's unpreditable to overrides the
advisory affinity from numad with cgroup afterwards.

Regards,
Osier