From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44376) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WG33s-0005hq-HP for qemu-devel@nongnu.org; Wed, 19 Feb 2014 04:03:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WG33m-0002SW-Dg for qemu-devel@nongnu.org; Wed, 19 Feb 2014 04:03:24 -0500 Received: from mail-qg0-x233.google.com ([2607:f8b0:400d:c04::233]:38406) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WG33m-0002SQ-89 for qemu-devel@nongnu.org; Wed, 19 Feb 2014 04:03:18 -0500 Received: by mail-qg0-f51.google.com with SMTP id q108so160110qgd.10 for ; Wed, 19 Feb 2014 01:03:17 -0800 (PST) Sender: Paolo Bonzini Message-ID: <53047351.80300@redhat.com> Date: Wed, 19 Feb 2014 10:03:13 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v18 13/14] memory backend: fill memory backend ram fields List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao , qemu-devel@nongnu.org Cc: imammedo@redhat.com, lersek@redhat.com, Wanlong Gao 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 > --- > 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 > index a496dbd..2da9341 100644 > --- a/backends/hostmem-ram.c > +++ b/backends/hostmem-ram.c > @@ -10,23 +10,179 @@ > * See the COPYING file in the top-level directory. > */ > #include "sysemu/hostmem.h" > +#include "sysemu/sysemu.h" > +#include "qemu/bitmap.h" > +#include "qapi-visit.h" > +#include "qemu/config-file.h" > +#include "qapi/opts-visitor.h" > > #define TYPE_MEMORY_BACKEND_RAM "memory-ram" > +#define MEMORY_BACKEND_RAM(obj) \ > + OBJECT_CHECK(HostMemoryBackendRam, (obj), TYPE_MEMORY_BACKEND_RAM) > > +typedef struct HostMemoryBackendRam HostMemoryBackendRam; > + > +/** > + * @HostMemoryBackendRam > + * > + * @parent: opaque parent object container > + * @host_nodes: host nodes bitmap used for memory policy > + * @policy: host memory policy > + * @relative: if the host nodes bitmap is relative > + */ > +struct HostMemoryBackendRam { > + /* private */ > + HostMemoryBackend parent; > + > + DECLARE_BITMAP(host_nodes, MAX_NODES); > + HostMemPolicy policy; > + bool relative; > +}; > + > +static void > +get_host_nodes(Object *obj, Visitor *v, void *opaque, const char *name, > + Error **errp) > +{ > + HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj); > + uint16List *host_nodes = NULL; > + uint16List **node = &host_nodes; > + unsigned long value; > + > + value = find_first_bit(ram_backend->host_nodes, MAX_NODES); > + if (value == MAX_NODES) { > + return; > + } > + > + *node = g_malloc0(sizeof(**node)); > + (*node)->value = value; > + node = &(*node)->next; > + > + do { > + value = find_next_bit(ram_backend->host_nodes, MAX_NODES, value + 1); > + if (value == MAX_NODES) { > + break; > + } > + > + *node = g_malloc0(sizeof(**node)); > + (*node)->value = value; > + node = &(*node)->next; > + } while (true); > + > + visit_type_uint16List(v, &host_nodes, name, errp); > +} > + > +static void > +set_host_nodes(Object *obj, Visitor *v, void *opaque, const char *name, > + Error **errp) > +{ > + HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj); > + uint16List *l = NULL; > + > + visit_type_uint16List(v, &l, name, errp); > + > + while (l) { > + bitmap_set(ram_backend->host_nodes, l->value, 1); > + l = l->next; > + } > +} > + > +static const char *policies[HOST_MEM_POLICY_MAX + 1] = { > + [HOST_MEM_POLICY_DEFAULT] = "default", > + [HOST_MEM_POLICY_PREFERRED] = "preferred", > + [HOST_MEM_POLICY_MEMBIND] = "membind", > + [HOST_MEM_POLICY_INTERLEAVE] = "interleave", > + [HOST_MEM_POLICY_MAX] = NULL, > +}; This is already available in qapi-types.c as HostMemPolicy_lookup. > +static void > +get_policy(Object *obj, Visitor *v, void *opaque, const char *name, > + Error **errp) > +{ > + HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj); > + int policy = ram_backend->policy; > + > + visit_type_enum(v, &policy, policies, NULL, name, errp); > +} > + > +static void > +set_policy(Object *obj, Visitor *v, void *opaque, const char *name, > + Error **errp) > +{ > + HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj); > + int policy; > + > + visit_type_enum(v, &policy, policies, NULL, name, errp); > + ram_backend->policy = policy; I think you need to set an error if backend->mr != NULL. > +} > + > + > +static bool get_relative(Object *obj, Error **errp) > +{ > + HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj); > + > + return ram_backend->relative; > +} > + > +static void set_relative(Object *obj, bool value, Error **errp) > +{ > + HostMemoryBackendRam *ram_backend = MEMORY_BACKEND_RAM(obj); > + > + ram_backend->relative = value; > +} Do we need relative vs. static? Also, the default right now is static, while in Linux kernel this is a tri-state: relative, static, default. I think that for now we should just omit this and only allow the default setting. We can introduce an enum later without make the API backwards-incompatible. > +#include > +#ifndef MPOL_F_RELATIVE_NODES > +#define MPOL_F_RELATIVE_NODES (1 << 14) > +#define MPOL_F_STATIC_NODES (1 << 15) > +#endif > > 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 static void host_memory_backend_alloc(HostMemoryBackend *backend, Error **errp) { Error *local_err = NULL; bc->memory_init(backend, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); return; } ... set policy ... } ... Error *local_err = NULL; host_memory_backend_alloc(backend, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); return NULL; } assert(memory_region_size(&backend->mr) != 0); return &backend->mr; } > + return -1; > + } > } > > return 0; > } > > static void > +ram_backend_initfn(Object *obj) > +{ > + object_property_add(obj, "host-nodes", "int", > + get_host_nodes, > + set_host_nodes, NULL, NULL, NULL); > + object_property_add(obj, "policy", "string", The convention is "str". > + get_policy, > + set_policy, NULL, NULL, NULL); > + object_property_add_bool(obj, "relative", > + get_relative, set_relative, NULL); > +} > + > +static void > ram_backend_class_init(ObjectClass *oc, void *data) > { > HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc); > @@ -38,6 +194,8 @@ static const TypeInfo ram_backend_info = { > .name = TYPE_MEMORY_BACKEND_RAM, > .parent = TYPE_MEMORY_BACKEND, > .class_init = ram_backend_class_init, > + .instance_size = sizeof(HostMemoryBackendRam), > + .instance_init = ram_backend_initfn, > }; > > static void register_types(void) > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index acfc0c7..a3d8c02 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -152,6 +152,8 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, > const char *name, > QEMUMachineInitArgs *args); > > +extern QemuOptsList qemu_memdev_opts; > + Not needed anymore, I think. Paolo > #define MAX_OPTION_ROMS 16 > typedef struct QEMUOptionRom { > const char *name; >