Re: [Qemu-devel] [PATCH v18 13/14] memory backend: fill memory backend ram fields

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 26 Feb 2014 13:57:06 +0800
Hu Tao <hutao@cn.fujitsu.com> wrote:

> On Wed, Feb 19, 2014 at 10:36:57AM +0100, Igor Mammedov wrote:
> > On Wed, 19 Feb 2014 10:03:13 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > >   19/02/2014 08:54, Hu Tao ha scritto:
> > > > Thus makes user control how to allocate memory for ram backend.
> > > >
> > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > ---
> > > >  backends/hostmem-ram.c  | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/sysemu/sysemu.h |   2 +
> > > >  2 files changed, 160 insertions(+)
> > > >
> > > > diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> > [...]
> > 
> > > >  static int
> > > >  ram_backend_memory_init(HostMemoryBackend *backend, Error **errp)
> > > >  {
> > > > +    HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(backend);
> > > > +    int mode = ram_backend->policy;
> > > > +    void *p;
> > > > +    unsigned long maxnode;
> > > > +
> > > >      if (!memory_region_size(&backend->mr)) {
> > > >          memory_region_init_ram(&backend->mr, OBJECT(backend),
> > > >                                 object_get_canonical_path(OBJECT(backend)),
> > > >                                 backend->size);
> > > > +
> > > > +        p = memory_region_get_ram_ptr(&backend->mr);
> > > > +        maxnode = find_last_bit(ram_backend->host_nodes, MAX_NODES);
> > > > +
> > > > +        mode |= ram_backend->relative ? MPOL_F_RELATIVE_NODES :
> > > > +            MPOL_F_STATIC_NODES;
> > > > +        /* This is a workaround for a long standing bug in Linux'
> > > > +         * mbind implementation, which cuts off the last specified
> > > > +         * node. To stay compatible should this bug be fixed, we
> > > > +         * specify one more node and zero this one out.
> > > > +         */
> > > > +        if (syscall(SYS_mbind, p, backend->size, mode,
> > > > +                    ram_backend->host_nodes, maxnode + 2, 0)) {
> > > 
> > > This does not compile on non-Linux; also, does libnuma include the 
> > > workaround?  If so, this is a hint that we should be using libnuma 
> > > instead...
> > > 
> > > Finally, all this code should be in hostmem.c, not hostmem-ram.c, 
> > > because the same policies can be applied to hugepage-backed memory.
> > > 
> > > Currently host_memory_backend_get_memory is calling bc->memory_init. 
> > > Probably the call should be replaced by something like
> > I've pushed to github updated version of memdev, where
> > host_memory_backend_get_memory() is just convenience wrapper to get
> > access to memdev's internal MemoryRegion.
> > 
> > All initialization now is done in user_creatable->complete() method
> > which calls ram_backend_memory_init() so leaving it as is should be fine.
> 
> If lines about memory polices are moved up to hostmem.c, the only thing
> left in ram_backend_memory_init() is calling memory_region_init_ram() to
> allocate memory. Then it comes a problem that when to apply memory
> polices? Choices:
> 
> 1. apply memory polices in hostmem.c since this is the place user sets
>    memory polices. But user_creatable_complete() seems not to support
>    this.( but fix me)
if we assume that NUMA policies apply to every hostmem derived backend,
then we could realize() approach used by DEVICE. i.e.
set NUMA policies in hostmem.c:hostmemory_backend_memory_init()

Add parent_complete field to ram-backend class and store there parent's
complete pointer. Then we can do:

ram_backend_memory_init(UserCreatable *uc, Error **errp) {
    memory_region_init_ram();
    ...
    MEMORY_BACKEND_RAM_CLASS(uc)->parent_complete(uc, errp);
    ...
}

> 
> 2. cast to HostMemoryBackend in ram_backend_memory_init() (or in other
>    memory backends) and add lines to apply memory polices.
> 
> 3. provide an interface in HostMemoryBackendClass to do the thing and
>    call it in subclasses. (this is basically the same as 2 except that
>    we can reuse code)
> 
> Opinions?
> 
>