Re: [Xen-devel] [PATCH 06 of 11] libxl: introduce libxl_get_numainfo()

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Dario Faggioli writes ("[PATCH 06 of 11] libxl: introduce 
libxl_get_numainfo()"):
> Make some NUMA node information available to the toolstack. Achieve
> this by means of xc_numainfo(), which exposes memory size and amount
> of free memory of each node, as well as the relative distances of
> each node to all the others.
...
> +#define LIBXL_NUMAINFO_INVALID_ENTRY (~(uint32_t)0)

Is there some reason we can't just make sure we use the same value for
this in both places ?  That would avoid the need for this:

> +#define V(mem, i) (mem[i] == INVALID_NUMAINFO_ID) ? \
> +    LIBXL_NUMAINFO_INVALID_ENTRY : mem[i]

I appreciate that the types aren't the same.  In libxc it's an
unsigned long.  But shouldn't they be the same ?

> +libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
> +{
> +    xc_numainfo_t ninfo;
> +    DECLARE_HYPERCALL_BUFFER(xc_node_to_memsize_t, memsize);
> +    DECLARE_HYPERCALL_BUFFER(xc_node_to_memfree_t, memfree);
> +    DECLARE_HYPERCALL_BUFFER(uint32_t, node_distances);
> +    libxl_numainfo *ret = NULL;
> +    int i, j, max_nodes;
> +
> +    max_nodes = libxl_get_max_nodes(ctx);
> +    if (max_nodes == 0)
> +    {
> +        LIBXL__LOG(ctx, XTL_ERROR, "Unable to determine number of NODES");
> +        return NULL;
> +    }
> +
> +    memsize = xc_hypercall_buffer_alloc
> +        (ctx->xch, memsize, sizeof(*memsize) * max_nodes);
> +    memfree = xc_hypercall_buffer_alloc
> +        (ctx->xch, memfree, sizeof(*memfree) * max_nodes);
> +    node_distances = xc_hypercall_buffer_alloc
> +        (ctx->xch, node_distances, sizeof(*node_distances) * max_nodes * 
> max_nodes);

This kind of stuff normally lives in libxc.  I appreciate that we have
a bit of it in libxl already, but do we want to perpetuate that ?

> +    ret = malloc(sizeof(libxl_numainfo) * max_nodes);
> +    if (ret == NULL) {

In libxl you can use libxl__zalloc(NULL,...) (and don't have to check
for errors because it can't fail).  But perhaps this is going into
libxc ?

I'd like to hear what other people say about putting this in libxl
vs. libxc.

> +        ret[i].dists = malloc(sizeof(*node_distances) * max_nodes);

Again.  And the lack of error exits will simplify this a lot.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel