Re: [Xen-devel] [PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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?

Wei.

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



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