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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 24, 2017 at 09:14:44AM +0800, Yi Sun wrote:
> This patch implements the new libxl get hw info interface,
> 'libxl_psr_get_hw_info', which is suitable to all psr allocation
> features. It also implements corresponding list free function,
> 'libxl_psr_hw_info_list_free' and make 'libxl_psr_cat_get_info' to call
> 'libxl_psr_get_hw_info' to avoid redundant codes in libxl_psr.c.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
> v2:
>     - split this patch out from a big patch in v1.
>       (suggested by Wei Liu)
>     - change 'CAT_INFO'/'MBA_INFO' to 'CAT' and 'MBA. Also the libxl structure
>       name 'cat_info'/'mba_info' is changed to 'cat'/'mba'.
>       (suggested by Chao Peng)
>     - call 'libxl_psr_hw_info_list_free' in 'libxl_psr_cat_get_info' to free
>       allocated resources.
>       (suggested by Chao Peng)
> ---
>  tools/libxl/libxl_psr.c | 145 
> +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 112 insertions(+), 33 deletions(-)
> 
> 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?

> +                      libxl_psr_feat_type type, libxl_psr_hw_info *hw_info,
> +                      libxl_psr_cat_info *cat_info)
> +{
> +    if (type != LIBXL_PSR_FEAT_TYPE_CAT)
> +        return ERROR_INVAL;
> +
> +    cat_info->id = hw_info->id;
> +    cat_info->cos_max = hw_info->u.cat.cos_max;
> +    cat_info->cbm_len = hw_info->u.cat.cbm_len;
> +    cat_info->cdp_enabled = hw_info->u.cat.cdp_enabled;
> +
> +    return 0;
> +}
> +
>  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?

> +                               LIBXL_PSR_FEAT_TYPE_CAT, lvl);
> +    if (rc)
>          goto out;
> -    }
>  
> -    xc_type = libxl__psr_feat_type_to_libxc_psr_feat_type(
> -                  LIBXL_PSR_FEAT_TYPE_CAT, lvl);
> +    ptr = libxl__malloc(NOGC, *nr * sizeof(libxl_psr_cat_info));
>  
> -    ptr = libxl__malloc(NOGC, nr_sockets * sizeof(libxl_psr_cat_info));
> -
> -    libxl_for_each_set_bit(socketid, socketmap) {
> -        ptr[i].id = socketid;
> -        if (xc_psr_get_hw_info(ctx->xch, socketid, xc_type, &hw_info)) {
> +    for (i = 0; i < *nr; i++) {
> +        if (libxl__psr_hw_info_to_libxl_psr_cat_info(
> +                    LIBXL_PSR_FEAT_TYPE_CAT,
> +                    &hw_info[i], &ptr[i])) {
> +            libxl_psr_hw_info_list_free(hw_info, (unsigned int)*nr);
>              rc = ERROR_FAIL;
>              free(ptr);
>              goto out;
>          }
> -
> -        ptr[i].cos_max = hw_info.u.xc_cat_info.cos_max;
> -        ptr[i].cbm_len = hw_info.u.xc_cat_info.cbm_len;
> -        ptr[i].cdp_enabled = hw_info.u.xc_cat_info.cdp_enabled;
> -
> -        i++;
>      }
>  
>      *info = ptr;
> -    *nr = i;
> +    libxl_psr_hw_info_list_free(hw_info, (unsigned int)*nr);
>  out:
> -    libxl_bitmap_dispose(&socketmap);
>      GC_FREE;
>      return rc;
>  }
> @@ -469,15 +464,99 @@ int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid,
>      return ERROR_FAIL;
>  }
>  
> +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?

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

> +{
> +    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?

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

Roger.

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