diff mbox series

[2/2] libvhost-user: replace qemu/memfd.h usage

Message ID 20201118080902.30033-3-marcandre.lureau@redhat.com
State New
Headers show
Series libvhost-user: lower dependency on QEMU headers | expand

Commit Message

Marc-André Lureau Nov. 18, 2020, 8:09 a.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Undo the damage from commit 5f9ff1eff3 ("libvhost-user: Support tracking
inflight I/O in shared memory") which introduced glib dependency through
osdep.h inclusion.

libvhost-user.c tries to stay free from glib usage.

Use glibc memfd_create directly when available (assumed so when
MFD_ALLOW_SEALING is defined).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/libvhost-user/libvhost-user.c | 50 +++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 7 deletions(-)

Comments

Dr. David Alan Gilbert Nov. 24, 2020, 11:54 a.m. UTC | #1 
* marcandre.lureau@redhat.com (marcandre.lureau@redhat.com) wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Undo the damage from commit 5f9ff1eff3 ("libvhost-user: Support tracking
> inflight I/O in shared memory") which introduced glib dependency through
> osdep.h inclusion.
> 
> libvhost-user.c tries to stay free from glib usage.
> 
> Use glibc memfd_create directly when available (assumed so when
> MFD_ALLOW_SEALING is defined).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  contrib/libvhost-user/libvhost-user.c | 50 +++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> index 1c1cfbf1e7..805521859d 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -42,8 +42,6 @@
>  #endif
>  
>  #include "qemu/atomic.h"
> -#include "qemu/osdep.h"
> -#include "qemu/memfd.h"
>  
>  #include "libvhost-user.h"
>  
> @@ -1615,11 +1613,45 @@ vu_inflight_queue_size(uint16_t queue_size)
>             sizeof(uint16_t), INFLIGHT_ALIGNMENT);
>  }
>  
> +#ifdef MFD_ALLOW_SEALING
> +static void *
> +memfd_alloc(const char *name, size_t size, unsigned int flags, int *fd)
> +{
> +    void *ptr;
> +    int ret;
> +
> +    *fd = memfd_create(name, MFD_ALLOW_SEALING);
> +    if (*fd < 0) {
> +        return NULL;
> +    }
> +
> +    ret = ftruncate(*fd, size);

Do you need to do any of the page alignment?

> +    if (ret < 0) {
> +        close(*fd);
> +        return NULL;
> +    }
> +
> +    ret = fcntl(*fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL);

I think you'd intended to use the 'flags' parameter there.

> +    if (ret < 0) {
> +        close(*fd);
> +        return NULL;
> +    }
> +
> +    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, *fd, 0);
> +    if (ptr == MAP_FAILED) {
> +        close(*fd);
> +        return NULL;
> +    }
> +
> +    return ptr;
> +}
> +#endif
> +
>  static bool
>  vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
>  {
> -    int fd;
> -    void *addr;
> +    int fd = -1;
> +    void *addr = NULL;
>      uint64_t mmap_size;
>      uint16_t num_queues, queue_size;
>  
> @@ -1637,9 +1669,13 @@ vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
>  
>      mmap_size = vu_inflight_queue_size(queue_size) * num_queues;
>  
> -    addr = qemu_memfd_alloc("vhost-inflight", mmap_size,
> -                            F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> -                            &fd, NULL);
> +#ifdef MFD_ALLOW_SEALING
> +    addr = memfd_alloc("vhost-inflight", mmap_size,
> +                       F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> +                       &fd);
> +#else
> +    vu_panic(dev, "Not implemented: memfd support is missing");

Should there be an ifdef somewhere on the declared features, so it
doesn't get this far because it wont negotiate the feature?

Dave

> +#endif
>  
>      if (!addr) {
>          vu_panic(dev, "Failed to alloc vhost inflight area");
> -- 
> 2.29.0
>
Marc-André Lureau Nov. 24, 2020, 1:32 p.m. UTC | #2 
Hi

On Tue, Nov 24, 2020 at 3:54 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * marcandre.lureau@redhat.com (marcandre.lureau@redhat.com) wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Undo the damage from commit 5f9ff1eff3 ("libvhost-user: Support tracking
> > inflight I/O in shared memory") which introduced glib dependency through
> > osdep.h inclusion.
> >
> > libvhost-user.c tries to stay free from glib usage.
> >
> > Use glibc memfd_create directly when available (assumed so when
> > MFD_ALLOW_SEALING is defined).
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  contrib/libvhost-user/libvhost-user.c | 50 +++++++++++++++++++++++----
> >  1 file changed, 43 insertions(+), 7 deletions(-)
> >
> > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> > index 1c1cfbf1e7..805521859d 100644
> > --- a/contrib/libvhost-user/libvhost-user.c
> > +++ b/contrib/libvhost-user/libvhost-user.c
> > @@ -42,8 +42,6 @@
> >  #endif
> >
> >  #include "qemu/atomic.h"
> > -#include "qemu/osdep.h"
> > -#include "qemu/memfd.h"
> >
> >  #include "libvhost-user.h"
> >
> > @@ -1615,11 +1613,45 @@ vu_inflight_queue_size(uint16_t queue_size)
> >             sizeof(uint16_t), INFLIGHT_ALIGNMENT);
> >  }
> >
> > +#ifdef MFD_ALLOW_SEALING
> > +static void *
> > +memfd_alloc(const char *name, size_t size, unsigned int flags, int *fd)
> > +{
> > +    void *ptr;
> > +    int ret;
> > +
> > +    *fd = memfd_create(name, MFD_ALLOW_SEALING);
> > +    if (*fd < 0) {
> > +        return NULL;
> > +    }
> > +
> > +    ret = ftruncate(*fd, size);
>
> Do you need to do any of the page alignment?

We don't do any in util/memfd.c, I don't see an explicit requirement
in memfd_create(). (however, util/memfd.c did check power of 2 for
hugetlb usage, but this isn't necessary here)

On mmap(), "if addr is NULL, then the kernel chooses the (page-aligned) address"

>
> > +    if (ret < 0) {
> > +        close(*fd);
> > +        return NULL;
> > +    }
> > +
> > +    ret = fcntl(*fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL);
>
> I think you'd intended to use the 'flags' parameter there.

indeed, thanks

>
> > +    if (ret < 0) {
> > +        close(*fd);
> > +        return NULL;
> > +    }
> > +
> > +    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, *fd, 0);
> > +    if (ptr == MAP_FAILED) {
> > +        close(*fd);
> > +        return NULL;
> > +    }
> > +
> > +    return ptr;
> > +}
> > +#endif
> > +
> >  static bool
> >  vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> >  {
> > -    int fd;
> > -    void *addr;
> > +    int fd = -1;
> > +    void *addr = NULL;
> >      uint64_t mmap_size;
> >      uint16_t num_queues, queue_size;
> >
> > @@ -1637,9 +1669,13 @@ vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> >
> >      mmap_size = vu_inflight_queue_size(queue_size) * num_queues;
> >
> > -    addr = qemu_memfd_alloc("vhost-inflight", mmap_size,
> > -                            F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > -                            &fd, NULL);
> > +#ifdef MFD_ALLOW_SEALING
> > +    addr = memfd_alloc("vhost-inflight", mmap_size,
> > +                       F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > +                       &fd);
> > +#else
> > +    vu_panic(dev, "Not implemented: memfd support is missing");
>
> Should there be an ifdef somewhere on the declared features, so it
> doesn't get this far because it wont negotiate the feature?

Sealing grow/shrink came together with memfd, it was one of the main
selling point. I assume if MFD_ALLOW_SEALING is defined, we have
memfd_create and basic libc defines. But yes, if we want to handle
weird corner cases, we should add more ifdef-ry. I'd pass for now.

thanks

>
> Dave
>
> > +#endif
> >
> >      if (!addr) {
> >          vu_panic(dev, "Failed to alloc vhost inflight area");
> > --
> > 2.29.0
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
Marc-André Lureau Nov. 24, 2020, 1:35 p.m. UTC | #3 
Hi

On Tue, Nov 24, 2020 at 5:32 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Tue, Nov 24, 2020 at 3:54 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * marcandre.lureau@redhat.com (marcandre.lureau@redhat.com) wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Undo the damage from commit 5f9ff1eff3 ("libvhost-user: Support tracking
> > > inflight I/O in shared memory") which introduced glib dependency through
> > > osdep.h inclusion.
> > >
> > > libvhost-user.c tries to stay free from glib usage.
> > >
> > > Use glibc memfd_create directly when available (assumed so when
> > > MFD_ALLOW_SEALING is defined).
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  contrib/libvhost-user/libvhost-user.c | 50 +++++++++++++++++++++++----
> > >  1 file changed, 43 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> > > index 1c1cfbf1e7..805521859d 100644
> > > --- a/contrib/libvhost-user/libvhost-user.c
> > > +++ b/contrib/libvhost-user/libvhost-user.c
> > > @@ -42,8 +42,6 @@
> > >  #endif
> > >
> > >  #include "qemu/atomic.h"
> > > -#include "qemu/osdep.h"
> > > -#include "qemu/memfd.h"
> > >
> > >  #include "libvhost-user.h"
> > >
> > > @@ -1615,11 +1613,45 @@ vu_inflight_queue_size(uint16_t queue_size)
> > >             sizeof(uint16_t), INFLIGHT_ALIGNMENT);
> > >  }
> > >
> > > +#ifdef MFD_ALLOW_SEALING
> > > +static void *
> > > +memfd_alloc(const char *name, size_t size, unsigned int flags, int *fd)
> > > +{
> > > +    void *ptr;
> > > +    int ret;
> > > +
> > > +    *fd = memfd_create(name, MFD_ALLOW_SEALING);
> > > +    if (*fd < 0) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    ret = ftruncate(*fd, size);
> >
> > Do you need to do any of the page alignment?
>
> We don't do any in util/memfd.c, I don't see an explicit requirement
> in memfd_create(). (however, util/memfd.c did check power of 2 for
> hugetlb usage, but this isn't necessary here)
>
> On mmap(), "if addr is NULL, then the kernel chooses the (page-aligned) address"
>
> >
> > > +    if (ret < 0) {
> > > +        close(*fd);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    ret = fcntl(*fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL);
> >
> > I think you'd intended to use the 'flags' parameter there.
>
> indeed, thanks
>
> >
> > > +    if (ret < 0) {
> > > +        close(*fd);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, *fd, 0);
> > > +    if (ptr == MAP_FAILED) {
> > > +        close(*fd);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    return ptr;
> > > +}
> > > +#endif
> > > +
> > >  static bool
> > >  vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> > >  {
> > > -    int fd;
> > > -    void *addr;
> > > +    int fd = -1;
> > > +    void *addr = NULL;
> > >      uint64_t mmap_size;
> > >      uint16_t num_queues, queue_size;
> > >
> > > @@ -1637,9 +1669,13 @@ vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> > >
> > >      mmap_size = vu_inflight_queue_size(queue_size) * num_queues;
> > >
> > > -    addr = qemu_memfd_alloc("vhost-inflight", mmap_size,
> > > -                            F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > > -                            &fd, NULL);
> > > +#ifdef MFD_ALLOW_SEALING
> > > +    addr = memfd_alloc("vhost-inflight", mmap_size,
> > > +                       F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > > +                       &fd);
> > > +#else
> > > +    vu_panic(dev, "Not implemented: memfd support is missing");
> >
> > Should there be an ifdef somewhere on the declared features, so it
> > doesn't get this far because it wont negotiate the feature?
>
> Sealing grow/shrink came together with memfd, it was one of the main
> selling point. I assume if MFD_ALLOW_SEALING is defined, we have
> memfd_create and basic libc defines. But yes, if we want to handle
> weird corner cases, we should add more ifdef-ry. I'd pass for now.


However, let's avoid compiling code that can panic. I am updating the
series with a standalone meson subproject.
Dr. David Alan Gilbert Nov. 24, 2020, 2:39 p.m. UTC | #4 
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Hi
> 
> On Tue, Nov 24, 2020 at 3:54 PM Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * marcandre.lureau@redhat.com (marcandre.lureau@redhat.com) wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Undo the damage from commit 5f9ff1eff3 ("libvhost-user: Support tracking
> > > inflight I/O in shared memory") which introduced glib dependency through
> > > osdep.h inclusion.
> > >
> > > libvhost-user.c tries to stay free from glib usage.
> > >
> > > Use glibc memfd_create directly when available (assumed so when
> > > MFD_ALLOW_SEALING is defined).
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  contrib/libvhost-user/libvhost-user.c | 50 +++++++++++++++++++++++----
> > >  1 file changed, 43 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> > > index 1c1cfbf1e7..805521859d 100644
> > > --- a/contrib/libvhost-user/libvhost-user.c
> > > +++ b/contrib/libvhost-user/libvhost-user.c
> > > @@ -42,8 +42,6 @@
> > >  #endif
> > >
> > >  #include "qemu/atomic.h"
> > > -#include "qemu/osdep.h"
> > > -#include "qemu/memfd.h"
> > >
> > >  #include "libvhost-user.h"
> > >
> > > @@ -1615,11 +1613,45 @@ vu_inflight_queue_size(uint16_t queue_size)
> > >             sizeof(uint16_t), INFLIGHT_ALIGNMENT);
> > >  }
> > >
> > > +#ifdef MFD_ALLOW_SEALING
> > > +static void *
> > > +memfd_alloc(const char *name, size_t size, unsigned int flags, int *fd)
> > > +{
> > > +    void *ptr;
> > > +    int ret;
> > > +
> > > +    *fd = memfd_create(name, MFD_ALLOW_SEALING);
> > > +    if (*fd < 0) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    ret = ftruncate(*fd, size);
> >
> > Do you need to do any of the page alignment?
> 
> We don't do any in util/memfd.c, I don't see an explicit requirement
> in memfd_create(). (however, util/memfd.c did check power of 2 for
> hugetlb usage, but this isn't necessary here)
> 
> On mmap(), "if addr is NULL, then the kernel chooses the (page-aligned) address"
> 
> >
> > > +    if (ret < 0) {
> > > +        close(*fd);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    ret = fcntl(*fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL);
> >
> > I think you'd intended to use the 'flags' parameter there.
> 
> indeed, thanks
> 
> >
> > > +    if (ret < 0) {
> > > +        close(*fd);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, *fd, 0);
> > > +    if (ptr == MAP_FAILED) {
> > > +        close(*fd);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    return ptr;
> > > +}
> > > +#endif
> > > +
> > >  static bool
> > >  vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> > >  {
> > > -    int fd;
> > > -    void *addr;
> > > +    int fd = -1;
> > > +    void *addr = NULL;
> > >      uint64_t mmap_size;
> > >      uint16_t num_queues, queue_size;
> > >
> > > @@ -1637,9 +1669,13 @@ vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
> > >
> > >      mmap_size = vu_inflight_queue_size(queue_size) * num_queues;
> > >
> > > -    addr = qemu_memfd_alloc("vhost-inflight", mmap_size,
> > > -                            F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > > -                            &fd, NULL);
> > > +#ifdef MFD_ALLOW_SEALING
> > > +    addr = memfd_alloc("vhost-inflight", mmap_size,
> > > +                       F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
> > > +                       &fd);
> > > +#else
> > > +    vu_panic(dev, "Not implemented: memfd support is missing");
> >
> > Should there be an ifdef somewhere on the declared features, so it
> > doesn't get this far because it wont negotiate the feature?
> 
> Sealing grow/shrink came together with memfd, it was one of the main
> selling point. I assume if MFD_ALLOW_SEALING is defined, we have
> memfd_create and basic libc defines. But yes, if we want to handle
> weird corner cases, we should add more ifdef-ry. I'd pass for now.

I wasn't being that selective; I just ment if any of the memfd
wasn't available then we should just say the vu_inflight feature isn't
available.

Dave

> thanks
> 
> >
> > Dave
> >
> > > +#endif
> > >
> > >      if (!addr) {
> > >          vu_panic(dev, "Failed to alloc vhost inflight area");
> > > --
> > > 2.29.0
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
diff mbox series

Patch

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 1c1cfbf1e7..805521859d 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -42,8 +42,6 @@ 
 #endif
 
 #include "qemu/atomic.h"
-#include "qemu/osdep.h"
-#include "qemu/memfd.h"
 
 #include "libvhost-user.h"
 
@@ -1615,11 +1613,45 @@  vu_inflight_queue_size(uint16_t queue_size)
            sizeof(uint16_t), INFLIGHT_ALIGNMENT);
 }
 
+#ifdef MFD_ALLOW_SEALING
+static void *
+memfd_alloc(const char *name, size_t size, unsigned int flags, int *fd)
+{
+    void *ptr;
+    int ret;
+
+    *fd = memfd_create(name, MFD_ALLOW_SEALING);
+    if (*fd < 0) {
+        return NULL;
+    }
+
+    ret = ftruncate(*fd, size);
+    if (ret < 0) {
+        close(*fd);
+        return NULL;
+    }
+
+    ret = fcntl(*fd, F_ADD_SEALS, F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL);
+    if (ret < 0) {
+        close(*fd);
+        return NULL;
+    }
+
+    ptr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, *fd, 0);
+    if (ptr == MAP_FAILED) {
+        close(*fd);
+        return NULL;
+    }
+
+    return ptr;
+}
+#endif
+
 static bool
 vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
 {
-    int fd;
-    void *addr;
+    int fd = -1;
+    void *addr = NULL;
     uint64_t mmap_size;
     uint16_t num_queues, queue_size;
 
@@ -1637,9 +1669,13 @@  vu_get_inflight_fd(VuDev *dev, VhostUserMsg *vmsg)
 
     mmap_size = vu_inflight_queue_size(queue_size) * num_queues;
 
-    addr = qemu_memfd_alloc("vhost-inflight", mmap_size,
-                            F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
-                            &fd, NULL);
+#ifdef MFD_ALLOW_SEALING
+    addr = memfd_alloc("vhost-inflight", mmap_size,
+                       F_SEAL_GROW | F_SEAL_SHRINK | F_SEAL_SEAL,
+                       &fd);
+#else
+    vu_panic(dev, "Not implemented: memfd support is missing");
+#endif
 
     if (!addr) {
         vu_panic(dev, "Failed to alloc vhost inflight area");