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

[John Ferlan]

[...]

>>> @@ -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.
> 
> ...

I suppose I was more grousing about the 3 times through, but then
starting thinking is it worth it to print unknown, but since this is
output only so I can agree it's fine to be explicit.

Sorry if taking the route of ACK'ing every 10 or so patches is/was hard
to work with - it's a large series and was not possible for me to review
in one sitting, so I tried to break it up into more manageable chunks.
In particular, this one I reviewed after 7PM (considering I typically am
up/online by 6AM - that's a long day). It was mostly a workflow thing
for me and I've seen other reviews take a similar approach with larger
series.  I should have also replied to the cover letter, but forgot.

In any case, I think you've answered my issues so (more explicit) ACK
for this one with the noted changes.

John
>>> @@ -2960,7 +2964,8 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const 
>>> char *filename,
>>>  }
>>>  
>>>  if (virDomainCapsCPUModelsAddSteal(qemuCaps->cpuDefinitions,
>>> -   ) < 0)
>>> +   ,
>>> +   
>>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Jiri Denemark]

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

Fixed.

> > +libvirt does not have enough information to provide the usability
> > +data.
> >
> >  
> >  
> > 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 @@
> >
> >
> >  
> > +  
> > +
> > +  yes
> > +  no
> > +  unknown
> > +
> > +  
> 
> 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,
> > -   ) < 0)
> > +   ,
> > +   
> > 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[John Ferlan]

On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> In case a hypervisor is able to tell us a list of supported CPU models
> and whether each CPU models can be used on the current host, we can
> propagate this to domain capabilities. This is a better alternative
> to calling virConnectCompareCPU for each supported CPU model.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  docs/formatdomaincaps.html.in  | 10 ++--
>  docs/schemas/domaincaps.rng|  7 +++
>  src/conf/domain_capabilities.c | 50 +-
>  src/conf/domain_capabilities.h | 16 +-
>  src/libvirt_private.syms   |  2 +
>  src/qemu/qemu_capabilities.c   | 15 --
>  tests/domaincapsschemadata/full.xml|  6 +--
>  tests/domaincapsschemadata/qemu_1.7.0.x86_64.xml   | 48 -
>  .../qemu_2.6.0-gicv2-virt.aarch64.xml  | 60 
> +++---
>  .../qemu_2.6.0-gicv3-virt.aarch64.xml  | 60 
> +++---
>  tests/domaincapsschemadata/qemu_2.6.0.aarch64.xml  | 60 
> +++---
>  tests/domaincapsschemadata/qemu_2.6.0.ppc64le.xml  |  4 +-
>  tests/domaincapsschemadata/qemu_2.6.0.x86_64.xml   | 56 ++--
>  tests/domaincapstest.c |  9 ++--
>  14 files changed, 229 insertions(+), 174 deletions(-)
> 

So 11-19 with at least some adjustments have an ACK from me. Since
you're fixing issues the point I noted in patch 11 is probably something
that patches 10-12 resolve, so no big deal.  The coverity issue in 15
would be nice to resolve, but since it already exists afaict it's doable
afterwards.  My note from patch 19 probably needs to be handled as I
imagine it'll be an "issue" on "some" platform with "some" compiler.

I'll hold off on an ACK for this one.  I'm going to stop here for today
and pick it up again tomorrow.  I do have some notations below. I have
some questions, but perhaps they'll be answered in future changes.


John
> diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
> index 15a13c1..49ccbfc 100644
> --- a/docs/formatdomaincaps.html.in
> +++ b/docs/formatdomaincaps.html.in
> @@ -156,9 +156,9 @@
>  mode name='host-passthrough' supported='yes'/
>  mode name='host-model' supported='yes'/
>  mode name='custom' supported='yes'
> -  modelBroadwell/model
> -  modelBroadwell-noTSX/model
> -  modelHaswell/model
> +  model usable='no'Broadwell/model
> +  model usable='yes'Broadwell-noTSX/model
> +  model usable='no'Haswell/model
>...
>  /mode
>/cpu
> @@ -183,6 +183,10 @@
>
>  The mode element contains a list of supported CPU
>  models, each described by a dedicated model element.
> +The usable attribute specifies whether the model can
> +be used on the host. A special value unknown says

s/says/indicates/

> +libvirt does not have enough information to provide the usability
> +data.
>
>  
>  
> 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 @@
>
>
>  
> +  
> +
> +  yes
> +  no
> +  unknown
> +
> +  

Why is this not optional? Hmm... Seems to be output only - still an odd
construct though. Not sure I have a better idea (yet).

>
>  
>
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 6f9f7e7..c9e3a28 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -29,6 +29,9 @@
>  
>  #define VIR_FROM_THIS VIR_FROM_CAPABILITIES
>  
> +VIR_ENUM_IMPL(virDomainCapsCPUUsable, VIR_DOMCAPS_CPU_USABLE_LAST,
> +  "unknown", "yes", "no");
> +
>  static virClassPtr virDomainCapsClass;
>  static virClassPtr virDomainCapsCPUModelsClass;
>  
> @@ -157,7 +160,9 @@ virDomainCapsCPUModelsCopy(virDomainCapsCPUModelsPtr old)
>  return NULL;
>  
>  for (i = 0; i < old->count; i++) {
> -if (virDomainCapsCPUModelsAdd(cpuModels, old->models[i].name, -1) < 
> 0)
> +if (virDomainCapsCPUModelsAdd(cpuModels,
> +  old->models[i].name, -1,
> +  old->models[i].usable) < 0)
>  goto error;
>  }
>  
> @@ -184,7 +189,8 @@ virDomainCapsCPUModelsFilter(virDomainCapsCPUModelsPtr 
> old,
>  continue;
>  
>  if (virDomainCapsCPUModelsAdd(cpuModels,
> -  old->models[i].name, -1) < 0)
> +  old->models[i].name, -1,
> +  old->models[i].usable) < 0)
>  goto error;
>  }
>  
> @@