* [Qemu-devel] [PATCH 1/3] tests: define qtest-obj-y before using it @ 2015-11-16 17:23 marcandre.lureau 2015-11-16 17:23 ` [Qemu-devel] [PATCH 2/3] tests: re-enable vhost-user-test marcandre.lureau 2015-11-16 17:23 ` [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest marcandre.lureau 0 siblings, 2 replies; 27+ messages in thread From: marcandre.lureau @ 2015-11-16 17:23 UTC (permalink / raw) To: qemu-devel; +Cc: Marc-André Lureau, mst From: Marc-André Lureau <marcandre.lureau@redhat.com> The qtest-obj-y variable is being used before it's defined, move the variable definition before its use. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- tests/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile b/tests/Makefile index 90c4141..c1e3353 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -479,6 +479,7 @@ libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o libqos-imx-obj-y = $(libqos-obj-y) tests/libqos/i2c-imx.o libqos-usb-obj-y = $(libqos-pc-obj-y) tests/libqos/usb.o libqos-virtio-obj-y = $(libqos-pc-obj-y) tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o tests/libqos/malloc-generic.o +qtest-obj-y = tests/libqtest.o $(test-util-obj-y) tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o tests/rtc-test$(EXESUF): tests/rtc-test.o @@ -553,7 +554,6 @@ else QTEST_TARGETS = endif -qtest-obj-y = tests/libqtest.o $(test-util-obj-y) $(check-qtest-y): $(qtest-obj-y) tests/test-qga: tests/test-qga.o $(qtest-obj-y) -- 2.5.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 2/3] tests: re-enable vhost-user-test 2015-11-16 17:23 [Qemu-devel] [PATCH 1/3] tests: define qtest-obj-y before using it marcandre.lureau @ 2015-11-16 17:23 ` marcandre.lureau 2015-11-16 17:23 ` [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest marcandre.lureau 1 sibling, 0 replies; 27+ messages in thread From: marcandre.lureau @ 2015-11-16 17:23 UTC (permalink / raw) To: qemu-devel; +Cc: Marc-André Lureau, mst From: Marc-André Lureau <marcandre.lureau@redhat.com> Commit 7fe34ca9c2e actually disabled vhost-user-test altogether, since CONFIG_VHOST_NET is a per-target config variable. tests/vhost-user-test is already x86/x64 softmmu specific test, in order to enable it correctly, kvm & vhost-net are also conditions. To check that, set CONFIG_VHOST_NET_TEST_$target when kvm is also enabled. Since "check-qtest-x86_64-y = $(check-qtest-i386-y)", avoid duplication when both x86 & x64 are enabled. Other targets than x86 aren't enabled yet, and is intentionally left as a future improvement, since I can't easily test those. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- configure | 1 + tests/Makefile | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/configure b/configure index f75df4b..0866bd8 100755 --- a/configure +++ b/configure @@ -5663,6 +5663,7 @@ case "$target_name" in echo "CONFIG_KVM=y" >> $config_target_mak if test "$vhost_net" = "yes" ; then echo "CONFIG_VHOST_NET=y" >> $config_target_mak + echo "CONFIG_VHOST_NET_TEST_$target_name=y" >> $config_host_mak fi fi esac diff --git a/tests/Makefile b/tests/Makefile index c1e3353..002881a 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -197,11 +197,12 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c check-qtest-i386-y += tests/pc-cpu-test$(EXESUF) check-qtest-i386-y += tests/q35-test$(EXESUF) gcov-files-i386-y += hw/pci-host/q35.c -ifeq ($(CONFIG_VHOST_NET),y) -check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF) -endif +check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF) check-qtest-i386-y += tests/test-netfilter$(EXESUF) check-qtest-x86_64-y = $(check-qtest-i386-y) +ifeq ($(CONFIG_VHOST_NET_TEST_i386),) +check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF) +endif gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y)) check-qtest-mips-y = tests/endianness-test$(EXESUF) -- 2.5.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest 2015-11-16 17:23 [Qemu-devel] [PATCH 1/3] tests: define qtest-obj-y before using it marcandre.lureau 2015-11-16 17:23 ` [Qemu-devel] [PATCH 2/3] tests: re-enable vhost-user-test marcandre.lureau @ 2015-11-16 17:23 ` marcandre.lureau 2015-11-16 21:50 ` Michael S. Tsirkin ` (2 more replies) 1 sibling, 3 replies; 27+ messages in thread From: marcandre.lureau @ 2015-11-16 17:23 UTC (permalink / raw) To: qemu-devel; +Cc: Marc-André Lureau, mst From: Marc-André Lureau <marcandre.lureau@redhat.com> vhost-user-test prints a warning. A test should not need to run on hugetlbfs, let's silence the warning under qtest. Unfortunately, the condition can't check on qtest_enabled() or qtest_driver() since they are initialized later. Moving configure_accelerator() earlier is problematic, as the memory regions aren't yet fully set up and vhost-user-test fails. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- exec.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/exec.c b/exec.c index b09f18b..3edc582 100644 --- a/exec.c +++ b/exec.c @@ -1184,6 +1184,7 @@ void qemu_mutex_unlock_ramlist(void) static long gethugepagesize(const char *path, Error **errp) { struct statfs fs; + const char *p; int ret; do { @@ -1196,8 +1197,11 @@ static long gethugepagesize(const char *path, Error **errp) return 0; } - if (fs.f_type != HUGETLBFS_MAGIC) + p = qemu_opt_get(qemu_get_machine_opts(), "accel"); + if (g_strcmp0(p, "qtest") && + fs.f_type != HUGETLBFS_MAGIC) { fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); + } return fs.f_bsize; } -- 2.5.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest 2015-11-16 17:23 ` [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest marcandre.lureau @ 2015-11-16 21:50 ` Michael S. Tsirkin 2015-11-17 21:32 ` Marc-André Lureau 2015-11-18 9:02 ` [Qemu-devel] [PATCH] " marcandre.lureau 2 siblings, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-16 21:50 UTC (permalink / raw) To: marcandre.lureau; +Cc: Peter Maydell, qemu-devel, pbonzini On Mon, Nov 16, 2015 at 06:23:38PM +0100, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost-user-test prints a warning. A test should not need to run on > hugetlbfs, let's silence the warning under qtest. Unfortunately, the > condition can't check on qtest_enabled() or qtest_driver() since they > are initialized later. Moving configure_accelerator() earlier is > problematic, as the memory regions aren't yet fully set up and > vhost-user-test fails. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- Peter, Paolo, any comments on this one? > exec.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index b09f18b..3edc582 100644 > --- a/exec.c > +++ b/exec.c > @@ -1184,6 +1184,7 @@ void qemu_mutex_unlock_ramlist(void) > static long gethugepagesize(const char *path, Error **errp) > { > struct statfs fs; > + const char *p; > int ret; > > do { > @@ -1196,8 +1197,11 @@ static long gethugepagesize(const char *path, Error **errp) > return 0; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) > + p = qemu_opt_get(qemu_get_machine_opts(), "accel"); > + if (g_strcmp0(p, "qtest") && > + fs.f_type != HUGETLBFS_MAGIC) { > fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + } > > return fs.f_bsize; > } > -- > 2.5.0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest 2015-11-16 17:23 ` [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest marcandre.lureau 2015-11-16 21:50 ` Michael S. Tsirkin @ 2015-11-17 21:32 ` Marc-André Lureau 2015-11-18 8:32 ` Markus Armbruster 2015-11-18 9:02 ` [Qemu-devel] [PATCH] " marcandre.lureau 2 siblings, 1 reply; 27+ messages in thread From: Marc-André Lureau @ 2015-11-17 21:32 UTC (permalink / raw) To: QEMU; +Cc: Marc-André Lureau, Michael S. Tsirkin nack, this isn't enough to silence the warning, as vhost-user-test uses -machine accel=tcg On Mon, Nov 16, 2015 at 6:23 PM, <marcandre.lureau@redhat.com> wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost-user-test prints a warning. A test should not need to run on > hugetlbfs, let's silence the warning under qtest. Unfortunately, the > condition can't check on qtest_enabled() or qtest_driver() since they > are initialized later. Moving configure_accelerator() earlier is > problematic, as the memory regions aren't yet fully set up and > vhost-user-test fails. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > exec.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index b09f18b..3edc582 100644 > --- a/exec.c > +++ b/exec.c > @@ -1184,6 +1184,7 @@ void qemu_mutex_unlock_ramlist(void) > static long gethugepagesize(const char *path, Error **errp) > { > struct statfs fs; > + const char *p; > int ret; > > do { > @@ -1196,8 +1197,11 @@ static long gethugepagesize(const char *path, Error **errp) > return 0; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) > + p = qemu_opt_get(qemu_get_machine_opts(), "accel"); > + if (g_strcmp0(p, "qtest") && > + fs.f_type != HUGETLBFS_MAGIC) { > fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + } > > return fs.f_bsize; > } > -- > 2.5.0 > > -- Marc-André Lureau ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest 2015-11-17 21:32 ` Marc-André Lureau @ 2015-11-18 8:32 ` Markus Armbruster 0 siblings, 0 replies; 27+ messages in thread From: Markus Armbruster @ 2015-11-18 8:32 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Marc-André Lureau, QEMU, Michael S. Tsirkin Marc-André Lureau <marcandre.lureau@gmail.com> writes: > nack, this isn't enough to silence the warning, as vhost-user-test > uses -machine accel=tcg Pity. > On Mon, Nov 16, 2015 at 6:23 PM, <marcandre.lureau@redhat.com> wrote: >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> vhost-user-test prints a warning. A test should not need to run on >> hugetlbfs, let's silence the warning under qtest. Unfortunately, the >> condition can't check on qtest_enabled() or qtest_driver() since they >> are initialized later. Moving configure_accelerator() earlier is >> problematic, as the memory regions aren't yet fully set up and >> vhost-user-test fails. That leaves qtest_init(), which makes qtest_driver() valid. Could that be done earlier? ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-16 17:23 ` [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest marcandre.lureau 2015-11-16 21:50 ` Michael S. Tsirkin 2015-11-17 21:32 ` Marc-André Lureau @ 2015-11-18 9:02 ` marcandre.lureau 2015-11-18 20:49 ` Michael S. Tsirkin 2 siblings, 1 reply; 27+ messages in thread From: marcandre.lureau @ 2015-11-18 9:02 UTC (permalink / raw) To: qemu-devel; +Cc: Marc-André Lureau, mst From: Marc-André Lureau <marcandre.lureau@redhat.com> vhost-user-test prints a warning. A test should not need to run on hugetlbfs, let's silence the warning under qtest. The condition can't check on qtest_enabled() since vhost-user-test actually doesn't use qtest accel. However, qtest_driver() can be used, if qtest_init() is called early enough. For that reason, move chardev and qtest initialization early. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- exec.c | 5 ++++- vl.c | 28 ++++++++++++++-------------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/exec.c b/exec.c index b09f18b..acbd4a2 100644 --- a/exec.c +++ b/exec.c @@ -51,6 +51,7 @@ #include "qemu/main-loop.h" #include "translate-all.h" #include "sysemu/replay.h" +#include "sysemu/qtest.h" #include "exec/memory-internal.h" #include "exec/ram_addr.h" @@ -1196,8 +1197,10 @@ static long gethugepagesize(const char *path, Error **errp) return 0; } - if (fs.f_type != HUGETLBFS_MAGIC) + if (!qtest_driver() && + fs.f_type != HUGETLBFS_MAGIC) { fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); + } return fs.f_bsize; } diff --git a/vl.c b/vl.c index 7d993a5..f9c661a 100644 --- a/vl.c +++ b/vl.c @@ -4288,14 +4288,23 @@ int main(int argc, char **argv, char **envp) page_size_init(); socket_init(); - if (qemu_opts_foreach(qemu_find_opts("object"), - object_create, - object_create_initial, NULL)) { + if (qemu_opts_foreach(qemu_find_opts("chardev"), + chardev_init_func, NULL, NULL)) { exit(1); } - if (qemu_opts_foreach(qemu_find_opts("chardev"), - chardev_init_func, NULL, NULL)) { + if (qtest_chrdev) { + Error *local_err = NULL; + qtest_init(qtest_chrdev, qtest_log, &local_err); + if (local_err) { + error_report_err(local_err); + exit(1); + } + } + + if (qemu_opts_foreach(qemu_find_opts("object"), + object_create, + object_create_initial, NULL)) { exit(1); } @@ -4325,15 +4334,6 @@ int main(int argc, char **argv, char **envp) configure_accelerator(current_machine); - if (qtest_chrdev) { - Error *local_err = NULL; - qtest_init(qtest_chrdev, qtest_log, &local_err); - if (local_err) { - error_report_err(local_err); - exit(1); - } - } - machine_opts = qemu_get_machine_opts(); kernel_filename = qemu_opt_get(machine_opts, "kernel"); initrd_filename = qemu_opt_get(machine_opts, "initrd"); -- 2.5.0 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-18 9:02 ` [Qemu-devel] [PATCH] " marcandre.lureau @ 2015-11-18 20:49 ` Michael S. Tsirkin 2015-11-23 17:40 ` Paolo Bonzini 0 siblings, 1 reply; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-18 20:49 UTC (permalink / raw) To: marcandre.lureau; +Cc: Amit Shah, Peter Maydell, qemu-devel, pbonzini On Wed, Nov 18, 2015 at 10:02:58AM +0100, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost-user-test prints a warning. A test should not need to run on > hugetlbfs, let's silence the warning under qtest. The > condition can't check on qtest_enabled() since vhost-user-test actually > doesn't use qtest accel. However, qtest_driver() can be used, if > qtest_init() is called early enough. For that reason, move chardev and > qtest initialization early. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Seems ok, and let us bring back the 2.4 test that was dropped in 2.5, but I'd like an ack on this from someone else. Anyone? > --- > exec.c | 5 ++++- > vl.c | 28 ++++++++++++++-------------- > 2 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/exec.c b/exec.c > index b09f18b..acbd4a2 100644 > --- a/exec.c > +++ b/exec.c > @@ -51,6 +51,7 @@ > #include "qemu/main-loop.h" > #include "translate-all.h" > #include "sysemu/replay.h" > +#include "sysemu/qtest.h" > > #include "exec/memory-internal.h" > #include "exec/ram_addr.h" > @@ -1196,8 +1197,10 @@ static long gethugepagesize(const char *path, Error **errp) > return 0; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) > + if (!qtest_driver() && > + fs.f_type != HUGETLBFS_MAGIC) { > fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + } > > return fs.f_bsize; > } > diff --git a/vl.c b/vl.c > index 7d993a5..f9c661a 100644 > --- a/vl.c > +++ b/vl.c > @@ -4288,14 +4288,23 @@ int main(int argc, char **argv, char **envp) > page_size_init(); > socket_init(); > > - if (qemu_opts_foreach(qemu_find_opts("object"), > - object_create, > - object_create_initial, NULL)) { > + if (qemu_opts_foreach(qemu_find_opts("chardev"), > + chardev_init_func, NULL, NULL)) { > exit(1); > } > > - if (qemu_opts_foreach(qemu_find_opts("chardev"), > - chardev_init_func, NULL, NULL)) { > + if (qtest_chrdev) { > + Error *local_err = NULL; > + qtest_init(qtest_chrdev, qtest_log, &local_err); > + if (local_err) { > + error_report_err(local_err); > + exit(1); > + } > + } > + > + if (qemu_opts_foreach(qemu_find_opts("object"), > + object_create, > + object_create_initial, NULL)) { > exit(1); > } > > @@ -4325,15 +4334,6 @@ int main(int argc, char **argv, char **envp) > > configure_accelerator(current_machine); > > - if (qtest_chrdev) { > - Error *local_err = NULL; > - qtest_init(qtest_chrdev, qtest_log, &local_err); > - if (local_err) { > - error_report_err(local_err); > - exit(1); > - } > - } > - > machine_opts = qemu_get_machine_opts(); > kernel_filename = qemu_opt_get(machine_opts, "kernel"); > initrd_filename = qemu_opt_get(machine_opts, "initrd"); > -- > 2.5.0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-18 20:49 ` Michael S. Tsirkin @ 2015-11-23 17:40 ` Paolo Bonzini 2015-11-23 17:46 ` Daniel P. Berrange 2015-11-23 18:01 ` Marc-André Lureau 0 siblings, 2 replies; 27+ messages in thread From: Paolo Bonzini @ 2015-11-23 17:40 UTC (permalink / raw) To: Michael S. Tsirkin, marcandre.lureau; +Cc: Amit Shah, Peter Maydell, qemu-devel On 18/11/2015 21:49, Michael S. Tsirkin wrote: > On Wed, Nov 18, 2015 at 10:02:58AM +0100, marcandre.lureau@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> vhost-user-test prints a warning. A test should not need to run on >> hugetlbfs, let's silence the warning under qtest. The >> condition can't check on qtest_enabled() since vhost-user-test actually >> doesn't use qtest accel. However, qtest_driver() can be used, if >> qtest_init() is called early enough. For that reason, move chardev and >> qtest initialization early. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Seems ok, and let us bring back the 2.4 test that was > dropped in 2.5, but I'd like an ack on this from someone else. I wonder if we need the warning at all, but this patch is okay. Anyhow: - if (qemu_opts_foreach(qemu_find_opts("object"), - object_create, - object_create_initial, NULL)) { + if (qemu_opts_foreach(qemu_find_opts("chardev"), + chardev_init_func, NULL, NULL)) { exit(1); } - if (qemu_opts_foreach(qemu_find_opts("chardev"), - chardev_init_func, NULL, NULL)) { + if (qtest_chrdev) { + Error *local_err = NULL; + qtest_init(qtest_chrdev, qtest_log, &local_err); + if (local_err) { + error_report_err(local_err); + exit(1); + } + } + + if (qemu_opts_foreach(qemu_find_opts("object"), + object_create, + object_create_initial, NULL)) { exit(1); } Before: object-initial, chardev, qtest, object-late (not in the patch) After: chardev, qtest, object-initial, object-late (not in the patch) Objects must be initialized before chardev (except rng-egd) since in the future chardev will need to use objects, in particular secret objects. Was the swap intentional? Paolo @@ -4325,15 +4334,6 @@ int main(int argc, char **argv, char **envp) configure_accelerator(current_machine); - if (qtest_chrdev) { - Error *local_err = NULL; - qtest_init(qtest_chrdev, qtest_log, &local_err); - if (local_err) { - error_report_err(local_err); - exit(1); - } - } > Anyone? > >> --- >> exec.c | 5 ++++- >> vl.c | 28 ++++++++++++++-------------- >> 2 files changed, 18 insertions(+), 15 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index b09f18b..acbd4a2 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -51,6 +51,7 @@ >> #include "qemu/main-loop.h" >> #include "translate-all.h" >> #include "sysemu/replay.h" >> +#include "sysemu/qtest.h" >> >> #include "exec/memory-internal.h" >> #include "exec/ram_addr.h" >> @@ -1196,8 +1197,10 @@ static long gethugepagesize(const char *path, Error **errp) >> return 0; >> } >> >> - if (fs.f_type != HUGETLBFS_MAGIC) >> + if (!qtest_driver() && >> + fs.f_type != HUGETLBFS_MAGIC) { >> fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); >> + } >> >> return fs.f_bsize; >> } >> diff --git a/vl.c b/vl.c >> index 7d993a5..f9c661a 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4288,14 +4288,23 @@ int main(int argc, char **argv, char **envp) >> page_size_init(); >> socket_init(); >> >> - if (qemu_opts_foreach(qemu_find_opts("object"), >> - object_create, >> - object_create_initial, NULL)) { >> + if (qemu_opts_foreach(qemu_find_opts("chardev"), >> + chardev_init_func, NULL, NULL)) { >> exit(1); >> } >> >> - if (qemu_opts_foreach(qemu_find_opts("chardev"), >> - chardev_init_func, NULL, NULL)) { >> + if (qtest_chrdev) { >> + Error *local_err = NULL; >> + qtest_init(qtest_chrdev, qtest_log, &local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + exit(1); >> + } >> + } >> + >> + if (qemu_opts_foreach(qemu_find_opts("object"), >> + object_create, >> + object_create_initial, NULL)) { >> exit(1); >> } >> >> @@ -4325,15 +4334,6 @@ int main(int argc, char **argv, char **envp) >> >> configure_accelerator(current_machine); >> >> - if (qtest_chrdev) { >> - Error *local_err = NULL; >> - qtest_init(qtest_chrdev, qtest_log, &local_err); >> - if (local_err) { >> - error_report_err(local_err); >> - exit(1); >> - } >> - } >> - >> machine_opts = qemu_get_machine_opts(); >> kernel_filename = qemu_opt_get(machine_opts, "kernel"); >> initrd_filename = qemu_opt_get(machine_opts, "initrd"); >> -- >> 2.5.0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-23 17:40 ` Paolo Bonzini @ 2015-11-23 17:46 ` Daniel P. Berrange 2015-11-23 17:49 ` Daniel P. Berrange 2015-11-23 18:01 ` Marc-André Lureau 1 sibling, 1 reply; 27+ messages in thread From: Daniel P. Berrange @ 2015-11-23 17:46 UTC (permalink / raw) To: Paolo Bonzini Cc: Amit Shah, marcandre.lureau, qemu-devel, Peter Maydell, Michael S. Tsirkin On Mon, Nov 23, 2015 at 06:40:58PM +0100, Paolo Bonzini wrote: > > > On 18/11/2015 21:49, Michael S. Tsirkin wrote: > > On Wed, Nov 18, 2015 at 10:02:58AM +0100, marcandre.lureau@redhat.com wrote: > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > >> vhost-user-test prints a warning. A test should not need to run on > >> hugetlbfs, let's silence the warning under qtest. The > >> condition can't check on qtest_enabled() since vhost-user-test actually > >> doesn't use qtest accel. However, qtest_driver() can be used, if > >> qtest_init() is called early enough. For that reason, move chardev and > >> qtest initialization early. > >> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Seems ok, and let us bring back the 2.4 test that was > > dropped in 2.5, but I'd like an ack on this from someone else. > > I wonder if we need the warning at all, but this patch is okay. > > Anyhow: > > > > - if (qemu_opts_foreach(qemu_find_opts("object"), > - object_create, > - object_create_initial, NULL)) { > + if (qemu_opts_foreach(qemu_find_opts("chardev"), > + chardev_init_func, NULL, NULL)) { > exit(1); > } > > - if (qemu_opts_foreach(qemu_find_opts("chardev"), > - chardev_init_func, NULL, NULL)) { > + if (qtest_chrdev) { > + Error *local_err = NULL; > + qtest_init(qtest_chrdev, qtest_log, &local_err); > + if (local_err) { > + error_report_err(local_err); > + exit(1); > + } > + } > + > + if (qemu_opts_foreach(qemu_find_opts("object"), > + object_create, > + object_create_initial, NULL)) { > exit(1); > } > > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > After: chardev, qtest, object-initial, object-late (not in the patch) > > Objects must be initialized before chardev (except rng-egd) since in the > future chardev will need to use objects, in particular secret objects. > Was the swap intentional? Yeah, that is essentially reverting my patch to split up object creation: commit f08f9271bfe3f19a5eb3d7a2f48532065304d5c8 Author: Daniel P. Berrange <berrange@redhat.com> Date: Wed May 13 17:14:04 2015 +0100 vl: Create (most) objects before creating chardev backends So I'd rather we did not do that. IMHO having the goal that all objects be created first is the right long term direction, if we view the future as being 100% object based. Hence most of the object types are created on object-initial, and only a few things in object-late. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-23 17:46 ` Daniel P. Berrange @ 2015-11-23 17:49 ` Daniel P. Berrange 0 siblings, 0 replies; 27+ messages in thread From: Daniel P. Berrange @ 2015-11-23 17:49 UTC (permalink / raw) To: Paolo Bonzini Cc: Amit Shah, marcandre.lureau, Michael S. Tsirkin, qemu-devel, Peter Maydell On Mon, Nov 23, 2015 at 05:46:06PM +0000, Daniel P. Berrange wrote: > On Mon, Nov 23, 2015 at 06:40:58PM +0100, Paolo Bonzini wrote: > > > > > > On 18/11/2015 21:49, Michael S. Tsirkin wrote: > > > On Wed, Nov 18, 2015 at 10:02:58AM +0100, marcandre.lureau@redhat.com wrote: > > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> > > >> > > >> vhost-user-test prints a warning. A test should not need to run on > > >> hugetlbfs, let's silence the warning under qtest. The > > >> condition can't check on qtest_enabled() since vhost-user-test actually > > >> doesn't use qtest accel. However, qtest_driver() can be used, if > > >> qtest_init() is called early enough. For that reason, move chardev and > > >> qtest initialization early. > > >> > > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > Seems ok, and let us bring back the 2.4 test that was > > > dropped in 2.5, but I'd like an ack on this from someone else. > > > > I wonder if we need the warning at all, but this patch is okay. > > > > Anyhow: > > > > > > > > - if (qemu_opts_foreach(qemu_find_opts("object"), > > - object_create, > > - object_create_initial, NULL)) { > > + if (qemu_opts_foreach(qemu_find_opts("chardev"), > > + chardev_init_func, NULL, NULL)) { > > exit(1); > > } > > > > - if (qemu_opts_foreach(qemu_find_opts("chardev"), > > - chardev_init_func, NULL, NULL)) { > > + if (qtest_chrdev) { > > + Error *local_err = NULL; > > + qtest_init(qtest_chrdev, qtest_log, &local_err); > > + if (local_err) { > > + error_report_err(local_err); > > + exit(1); > > + } > > + } > > + > > + if (qemu_opts_foreach(qemu_find_opts("object"), > > + object_create, > > + object_create_initial, NULL)) { > > exit(1); > > } > > > > > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > > > After: chardev, qtest, object-initial, object-late (not in the patch) > > > > Objects must be initialized before chardev (except rng-egd) since in the > > future chardev will need to use objects, in particular secret objects. BTW, it was TLS credentials object that prompted my initial re-ordering, but you're right that secret objects will need creation before chardevs too, since TLS credentials will need secrets. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-23 17:40 ` Paolo Bonzini 2015-11-23 17:46 ` Daniel P. Berrange @ 2015-11-23 18:01 ` Marc-André Lureau 2015-11-23 18:10 ` Daniel P. Berrange 2015-11-24 14:12 ` Michael S. Tsirkin 1 sibling, 2 replies; 27+ messages in thread From: Marc-André Lureau @ 2015-11-23 18:01 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Amit Shah, Peter Maydell, QEMU, Michael S. Tsirkin On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Before: object-initial, chardev, qtest, object-late (not in the patch) > > After: chardev, qtest, object-initial, object-late (not in the patch) > > Objects must be initialized before chardev (except rng-egd) since in the > future chardev will need to use objects, in particular secret objects. > Was the swap intentional? Yes, without the swap, qtest was not initialized before memory is allocated. The alternative I could think of is to check the QTEST_QEMU_BINARY variable: http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html -- Marc-André Lureau ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-23 18:01 ` Marc-André Lureau @ 2015-11-23 18:10 ` Daniel P. Berrange 2015-11-24 14:10 ` Michael S. Tsirkin 2015-11-24 14:12 ` Michael S. Tsirkin 1 sibling, 1 reply; 27+ messages in thread From: Daniel P. Berrange @ 2015-11-23 18:10 UTC (permalink / raw) To: Marc-André Lureau Cc: Amit Shah, Paolo Bonzini, Michael S. Tsirkin, QEMU, Peter Maydell On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote: > On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > > > After: chardev, qtest, object-initial, object-late (not in the patch) > > > > Objects must be initialized before chardev (except rng-egd) since in the > > future chardev will need to use objects, in particular secret objects. > > Was the swap intentional? > > Yes, without the swap, qtest was not initialized before memory is allocated. > > The alternative I could think of is to check the QTEST_QEMU_BINARY > variable: http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html Why do we not simply delete the warning message about the path not being on hugetlbfs ? ie, why does QEMU try to force a policy that a memory-file backend has to be on hugetlbfs, as opposed to on a plain tmpfs ? I've previously had user request that we allow use of plain tmpfs, because they want to use vhost-user without also using hugepages, and that could be done with plain tmpfs. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-23 18:10 ` Daniel P. Berrange @ 2015-11-24 14:10 ` Michael S. Tsirkin 2015-11-24 14:19 ` Daniel P. Berrange 0 siblings, 1 reply; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-24 14:10 UTC (permalink / raw) To: Daniel P. Berrange Cc: Amit Shah, Paolo Bonzini, Marc-André Lureau, QEMU, Peter Maydell On Mon, Nov 23, 2015 at 06:10:08PM +0000, Daniel P. Berrange wrote: > On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote: > > On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > > > > > After: chardev, qtest, object-initial, object-late (not in the patch) > > > > > > Objects must be initialized before chardev (except rng-egd) since in the > > > future chardev will need to use objects, in particular secret objects. > > > Was the swap intentional? > > > > Yes, without the swap, qtest was not initialized before memory is allocated. > > > > The alternative I could think of is to check the QTEST_QEMU_BINARY > > variable: http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html > > Why do we not simply delete the warning message about the path not > being on hugetlbfs ? ie, why does QEMU try to force a policy that > a memory-file backend has to be on hugetlbfs, as opposed to on > a plain tmpfs ? I've previously had user request that we allow > use of plain tmpfs, because they want to use vhost-user without > also using hugepages, and that could be done with plain tmpfs. > > Regards, > Daniel Because THP does not work on any other filesystem, so many workloads are much slower. That's why it's a warning, not an error. > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-24 14:10 ` Michael S. Tsirkin @ 2015-11-24 14:19 ` Daniel P. Berrange 2015-11-24 14:25 ` Michael S. Tsirkin 2015-11-24 15:09 ` Markus Armbruster 0 siblings, 2 replies; 27+ messages in thread From: Daniel P. Berrange @ 2015-11-24 14:19 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Amit Shah, Paolo Bonzini, Marc-André Lureau, QEMU, Peter Maydell On Tue, Nov 24, 2015 at 04:10:54PM +0200, Michael S. Tsirkin wrote: > On Mon, Nov 23, 2015 at 06:10:08PM +0000, Daniel P. Berrange wrote: > > On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote: > > > On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > > > > > > > After: chardev, qtest, object-initial, object-late (not in the patch) > > > > > > > > Objects must be initialized before chardev (except rng-egd) since in the > > > > future chardev will need to use objects, in particular secret objects. > > > > Was the swap intentional? > > > > > > Yes, without the swap, qtest was not initialized before memory is allocated. > > > > > > The alternative I could think of is to check the QTEST_QEMU_BINARY > > > variable: http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html > > > > Why do we not simply delete the warning message about the path not > > being on hugetlbfs ? ie, why does QEMU try to force a policy that > > a memory-file backend has to be on hugetlbfs, as opposed to on > > a plain tmpfs ? I've previously had user request that we allow > > use of plain tmpfs, because they want to use vhost-user without > > also using hugepages, and that could be done with plain tmpfs. > > Because THP does not work on any other filesystem, > so many workloads are much slower. > That's why it's a warning, not an error. AFAICT this warning message is not in a codepath that is specific to use of THP. This is just generic code for allocating guest memory backed by a file, which does not have any assumption / prerequisite that THP is wanted or enabled. So adding warnings that are specifically related to THP is inappropriate. The fact that THP only works with a hugetlbfs path is merely a documentation item to record against the command line option for -mem-path. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-24 14:19 ` Daniel P. Berrange @ 2015-11-24 14:25 ` Michael S. Tsirkin 2015-11-24 14:30 ` Daniel P. Berrange 2015-11-24 14:30 ` Paolo Bonzini 2015-11-24 15:09 ` Markus Armbruster 1 sibling, 2 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-24 14:25 UTC (permalink / raw) To: Daniel P. Berrange Cc: Amit Shah, Paolo Bonzini, Marc-André Lureau, QEMU, Peter Maydell On Tue, Nov 24, 2015 at 02:19:36PM +0000, Daniel P. Berrange wrote: > On Tue, Nov 24, 2015 at 04:10:54PM +0200, Michael S. Tsirkin wrote: > > On Mon, Nov 23, 2015 at 06:10:08PM +0000, Daniel P. Berrange wrote: > > > On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote: > > > > On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > > > > > > > > > After: chardev, qtest, object-initial, object-late (not in the patch) > > > > > > > > > > Objects must be initialized before chardev (except rng-egd) since in the > > > > > future chardev will need to use objects, in particular secret objects. > > > > > Was the swap intentional? > > > > > > > > Yes, without the swap, qtest was not initialized before memory is allocated. > > > > > > > > The alternative I could think of is to check the QTEST_QEMU_BINARY > > > > variable: http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html > > > > > > Why do we not simply delete the warning message about the path not > > > being on hugetlbfs ? ie, why does QEMU try to force a policy that > > > a memory-file backend has to be on hugetlbfs, as opposed to on > > > a plain tmpfs ? I've previously had user request that we allow > > > use of plain tmpfs, because they want to use vhost-user without > > > also using hugepages, and that could be done with plain tmpfs. > > > > Because THP does not work on any other filesystem, > > so many workloads are much slower. > > That's why it's a warning, not an error. > > AFAICT this warning message is not in a codepath that is specific to > use of THP. This is just generic code for allocating guest memory > backed by a file, which does not have any assumption / prerequisite > that THP is wanted or enabled. So adding warnings that are specifically > related to THP is inappropriate. > > The fact that THP only works with a hugetlbfs path is merely a > documentation item to record against the command line option for > -mem-path. I'm worried that things go slow and people don't have a way to find out why, and documentation isn't the first place people look for when facing a performance issue. At the moment we call MADV_HUGEPAGE unconditionally and unfortunately there's no way to report it's not having the intended effect. Maybe we want a "don't enable hugepages" flag. That could also have the effect of suppressing the warnings. > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-24 14:25 ` Michael S. Tsirkin @ 2015-11-24 14:30 ` Daniel P. Berrange 2015-11-24 14:30 ` Paolo Bonzini 1 sibling, 0 replies; 27+ messages in thread From: Daniel P. Berrange @ 2015-11-24 14:30 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Amit Shah, Paolo Bonzini, Marc-André Lureau, QEMU, Peter Maydell On Tue, Nov 24, 2015 at 04:25:40PM +0200, Michael S. Tsirkin wrote: > On Tue, Nov 24, 2015 at 02:19:36PM +0000, Daniel P. Berrange wrote: > > On Tue, Nov 24, 2015 at 04:10:54PM +0200, Michael S. Tsirkin wrote: > > > On Mon, Nov 23, 2015 at 06:10:08PM +0000, Daniel P. Berrange wrote: > > > > On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote: > > > > > On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > > > > > > > > > > > After: chardev, qtest, object-initial, object-late (not in the patch) > > > > > > > > > > > > Objects must be initialized before chardev (except rng-egd) since in the > > > > > > future chardev will need to use objects, in particular secret objects. > > > > > > Was the swap intentional? > > > > > > > > > > Yes, without the swap, qtest was not initialized before memory is allocated. > > > > > > > > > > The alternative I could think of is to check the QTEST_QEMU_BINARY > > > > > variable: http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html > > > > > > > > Why do we not simply delete the warning message about the path not > > > > being on hugetlbfs ? ie, why does QEMU try to force a policy that > > > > a memory-file backend has to be on hugetlbfs, as opposed to on > > > > a plain tmpfs ? I've previously had user request that we allow > > > > use of plain tmpfs, because they want to use vhost-user without > > > > also using hugepages, and that could be done with plain tmpfs. > > > > > > Because THP does not work on any other filesystem, > > > so many workloads are much slower. > > > That's why it's a warning, not an error. > > > > AFAICT this warning message is not in a codepath that is specific to > > use of THP. This is just generic code for allocating guest memory > > backed by a file, which does not have any assumption / prerequisite > > that THP is wanted or enabled. So adding warnings that are specifically > > related to THP is inappropriate. > > > > The fact that THP only works with a hugetlbfs path is merely a > > documentation item to record against the command line option for > > -mem-path. > > I'm worried that things go slow and people don't have a way to find out > why, and documentation isn't the first place people look for when facing > a performance issue. If you are launching QEMU directly that I think it reasonable to expect people know the difference between giving a guest a hugetlbfs path vs a regular tmpfs path, and can specifically choose with they prefer. If launching via libvirt, the error scenario you're concerned about is impossible, as libvirt will be choosing the hugetlbfs path to match the requested huge page size from teh guest XML. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-24 14:25 ` Michael S. Tsirkin 2015-11-24 14:30 ` Daniel P. Berrange @ 2015-11-24 14:30 ` Paolo Bonzini 2015-11-24 15:29 ` Michael S. Tsirkin 1 sibling, 1 reply; 27+ messages in thread From: Paolo Bonzini @ 2015-11-24 14:30 UTC (permalink / raw) To: Michael S. Tsirkin, Daniel P. Berrange Cc: Amit Shah, Peter Maydell, Marc-André Lureau, QEMU On 24/11/2015 15:25, Michael S. Tsirkin wrote: > I'm worried that things go slow and people don't have a way to find out > why, and documentation isn't the first place people look for when facing > a performance issue. > > At the moment we call MADV_HUGEPAGE unconditionally and unfortunately > there's no way to report it's not having the intended effect. > > Maybe we want a "don't enable hugepages" flag. > That could also have the effect of suppressing the warnings. THP is anyway slower than hugetlbfs. If people really want performance, they need to use hugetlbfs. For everything else I'd be surprised if they even notice. Paolo ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-24 14:30 ` Paolo Bonzini @ 2015-11-24 15:29 ` Michael S. Tsirkin 0 siblings, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-24 15:29 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Amit Shah, Marc-André Lureau, QEMU, Peter Maydell On Tue, Nov 24, 2015 at 03:30:23PM +0100, Paolo Bonzini wrote: > > > On 24/11/2015 15:25, Michael S. Tsirkin wrote: > > I'm worried that things go slow and people don't have a way to find out > > why, and documentation isn't the first place people look for when facing > > a performance issue. > > > > At the moment we call MADV_HUGEPAGE unconditionally and unfortunately > > there's no way to report it's not having the intended effect. > > > > Maybe we want a "don't enable hugepages" flag. > > That could also have the effect of suppressing the warnings. > > THP is anyway slower than hugetlbfs. If people really want performance, > they need to use hugetlbfs. For everything else I'd be surprised if > they even notice. > > Paolo If you want to post and merge a patch removing this warning completely, and revert the original one, that's fine with me. -- MST ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-24 14:19 ` Daniel P. Berrange 2015-11-24 14:25 ` Michael S. Tsirkin @ 2015-11-24 15:09 ` Markus Armbruster 1 sibling, 0 replies; 27+ messages in thread From: Markus Armbruster @ 2015-11-24 15:09 UTC (permalink / raw) To: Daniel P. Berrange Cc: Peter Maydell, Michael S. Tsirkin, QEMU, Marc-André Lureau, Amit Shah, Paolo Bonzini "Daniel P. Berrange" <berrange@redhat.com> writes: > On Tue, Nov 24, 2015 at 04:10:54PM +0200, Michael S. Tsirkin wrote: >> On Mon, Nov 23, 2015 at 06:10:08PM +0000, Daniel P. Berrange wrote: >> > On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote: >> > > On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini >> > > <pbonzini@redhat.com> wrote: >> > > > Before: object-initial, chardev, qtest, object-late (not in the patch) >> > > > >> > > > After: chardev, qtest, object-initial, object-late (not in the patch) >> > > > >> > > > Objects must be initialized before chardev (except rng-egd) since in the >> > > > future chardev will need to use objects, in particular secret objects. >> > > > Was the swap intentional? >> > > >> > > Yes, without the swap, qtest was not initialized before memory >> > > is allocated. >> > > >> > > The alternative I could think of is to check the QTEST_QEMU_BINARY >> > > variable: >> > > http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html >> > >> > Why do we not simply delete the warning message about the path not >> > being on hugetlbfs ? ie, why does QEMU try to force a policy that >> > a memory-file backend has to be on hugetlbfs, as opposed to on >> > a plain tmpfs ? I've previously had user request that we allow >> > use of plain tmpfs, because they want to use vhost-user without >> > also using hugepages, and that could be done with plain tmpfs. >> >> Because THP does not work on any other filesystem, >> so many workloads are much slower. >> That's why it's a warning, not an error. > > AFAICT this warning message is not in a codepath that is specific to > use of THP. This is just generic code for allocating guest memory > backed by a file, which does not have any assumption / prerequisite > that THP is wanted or enabled. So adding warnings that are specifically > related to THP is inappropriate. If a certain workload needs hugepages to perform, it should ask for them explicitly. Any means we provide for that purpose should error out when it can provide them. Guessing what the user meant and warning him when we're giving him something else is generally not a good idea. Whether -object memory-backend-file is a means to explicitly ask for hugepages is less than clear. Its file comment says "for hugetlbfs", but it works just fine with files on other file systems. Aside: we have *three* copies of gethugepagesize() in the tree: one that warns in exec.c, one in target-ppc/kvm.c and a third one in ivshmem-server.c. > The fact that THP only works with a hugetlbfs path is merely a > documentation item to record against the command line option for > -mem-path. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-23 18:01 ` Marc-André Lureau 2015-11-23 18:10 ` Daniel P. Berrange @ 2015-11-24 14:12 ` Michael S. Tsirkin 1 sibling, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-24 14:12 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Amit Shah, Paolo Bonzini, QEMU, Peter Maydell On Mon, Nov 23, 2015 at 07:01:33PM +0100, Marc-André Lureau wrote: > On Mon, Nov 23, 2015 at 6:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Before: object-initial, chardev, qtest, object-late (not in the patch) > > > > After: chardev, qtest, object-initial, object-late (not in the patch) > > > > Objects must be initialized before chardev (except rng-egd) since in the > > future chardev will need to use objects, in particular secret objects. > > Was the swap intentional? > > Yes, without the swap, qtest was not initialized before memory is allocated. > > The alternative I could think of is to check the QTEST_QEMU_BINARY > variable: http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg01527.html Better add a custom flag just for running tests. environment setting are too fragile. > > -- > Marc-André Lureau ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest @ 2015-10-27 16:29 marcandre.lureau 2015-10-27 18:37 ` Michael S. Tsirkin ` (3 more replies) 0 siblings, 4 replies; 27+ messages in thread From: marcandre.lureau @ 2015-10-27 16:29 UTC (permalink / raw) To: qemu-devel; +Cc: Marc-André Lureau, mst From: Marc-André Lureau <marcandre.lureau@redhat.com> vhost-user-test prints a warning. A test should not need to run on hugetlbfs, let's silence the warning under qtest. Unfortunately, the condition can't check on qtest_enabled() or qtest_driver() since they are initialized later. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- exec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/exec.c b/exec.c index 8af2570..d9c231d 100644 --- a/exec.c +++ b/exec.c @@ -1194,8 +1194,9 @@ static long gethugepagesize(const char *path, Error **errp) return 0; } - if (fs.f_type != HUGETLBFS_MAGIC) + if (fs.f_type != HUGETLBFS_MAGIC && !getenv("QTEST_QEMU_BINARY")) { fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); + } return fs.f_bsize; } -- 2.4.3 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-10-27 16:29 marcandre.lureau @ 2015-10-27 18:37 ` Michael S. Tsirkin 2015-10-29 9:10 ` Michael S. Tsirkin ` (2 subsequent siblings) 3 siblings, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2015-10-27 18:37 UTC (permalink / raw) To: marcandre.lureau; +Cc: qemu-devel On Tue, Oct 27, 2015 at 05:29:43PM +0100, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost-user-test prints a warning. A test should not need to run on > hugetlbfs, let's silence the warning under qtest. Unfortunately, the > condition can't check on qtest_enabled() or qtest_driver() since they > are initialized later. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Let's move the warning to somewhere later then. > --- > exec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index 8af2570..d9c231d 100644 > --- a/exec.c > +++ b/exec.c > @@ -1194,8 +1194,9 @@ static long gethugepagesize(const char *path, Error **errp) > return 0; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) > + if (fs.f_type != HUGETLBFS_MAGIC && !getenv("QTEST_QEMU_BINARY")) { > fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + } > > return fs.f_bsize; > } > -- > 2.4.3 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-10-27 16:29 marcandre.lureau 2015-10-27 18:37 ` Michael S. Tsirkin @ 2015-10-29 9:10 ` Michael S. Tsirkin 2015-11-08 17:02 ` Michael S. Tsirkin 2015-11-12 11:54 ` Michael S. Tsirkin 3 siblings, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2015-10-29 9:10 UTC (permalink / raw) To: marcandre.lureau; +Cc: Peter Maydell, qemu-devel On Tue, Oct 27, 2015 at 05:29:43PM +0100, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost-user-test prints a warning. A test should not need to run on > hugetlbfs, let's silence the warning under qtest. Unfortunately, the > condition can't check on qtest_enabled() or qtest_driver() since they > are initialized later. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> I guess we need a command line option to suppress the warning. Seems a lot of work for very little benefit, but that's the best I could come up with. > --- > exec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index 8af2570..d9c231d 100644 > --- a/exec.c > +++ b/exec.c > @@ -1194,8 +1194,9 @@ static long gethugepagesize(const char *path, Error **errp) > return 0; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) > + if (fs.f_type != HUGETLBFS_MAGIC && !getenv("QTEST_QEMU_BINARY")) { > fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + } > > return fs.f_bsize; > } > -- > 2.4.3 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-10-27 16:29 marcandre.lureau 2015-10-27 18:37 ` Michael S. Tsirkin 2015-10-29 9:10 ` Michael S. Tsirkin @ 2015-11-08 17:02 ` Michael S. Tsirkin 2015-11-09 7:50 ` Markus Armbruster 2015-11-12 11:54 ` Michael S. Tsirkin 3 siblings, 1 reply; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-08 17:02 UTC (permalink / raw) To: marcandre.lureau; +Cc: Peter Maydell, qemu-devel On Tue, Oct 27, 2015 at 05:29:43PM +0100, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost-user-test prints a warning. A test should not need to run on > hugetlbfs, let's silence the warning under qtest. Unfortunately, the > condition can't check on qtest_enabled() or qtest_driver() since they > are initialized later. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Any idea what's the best way to address this? Is poking at environment like this appropriate? Maybe a command line flag to silence the warning? > --- > exec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index 8af2570..d9c231d 100644 > --- a/exec.c > +++ b/exec.c > @@ -1194,8 +1194,9 @@ static long gethugepagesize(const char *path, Error **errp) > return 0; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) > + if (fs.f_type != HUGETLBFS_MAGIC && !getenv("QTEST_QEMU_BINARY")) { > fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + } > > return fs.f_bsize; > } > -- > 2.4.3 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-11-08 17:02 ` Michael S. Tsirkin @ 2015-11-09 7:50 ` Markus Armbruster 0 siblings, 0 replies; 27+ messages in thread From: Markus Armbruster @ 2015-11-09 7:50 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: marcandre.lureau, qemu-devel, Peter Maydell "Michael S. Tsirkin" <mst@redhat.com> writes: > On Tue, Oct 27, 2015 at 05:29:43PM +0100, marcandre.lureau@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> vhost-user-test prints a warning. A test should not need to run on >> hugetlbfs, let's silence the warning under qtest. Unfortunately, the >> condition can't check on qtest_enabled() or qtest_driver() since they >> are initialized later. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Any idea what's the best way to address this? Can we reorder things so that qtest_enabled() can be used? > Is poking at environment like this appropriate? I'm afraid it isn't. QTEST_QEMU_BINARY can be in the environment even when you're not running under qtest. > Maybe a command line flag to silence the warning? There's -qtest and -qtest-log. I guess keying on those would be less unclean than QTEST_QEMU_BINARY. >> --- >> exec.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/exec.c b/exec.c >> index 8af2570..d9c231d 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1194,8 +1194,9 @@ static long gethugepagesize(const char *path, Error **errp) >> return 0; >> } >> >> - if (fs.f_type != HUGETLBFS_MAGIC) >> + if (fs.f_type != HUGETLBFS_MAGIC && !getenv("QTEST_QEMU_BINARY")) { >> fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); >> + } >> >> return fs.f_bsize; >> } >> -- >> 2.4.3 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH] exec: silence hugetlbfs warning under qtest 2015-10-27 16:29 marcandre.lureau ` (2 preceding siblings ...) 2015-11-08 17:02 ` Michael S. Tsirkin @ 2015-11-12 11:54 ` Michael S. Tsirkin 3 siblings, 0 replies; 27+ messages in thread From: Michael S. Tsirkin @ 2015-11-12 11:54 UTC (permalink / raw) To: marcandre.lureau; +Cc: qemu-devel On Tue, Oct 27, 2015 at 05:29:43PM +0100, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > vhost-user-test prints a warning. A test should not need to run on > hugetlbfs, let's silence the warning under qtest. Unfortunately, the > condition can't check on qtest_enabled() or qtest_driver() since they > are initialized later. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> OK so far this is the best we could come up with, so I'll queue this up for after 2.5 unless someone objects. > --- > exec.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/exec.c b/exec.c > index 8af2570..d9c231d 100644 > --- a/exec.c > +++ b/exec.c > @@ -1194,8 +1194,9 @@ static long gethugepagesize(const char *path, Error **errp) > return 0; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) > + if (fs.f_type != HUGETLBFS_MAGIC && !getenv("QTEST_QEMU_BINARY")) { > fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + } > > return fs.f_bsize; > } > -- > 2.4.3 ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-11-24 15:29 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-16 17:23 [Qemu-devel] [PATCH 1/3] tests: define qtest-obj-y before using it marcandre.lureau 2015-11-16 17:23 ` [Qemu-devel] [PATCH 2/3] tests: re-enable vhost-user-test marcandre.lureau 2015-11-16 17:23 ` [Qemu-devel] [PATCH 3/3] exec: silence hugetlbfs warning under qtest marcandre.lureau 2015-11-16 21:50 ` Michael S. Tsirkin 2015-11-17 21:32 ` Marc-André Lureau 2015-11-18 8:32 ` Markus Armbruster 2015-11-18 9:02 ` [Qemu-devel] [PATCH] " marcandre.lureau 2015-11-18 20:49 ` Michael S. Tsirkin 2015-11-23 17:40 ` Paolo Bonzini 2015-11-23 17:46 ` Daniel P. Berrange 2015-11-23 17:49 ` Daniel P. Berrange 2015-11-23 18:01 ` Marc-André Lureau 2015-11-23 18:10 ` Daniel P. Berrange 2015-11-24 14:10 ` Michael S. Tsirkin 2015-11-24 14:19 ` Daniel P. Berrange 2015-11-24 14:25 ` Michael S. Tsirkin 2015-11-24 14:30 ` Daniel P. Berrange 2015-11-24 14:30 ` Paolo Bonzini 2015-11-24 15:29 ` Michael S. Tsirkin 2015-11-24 15:09 ` Markus Armbruster 2015-11-24 14:12 ` Michael S. Tsirkin -- strict thread matches above, loose matches on Subject: below -- 2015-10-27 16:29 marcandre.lureau 2015-10-27 18:37 ` Michael S. Tsirkin 2015-10-29 9:10 ` Michael S. Tsirkin 2015-11-08 17:02 ` Michael S. Tsirkin 2015-11-09 7:50 ` Markus Armbruster 2015-11-12 11:54 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.