Re: [Qemu-devel] [PATCH v4 18/29] hostmem: add file-based HostMemoryBackend

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 9 Jun 2014 14:35:53 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jun 09, 2014 at 01:32:46PM +0200, Igor Mammedov wrote:
> > On Mon, 9 Jun 2014 18:25:23 +0800
> > Hu Tao <hutao@cn.fujitsu.com> wrote:
> > 
> > > From: Paolo Bonzini <pbonzini@redhat.com>
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > ---
> > >  backends/Makefile.objs  |   1 +
> > >  backends/hostmem-file.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 108 insertions(+)
> > >  create mode 100644 backends/hostmem-file.c
> > > 
> > > diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> > > index 7fb7acd..506a46c 100644
> > > --- a/backends/Makefile.objs
> > > +++ b/backends/Makefile.objs
> > > @@ -8,3 +8,4 @@ baum.o-cflags := $(SDL_CFLAGS)
> > >  common-obj-$(CONFIG_TPM) += tpm.o
> > >  
> > >  common-obj-y += hostmem.o hostmem-ram.o
> > > +common-obj-$(CONFIG_LINUX) += hostmem-file.o
> > > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > > new file mode 100644
> > > index 0000000..b8df933
> > > --- /dev/null
> > > +++ b/backends/hostmem-file.c
> > > @@ -0,0 +1,107 @@
> > > +/*
> > > + * QEMU Host Memory Backend for hugetlbfs
> > > + *
> > > + * Copyright (C) 2013 Red Hat Inc
> > > + *
> > > + * Authors:
> > > + *   Paolo Bonzini <pbonzini@redhat.com>
> > > + *
> > > + * 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"
> > > +
> > > +/* hostmem-file.c */
> > > +/**
> > > + * @TYPE_MEMORY_BACKEND_FILE:
> > > + * name of backend that uses mmap on a file descriptor
> > > + */
> > > +#define TYPE_MEMORY_BACKEND_FILE "memory-backend-file"
> > how about naming it after what it really is? "memory-backend-hugepage"
> > Later we could split it into generic superclass mmap-ed "memory-backend-file"
> > and have TPH specific code moved into this backend.
> 
> What does this last sentence mean?
1. currently file_ram_alloc() uses TPH specific code, I suggest to keep name
   "memory-backend-file" free for now so that in case if there would be need in
   a generic file backend, we could introduce it without causing confusion
   with TPH backend.
2. There is not much point to build TPH backend for every host, we can exclude
   it safely from non linux builds, instead of building it and make it
   failing at runtime. 

> 
> THP is transparent huge pages, right?
yes.

> 
> 
> 
> > > +
> > > +#define MEMORY_BACKEND_FILE(obj) \
> > > +    OBJECT_CHECK(HostMemoryBackendFile, (obj), TYPE_MEMORY_BACKEND_FILE)
> > > +
> > > +typedef struct HostMemoryBackendFile HostMemoryBackendFile;
> > > +
> > > +struct HostMemoryBackendFile {
> > > +    HostMemoryBackend parent_obj;
> > > +    char *mem_path;
> > > +};
> > > +
> > > +static void
> > > +file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> > > +{
> > > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(backend);
> > > +
> > > +    if (!backend->size) {
> > > +        error_setg(errp, "can't create backend with size 0");
> > > +        return;
> > > +    }
> > > +    if (!fb->mem_path) {
> > > +        error_setg(errp, "mem-path property not set");
> > > +        return;
> > > +    }
> > > +#ifndef CONFIG_LINUX
> > > +    error_setg(errp, "-mem-path not supported on this host");
> > Is it possible to not compile this backend on non linux host at all, instead
> > of ifdefs.
> > 
> > > +#else
> > > +    if (!memory_region_size(&backend->mr)) {
> > > +        memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
> > > +                                 object_get_canonical_path(OBJECT(backend)),
> > > +                                 backend->size,
> > > +                                 fb->mem_path, errp);
> > > +    }
> > > +#endif
> > > +}
> > > +
> > > +static void
> > > +file_backend_class_init(ObjectClass *oc, void *data)
> > > +{
> > > +    HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
> > > +
> > > +    bc->alloc = file_backend_memory_alloc;
> > > +}
> > > +
> > > +static char *get_mem_path(Object *o, Error **errp)
> > > +{
> > > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> > > +
> > > +    return g_strdup(fb->mem_path);
> > > +}
> > > +
> > > +static void set_mem_path(Object *o, const char *str, Error **errp)
> > > +{
> > > +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> > > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> > > +
> > > +    if (memory_region_size(&backend->mr)) {
> > > +        error_setg(errp, "cannot change property value");
> > > +        return;
> > > +    }
> > > +    if (fb->mem_path) {
> > > +        g_free(fb->mem_path);
> > > +    }
> > > +    fb->mem_path = g_strdup(str);
> > > +}
> > > +
> > > +static void
> > > +file_backend_instance_init(Object *o)
> > > +{
> > > +    object_property_add_str(o, "mem-path", get_mem_path,
> > > +                            set_mem_path, NULL);
> > s/"mem-path"/"path"/
> > 
> > 
> > > +}
> > > +
> > > +static const TypeInfo file_backend_info = {
> > > +    .name = TYPE_MEMORY_BACKEND_FILE,
> > > +    .parent = TYPE_MEMORY_BACKEND,
> > > +    .class_init = file_backend_class_init,
> > > +    .instance_init = file_backend_instance_init,
> > > +    .instance_size = sizeof(HostMemoryBackendFile),
> > > +};
> > > +
> > > +static void register_types(void)
> > > +{
> > > +    type_register_static(&file_backend_info);
> > > +}
> > > +
> > > +type_init(register_types);
> > > -- 
> > > 1.9.3
> > > 
> > 
> > 
> > -- 
> > Regards,
> >   Igor
> 


-- 
Regards,
  Igor