[libvirt] [PATCH] qemu: Warn when using vhost-user without shared memory
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@redhat.com> --- 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; + } + } + + if (!shmem) { + VIR_WARN("Detected vhost-user interface without any shared memory. " + "The interface might not be operational"); + } + } + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON, priv->qemuCaps, -- 2.6.3
On 12/08/2015 08: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@redhat.com> --- src/qemu/qemu_process.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
+ /* + * This check is by no means complete. We merely check + * whetere there are *some* hugepages enabled and *some* NUMA
s/whetere/whether/ ACK with the spelling fix -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 08, 2015 at 09:22:35AM -0700, Eric Blake wrote:
On 12/08/2015 08: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@redhat.com> --- src/qemu/qemu_process.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
+ /* + * This check is by no means complete. We merely check + * whetere there are *some* hugepages enabled and *some* NUMA
s/whetere/whether/
ACK with the spelling fix
Thanks for the review, I fixed the spelling and I'll push it after the release in case there are any controversies caused by that.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
Another typo perhaps? s/once/ones
+ * 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
s/there is none of that configured/neither is configured ?? Current sentance just read strangely.
+ * 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; + } + } + + if (!shmem) { + VIR_WARN("Detected vhost-user interface without any shared memory. " + "The interface might not be operational");
You did add a period to the end of the first sentance, but not the second...
+ } + } + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON, priv->qemuCaps,
On Tue, Dec 08, 2015 at 11:35:50AM -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@redhat.com> --- 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
Another typo perhaps?
s/once/ones
Oh, what's happening with me again. Thanks for that, I'll fix all of them.
+ * 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
s/there is none of that configured/neither is configured
??
I wans't sure that's grammatically correct, but I wanted to spell it out just like that.
Current sentance just read strangely.
I immediatelly feel better after this "sentance" ;)
+ * 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; + } + } + + if (!shmem) { + VIR_WARN("Detected vhost-user interface without any shared memory. " + "The interface might not be operational");
You did add a period to the end of the first sentance, but not the second...
And even better now =)
+ } + } + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON, priv->qemuCaps,
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com> --- 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
+ } + } + + if (!shmem) { + VIR_WARN("Detected vhost-user interface without any shared memory. " + "The interface might not be operational"); + } + } + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON, priv->qemuCaps,
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@redhat.com> --- 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
[...]
+ 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
Looks OK to me ACK John
participants (3)
-
Eric Blake -
John Ferlan -
Martin Kletzander