Re: [PATCH v5 4/5] qemu: Add support to pin IOThreads to specific CPU

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]