Re: [Qemu-devel] [PATCH v3 05/34] add memdev backend infrastructure

[Hu Tao]

On Mon, Jun 09, 2014 at 06:54:54PM +1000, Peter Crosthwaite wrote:
> On Mon, Jun 9, 2014 at 12:44 PM, Hu Tao <address@hidden> wrote:
> > On Fri, Jun 06, 2014 at 02:48:15PM +0200, Igor Mammedov wrote:
> >> On Fri, 6 Jun 2014 17:29:38 +0800
> >> Hu Tao <address@hidden> wrote:
> >>
> >> > On Fri, May 30, 2014 at 09:59:55AM +0200, Igor Mammedov wrote:
> >> > > On Fri, 30 May 2014 00:05:51 +1000
> >> > > Peter Crosthwaite <address@hidden> wrote:
> >> > >
> >> > > > On Tue, May 27, 2014 at 11:01 PM, Igor Mammedov <address@hidden> 
> >> > > > wrote:
> >> > > > > Provides framework for splitting host RAM allocation/
> >> > > > > policies into a separate backend that could be used
> >> > > > > by devices.
> >> > > > >
> >> > > > > Initially only legacy RAM backend is provided, which
> >> > > > > uses memory_region_init_ram() allocator and compatible
> >> > > > > with every CLI option that affects memory_region_init_ram().
> >> > > > >
> >> > > > > Signed-off-by: Igor Mammedov <address@hidden>
> >> > > > > Signed-off-by: Paolo Bonzini <address@hidden>
> >> > > > > ---
> >> > > > > v4:
> >> > > > >  - don't use nonexisting anymore error_is_set()
> >> > > > > v3:
> >> > > > >  - fix path leak & use object_get_canonical_path_component()
> >> > > > >    for getting object name
> >> > > > > v2:
> >> > > > >  - reuse UserCreatable interface instead of custom callbacks
> >> > > > > ---
> >> > > > >  backends/Makefile.objs   |    2 +
> >> > > > >  backends/hostmem-ram.c   |   54 ++++++++++++++++++++++
> >> > > > >  backends/hostmem.c       |  113 
> >> > > > > ++++++++++++++++++++++++++++++++++++++++++++++
> >> > > > >  include/sysemu/hostmem.h |   60 ++++++++++++++++++++++++
> >> > > > >  4 files changed, 229 insertions(+), 0 deletions(-)
> >> > > > >  create mode 100644 backends/hostmem-ram.c
> >> > > > >  create mode 100644 backends/hostmem.c
> >> > > > >  create mode 100644 include/sysemu/hostmem.h
> >> > > > >
> >> > > > > diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> >> > > > > index 591ddcf..7fb7acd 100644
> >> > > > > --- a/backends/Makefile.objs
> >> > > > > +++ b/backends/Makefile.objs
> >> > > > > @@ -6,3 +6,5 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
> >> > > > >  baum.o-cflags := $(SDL_CFLAGS)
> >> > > > >
> >> > > > >  common-obj-$(CONFIG_TPM) += tpm.o
> >> > > > > +
> >> > > > > +common-obj-y += hostmem.o hostmem-ram.o
> >> > > > > diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> >> > > > > new file mode 100644
> >> > > > > index 0000000..cbf7e5a
> >> > > > > --- /dev/null
> >> > > > > +++ b/backends/hostmem-ram.c
> >> > > > > @@ -0,0 +1,54 @@
> >> > > > > +/*
> >> > > > > + * QEMU Host Memory Backend
> >> > > > > + *
> >> > > > > + * Copyright (C) 2013 Red Hat Inc
> >> > > > > + *
> >> > > > > + * Authors:
> >> > > > > + *   Igor Mammedov <address@hidden>
> >> > > > > + *
> >> > > > > + * This work is licensed under the terms of the GNU GPL, version 
> >> > > > > 2 or later.
> >> > > > > + * See the COPYING file in the top-level directory.
> >> > > > > + */
> >> > > > > +#include "sysemu/hostmem.h"
> >> > > > > +#include "qom/object_interfaces.h"
> >> > > > > +
> >> > > > > +#define TYPE_MEMORY_BACKEND_RAM "memory-ram"
> >> > > > > +
> >> > > > > +
> >> > > > > +static void
> >> > > > > +ram_backend_memory_init(UserCreatable *uc, Error **errp)
> >> > > > > +{
> >> > > > > +    HostMemoryBackend *backend = MEMORY_BACKEND(uc);
> >> > > > > +    char *path;
> >> > > > > +
> >> > > > > +    if (!backend->size) {
> >> > > > > +        error_setg(errp, "can't create backend with size 0");
> >> > > > > +        return;
> >> > > > > +    }
> >> > > > > +
> >> > > > > +    path = object_get_canonical_path_component(OBJECT(backend));
> >> > > > > +    memory_region_init_ram(&backend->mr, OBJECT(backend), path,
> >> > > >
> >> > > > Passing the full canonical path as the name of memory region is
> >> > > > redundant as that information is already passed via the owner
> >> > > > argument. It should just be a shorthand.
> >> > > >
> >> > > > > +                           backend->size);
> >> > > > > +    g_free(path);
> >> > > > > +}
> >> > > > > +
> >> > > > > +static void
> >> > > > > +ram_backend_class_init(ObjectClass *oc, void *data)
> >> > > > > +{
> >> > > > > +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> >> > > > > +
> >> > > > > +    ucc->complete = ram_backend_memory_init;
> >> > > > > +}
> >> > > > > +
> >> > > > > +static const TypeInfo ram_backend_info = {
> >> > > > > +    .name = TYPE_MEMORY_BACKEND_RAM,
> >> > > > > +    .parent = TYPE_MEMORY_BACKEND,
> >> > > > > +    .class_init = ram_backend_class_init,
> >> > > > > +};
> >> > > > > +
> >> > > > > +static void register_types(void)
> >> > > > > +{
> >> > > > > +    type_register_static(&ram_backend_info);
> >> > > > > +}
> >> > > > > +
> >> > > > > +type_init(register_types);
> >> > > > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> >> > > > > new file mode 100644
> >> > > > > index 0000000..8a34b0f
> >> > > > > --- /dev/null
> >> > > > > +++ b/backends/hostmem.c
> >> > > > > @@ -0,0 +1,113 @@
> >> > > > > +/*
> >> > > > > + * QEMU Host Memory Backend
> >> > > > > + *
> >> > > > > + * Copyright (C) 2013 Red Hat Inc
> >> > > > > + *
> >> > > > > + * Authors:
> >> > > > > + *   Igor Mammedov <address@hidden>
> >> > > > > + *
> >> > > > > + * This work is licensed under the terms of the GNU GPL, version 
> >> > > > > 2 or later.
> >> > > > > + * See the COPYING file in the top-level directory.
> >> > > > > + */
> >> > > > > +#include "sysemu/hostmem.h"
> >> > > > > +#include "sysemu/sysemu.h"
> >> > > > > +#include "qapi/visitor.h"
> >> > > > > +#include "qapi/qmp/qerror.h"
> >> > > > > +#include "qemu/config-file.h"
> >> > > > > +#include "qom/object_interfaces.h"
> >> > > > > +
> >> > > > > +static void
> >> > > > > +hostmemory_backend_get_size(Object *obj, Visitor *v, void *opaque,
> >> > > > > +                            const char *name, Error **errp)
> >> > > > > +{
> >> > > > > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> >> > > > > +    uint64_t value = backend->size;
> >> > > > > +
> >> > > > > +    visit_type_size(v, &value, name, errp);
> >> > > > > +}
> >> > > > > +
> >> > > > > +static void
> >> > > > > +hostmemory_backend_set_size(Object *obj, Visitor *v, void *opaque,
> >> > > > > +                            const char *name, Error **errp)
> >> > > > > +{
> >> > > > > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> >> > > > > +    Error *local_err = NULL;
> >> > > > > +    uint64_t value;
> >> > > > > +
> >> > > > > +    if (memory_region_size(&backend->mr)) {
> >> > > > > +        error_setg(&local_err, "cannot change property value\n");
> >> > > > > +        goto out;
> >> > > > > +    }
> >> > > > > +
> >> > > > > +    visit_type_size(v, &value, name, errp);
> >> > > > > +    if (local_err) {
> >> > > > > +        goto out;
> >> > > > > +    }
> >> > > > > +    if (!value) {
> >> > > > > +        error_setg(&local_err, "Property '%s.%s' doesn't take 
> >> > > > > value '%"
> >> > > > > +                   PRIu64 "'", object_get_typename(obj), name , 
> >> > > > > value);
> >> > > > > +        goto out;
> >> > > > > +    }
> >> > > > > +    backend->size = value;
> >> > > > > +out:
> >> > > > > +    error_propagate(errp, local_err);
> >> > > > > +}
> >> > > > > +
> >> > > > > +static void hostmemory_backend_initfn(Object *obj)
> >> > > >
> >> > > > can you just call this _init and ..
> >> > > >
> >> > > > > +{
> >> > > > > +    object_property_add(obj, "size", "int",
> >> > > > > +                        hostmemory_backend_get_size,
> >> > > > > +                        hostmemory_backend_set_size, NULL, NULL, 
> >> > > > > NULL);
> >> > > > > +}
> >> > > > > +
> >> > > > > +static void hostmemory_backend_finalize(Object *obj)
> >> > > > > +{
> >> > > > > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> >> > > > > +
> >> > > > > +    if (memory_region_size(&backend->mr)) {
> >> > > > > +        memory_region_destroy(&backend->mr);
> >> > > > > +    }
> >> > > > > +}
> >> > > > > +
> >> > > > > +static void
> >> > > > > +hostmemory_backend_memory_init(UserCreatable *uc, Error **errp)
> >> > > >
> >> > > > And this becomes "_complete" for naming consistency?
> >> > > >
> >> > > > > +{
> >> > > > > +    error_setg(errp, "memory_init is not implemented for type 
> >> > > > > [%s]",
> >> > > > > +               object_get_typename(OBJECT(uc)));
> >> > > >
> >> > > > What if complete for the concrete class is a genuine NOP? I think
> >> > > > that's for the child class decide. All this check is doing is
> >> > > > mandating that the child does "something" without any form of 
> >> > > > validity
> >> > > > checking. I would just drop it completely.
> >> >
> >> > NUMA binding patch needs it. HostMemoryBackend can do some common task
> >> > such as setting memory policies, prealloc memory, etc. after subclasses
> >> > allocate memory regions. see https://github.com/taohu/qemu/commits/numa
> >> > for codes doing this.
> >> >
> >> > For now the complete function can be just left empty and fill later.
> >> I've think Peter's suggested not to rise a error form dummy function.
> >> So I've dropped it from abstract HostMemoryBackend class in v4 so later
> >> concrete class should set it's own implementation, which
> >> TYPE_MEMORY_BACKEND_RAM does.
> >
> > I'd suggest adding a new function HostMemoryBackend::alloc() then in
> > HostmemoryBackend::complete() we can do things like:
> >
> >   alloc();
> >   mbind();
> >   preallocate();
> >
> >
> > If letting subclasses do the allocation in its' own complete() function,
> > we will have trouble setting memory policies and like in
> > HostmemoryBackend::complete().
> >
> 
> If both the abstract and concrete class want to do something for a
> particular hook that's ok. The concrete class overrides, but should
> just call the superclass implementation from it's own hook. No need
> for new abstract APIs.

For doing things that are specific to subclasses I agree with you. But
in this case the superclass is aware of alloc() and the whole progress
of allocating memory, setting policies, preallocating, etc. may be
complex enough that it can't be done by a call to superclass::complete
followed by a subclass::complete(or the reserve order). Think about that
if the whole progress needs to be done like this:

  step1(); /* done by subclass */
  step2(); /* done by superclass */
  step3(); /* done by subclass */
  step4(); /* done by superclass */


Regards,
Hu