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

[Dario Faggioli <raistlin@xxxxxxxx>]

On Thu, 2012-05-31 at 15:22 +0100, Ian Jackson wrote:
> 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:
> 
Sorry, with "both places" being? Maybe you're talking about reusing
LIBXL_CPUTOPOLOGY_INVALID_ENTRY here as well? If yes, I think it is
possible and I also thought doing it like that... Or was it something
different you were saying?

> > +#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 ?
> 
I again am not sure about what types you are talking about here I'm
afraid... :-(

> > +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 ?
> 
Yes, I did this like it is mainly because it is exactly what
libxl_get_cpu_topology() does and thus I thought it to be the way to
go. :-)

> > +    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 ?
> 
About libxl__zalloc(), noted. Thanks.

About moving this all, I can of couse look into doing that, if wanted.

> I'd like to hear what other people say about putting this in libxl
> vs. libxc.
>
Sure, just let me know what people prefer...

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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