[libvirt] [PATCH 1/2] bhyve: add CPU topology support

[Roman Bogorodskiy]

  Peter Krempa wrote:

> On Mon, May 28, 2018 at 20:27:50 +0400, Roman Bogorodskiy wrote:
> > Recently, bhyve started supporting specifying guest CPU topology.
> > It looks this way:
> > 
> >   bhyve -c cpus=C,sockets=S,cores=C,threads=T ...
> > 
> > The old behaviour with bhyve -c C, where C is a number of vCPUs, is
> > still supported.
> > 
> > So if we have CPU topology in the domain XML, use the new syntax,
> > otherwise keeps the old behaviour.
> > 
> > Signed-off-by: Roman Bogorodskiy <bogorodskiy at gmail.com>
> > ---
> >  src/bhyve/bhyve_capabilities.c                |  7 +++--
> >  src/bhyve/bhyve_capabilities.h                |  1 +
> >  src/bhyve/bhyve_command.c                     | 17 +++++++++++-
> >  .../bhyvexml2argv-cputopology.args            |  9 +++++++
> >  .../bhyvexml2argv-cputopology.ldargs          |  3 +++
> >  .../bhyvexml2argv-cputopology.xml             | 26 +++++++++++++++++++
> >  tests/bhyvexml2argvtest.c                     |  7 ++++-
> >  7 files changed, 66 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml
> 
> [...]
> 
> > diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> > index e3f7ded7db..b319518520 100644
> > --- a/src/bhyve/bhyve_command.c
> > +++ b/src/bhyve/bhyve_command.c
> > @@ -467,7 +467,22 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> >  
> >      /* CPUs */
> >      virCommandAddArg(cmd, "-c");
> > -    virCommandAddArgFormat(cmd, "%d", virDomainDefGetVcpus(def));
> > +    if (def->cpu && def->cpu->sockets) {
> > +        if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_CPUTOPOLOGY) != 0) {
> > +            virCommandAddArgFormat(cmd, "cpus=%d,sockets=%d,cores=%d,threads=%d",
> > +                                   virDomainDefGetVcpus(def),
> > +                                   def->cpu->sockets,
> > +                                   def->cpu->cores,
> > +                                   def->cpu->threads);
> 
> Note that libvirt XML->def conversion does not validate that def->nvcpus
> equals to def->cpu->sockets * def->cpu->cores * def->cpu->threads.
> 
> This is a historic artefact since qemu did not do it either. They
> started doing it just recently. It might be worth adding that check to
> be sure in the future.

Good point!
Indeed, bhyve validates CPU topology in a way you described, so it makes
sense to fail early instead of waiting the command to fail. I'll roll a
v2.

> > +        } else {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("Installed bhyve binary does not support "
> > +                             "defining CPU topology"));
> > +            goto error;
> > +        }
> 
> The rest looks good to me, so ACK if you don't think the check for the
> topology<->vcpu count is important enough.



Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180529/a33a6bb1/attachment-0001.sig>