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

[Paolo Bonzini]

 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 <address@hidden>
> ---
>  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 <sys/syscall.h>
> +#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;