Re: [PATCH 20/41] domcaps: Add CPU usable flag

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

 



On Mon, Aug 29, 2016 at 19:21:24 -0400, John Ferlan wrote:
...
> > @@ -183,6 +183,10 @@
> >        <dd>
> >          The <code>mode</code> element contains a list of supported CPU
> >          models, each described by a dedicated <code>model</code> element.
> > +        The <code>usable</code> attribute specifies whether the model can
> > +        be used on the host. A special value <code>unknown</code> says
> 
> s/says/indicates/

Fixed.

> > +        libvirt does not have enough information to provide the usability
> > +        data.
> >        </dd>
> >      </dl>
> >  
> > diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
> > index 9f3d225..5a605a7 100644
> > --- a/docs/schemas/domaincaps.rng
> > +++ b/docs/schemas/domaincaps.rng
> > @@ -105,6 +105,13 @@
> >        <ref name='supported'/>
> >        <zeroOrMore>
> >          <element name='model'>
> > +          <attribute name='usable'>
> > +            <choice>
> > +              <value>yes</value>
> > +              <value>no</value>
> > +              <value>unknown</value>
> > +            </choice>
> > +          </attribute>
> 
> Why is this not optional? Hmm... Seems to be output only - still an odd
> construct though. Not sure I have a better idea (yet).

Exactly, it's output only and we always format it so there's no reason
for the attribute to be optional.

...
> > @@ -399,7 +412,16 @@ virDomainCapsCPUFormat(virBufferPtr buf,
> >                        virCPUModeTypeToString(VIR_CPU_MODE_CUSTOM));
> >      if (cpu->custom && cpu->custom->count) {
> >          virBufferAddLit(buf, "supported='yes'>\n");
> > -        virDomainCapsCPUCustomFormat(buf, cpu->custom);
> > +        virBufferAdjustIndent(buf, 2);
> > +
> > +        virDomainCapsCPUCustomFormat(buf, cpu->custom,
> > +                                     VIR_DOMCAPS_CPU_USABLE_YES);
> > +        virDomainCapsCPUCustomFormat(buf, cpu->custom,
> > +                                     VIR_DOMCAPS_CPU_USABLE_NO);
> > +        virDomainCapsCPUCustomFormat(buf, cpu->custom,
> > +                                     VIR_DOMCAPS_CPU_USABLE_UNKNOWN);
> 
> So we're listing all the usable ones first, followed by the unusable,
> followed by the unknown. Hence the full.xml output change.

Hmm, it really seems pretty stupid to group them by usable value,
doesn't it? I just fixed the code so that the CPU models are printed in
a single loop no matter whether they're usable or not.

> In any case, that seem like something that would be documentable - the
> sorting algorithm...  It wasn't listed in the cover letter either (the
> usable attribute isn't there).

Hmm, you're right, I missed this in the cover letter.

> I guess it just seems inefficient to run through the custom list 3 times
> just so we can print out in a specific/sorted order.  Not sure what
> printing "unknown" really buys us - seems to be ignorable to me at least
> at this point in the review process.

For any hypervisor that doesn't give us the usability data, all CPUs
will have usable='unknown'. Or were you thinking about why don't we just
skip this attribute if it's value is unknown? If so, I think it's better
to be explicit.

...
> > @@ -2960,7 +2964,8 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename,
> >              }
> >  
> >              if (virDomainCapsCPUModelsAddSteal(qemuCaps->cpuDefinitions,
> > -                                               &str) < 0)
> > +                                               &str,
> > +                                               VIR_DOMCAPS_CPU_USABLE_UNKNOWN) < 0)]
> 
> So we can go from 'yes' or 'no' to 'unknown' (for at least a short
> period of time). I guess I would have expected to "read" and use the
> cached data like other places...  Leads me to wonder why it's being
> saved.  Can it be possible to go from 'yes' to 'no' if we go through
> unknown?  Guess it's just not clear what the dynamics of the conversion
> are and when is (should be) expected.

There's no code in QEMU driver which would set the value to anything but
'unknown' yet. There's no conversion involved. This is just a
preparation for a later patch that would really ask QEMU for this stuff.

Jirka

--
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]