Re: [Xen-devel] [PATCH v3 11/14] libxl: get and set soft affinity

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-11-19 at 18:51 +0100, Dario Faggioli wrote:
> > > +{
> > > +    libxl_cputopology *topology;
> > > +    libxl_bitmap ecpumap;
> > > +    int nr_cpus = 0, rc;
> > > +
> > > +    topology = libxl_get_cpu_topology(ctx, &nr_cpus);
> > > +    if (!topology) {
> > > +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to retrieve CPU 
> > > topology");
> > 
> > It's not consistent within the file but I think for new functions we
> > should use the LOG macro variants.
> > 
> Right, but don't I need a gc to use it? Should I "make up" one just for
> the purpose of using LOG/LOGE?

I think a call to GC_INIT/GC_FREE should be cheap enough.

> > > +        return ERROR_FAIL;
> > > +    }
> > > +    libxl_cputopology_list_free(topology, nr_cpus);
> > 
> > Why are you retrieving this only to immediately throw it away?
> > 
> Because I need nr_cpus. :-)

Surely this is not the recommended way to get nr_cpus!

libxl_get_cpu_topology() itself calls libxl_get_max_cpus() which seems
like the obvious candidate.


> > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > > index c7dceda..504c57b 100644
> > > --- a/tools/libxl/libxl.h
> > > +++ b/tools/libxl/libxl.h
> > > @@ -82,6 +82,20 @@
> > >  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> > >  
> > >  /*
> > > + * LIBXL_HAVE_VCPUINFO_SOFTAFFINITY indicates that a 'cpumap_soft'
> > > + * field (of libxl_bitmap type) is present in libxl_vcpuinfo,
> > > + * containing the soft affinity for the vcpu.
> > > + */
> > > +#define LIBXL_HAVE_VCPUINFO_SOFTAFFINITY 1
> > > +
> > > +/*
> > > + * LIBXL_HAVE_BUILDINFO_SOFTAFFINITY indicates that a 'cpumap_soft'
> > > + * field (of libxl_bitmap type) is present in libxl_domain_build_info,
> > > + * containing the soft affinity for the vcpu.
> > > + */
> > > +#define LIBXL_HAVE_BUILDINFO_SOFTAFFINITY 1
> > 
> > Given that they arrive can we just use HAVE_SOFTRAFFINITY?
> > 
> You mean just introducing one #define? Sure... For some reason I assumed
> that every new field should come with it's own symbol. But if it's fine
> to have one, I'm all for it. :-)

I think it's ok.

> 
> > > +/* Flags, consistent with domctl.h */
> > > +#define LIBXL_VCPUAFFINITY_HARD 1
> > > +#define LIBXL_VCPUAFFINITY_SOFT 2
> > 
> > Can these be an enum in the idl?
> > 
> I think yes.
> 
> I did actually check and, of all the enum-s in the IDL, none are used as
> flags, they're rather used as "single values". OTOH, the only actual
> flags I found (I think it was LIBXL_SUSPEND_DEBUG, LIBXL_SUSPEND_LIVE)
> were defined like I did myself above... That's why I went for it.

I have a feeling they predate the IDL, or at least the Enumeration
support. It's true that we don't have any other bit fields in enums
though. I can't see the harm, it's probably not worth introducing a new
IDL type for them.

> 
> But again, if you're fine with these being enum, I will make them so.
> 
> Thanks and Regards,
> Dario
> 



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