Re: [PATCH v2 10/15] tools: implement the new libxl get hw info interface

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-08-30 10:15:22, Roger Pau Monn� wrote:
> On Thu, Aug 24, 2017 at 09:14:44AM +0800, Yi Sun wrote:
> > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > index b183305..d7da7d7 100644
> > --- a/tools/libxl/libxl_psr.c
> > +++ b/tools/libxl/libxl_psr.c
> > @@ -382,56 +382,51 @@ static inline xc_psr_feat_type libxl__psr_feat_type_to_libxc_psr_feat_type(
> >      return xc_type;
> >  }
> >  
> > +static inline int libxl__psr_hw_info_to_libxl_psr_cat_info(
> 
> No inline. Maybe you could try to shorter the name?
> 
Got it. Will remove the last '_psr'.

[...]

> >  int libxl_psr_cat_get_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
> >                             int *nr, unsigned int lvl)
> >  {
> >      GC_INIT(ctx);
> >      int rc;
> > -    int i = 0, socketid, nr_sockets;
> > -    libxl_bitmap socketmap;
> > +    unsigned int i;
> > +    libxl_psr_hw_info *hw_info;
> >      libxl_psr_cat_info *ptr;
> > -    xc_psr_hw_info hw_info;
> > -    xc_psr_feat_type xc_type;
> > -
> > -    libxl_bitmap_init(&socketmap);
> > -
> > -    rc = libxl__count_physical_sockets(gc, &nr_sockets);
> > -    if (rc) {
> > -        LOGE(ERROR, "failed to get system socket count");
> > -        goto out;
> > -    }
> >  
> > -    libxl_socket_bitmap_alloc(ctx, &socketmap, nr_sockets);
> > -    rc = libxl_get_online_socketmap(ctx, &socketmap);
> > -    if (rc < 0) {
> > -        LOGE(ERROR, "failed to get available sockets");
> > +    rc = libxl_psr_get_hw_info(ctx, &hw_info, (unsigned int *)nr,
> 
> Is there any reason nr is int instead of unsigned int?
> 
> I would rather avoid casting things. Since this interface has not been
> present in a release yet, could you please send a separate patch to
> fix this if nr has no reason to be signed?
> 
This is a historical issue.

The first version of PSR introduced 'libxl_psr_cat_get_l3_info'. The input
parameter is 'int *nr'.

I think we cannot change the interface which has been merged and used by
others. Right?

> > +                               LIBXL_PSR_FEAT_TYPE_CAT, lvl);

[...]

> >  
> > +static inline int libxc__psr_hw_info_to_libxl_psr_hw_info(
> 
> No inline. Again shorter names would be better (although I understand
> this might not be possible).
> 
> Also, why are you adding a libxc__ prefixed function to libxl?
> 
Because this function is to convert 'xc_psr_hw_info' to 'libxl_psr_hw_info'.
I think I may change the name to 'libxl__xc_psr_info_to_libxl_psr_info'.

> > +                      libxl_psr_feat_type type, xc_psr_hw_info *xc_hw_info,
> > +                      libxl_psr_hw_info *xl_hw_info)
> 
> you could drop the '_hw' in the parameter names, so all the
> assignments below would fit on a single line.
> 
Ok, thanks!

> > +{
> > +    switch (type) {
> > +    case LIBXL_PSR_FEAT_TYPE_CAT:
> > +        xl_hw_info->u.cat.cos_max = xc_hw_info->u.xc_cat_info.cos_max;
> > +        xl_hw_info->u.cat.cbm_len = xc_hw_info->u.xc_cat_info.cbm_len;
> > +        xl_hw_info->u.cat.cdp_enabled =
> > +                                    xc_hw_info->u.xc_cat_info.cdp_enabled;
> > +        break;
> > +    case LIBXL_PSR_FEAT_TYPE_MBA:
> > +        xl_hw_info->u.mba.cos_max = xc_hw_info->u.xc_mba_info.cos_max;
> > +        xl_hw_info->u.mba.thrtl_max = xc_hw_info->u.xc_mba_info.thrtl_max;
> > +        xl_hw_info->u.mba.linear = xc_hw_info->u.xc_mba_info.linear;
> > +        break;
> > +    default:
> > +        return ERROR_INVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info,
> >                            unsigned int *nr, libxl_psr_feat_type type,
> >                            unsigned int lvl)
> >  {
> > -    return ERROR_FAIL;
> > +    GC_INIT(ctx);
> > +    int rc, nr_sockets;
> > +    unsigned int i = 0, socketid;
> > +    libxl_bitmap socketmap;
> > +    libxl_psr_hw_info *ptr;
> > +    xc_psr_feat_type xc_type;
> > +    xc_psr_hw_info hw_info;
> > +
> > +    libxl_bitmap_init(&socketmap);
> > +
> > +    if (type == LIBXL_PSR_FEAT_TYPE_CAT && lvl != 3 && lvl != 2) {
> > +        LOGE(ERROR, "input lvl %d is wrong!\n", lvl);
> 
> LOGE is used when errno is set, which I don't think it's the case
> here. Please use plain LOG.
> 
> > +        rc = ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> > +    xc_type = libxl__psr_feat_type_to_libxc_psr_feat_type(type, lvl);
> 
> Isn't the above function going to return and invalid type if the
> feature is CAT and the level is not correct? In which case you could
> remove the above if and just check that the returned type here is not
> invalid?
> 
Ok, will check remove the first check and check return value here.

> > +
> > +    rc = libxl__count_physical_sockets(gc, &nr_sockets);
> > +    if (rc) {
> > +        LOGE(ERROR, "failed to get system socket count");
> 
> Again, libxl__ functions don't set errno. In this case this might be
> fine, because libxl__count_physical_sockets calls into libxc which
> sets errno, but you should only use LOGE when reporting errors from
> system calls or libxc functions.
> 
Ok will change it to LOG.

> Roger.

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