From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info Date: Tue, 29 Sep 2015 11:05:27 +0800 Message-ID: <20150929030527.GF5813@pengc-linux.bj.intel.com> References: <1443441293-4287-1-git-send-email-chao.p.peng@linux.intel.com> <1443441293-4287-4-git-send-email-chao.p.peng@linux.intel.com> <20150928141356.GD13821@zion.uk.xensource.com> <1443454556.3276.98.camel@citrix.com> <20150928154616.GO13821@zion.uk.xensource.com> Reply-To: Chao Peng Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20150928154616.GO13821@zion.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: Wei Liu Cc: Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, Dario Faggioli , g@zion.uk.xensource.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Mon, Sep 28, 2015 at 04:46:17PM +0100, Wei Liu wrote: > On Mon, Sep 28, 2015 at 05:35:56PM +0200, Dario Faggioli wrote: > > On Mon, 2015-09-28 at 15:13 +0100, Wei Liu wrote: > > > On Mon, Sep 28, 2015 at 07:54:51PM +0800, Chao Peng wrote: > > > > > > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c > > > > index 3378239..10e1113 100644 > > > > > > @@ -339,7 +339,8 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, > > > > libxl_psr_cat_info **info, > > > > { > > > > GC_INIT(ctx); > > > > int rc; > > > > - int i, nr_sockets; > > > > + int i = 0, socket, nr_sockets; > > > > + libxl_bitmap socketmap; > > > > libxl_psr_cat_info *ptr; > > > > > > > > rc = libxl__count_physical_sockets(gc, &nr_sockets); > > > > @@ -348,21 +349,31 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, > > > > libxl_psr_cat_info **info, > > > > goto out; > > > > } > > > > > > This is a path that you call libxl_bitmap_dispose on an uninitialised > > > socketmap. > > > > > Yep. > > > > However, do we still need to go through libxl__count_physical_sockets() > > explicitly? AFAICS, you need it for allocating ptr, and for returning > > it back. Not just for allocating ptr only, but also for allocating socketmap. > > > > But since now you're building the full bitmap, we can use > > libxl_bitmap_count_set(), for that. > > > > This may not be a bit deal, but if I'm not wrong, it saves us an > > hypercall (the PHYSINFO that libxl__count_physical_socket() issues). > > I can pass 0 to libxl_socket_bitmap_alloc() but it will call hypercall internally. We need the size for the socketmap anyway before we allocating it. > > Right, this is optimisation. I'm not too fussed either way as long as > this function is functionally correct. :-) > > > > > --- a/tools/libxl/libxl_types.idl > > > > +++ b/tools/libxl/libxl_types.idl > > > > @@ -792,6 +792,7 @@ libxl_psr_cbm_type = > > > > Enumeration("psr_cbm_type", [ > > > > ]) > > > > > > > > libxl_psr_cat_info = Struct("psr_cat_info", [ > > > > + ("target_id", uint32), > > > > > > Or just call it "socket_id"? Or even just "id" because you know this > > > structure is for socket? > > > > > Yeah, we discussed this already, AFAICR. I think the point is that, at > > least in theory, these features may be extended to become more fine > > -grained than per-socket, hence the point of not binding the interface > > to sockets. > > > > That being said, FWIW, I'm fine either way. > > > > The thing that bugs me a little is that "target_id" doesn't seem very > meaningful. I would just go for "id" if it is not bound to socket. > > Chao, what do you think? "id" looks right for me. I don't like "target_id" as well but just can't find out a better name :) Chao