WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [Patch][resend] implementation of cpupool support in xl

To: Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [Patch][resend] implementation of cpupool support in xl
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Thu, 16 Sep 2010 11:45:14 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 16 Sep 2010 03:46:07 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4C907510.3070904@xxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <4C907510.3070904@xxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Wed, 2010-09-15 at 08:26 +0100, Juergen Gross wrote:
> Hi,
> 
> attached patch implements cpupool support in xl. Rewrite of previous patch
> after rework of libxl.

Thanks!

Please could you provide a changelog message? It seems you needed to
modify the libxc interface in order to support libxl, it would be worth
commenting on what/why you changed.

In several places I notice you calculate (X + 64) / 64. Normally I would
expect a (X + 63) / 64 pattern to round something up (for example in
other places you use + 7 / 8). Is this deliberate? Probably worth a
comment, or maybe wrap the concept into a function with an appropriate
name?

I think the units of libxl_cpumap.size might be worth a comment too,
especially since appears to differ from the units used in the libxc
interface. You can do this in the IDL with e.g.
-    ("size", uint32),
+    ("size", uint32, False, "number of <bananas> in map"),
     ("map",  Reference(uint64)),

I think <bananas>=="uint64_t sized words"? Perhaps size would be better
represented as the maximum CPU ID in the map? The fact that the map is
rounded up to the next 64 bit boundary isn't too interesting to callers,
is it?

I saw a "size * 8" which I think would better as "size *
sizeof(*....map)", although perhaps this usage reinforces the idea that
the size should be in CPUs not multiples of some word size? Especially
given that this is seen in the caller who I guess shouldn't really need
to know these details.

Similarly callers seem to be doing "map[i / 64] & (i %64)" type
arithmetic, I think providing a libxl_cpumap_cpu_present helper (and
_set/_clear if necessary) would help here.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel