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