On 10/12/2012 11:50 AM, Osier Yang wrote: > These 3 elements conflicts with each other in either the doc > or the underlying codes. > > 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> > > 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>. > > 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. > > 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. > > 5) If "placement" of <vcpu> is "auto", <emulatorpin> is not allowed. > > 6) hotplugged vcpus will also inherit "cpuset" of <vcpu> > > Codes changes according to above document changes: > > 1) Inherit def->cpumask for each vcpu which doesn't have <vcpupin> > specified, during parsing. > > 2) ping the vcpu which doesn't have <vcpupin> specified to def->cpumask > either by cgroup for sched_setaffinity(2), which is actually done > by 1). > > 3) Error out if "placement" == "auto", and <emulatorpin> is specified. > Otherwise, <emulatorpin> is honored, and "cpuset" of <cpuset> is > ignored. > > 4) Setup cgroup for each hotplugged vcpu, and setup the pinning policy > by either cgroup or sched_setaffinity(2). > > 5) Remove cgroup and <vcpupin> for each hot unplugged vcpu. > > Patches are following. > > --- > docs/formatdomain.html.in | 42 +++++++++++++++++++++++++++--------------- > 1 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index d664e7e..1ae8cf4 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -357,8 +357,18 @@ > the maximum supported by the hypervisor. <span class="since">Since > 0.4.4</span>, this element can contain an optional > <code>cpuset</code> attribute, which is a comma-separated > - list of physical CPU numbers that virtual CPUs can be pinned > - to. Each element in that list is either a single CPU number, > + list of physical CPU numbers that domain process and virtual CPUs > + can be pinned to by default. (NB: The pinning policy of domain > + process and virtual CPUs can be specified separately by > + <code>cputune</code>. If attribute <code>emulatorpin</code> > + of <code>cputune</code> is specified, <code>cpuset</code> > + specified by <code>vcpu</code> here will be ingored; Similarly, s/ingored/ignored/ > + For virtual CPUs which has <code>vcpupin</code> specified, s/F/f/; s/has/have/ > + <code>cpuset</code> specified by <code>cpuset</code> here The second 'cpuset' should be 'vcpu', doesn't it? > + will be ignored; For virtual CPUs which doesn't have s/doesn't/don't/ > + <code>vcpupin</code> specified, it will be pinned to the physical > + CPUs specified by <code>cpuset</code> here). I think you can leave out the sentence from '; For' onwards since it is explained already in previous sentences. > + Each element in that list is either a single CPU number, > a range of CPU numbers, or a caret followed by a CPU number to > be excluded from a previous range. <span class="since">Since > 0.8.5</span>, the optional attribute <code>current</code> can > @@ -374,8 +384,7 @@ > if it's specified. If both <code>cpuset</code> and <code>placement</code> > are not specified, or if <code>placement</code> is "static", but no > <code>cpuset</code> is specified, the domain process will be pinned to > - all the available physical CPUs. These settings are superseded > - by <a href="#elementsCPUTuning">CPU tuning</a>. > + all the available physical CPUs. > </dd> > </dl> > > @@ -411,23 +420,26 @@ > <dt><code>vcpupin</code></dt> > <dd> > The optional <code>vcpupin</code> element specifies which of host's > - physical CPUs the domain VCPU will be pinned to. This setting supersedes > - previous VCPU placement specified in <a href="#elementsCPUAllocation">CPU > - Allocation</a> using <code>vcpu</code> element. If this is omitted, > - each VCPU is pinned to all the physical CPUs by default. It contains two > - required attributes, the attribute <code>vcpu</code> specifies vcpu id, > - and the attribute <code>cpuset</code> is same as > - attribute <code>cpuset</code> > - of element <code>vcpu</code>. (NB: Only qemu driver support) > + physical CPUs the domain VCPU will be pinned to. If this is omitted, > + and attribute <code>cpuset</code> of element <code>vcpu</code> is > + not specified, the vCPU is pinned to all the physical CPUs by default. > + It contains two required attributes, the attribute <code>vcpu</code> > + specifies vcpu id, and the attribute <code>cpuset</code> is same as > + attribute <code>cpuset</code> of element <code>vcpu</code>. > + (NB: Only qemu driver support) > <span class="since">Since 0.9.0</span> > </dd> > <dt><code>emulatorpin</code></dt> > <dd> > The optional <code>emulatorpin</code> element specifies which of host > physical CPUs the "emulator", a subset of a domain not including vcpu, > - will be pinned to. If this is omitted, "emulator" is pinned to all > - the physical CPUs by default. It contains one required attribute > - <code>cpuset</code> specifying which physical CPUs to pin to. > + will be pinned to. If this is omitted, and attribute > + <code>cpuset</code> of element <code>vcpu</code> is not specified, > + "emulator" is pinned to all the physical CPUs by default. It contains > + one required attribute <code>cpuset</code> specifying which physical > + CPUs to pin to. NB, <code>emulatorpin</code> is not allowed if > + attribute <code>placement</code> of element <code>vcpu</code> is > + "auto". > </dd> > <dt><code>shares</code></dt> > <dd> > ACK with those fixes. Martin -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list