| Message ID | 20181127135030.1671-2-i.maximets@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | memfd fixes. | expand |
Hi On Tue, Nov 27, 2018 at 5:50 PM Ilya Maximets <i.maximets@samsung.com> wrote: > > If seals are not supported, memfd_create() will fail. > Furthermore, there is no way to disable it in this case because > '.seal' property is not registered. > > This issue leads to vhost-user-test failures on RHEL 7.2: > > qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > failed to create memfd: Invalid argument > > and actually breaks the feature on such systems. > > Let's restrict memfd backend to systems with sealing support. > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> thanks > --- > backends/hostmem-memfd.c | 18 ++++++++---------- > tests/vhost-user-test.c | 6 +++--- > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > index b6836b28e5..a3455da9c9 100644 > --- a/backends/hostmem-memfd.c > +++ b/backends/hostmem-memfd.c > @@ -156,15 +156,13 @@ memfd_backend_class_init(ObjectClass *oc, void *data) > "Huge pages size (ex: 2M, 1G)", > &error_abort); > } > - if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > - object_class_property_add_bool(oc, "seal", > - memfd_backend_get_seal, > - memfd_backend_set_seal, > - &error_abort); > - object_class_property_set_description(oc, "seal", > - "Seal growing & shrinking", > - &error_abort); > - } > + object_class_property_add_bool(oc, "seal", > + memfd_backend_get_seal, > + memfd_backend_set_seal, > + &error_abort); > + object_class_property_set_description(oc, "seal", > + "Seal growing & shrinking", > + &error_abort); > } > > static const TypeInfo memfd_backend_info = { > @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { > > static void register_types(void) > { > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > type_register_static(&memfd_backend_info); > } > } > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c > index 45d58d8ea2..e3e9a33580 100644 > --- a/tests/vhost-user-test.c > +++ b/tests/vhost-user-test.c > @@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s, > int mem, enum test_memfd memfd, const char *mem_path, > const char *chr_opts, const char *extra) > { > - if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) { > + if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(MFD_ALLOW_SEALING)) { > memfd = TEST_MEMFD_YES; > } > > @@ -903,7 +903,7 @@ static void test_multiqueue(void) > s->queues = 2; > test_server_listen(s); > > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > cmd = g_strdup_printf( > QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d " > "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d", > @@ -963,7 +963,7 @@ int main(int argc, char **argv) > /* run the main loop thread so the chardev may operate */ > thread = g_thread_new(NULL, thread_function, loop); > > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > qtest_add_data_func("/vhost-user/read-guest-mem/memfd", > GINT_TO_POINTER(TEST_MEMFD_YES), > test_read_guest_mem); > -- > 2.17.1 >
On Tue, 27 Nov 2018 16:50:27 +0300 Ilya Maximets <i.maximets@samsung.com> wrote: s/wihtout/without/ in subj > If seals are not supported, memfd_create() will fail. > Furthermore, there is no way to disable it in this case because > '.seal' property is not registered. > > This issue leads to vhost-user-test failures on RHEL 7.2: > > qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > failed to create memfd: Invalid argument > > and actually breaks the feature on such systems. > > Let's restrict memfd backend to systems with sealing support. > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > backends/hostmem-memfd.c | 18 ++++++++---------- > tests/vhost-user-test.c | 6 +++--- > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c > index b6836b28e5..a3455da9c9 100644 > --- a/backends/hostmem-memfd.c > +++ b/backends/hostmem-memfd.c > @@ -156,15 +156,13 @@ memfd_backend_class_init(ObjectClass *oc, void *data) > "Huge pages size (ex: 2M, 1G)", > &error_abort); > } > - if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > - object_class_property_add_bool(oc, "seal", > - memfd_backend_get_seal, > - memfd_backend_set_seal, > - &error_abort); > - object_class_property_set_description(oc, "seal", > - "Seal growing & shrinking", > - &error_abort); > - } > + object_class_property_add_bool(oc, "seal", > + memfd_backend_get_seal, > + memfd_backend_set_seal, > + &error_abort); > + object_class_property_set_description(oc, "seal", > + "Seal growing & shrinking", > + &error_abort); > } > > static const TypeInfo memfd_backend_info = { > @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { > > static void register_types(void) > { > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > type_register_static(&memfd_backend_info); that would either lead to not clear error that type doesn't exist. it could be better to report sensible error from memfd_backend_memory_alloc() if the feature is required but not supported by host > } > } > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c > index 45d58d8ea2..e3e9a33580 100644 > --- a/tests/vhost-user-test.c > +++ b/tests/vhost-user-test.c > @@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s, > int mem, enum test_memfd memfd, const char *mem_path, > const char *chr_opts, const char *extra) > { > - if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) { > + if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(MFD_ALLOW_SEALING)) { > memfd = TEST_MEMFD_YES; > } > > @@ -903,7 +903,7 @@ static void test_multiqueue(void) > s->queues = 2; > test_server_listen(s); > > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > cmd = g_strdup_printf( > QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d " > "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d", > @@ -963,7 +963,7 @@ int main(int argc, char **argv) > /* run the main loop thread so the chardev may operate */ > thread = g_thread_new(NULL, thread_function, loop); > > - if (qemu_memfd_check(0)) { > + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > qtest_add_data_func("/vhost-user/read-guest-mem/memfd", > GINT_TO_POINTER(TEST_MEMFD_YES), > test_read_guest_mem);
On 10.12.2018 19:18, Igor Mammedov wrote: > On Tue, 27 Nov 2018 16:50:27 +0300 > Ilya Maximets <i.maximets@samsung.com> wrote: > > s/wihtout/without/ in subj > >> If seals are not supported, memfd_create() will fail. >> Furthermore, there is no way to disable it in this case because >> '.seal' property is not registered. >> >> This issue leads to vhost-user-test failures on RHEL 7.2: >> >> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ >> failed to create memfd: Invalid argument >> >> and actually breaks the feature on such systems. >> >> Let's restrict memfd backend to systems with sealing support. >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> --- >> backends/hostmem-memfd.c | 18 ++++++++---------- >> tests/vhost-user-test.c | 6 +++--- >> 2 files changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c >> index b6836b28e5..a3455da9c9 100644 >> --- a/backends/hostmem-memfd.c >> +++ b/backends/hostmem-memfd.c >> @@ -156,15 +156,13 @@ memfd_backend_class_init(ObjectClass *oc, void *data) >> "Huge pages size (ex: 2M, 1G)", >> &error_abort); >> } >> - if (qemu_memfd_check(MFD_ALLOW_SEALING)) { >> - object_class_property_add_bool(oc, "seal", >> - memfd_backend_get_seal, >> - memfd_backend_set_seal, >> - &error_abort); >> - object_class_property_set_description(oc, "seal", >> - "Seal growing & shrinking", >> - &error_abort); >> - } >> + object_class_property_add_bool(oc, "seal", >> + memfd_backend_get_seal, >> + memfd_backend_set_seal, >> + &error_abort); >> + object_class_property_set_description(oc, "seal", >> + "Seal growing & shrinking", >> + &error_abort); >> } >> >> static const TypeInfo memfd_backend_info = { >> @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { >> >> static void register_types(void) >> { >> - if (qemu_memfd_check(0)) { >> + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { >> type_register_static(&memfd_backend_info); > that would either lead to not clear error that type doesn't exist. > it could be better to report sensible error from memfd_backend_memory_alloc() if > the feature is required but not supported by host I'm not sure, but this could break the libvirt capability discovering. Current patch changes behaviour probably only for RHEL/CentOS 7.2. All other systems are not affected. Do you think that we need to change behaviour on all the systems? > >> } >> } >> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c >> index 45d58d8ea2..e3e9a33580 100644 >> --- a/tests/vhost-user-test.c >> +++ b/tests/vhost-user-test.c >> @@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s, >> int mem, enum test_memfd memfd, const char *mem_path, >> const char *chr_opts, const char *extra) >> { >> - if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) { >> + if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(MFD_ALLOW_SEALING)) { >> memfd = TEST_MEMFD_YES; >> } >> >> @@ -903,7 +903,7 @@ static void test_multiqueue(void) >> s->queues = 2; >> test_server_listen(s); >> >> - if (qemu_memfd_check(0)) { >> + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { >> cmd = g_strdup_printf( >> QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d " >> "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d", >> @@ -963,7 +963,7 @@ int main(int argc, char **argv) >> /* run the main loop thread so the chardev may operate */ >> thread = g_thread_new(NULL, thread_function, loop); >> >> - if (qemu_memfd_check(0)) { >> + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { >> qtest_add_data_func("/vhost-user/read-guest-mem/memfd", >> GINT_TO_POINTER(TEST_MEMFD_YES), >> test_read_guest_mem); > > >
On Tue, Nov 27, 2018 at 04:50:27PM +0300, Ilya Maximets wrote: > If seals are not supported, memfd_create() will fail. > Furthermore, there is no way to disable it in this case because > '.seal' property is not registered. Isn't the real problem here that memfd_backend_instance_init() has unconditionally set "m->seal = true" Surely, if we don't register the '.seal' property, we should default that flag to false. > > This issue leads to vhost-user-test failures on RHEL 7.2: > > qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > failed to create memfd: Invalid argument > > and actually breaks the feature on such systems. > > Let's restrict memfd backend to systems with sealing support. I don't think we need todo that - sealing is optional in the QEMU code, we simply have it set to the wrong default when sealing is not available. > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> Regards, Daniel
On 11.12.2018 13:53, Daniel P. Berrangé wrote: > On Tue, Nov 27, 2018 at 04:50:27PM +0300, Ilya Maximets wrote: >> If seals are not supported, memfd_create() will fail. >> Furthermore, there is no way to disable it in this case because >> '.seal' property is not registered. > > Isn't the real problem here that memfd_backend_instance_init() has > unconditionally set "m->seal = true" > > Surely, if we don't register the '.seal' property, we should default > that flag to false. > >> >> This issue leads to vhost-user-test failures on RHEL 7.2: >> >> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ >> failed to create memfd: Invalid argument >> >> and actually breaks the feature on such systems. >> >> Let's restrict memfd backend to systems with sealing support. > > I don't think we need todo that - sealing is optional in the QEMU code, > we simply have it set to the wrong default when sealing is not available. That was literally what I've fixed in v1: https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html but 2 people suggested me to disable memfd entirely for this case. Do you think I need to get patch from v1 back ? Gerd, Marc-André, what do you think? > >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > Regards, > Daniel >
On Tue, 11 Dec 2018 13:29:19 +0300 Ilya Maximets <i.maximets@samsung.com> wrote: CCing libvirt folk for an opinion > On 10.12.2018 19:18, Igor Mammedov wrote: > > On Tue, 27 Nov 2018 16:50:27 +0300 > > Ilya Maximets <i.maximets@samsung.com> wrote: > > > > s/wihtout/without/ in subj > > > >> If seals are not supported, memfd_create() will fail. > >> Furthermore, there is no way to disable it in this case because > >> '.seal' property is not registered. > >> > >> This issue leads to vhost-user-test failures on RHEL 7.2: > >> > >> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > >> failed to create memfd: Invalid argument > >> > >> and actually breaks the feature on such systems. > >> > >> Let's restrict memfd backend to systems with sealing support. > >> [...] > >> @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { > >> > >> static void register_types(void) > >> { > >> - if (qemu_memfd_check(0)) { > >> + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > >> type_register_static(&memfd_backend_info); > > that would either lead to not clear error that type doesn't exist. > > it could be better to report sensible error from memfd_backend_memory_alloc() if > > the feature is required but not supported by host > > I'm not sure, but this could break the libvirt capability discovering. > > Current patch changes behaviour probably only for RHEL/CentOS 7.2. > All other systems are not affected. Do you think that we need to > change behaviour on all the systems? you are changing behavior anyways, so when users start getting on some of 'All other systems' start getting 'type doesn't exist' error, they won't have a clue what's wrong. In case where we are fixing broken defaults, shouldn't we at least do it the way that would inform user about misconfiguration. But I'm not insisting since memfd is fairly new, it might be fine for device to just disappear. [...]
On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote: > On 11.12.2018 13:53, Daniel P. Berrangé wrote: > >> > >> Let's restrict memfd backend to systems with sealing support. > > > > I don't think we need todo that - sealing is optional in the QEMU code, > > we simply have it set to the wrong default when sealing is not available. > > That was literally what I've fixed in v1: > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html > > but 2 people suggested me to disable memfd entirely for this case. > Do you think I need to get patch from v1 back ? > > Gerd, Marc-André, what do you think? I still think it makes sense to require sealing support. Sealing is very useful, and there are only a few kernel versions with memfd but without sealing. So finding such kernels in the wild will become more rare over time. I wouldn't worry too much about them. cheers, Gerd
On Tue, Dec 11, 2018 at 04:48:23PM +0100, Igor Mammedov wrote: > On Tue, 11 Dec 2018 13:29:19 +0300 > Ilya Maximets <i.maximets@samsung.com> wrote: > > CCing libvirt folk for an opinion > > > On 10.12.2018 19:18, Igor Mammedov wrote: > > > On Tue, 27 Nov 2018 16:50:27 +0300 > > > Ilya Maximets <i.maximets@samsung.com> wrote: > > > > > > s/wihtout/without/ in subj > > > > > >> If seals are not supported, memfd_create() will fail. > > >> Furthermore, there is no way to disable it in this case because > > >> '.seal' property is not registered. > > >> > > >> This issue leads to vhost-user-test failures on RHEL 7.2: > > >> > > >> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ > > >> failed to create memfd: Invalid argument > > >> > > >> and actually breaks the feature on such systems. > > >> > > >> Let's restrict memfd backend to systems with sealing support. > > >> > [...] > > >> @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { > > >> > > >> static void register_types(void) > > >> { > > >> - if (qemu_memfd_check(0)) { > > >> + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { > > >> type_register_static(&memfd_backend_info); > > > that would either lead to not clear error that type doesn't exist. > > > it could be better to report sensible error from memfd_backend_memory_alloc() if > > > the feature is required but not supported by host > > > > I'm not sure, but this could break the libvirt capability discovering. > > > > Current patch changes behaviour probably only for RHEL/CentOS 7.2. > > All other systems are not affected. Do you think that we need to > > change behaviour on all the systems? > you are changing behavior anyways, so when users start getting > on some of 'All other systems' start getting 'type doesn't exist' > error, they won't have a clue what's wrong. In case where we are > fixing broken defaults, shouldn't we at least do it the way that > would inform user about misconfiguration. > > But I'm not insisting since memfd is fairly new, it might be fine > for device to just disappear. (Sorry for taking so long to reply on this series. I couldn't review the code yet.) I'd like to make the QOM type hierarchy static, and not depend on any runtime host capability checks. But this is not a new problem in the code, so I don't think it should block this series.
So, can we have any conclusion about this patch and the series? Best regards, Ilya Maximets. On 05.01.2019 5:43, Eduardo Habkost wrote: > On Tue, Dec 11, 2018 at 04:48:23PM +0100, Igor Mammedov wrote: >> On Tue, 11 Dec 2018 13:29:19 +0300 >> Ilya Maximets <i.maximets@samsung.com> wrote: >> >> CCing libvirt folk for an opinion >> >>> On 10.12.2018 19:18, Igor Mammedov wrote: >>>> On Tue, 27 Nov 2018 16:50:27 +0300 >>>> Ilya Maximets <i.maximets@samsung.com> wrote: >>>> >>>> s/wihtout/without/ in subj >>>> >>>>> If seals are not supported, memfd_create() will fail. >>>>> Furthermore, there is no way to disable it in this case because >>>>> '.seal' property is not registered. >>>>> >>>>> This issue leads to vhost-user-test failures on RHEL 7.2: >>>>> >>>>> qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ >>>>> failed to create memfd: Invalid argument >>>>> >>>>> and actually breaks the feature on such systems. >>>>> >>>>> Let's restrict memfd backend to systems with sealing support. >>>>> >> [...] >>>>> @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { >>>>> >>>>> static void register_types(void) >>>>> { >>>>> - if (qemu_memfd_check(0)) { >>>>> + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { >>>>> type_register_static(&memfd_backend_info); >>>> that would either lead to not clear error that type doesn't exist. >>>> it could be better to report sensible error from memfd_backend_memory_alloc() if >>>> the feature is required but not supported by host >>> >>> I'm not sure, but this could break the libvirt capability discovering. >>> >>> Current patch changes behaviour probably only for RHEL/CentOS 7.2. >>> All other systems are not affected. Do you think that we need to >>> change behaviour on all the systems? >> you are changing behavior anyways, so when users start getting >> on some of 'All other systems' start getting 'type doesn't exist' >> error, they won't have a clue what's wrong. In case where we are >> fixing broken defaults, shouldn't we at least do it the way that >> would inform user about misconfiguration. >> >> But I'm not insisting since memfd is fairly new, it might be fine >> for device to just disappear. > > (Sorry for taking so long to reply on this series. I couldn't > review the code yet.) > > I'd like to make the QOM type hierarchy static, and not depend on > any runtime host capability checks. But this is not a new > problem in the code, so I don't think it should block this > series. >
On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote: > On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote: > > On 11.12.2018 13:53, Daniel P. Berrangé wrote: > > >> > > >> Let's restrict memfd backend to systems with sealing support. > > > > > > I don't think we need todo that - sealing is optional in the QEMU code, > > > we simply have it set to the wrong default when sealing is not available. > > > > That was literally what I've fixed in v1: > > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html > > > > but 2 people suggested me to disable memfd entirely for this case. > > Do you think I need to get patch from v1 back ? > > > > Gerd, Marc-André, what do you think? > > I still think it makes sense to require sealing support. Sealing is > very useful, and there are only a few kernel versions with memfd but > without sealing. So finding such kernels in the wild will become more > rare over time. I wouldn't worry too much about them. -object memory-backend-memfd,id=mem,size=2M,seal=off still works on those systems, doesn't it? What's the rationale for breaking a working configuration without following the deprecation policy?
On 16.01.2019 18:30, Eduardo Habkost wrote: > On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote: >> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote: >>> On 11.12.2018 13:53, Daniel P. Berrangé wrote: >>>>> >>>>> Let's restrict memfd backend to systems with sealing support. >>>> >>>> I don't think we need todo that - sealing is optional in the QEMU code, >>>> we simply have it set to the wrong default when sealing is not available. >>> >>> That was literally what I've fixed in v1: >>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html >>> >>> but 2 people suggested me to disable memfd entirely for this case. >>> Do you think I need to get patch from v1 back ? >>> >>> Gerd, Marc-André, what do you think? >> >> I still think it makes sense to require sealing support. Sealing is >> very useful, and there are only a few kernel versions with memfd but >> without sealing. So finding such kernels in the wild will become more >> rare over time. I wouldn't worry too much about them. > > -object memory-backend-memfd,id=mem,size=2M,seal=off still > works on those systems, doesn't it? What's the rationale for > breaking a working configuration without following the > deprecation policy? > See the commit message. '.seal' property is not registered if sealing is not supported. So, there is no way to disable sealing on the system that does not support it.
On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote: > > > On 16.01.2019 18:30, Eduardo Habkost wrote: > > On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote: > >> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote: > >>> On 11.12.2018 13:53, Daniel P. Berrangé wrote: > >>>>> > >>>>> Let's restrict memfd backend to systems with sealing support. > >>>> > >>>> I don't think we need todo that - sealing is optional in the QEMU code, > >>>> we simply have it set to the wrong default when sealing is not available. > >>> > >>> That was literally what I've fixed in v1: > >>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html > >>> > >>> but 2 people suggested me to disable memfd entirely for this case. > >>> Do you think I need to get patch from v1 back ? > >>> > >>> Gerd, Marc-André, what do you think? > >> > >> I still think it makes sense to require sealing support. Sealing is > >> very useful, and there are only a few kernel versions with memfd but > >> without sealing. So finding such kernels in the wild will become more > >> rare over time. I wouldn't worry too much about them. > > > > -object memory-backend-memfd,id=mem,size=2M,seal=off still > > works on those systems, doesn't it? What's the rationale for > > breaking a working configuration without following the > > deprecation policy? > > > > See the commit message. > '.seal' property is not registered if sealing is not supported. > So, there is no way to disable sealing on the system that does not support it. As I pointed out a few lines up, this is simply because QEMU has a bug setting seal=true as the built-in default value even when it isn't supported. Regards, Daniel
On 16.01.2019 18:48, Daniel P. Berrangé wrote: > On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote: >> >> >> On 16.01.2019 18:30, Eduardo Habkost wrote: >>> On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote: >>>> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote: >>>>> On 11.12.2018 13:53, Daniel P. Berrangé wrote: >>>>>>> >>>>>>> Let's restrict memfd backend to systems with sealing support. >>>>>> >>>>>> I don't think we need todo that - sealing is optional in the QEMU code, >>>>>> we simply have it set to the wrong default when sealing is not available. >>>>> >>>>> That was literally what I've fixed in v1: >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html >>>>> >>>>> but 2 people suggested me to disable memfd entirely for this case. >>>>> Do you think I need to get patch from v1 back ? >>>>> >>>>> Gerd, Marc-André, what do you think? >>>> >>>> I still think it makes sense to require sealing support. Sealing is >>>> very useful, and there are only a few kernel versions with memfd but >>>> without sealing. So finding such kernels in the wild will become more >>>> rare over time. I wouldn't worry too much about them. >>> >>> -object memory-backend-memfd,id=mem,size=2M,seal=off still >>> works on those systems, doesn't it? What's the rationale for >>> breaking a working configuration without following the >>> deprecation policy? >>> >> >> See the commit message. >> '.seal' property is not registered if sealing is not supported. >> So, there is no way to disable sealing on the system that does not support it. > > As I pointed out a few lines up, this is simply because QEMU has a bug > setting seal=true as the built-in default value even when it isn't > supported. So, do you think I need to return to the solution from my v1: https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html ?
On Wed, Jan 16, 2019 at 06:54:38PM +0300, Ilya Maximets wrote: > On 16.01.2019 18:48, Daniel P. Berrangé wrote: > > On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote: > >> > >> > >> On 16.01.2019 18:30, Eduardo Habkost wrote: > >>> On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote: > >>>> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote: > >>>>> On 11.12.2018 13:53, Daniel P. Berrangé wrote: > >>>>>>> > >>>>>>> Let's restrict memfd backend to systems with sealing support. > >>>>>> > >>>>>> I don't think we need todo that - sealing is optional in the QEMU code, > >>>>>> we simply have it set to the wrong default when sealing is not available. > >>>>> > >>>>> That was literally what I've fixed in v1: > >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html > >>>>> > >>>>> but 2 people suggested me to disable memfd entirely for this case. > >>>>> Do you think I need to get patch from v1 back ? > >>>>> > >>>>> Gerd, Marc-André, what do you think? > >>>> > >>>> I still think it makes sense to require sealing support. Sealing is > >>>> very useful, and there are only a few kernel versions with memfd but > >>>> without sealing. So finding such kernels in the wild will become more > >>>> rare over time. I wouldn't worry too much about them. > >>> > >>> -object memory-backend-memfd,id=mem,size=2M,seal=off still > >>> works on those systems, doesn't it? What's the rationale for > >>> breaking a working configuration without following the > >>> deprecation policy? > >>> > >> > >> See the commit message. > >> '.seal' property is not registered if sealing is not supported. > >> So, there is no way to disable sealing on the system that does not support it. > > > > As I pointed out a few lines up, this is simply because QEMU has a bug > > setting seal=true as the built-in default value even when it isn't > > supported. > > So, do you think I need to return to the solution from my v1: > https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html That is my preference, but we don't have universal agreement :-( Regards, Daniel
On Wed, Jan 16, 2019 at 03:48:57PM +0000, Daniel P. Berrangé wrote: > On Wed, Jan 16, 2019 at 06:46:39PM +0300, Ilya Maximets wrote: > > > > > > On 16.01.2019 18:30, Eduardo Habkost wrote: > > > On Wed, Dec 12, 2018 at 07:49:36AM +0100, Gerd Hoffmann wrote: > > >> On Tue, Dec 11, 2018 at 02:09:11PM +0300, Ilya Maximets wrote: > > >>> On 11.12.2018 13:53, Daniel P. Berrangé wrote: > > >>>>> > > >>>>> Let's restrict memfd backend to systems with sealing support. > > >>>> > > >>>> I don't think we need todo that - sealing is optional in the QEMU code, > > >>>> we simply have it set to the wrong default when sealing is not available. > > >>> > > >>> That was literally what I've fixed in v1: > > >>> https://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg05483.html > > >>> > > >>> but 2 people suggested me to disable memfd entirely for this case. > > >>> Do you think I need to get patch from v1 back ? > > >>> > > >>> Gerd, Marc-André, what do you think? > > >> > > >> I still think it makes sense to require sealing support. Sealing is > > >> very useful, and there are only a few kernel versions with memfd but > > >> without sealing. So finding such kernels in the wild will become more > > >> rare over time. I wouldn't worry too much about them. > > > > > > -object memory-backend-memfd,id=mem,size=2M,seal=off still > > > works on those systems, doesn't it? What's the rationale for > > > breaking a working configuration without following the > > > deprecation policy? > > > > > > > See the commit message. > > '.seal' property is not registered if sealing is not supported. > > So, there is no way to disable sealing on the system that does not support it. > > As I pointed out a few lines up, this is simply because QEMU has a bug > setting seal=true as the built-in default value even when it isn't > supported. Changing to seal=false by default may make it work on some hosts, but I don't see the point of increasing our support burden just for a few kernel versions. I agree with Gerd, I think it's simpler to keep it unsupported.
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c index b6836b28e5..a3455da9c9 100644 --- a/backends/hostmem-memfd.c +++ b/backends/hostmem-memfd.c @@ -156,15 +156,13 @@ memfd_backend_class_init(ObjectClass *oc, void *data) "Huge pages size (ex: 2M, 1G)", &error_abort); } - if (qemu_memfd_check(MFD_ALLOW_SEALING)) { - object_class_property_add_bool(oc, "seal", - memfd_backend_get_seal, - memfd_backend_set_seal, - &error_abort); - object_class_property_set_description(oc, "seal", - "Seal growing & shrinking", - &error_abort); - } + object_class_property_add_bool(oc, "seal", + memfd_backend_get_seal, + memfd_backend_set_seal, + &error_abort); + object_class_property_set_description(oc, "seal", + "Seal growing & shrinking", + &error_abort); } static const TypeInfo memfd_backend_info = { @@ -177,7 +175,7 @@ static const TypeInfo memfd_backend_info = { static void register_types(void) { - if (qemu_memfd_check(0)) { + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { type_register_static(&memfd_backend_info); } } diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c index 45d58d8ea2..e3e9a33580 100644 --- a/tests/vhost-user-test.c +++ b/tests/vhost-user-test.c @@ -169,7 +169,7 @@ static char *get_qemu_cmd(TestServer *s, int mem, enum test_memfd memfd, const char *mem_path, const char *chr_opts, const char *extra) { - if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(0)) { + if (memfd == TEST_MEMFD_AUTO && qemu_memfd_check(MFD_ALLOW_SEALING)) { memfd = TEST_MEMFD_YES; } @@ -903,7 +903,7 @@ static void test_multiqueue(void) s->queues = 2; test_server_listen(s); - if (qemu_memfd_check(0)) { + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { cmd = g_strdup_printf( QEMU_CMD_MEMFD QEMU_CMD_CHR QEMU_CMD_NETDEV ",queues=%d " "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d", @@ -963,7 +963,7 @@ int main(int argc, char **argv) /* run the main loop thread so the chardev may operate */ thread = g_thread_new(NULL, thread_function, loop); - if (qemu_memfd_check(0)) { + if (qemu_memfd_check(MFD_ALLOW_SEALING)) { qtest_add_data_func("/vhost-user/read-guest-mem/memfd", GINT_TO_POINTER(TEST_MEMFD_YES), test_read_guest_mem);
If seals are not supported, memfd_create() will fail. Furthermore, there is no way to disable it in this case because '.seal' property is not registered. This issue leads to vhost-user-test failures on RHEL 7.2: qemu-system-x86_64: -object memory-backend-memfd,id=mem,size=2M,: \ failed to create memfd: Invalid argument and actually breaks the feature on such systems. Let's restrict memfd backend to systems with sealing support. Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- backends/hostmem-memfd.c | 18 ++++++++---------- tests/vhost-user-test.c | 6 +++--- 2 files changed, 11 insertions(+), 13 deletions(-)