[Qemu-devel] [PATCH] hostmem: no need to check for host_memory_backend_mr_inited() in alloc()

[Qemu-devel] [PATCH] hostmem: no need to check for host_memory_backend_mr_inited() in alloc()

[Marc-André Lureau]

memfd_backend_memory_alloc/file_backend_memory_alloc both needlessly
are are calling host_memory_backend_mr_inited() which creates an
illusion that alloc could be called multiple times but it isn't, it's
called once from UserCreatable complete().

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 backends/hostmem-file.c  | 2 +-
 backends/hostmem-memfd.c | 4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 2476dcb435..5cd5fa75a7 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -54,7 +54,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 #ifndef CONFIG_LINUX
     error_setg(errp, "-mem-path not supported on this host");
 #else
-    if (!host_memory_backend_mr_inited(backend)) {
+    {
         gchar *path;
         backend->force_prealloc = mem_prealloc;
         path = object_get_canonical_path(OBJECT(backend));
diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
index abd52364db..7184918112 100644
--- a/backends/hostmem-memfd.c
+++ b/backends/hostmem-memfd.c
@@ -44,10 +44,6 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         return;
     }
 
-    if (host_memory_backend_mr_inited(backend)) {
-        return;
-    }
-
     backend->force_prealloc = mem_prealloc;
     fd = qemu_memfd_create(TYPE_MEMORY_BACKEND_MEMFD, backend->size,
                            m->hugetlb, m->hugetlbsize, m->seal ?
-- 
2.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [PATCH] hostmem: no need to check for host_memory_backend_mr_inited() in alloc()

[Eduardo Habkost]

On Thu, Aug 30, 2018 at 07:50:19PM +0200, Marc-André Lureau wrote:
> memfd_backend_memory_alloc/file_backend_memory_alloc both needlessly
> are are calling host_memory_backend_mr_inited() which creates an
> illusion that alloc could be called multiple times but it isn't, it's
> called once from UserCreatable complete().
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

If calling the function multiple times is a mistake, should we
replace the checks with:
  assert(!host_memory_backend_mr_inited(backend));
?

> ---
>  backends/hostmem-file.c  | 2 +-
>  backends/hostmem-memfd.c | 4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 2476dcb435..5cd5fa75a7 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -54,7 +54,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>  #ifndef CONFIG_LINUX
>      error_setg(errp, "-mem-path not supported on this host");
>  #else
> -    if (!host_memory_backend_mr_inited(backend)) {
> +    {
>          gchar *path;
>          backend->force_prealloc = mem_prealloc;
>          path = object_get_canonical_path(OBJECT(backend));
> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> index abd52364db..7184918112 100644
> --- a/backends/hostmem-memfd.c
> +++ b/backends/hostmem-memfd.c
> @@ -44,10 +44,6 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>          return;
>      }
>  
> -    if (host_memory_backend_mr_inited(backend)) {
> -        return;
> -    }
> -
>      backend->force_prealloc = mem_prealloc;
>      fd = qemu_memfd_create(TYPE_MEMORY_BACKEND_MEMFD, backend->size,
>                             m->hugetlb, m->hugetlbsize, m->seal ?
> -- 
> 2.19.0.rc0.48.gb9dfa238d5
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hostmem: no need to check for host_memory_backend_mr_inited() in alloc()

[Igor Mammedov]

On Thu, 30 Aug 2018 16:09:58 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Aug 30, 2018 at 07:50:19PM +0200, Marc-André Lureau wrote:
> > memfd_backend_memory_alloc/file_backend_memory_alloc both needlessly
> > are are calling host_memory_backend_mr_inited() which creates an
> > illusion that alloc could be called multiple times but it isn't, it's
> > called once from UserCreatable complete().

could you extend this patch to cover file_backend_memory_alloc() as well?
Otherwise this logic would be copy-pasted again in the future.

> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>  
> 
> If calling the function multiple times is a mistake, should we
> replace the checks with:
>   assert(!host_memory_backend_mr_inited(backend));
> ?
it is confusing calling it where it's not necessary, hence this cleanup.
Considering that HostMemoryBackendClass::alloc is internal bussines
of hostmem backends, assert() would just add clutter.



> > ---
> >  backends/hostmem-file.c  | 2 +-
> >  backends/hostmem-memfd.c | 4 ----
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index 2476dcb435..5cd5fa75a7 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -54,7 +54,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >  #ifndef CONFIG_LINUX
> >      error_setg(errp, "-mem-path not supported on this host");
> >  #else
> > -    if (!host_memory_backend_mr_inited(backend)) {
> > +    {
> >          gchar *path;
> >          backend->force_prealloc = mem_prealloc;
> >          path = object_get_canonical_path(OBJECT(backend));
> > diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> > index abd52364db..7184918112 100644
> > --- a/backends/hostmem-memfd.c
> > +++ b/backends/hostmem-memfd.c
> > @@ -44,10 +44,6 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >          return;
> >      }
> >  
> > -    if (host_memory_backend_mr_inited(backend)) {
> > -        return;
> > -    }
> > -
> >      backend->force_prealloc = mem_prealloc;
> >      fd = qemu_memfd_create(TYPE_MEMORY_BACKEND_MEMFD, backend->size,
> >                             m->hugetlb, m->hugetlbsize, m->seal ?
> > -- 
> > 2.19.0.rc0.48.gb9dfa238d5
> >   
>