Re: [Qemu-devel] [PATCH for-2.13] Add host_memory_backend_pagesize() helper

From: David Gibson <david@gibson.dropbear.id.au>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: imammedo@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org,
	qemu-ppc@nongnu.org
Subject: Re: [Qemu-devel] [PATCH for-2.13] Add host_memory_backend_pagesize() helper
Date: Tue, 3 Apr 2018 15:29:48 +1000	[thread overview]
Message-ID: <20180403052948.GA1984@umbus.fritz.box> (raw)
In-Reply-To: <20180403022206.GK5046@localhost.localdomain>

[-- Attachment #1: Type: text/plain, Size: 6395 bytes --]

On Mon, Apr 02, 2018 at 11:22:06PM -0300, Eduardo Habkost wrote:
> On Thu, Mar 29, 2018 at 04:25:37PM +1100, David Gibson wrote:
> > There are a couple places (one generic, one target specific) where we need
> > to get the host page size associated with a particular memory backend.  I
> > have some upcoming code which will add another place which wants this.  So,
> > for convenience, add a helper function to calculate this.
> > 
> > host_memory_backend_pagesize() returns the host pagesize for a given
> > HostMemoryBackend object, or for the default backend (-mem-path) if passed
> > NULL.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  backends/hostmem.c       | 20 ++++++++++++++++++++
> >  exec.c                   | 21 +++++----------------
> >  include/sysemu/hostmem.h |  2 ++
> >  target/ppc/kvm.c         | 10 +---------
> >  4 files changed, 28 insertions(+), 25 deletions(-)
> > 
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index f61093654e..b6a60cfc5d 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -18,6 +18,7 @@
> >  #include "qapi/visitor.h"
> >  #include "qemu/config-file.h"
> >  #include "qom/object_interfaces.h"
> > +#include "qemu/mmap-alloc.h"
> >  
> >  #ifdef CONFIG_NUMA
> >  #include <numaif.h>
> > @@ -262,6 +263,25 @@ bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
> >      return backend->is_mapped;
> >  }
> >  
> > +long host_memory_backend_pagesize(HostMemoryBackend *memdev)
> 
> qemu_mempath_getpagesize() returns size_t.  Why are you using
> long here?

Because qemu_getrampagesize() returns long.  But size_t is better, so
I've changed it.

> > +{
> > +    const char *path = NULL;
> > +
> > +#ifdef __linux__
> > +    if (memdev) {
> > +        path = object_property_get_str(OBJECT(memdev), "mem-path", NULL);
> > +    } else {
> > +        path = mem_path;
> > +    }
> > +#endif
> > +
> > +    if (path) {
> > +        return qemu_mempath_getpagesize(path);
> > +    } else {
> > +        return getpagesize();
> 
> Isn't it simpler to make qemu_mempath_getpagesize() handle
> path==NULL?

Uh.. possibly not, but things you bring up later kind of make it moot.

> > +    }
> > +}
> > +
> >  static void
> >  host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
> >  {
> > diff --git a/exec.c b/exec.c
> > index c09bd93df3..04856c2402 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1488,18 +1488,13 @@ void ram_block_dump(Monitor *mon)
> >   */
> >  static int find_max_supported_pagesize(Object *obj, void *opaque)
> >  {
> > -    char *mem_path;
> >      long *hpsize_min = opaque;
> >  
> >      if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
> > -        mem_path = object_property_get_str(obj, "mem-path", NULL);
> > -        if (mem_path) {
> > -            long hpsize = qemu_mempath_getpagesize(mem_path);
> > -            if (hpsize < *hpsize_min) {
> > -                *hpsize_min = hpsize;
> > -            }
> > -        } else {
> > -            *hpsize_min = getpagesize();
> > +        long hpsize = host_memory_backend_pagesize(MEMORY_BACKEND(obj));
> 
> So in this caller, `backend` is always non-NULL...
> 
> > +
> > +        if (hpsize < *hpsize_min) {
> > +            *hpsize_min = hpsize;
> >          }
> >      }
> >  
> > @@ -1509,15 +1504,9 @@ static int find_max_supported_pagesize(Object *obj, void *opaque)
> >  long qemu_getrampagesize(void)
> >  {
> >      long hpsize = LONG_MAX;
> > -    long mainrampagesize;
> > +    long mainrampagesize = host_memory_backend_pagesize(NULL);
> 
> ...on this caller `backend` is always NULL...
> 
> 
> >      Object *memdev_root;
> >  
> > -    if (mem_path) {
> > -        mainrampagesize = qemu_mempath_getpagesize(mem_path);
> > -    } else {
> > -        mainrampagesize = getpagesize();
> > -    }
> > -
> >      /* it's possible we have memory-backend objects with
> >       * hugepage-backed RAM. these may get mapped into system
> >       * address space via -numa parameters or memory hotplug
> [...]
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index b329cd8173..0adcf18c9f 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -493,15 +493,7 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >  bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
> >  {
> >      Object *mem_obj = object_resolve_path(obj_path, NULL);
> > -    char *mempath = object_property_get_str(mem_obj, "mem-path", NULL);
> > -    long pagesize;
> > -
> > -    if (mempath) {
> > -        pagesize = qemu_mempath_getpagesize(mempath);
> > -        g_free(mempath);
> > -    } else {
> > -        pagesize = getpagesize();
> > -    }
> > +    long pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(mem_obj));
> >  
> 
> ...and here `backend` is always non-NULL.
> 
> If there's no caller that relies on both code paths, why overload
> a single function for two different operations?

Good point.  

> I would simply make qemu_mempath_getpagesize() work if path is
> NULL, use qemu_mempath_getpagesize(mem_path) at
> qemu_getrampagesize(), and make host_memory_backend_pagesize()
> assume require a non-NULL `memdev`.

Yeah, that makes sense.  I thought I might have more need for a
unified entry point in the code I have in the works, but looking at
the latest draft, probably not.

I'll change it as you suggest.

> > +long host_memory_backend_pagesize(HostMemoryBackend *memdev)
> 
> Why is the code duplicated in target/ppc/kvm.c?

Leftover from an early version, I think.  I'll fix that.

> 
> 
> > +{
> > +    const char *path = NULL;
> > +
> > +#ifdef __linux__
> > +    if (memdev) {
> > +        path = object_property_get_str(OBJECT(memdev), "mem-path", NULL);
> > +    } else {
> > +        path = mem_path;
> > +    }
> > +#endif
> > +
> > +    if (path) {
> > +        return qemu_mempath_getpagesize(path);
> > +    } else {
> > +        return getpagesize();
> > +    }
> > +}
> > +
> >      return pagesize >= max_cpu_page_size;
> >  }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]