Re: [PATCH] qemu: Warn when using vhost-user without shared memory

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Tue, Dec 15, 2015 at 06:44:25AM -0500, John Ferlan wrote:
> On 12/08/2015 10:24 AM, Martin Kletzander wrote:
> > When user configures vhost-user interface and forgets to also configure
> > any shared memory, the search for the root cause of non-operational
> > interface might take unpleasantly long time.  Let's enhance user
> > experience by emitting a warning in the logs.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1266982
> >
> > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > ---
> >  src/qemu/qemu_process.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 420196264685..fb471342e790 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -4542,6 +4542,7 @@ qemuProcessLaunch(virConnectPtr conn,
> >      unsigned int hostdev_flags = 0;
> >      size_t nnicindexes = 0;
> >      int *nicindexes = NULL;
> > +    bool check_shmem = false;
> >
> >      VIR_DEBUG("vm=%p name=%s id=%d asyncJob=%d "
> >                "incoming.launchURI=%s incoming.deferredURI=%s "
> > @@ -4749,6 +4750,49 @@ qemuProcessLaunch(virConnectPtr conn,
> >              goto cleanup;
> >      }
> >
> > +    VIR_DEBUG("Checking for any possible (non-fatal) issues");
> > +
> > +    /*
> > +     * For vhost-user to work, the domain has to have some type of
> > +     * shared memory configured.  We're not the proper once to judge
> > +     * whether shared hugepages or shm are enough and will be in the
> > +     * future, so we'll just warn in case there is none of that
> > +     * configured.  Moreover failing would give the false illusion
> > +     * that libvirt is really checking that everything works before
> > +     * running the domain and not only we are unable to do that, but
> > +     * it's also not our aim to do so.
> > +     */
> > +    for (i = 0; i < vm->def->nnets; i++) {
> > +        if (virDomainNetGetActualType(vm->def->nets[i]) ==
> > +                                      VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> > +            check_shmem = true;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (check_shmem) {
> > +        bool shmem = vm->def->nshmems;
> > +
> > +        /*
> > +         * This check is by no means complete.  We merely check
> > +         * whetere there are *some* hugepages enabled and *some* NUMA
> > +         * nodes with shared memory access.
> > +         */
> > +        if (!shmem && vm->def->mem.nhugepages) {
> > +            for (i = 0; i < virDomainNumaGetNodeCount(vm->def->numa); i++) {
> > +                if (virDomainNumaGetNodeMemoryAccessMode(vm->def->numa, i) ==
> > +                    VIR_NUMA_MEM_ACCESS_SHARED)
> > +                    shmem = true;
> > +                break;
>
> Coverity complains here that i++ is not reachable.  I think you meant to
> put the break; inside the if, right?
>
> John

Yes, exactly, thanks for noticing, this should be the diff:

diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index ba8dfebd1357..f2740687f655 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -4781,9 +4781,10 @@ qemuProcessLaunch(virConnectPtr conn,
        if (!shmem && vm->def->mem.nhugepages) {
            for (i = 0; i < virDomainNumaGetNodeCount(vm->def->numa); i++) {
                if (virDomainNumaGetNodeMemoryAccessMode(vm->def->numa, i) ==
-                    VIR_NUMA_MEM_ACCESS_SHARED)
+                    VIR_NUMA_MEM_ACCESS_SHARED) {
                    shmem = true;
-                break;
+                    break;
+                }
            }
        }

--

I will push it later on if you agree.

Martin

Attachment: signature.asc
Description: PGP signature

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