From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v3 11/14] libxl: get and set soft affinity Date: Wed, 20 Nov 2013 11:29:17 +0000 Message-ID: <528C9D0D.8020302@eu.citrix.com> References: <20131118175544.31002.79574.stgit@Solace> <20131118181813.31002.61195.stgit@Solace> <1384881864.16252.48.camel@hastur.hellion.org.uk> <1384883478.19880.170.camel@Abyss> <1384946825.28441.56.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1384946825.28441.56.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Dario Faggioli Cc: Marcus Granado , Keir Fraser , Matt Wilson , Li Yechen , Andrew Cooper , Juergen Gross , Ian Jackson , xen-devel@lists.xen.org, Jan Beulich , Justin Weaver , Elena Ufimtseva List-Id: xen-devel@lists.xenproject.org On 20/11/13 11:27, Ian Campbell wrote: > 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. Since these are bits, not numbers, I don't think an enum is the right construct. Or, the enum values should be the *bit numbers*, and the flags should be (1<<[bit_humber]). -George