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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mar, 2013-11-19 at 17:24 +0000, Ian Campbell wrote:
> > Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

> > +int libxl_set_vcpuaffinity2(libxl_ctx *ctx, uint32_t domid, uint32_t 
> > vcpuid,
> > +                            const libxl_bitmap *cpumap, int flags)
> 
> I think if we are going to duplicate hte API in this way then we should
> still combine the implementation, either with an internal private helper
> or by making the old API a wrapper around the new one.
> 
> The internals of ...2 and ...3 should be shared as far as possible too.
> 
Ok, this can be done, I think.

> > +{
> > +    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?

> > +        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. :-)

> > +int libxl_set_vcpuaffinity3(libxl_ctx *ctx, uint32_t domid, uint32_t 
> > vcpuid,
> > +                            const libxl_bitmap *cpumap_hard,
> > +                            const libxl_bitmap *cpumap_soft)
> 
> Insert the same comments as ...2, because AFAICT it is mostly the same
> function.
> 
I will.

> > 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. :-)

> > +/* 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.

But again, if you're fine with these being enum, I will make them so.

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.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