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