[libvirt] [PATCH v3 0/6] BaselineHypervisorCPU using QEMU QMP exchanges

Chris Venteicher posted 6 patches 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180927212645.20758-1-cventeic@redhat.com
There is a newer version of this series
src/qemu/qemu_capabilities.c                  | 559 ++++++++----------
src/qemu/qemu_capabilities.h                  |   4 +
src/qemu/qemu_driver.c                        | 143 ++++-
src/qemu/qemu_monitor.c                       | 199 ++++++-
src/qemu/qemu_monitor.h                       |  29 +-
src/qemu/qemu_monitor_json.c                  | 210 +++++--
src/qemu/qemu_monitor_json.h                  |  13 +-
src/qemu/qemu_process.c                       | 398 +++++++++++++
src/qemu/qemu_process.h                       |  35 ++
tests/cputest.c                               |  11 +-
.../caps_2.10.0.s390x.xml                     |  60 +-
.../caps_2.11.0.s390x.xml                     |  58 +-
.../caps_2.12.0.s390x.xml                     |  56 +-
.../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  32 +-
.../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  34 +-
.../qemucapabilitiesdata/caps_3.0.0.s390x.xml |  64 +-
tests/qemucapabilitiestest.c                  |   7 +
17 files changed, 1375 insertions(+), 537 deletions(-)
[libvirt] [PATCH v3 0/6] BaselineHypervisorCPU using QEMU QMP exchanges
Posted by Chris Venteicher 5 years, 6 months ago
Some architectures (S390) depend on QEMU to compute baseline CPU model and
expand a models feature set.

Interacting with QEMU requires starting the QEMU process and completing one or
more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
query-cpu-model-expansion QMP exchange to expand all features in the model.

See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion
of QEMU aspects.

This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999

Version 3 attempts to address all issues from V1 and V2 including making the 
QEMU process activation for QMP Queries generic (not specific to capabilities)
and moving that code from qemu_capabilities to qemu_process. 

I attempted to make the new qemu_process functions consistent with the functions
but mostly ended up reusing the guts of the previous functions from qemu_capabilities.

Of interest may be the choice to reuse a domain structure to hold the Qmp Query
process state and connection information.  Also, pay attention to the use of a large
random number to uniquely identify decoupled processes in terms of sockets and
file system footprint.  If you believe it's worth the effort I think there are some
pre-existing library functions to establish files with unique identifiers in a
thread safe way.

The last patch may also be interesting and is based on past discussion of QEMU cpu
expansion only returning migratable features except for one x86 case where
non-migratable features are explicitly requested.  The patch records that features
are migratable based on QEMU only returning migratable features.  The main result
is the CPU xml for S390 CPU's contains the same migratability info the X86 currently
does.  The testcases were updated to reflect the change.  Is this ok?  

Unlike the previous versions every patch should compile independently if applied
in sequence.

Thanks,
Chris


Chris Venteicher (6):
  qemu_monitor: Introduce qemuMonitorCPUModelInfoNew
  qemu_monitor: Introduce qemuMonitorCPUModelInfo / JSON conversion
  qemu_monitor: qemuMonitorGetCPUModelExpansion inputs and outputs
    CPUModelInfo
  qemu_process: Use common processes mgmt funcs for all QMP query types
  qemu_driver: BaselineHypervisorCPU support for S390
  qemu_monitor: Default props to migratable when expanding cpu model

 src/qemu/qemu_capabilities.c                  | 559 ++++++++----------
 src/qemu/qemu_capabilities.h                  |   4 +
 src/qemu/qemu_driver.c                        | 143 ++++-
 src/qemu/qemu_monitor.c                       | 199 ++++++-
 src/qemu/qemu_monitor.h                       |  29 +-
 src/qemu/qemu_monitor_json.c                  | 210 +++++--
 src/qemu/qemu_monitor_json.h                  |  13 +-
 src/qemu/qemu_process.c                       | 398 +++++++++++++
 src/qemu/qemu_process.h                       |  35 ++
 tests/cputest.c                               |  11 +-
 .../caps_2.10.0.s390x.xml                     |  60 +-
 .../caps_2.11.0.s390x.xml                     |  58 +-
 .../caps_2.12.0.s390x.xml                     |  56 +-
 .../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  32 +-
 .../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  34 +-
 .../qemucapabilitiesdata/caps_3.0.0.s390x.xml |  64 +-
 tests/qemucapabilitiestest.c                  |   7 +
 17 files changed, 1375 insertions(+), 537 deletions(-)

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/6] BaselineHypervisorCPU using QEMU QMP exchanges
Posted by Jiri Denemark 5 years, 5 months ago
Hmm, I guess I could have checked the cover letter before looking at the
individual patches. That would safe me some time and thinking :-)

On Thu, Sep 27, 2018 at 16:26:39 -0500, Chris Venteicher wrote:
> Some architectures (S390) depend on QEMU to compute baseline CPU model and
> expand a models feature set.
> 
> Interacting with QEMU requires starting the QEMU process and completing one or
> more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
> query-cpu-model-expansion QMP exchange to expand all features in the model.
> 
> See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion
> of QEMU aspects.
> 
> This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> 
> Version 3 attempts to address all issues from V1 and V2 including making the 
> QEMU process activation for QMP Queries generic (not specific to capabilities)
> and moving that code from qemu_capabilities to qemu_process. 

So far so good.

> I attempted to make the new qemu_process functions consistent with the functions
> but mostly ended up reusing the guts of the previous functions from qemu_capabilities.

I guess you wanted to say "... consistent with the existing functions in
qemu_process.c" or something similar, right? It's fine to just move and
generalize the functions from qemu_capabilities (and perhaps make the
original functions just wrappers around the new ones if needed).

> Of interest may be the choice to reuse a domain structure to hold the Qmp Query
> process state and connection information.

This sounds like a bad idea to me.

> Also, pay attention to the use of a large random number to uniquely
> identify decoupled processes in terms of sockets and file system
> footprint.  If you believe it's worth the effort I think there are
> some pre-existing library functions to establish files with unique
> identifiers in a thread safe way.

We already have src/util/virrandom.c for random numbers, but it doesn't
really matter anyway since we don't need or either want to use them
here. Just use mkdtemp() and store the socket, pid, whatever you need in
there.

> The last patch may also be interesting and is based on past discussion of QEMU cpu
> expansion only returning migratable features except for one x86 case where
> non-migratable features are explicitly requested.  The patch records that features
> are migratable based on QEMU only returning migratable features.  The main result
> is the CPU xml for S390 CPU's contains the same migratability info the X86 currently
> does.  The testcases were updated to reflect the change.  Is this ok?  

Yeah, looks OK, although I didn't look too closely at it.

> Unlike the previous versions every patch should compile independently if applied
> in sequence.

Good, each series have to make sure the code can be compiled after every
single patch in the series (so that we can easily use git bisect). But
please, don't achieve the goal by squashing patches until the result can
be compiled.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/6] BaselineHypervisorCPU using QEMU QMP exchanges
Posted by Chris Venteicher 5 years, 5 months ago
Quoting Jiri Denemark (2018-10-03 09:36:34)
> Hmm, I guess I could have checked the cover letter before looking at the
> individual patches. That would safe me some time and thinking :-)
> 
> On Thu, Sep 27, 2018 at 16:26:39 -0500, Chris Venteicher wrote:
> > Some architectures (S390) depend on QEMU to compute baseline CPU model and
> > expand a models feature set.
> > 
> > Interacting with QEMU requires starting the QEMU process and completing one or
> > more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
> > query-cpu-model-expansion QMP exchange to expand all features in the model.
> > 
> > See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion
> > of QEMU aspects.
> > 
> > This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> > 
> > Version 3 attempts to address all issues from V1 and V2 including making the 
> > QEMU process activation for QMP Queries generic (not specific to capabilities)
> > and moving that code from qemu_capabilities to qemu_process. 
> 
> So far so good.
> 
> > I attempted to make the new qemu_process functions consistent with the functions
> > but mostly ended up reusing the guts of the previous functions from qemu_capabilities.
> 
> I guess you wanted to say "... consistent with the existing functions in
> qemu_process.c" or something similar, right? It's fine to just move and
> generalize the functions from qemu_capabilities (and perhaps make the
> original functions just wrappers around the new ones if needed).
> 
> > Of interest may be the choice to reuse a domain structure to hold the Qmp Query
> > process state and connection information.
> 
> This sounds like a bad idea to me.
> 
> > Also, pay attention to the use of a large random number to uniquely
> > identify decoupled processes in terms of sockets and file system
> > footprint.  If you believe it's worth the effort I think there are
> > some pre-existing library functions to establish files with unique
> > identifiers in a thread safe way.
> 
> We already have src/util/virrandom.c for random numbers, but it doesn't
> really matter anyway since we don't need or either want to use them
> here. Just use mkdtemp() and store the socket, pid, whatever you need in
> there.
> 
> > The last patch may also be interesting and is based on past discussion of QEMU cpu
> > expansion only returning migratable features except for one x86 case where
> > non-migratable features are explicitly requested.  The patch records that features
> > are migratable based on QEMU only returning migratable features.  The main result
> > is the CPU xml for S390 CPU's contains the same migratability info the X86 currently
> > does.  The testcases were updated to reflect the change.  Is this ok?  
> 
> Yeah, looks OK, although I didn't look too closely at it.
> 
> > Unlike the previous versions every patch should compile independently if applied
> > in sequence.
> 
> Good, each series have to make sure the code can be compiled after every
> single patch in the series (so that we can easily use git bisect). But
> please, don't achieve the goal by squashing patches until the result can
> be compiled.

Hi Jiri, At first look I am getting what your saying on everything but
am concerned I am not understanding something about the rules on
splitting out patches. 

Thanks for pointing out that I could split out virQEMUCapsMigratablePropsDiff.
I missed that one.

I am fully on board with making the patches as small and easy to review
as possible.

But I think I got pretty strong feedback in the last review that
there should be no floating (unused) functions in a patch and each patch should
compile and pass test cases and function without crashing.

I tried pretty hard to create / retain as many distinct patches as I
could but those rules, as I understand them, didn't seem to leave me
many options.

Should I be creating unit tests or something for what would otherwise
be unused functions to get patches with working compiles?  Am I
misunderstanding the rules?  Some other trick I am missing that would
enable me to get to smaller patches?

Thanks for any feedback you can give on this.

Chris

> 
> Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/6] BaselineHypervisorCPU using QEMU QMP exchanges
Posted by Jiri Denemark 5 years, 5 months ago
On Wed, Oct 03, 2018 at 16:16:50 -0500, Chris Venteicher wrote:
> Quoting Jiri Denemark (2018-10-03 09:36:34)
> > Hmm, I guess I could have checked the cover letter before looking at the
> > individual patches. That would safe me some time and thinking :-)
> > 
> > On Thu, Sep 27, 2018 at 16:26:39 -0500, Chris Venteicher wrote:
> > > Some architectures (S390) depend on QEMU to compute baseline CPU model and
> > > expand a models feature set.
> > > 
> > > Interacting with QEMU requires starting the QEMU process and completing one or
> > > more query-cpu-model-baseline QMP exchanges with QEMU in addition to a
> > > query-cpu-model-expansion QMP exchange to expand all features in the model.
> > > 
> > > See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion
> > > of QEMU aspects.
> > > 
> > > This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999
> > > 
> > > Version 3 attempts to address all issues from V1 and V2 including making the 
> > > QEMU process activation for QMP Queries generic (not specific to capabilities)
> > > and moving that code from qemu_capabilities to qemu_process. 
> > 
> > So far so good.
> > 
> > > I attempted to make the new qemu_process functions consistent with the functions
> > > but mostly ended up reusing the guts of the previous functions from qemu_capabilities.
> > 
> > I guess you wanted to say "... consistent with the existing functions in
> > qemu_process.c" or something similar, right? It's fine to just move and
> > generalize the functions from qemu_capabilities (and perhaps make the
> > original functions just wrappers around the new ones if needed).
> > 
> > > Of interest may be the choice to reuse a domain structure to hold the Qmp Query
> > > process state and connection information.
> > 
> > This sounds like a bad idea to me.
> > 
> > > Also, pay attention to the use of a large random number to uniquely
> > > identify decoupled processes in terms of sockets and file system
> > > footprint.  If you believe it's worth the effort I think there are
> > > some pre-existing library functions to establish files with unique
> > > identifiers in a thread safe way.
> > 
> > We already have src/util/virrandom.c for random numbers, but it doesn't
> > really matter anyway since we don't need or either want to use them
> > here. Just use mkdtemp() and store the socket, pid, whatever you need in
> > there.
> > 
> > > The last patch may also be interesting and is based on past discussion of QEMU cpu
> > > expansion only returning migratable features except for one x86 case where
> > > non-migratable features are explicitly requested.  The patch records that features
> > > are migratable based on QEMU only returning migratable features.  The main result
> > > is the CPU xml for S390 CPU's contains the same migratability info the X86 currently
> > > does.  The testcases were updated to reflect the change.  Is this ok?  
> > 
> > Yeah, looks OK, although I didn't look too closely at it.
> > 
> > > Unlike the previous versions every patch should compile independently if applied
> > > in sequence.
> > 
> > Good, each series have to make sure the code can be compiled after every
> > single patch in the series (so that we can easily use git bisect). But
> > please, don't achieve the goal by squashing patches until the result can
> > be compiled.
> 
> Hi Jiri, At first look I am getting what your saying on everything but
> am concerned I am not understanding something about the rules on
> splitting out patches. 
> 
> Thanks for pointing out that I could split out virQEMUCapsMigratablePropsDiff.
> I missed that one.
> 
> I am fully on board with making the patches as small and easy to review
> as possible.
> 
> But I think I got pretty strong feedback in the last review that
> there should be no floating (unused) functions in a patch and each patch should
> compile and pass test cases and function without crashing.

I don't think I would strongly oppose to unused functions. It always
depends on what a patch is doing. Of course, you can't do unused static
functions, because the could would fail to compile. Sometimes unused
functions are OK, but it's better to avoid them.

You can move and rename existing functions in one patch, followed by
other patches that would incrementally change the functions and their
callers. Or you can keep the old functions as wrappers around the new
ones (which will become more general) so that you don't have to fix all
the callers while changing the new functions. It's hard to give more
specific tips since the solution often depends on the code.

What you can always do is to refactor functions and update callers
(ideally in several patches if possible, especially when moving code
between files) and follow up with patches which add a new code using the
better functions.

I know some of the changes are hard to split once done, but it's still
doable and the result is much better and easier to review, comment, and
spot possible issues.

Jirka

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