For some reason memfd never used share=on. vhost-user relies on
mmap(MAP_SHARED) so this seems like a problem, but the tests still run
without it.

Add share=on for consistency and to prevent future bugs in the test.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qtest/vhost-user-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 1a5f5313ff..2db98c4920 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -40,7 +40,7 @@
 #define QEMU_CMD_MEM    " -m %d -object memory-backend-file,id=mem,size=%dM," \
                         "mem-path=%s,share=on -numa node,memdev=mem"
 #define QEMU_CMD_MEMFD  " -m %d -object memory-backend-memfd,id=mem,size=%dM," \
-                        " -numa node,memdev=mem"
+                        "share=on -numa node,memdev=mem"
 #define QEMU_CMD_CHR    " -chardev socket,id=%s,path=%s%s"
 #define QEMU_CMD_NETDEV " -netdev vhost-user,id=hs0,chardev=%s,vhostforce"
 
-- 
2.29.2

Hi

On Mon, Feb 22, 2021 at 8:11 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> For some reason memfd never used share=on. vhost-user relies on
> mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> without it.
>
>
Simply because it's on by default with memory-backend-memfd (it wouldn't
make much sense to use memfd in the first place without share)

Add share=on for consistency and to prevent future bugs in the test.
>

But it doesn't hurt to be explicit though.


> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  tests/qtest/vhost-user-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index 1a5f5313ff..2db98c4920 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -40,7 +40,7 @@
>  #define QEMU_CMD_MEM    " -m %d -object
> memory-backend-file,id=mem,size=%dM," \
>                          "mem-path=%s,share=on -numa node,memdev=mem"
>  #define QEMU_CMD_MEMFD  " -m %d -object
> memory-backend-memfd,id=mem,size=%dM," \
> -                        " -numa node,memdev=mem"
> +                        "share=on -numa node,memdev=mem"
>  #define QEMU_CMD_CHR    " -chardev socket,id=%s,path=%s%s"
>  #define QEMU_CMD_NETDEV " -netdev vhost-user,id=hs0,chardev=%s,vhostforce"
>
> --
> 2.29.2
>
>

On Wed, Feb 24, 2021 at 02:22:31PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 22, 2021 at 8:11 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > For some reason memfd never used share=on. vhost-user relies on
> > mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> > without it.
> >
> >
> Simply because it's on by default with memory-backend-memfd (it wouldn't
> make much sense to use memfd in the first place without share)

Thanks for solving this mystery!

Please update the commit description with this information when merging
(or I'll update it when respinning).

Stefan

On 22/02/2021 17.10, Stefan Hajnoczi wrote:
> For some reason memfd never used share=on. vhost-user relies on
> mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> without it.
> 
> Add share=on for consistency and to prevent future bugs in the test.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   tests/qtest/vhost-user-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index 1a5f5313ff..2db98c4920 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -40,7 +40,7 @@
>   #define QEMU_CMD_MEM    " -m %d -object memory-backend-file,id=mem,size=%dM," \
>                           "mem-path=%s,share=on -numa node,memdev=mem"
>   #define QEMU_CMD_MEMFD  " -m %d -object memory-backend-memfd,id=mem,size=%dM," \
> -                        " -numa node,memdev=mem"
> +                        "share=on -numa node,memdev=mem"

Even if it's not required, it seems to be a good clean up, also with regards 
to the lonely comma at the end of the previous line.

Acked-by: Thomas Huth <thuth@redhat.com>

I assume this will go through the vhost tree, or do you want me to take this 
single patch through my qtest tree?

On Mon, Mar 08, 2021 at 07:31:25AM +0100, Thomas Huth wrote:
> On 22/02/2021 17.10, Stefan Hajnoczi wrote:
> > For some reason memfd never used share=on. vhost-user relies on
> > mmap(MAP_SHARED) so this seems like a problem, but the tests still run
> > without it.
> > 
> > Add share=on for consistency and to prevent future bugs in the test.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   tests/qtest/vhost-user-test.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> > index 1a5f5313ff..2db98c4920 100644
> > --- a/tests/qtest/vhost-user-test.c
> > +++ b/tests/qtest/vhost-user-test.c
> > @@ -40,7 +40,7 @@
> >   #define QEMU_CMD_MEM    " -m %d -object memory-backend-file,id=mem,size=%dM," \
> >                           "mem-path=%s,share=on -numa node,memdev=mem"
> >   #define QEMU_CMD_MEMFD  " -m %d -object memory-backend-memfd,id=mem,size=%dM," \
> > -                        " -numa node,memdev=mem"
> > +                        "share=on -numa node,memdev=mem"
> 
> Even if it's not required, it seems to be a good clean up, also with regards
> to the lonely comma at the end of the previous line.
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 
> I assume this will go through the vhost tree, or do you want me to take this
> single patch through my qtest tree?

I think this entire series will go through the vhost tree.

Thanks,
Stefan