Re: [libvirt] [PATCH v5 4/5] qemu: Add support to pin IOThreads to specific CPU
On Tue, Mar 10, 2015 at 11:20:43AM -0400, John Ferlan wrote:
On 03/10/2015 09:51 AM, Ján Tomko wrote:
> On Fri, Mar 06, 2015 at 09:05:44AM -0500, John Ferlan wrote:
<...snip...>
>> +
>> + /* pinning to all physical cpus means resetting,
>
> It doesn't.
> By default the iothreads inherit the pinning from the domain's cpumask.
>
> A completely clear bitmap would be a better value to mean resetting,
> since it makes no sense otherwise. But getting the cpumask in that case
> won't be that easy.
>
So again - this is taken from qemuDomainPinVcpuFlags() - if it's invalid
here, then it's invalid there as well.
Yes, that function might also not behave properly on the corner case of
pinning a vcpu to all CPUs.
But that one has been released for ages. This is new code that doesn't
have to repeat its mistakes.
Is your objection the comment? Should it be striken or restated?
It's not the comment I have a problem with, it's the behavior that follows it.
Calling this API on a domain with a per-domain cpuset to pin an iothread
to all pCPUs will result in:
a) for AFFECT_LIVE
The iothread will get pinned to all pCPUs
The pininfo will get deleted from the definition. But missing pininfo
does not mean all pCPUs if there is a per-domain cpuset.
b) for AFFECT_CONFIG
The pininfo will be deleted, even though it does not match the
cpumask.
I think the easiest 'fix' is to just drop all the 'doReset' functionality
and do not allow deleting pininfo with this API.
Alternatively, (since you already rejected
VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO above) you can pin the iothread to
the domain's cpuset.
Also, the full bitmap is ambiguous - does it means 'reset
it to default' or 'pin it to all pCPUs'?
>> + * so check if we can reset setting.
>> + */
>> + if (virBitmapIsAllSet(pcpumap))
>> + doReset = true;
>> +
>> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>> +
>> + if (priv->iothreadpids == NULL) {
>> + virReportError(VIR_ERR_OPERATION_INVALID,
>> + "%s", _("IOThread affinity is not
supported"));
>> + goto endjob;
>> + }
>> +
>> + if (iothread_id > priv->niothreadpids) {
>> + virReportError(VIR_ERR_INVALID_ARG,
>> + _("iothread value out of range %d >
%d"),
>> + iothread_id, priv->niothreadpids);
>> + goto endjob;
>> + }
>> +
>> + if (vm->def->cputune.iothreadspin) {
>> + /* The VcpuPinDefCopy works for IOThreads too */
>
> Maybe it should be renamed to something like virDomainPinDefCopy then?
>
Perhaps, but that seems outside the scope of these changes.
Yes, that one belongs to a separate patch.
The 'reuse'
of virDomainVcpuPinDefPtr by the IOThreads code was an "optimization"
that could also then be generalized I suppose (eg change from
virDomainVcpuPinDefPtr to virDomainPinDefPtr), but that's a much more
invasive change which would seemingly require change the structure
"vcpuid" value to just "id".
It's just a name change. And they are generalized already, it's just the
naming that's behind :)
Jan
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)