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

[Chao Peng <chao.p.peng@linux.intel.com>]

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