| Message ID | 1504603957-5389-11-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 05, 2017 at 05:32:32PM +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 ^makes ^call > 'libxl_psr_get_hw_info' to avoid redundant codes in libxl_psr.c. ^code > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > v3: > - remove casting. > (suggested by Roger Pau Monné) > - remove inline. > (suggested by Roger Pau Monné) > - change 'libxc__psr_hw_info_to_libxl_psr_hw_info' to > 'libxl__xc_hw_info_to_libxl_hw_info'. > (suggested by Roger Pau Monné) > - remove '_hw' from parameter names. > (suggested by Roger Pau Monné) > - change some 'LOGE' to 'LOG'. > (suggested by Roger Pau Monné) > - check returned 'xc_type' and remove redundant 'lvl' check. > (suggested by Roger Pau Monné) > 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, 108 insertions(+), 37 deletions(-) > > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c > index dd412cc..d534ec2 100644 > --- a/tools/libxl/libxl_psr.c > +++ b/tools/libxl/libxl_psr.c > @@ -382,60 +382,49 @@ static xc_psr_feat_type libxl__feat_type_to_libxc_feat_type( > return xc_type; > } > > +static int libxl__hw_info_to_libxl_cat_info( > + 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; Since this is an internal libxl function, is there any possible valid scenario where this function is called with type != LIBXL_PSR_FEAT_TYPE_CAT? If not this should be an assert instead, and the function could return void. > + > + 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, > unsigned 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"); > + rc = libxl_psr_get_hw_info(ctx, &hw_info, nr, LIBXL_PSR_FEAT_TYPE_CAT, lvl); > + if (rc) > 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"); > - goto out; > - } > - > - xc_type = libxl__feat_type_to_libxc_feat_type(LIBXL_PSR_FEAT_TYPE_CAT, lvl); > - if (xc_type == XC_PSR_FEAT_UNKNOWN) { > - LOG(ERROR, "feature type or lvl is wrong"); > - rc = ERROR_FAIL; > - goto out; > - } > + 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__hw_info_to_libxl_cat_info(LIBXL_PSR_FEAT_TYPE_CAT, > + &hw_info[i], &ptr[i])) { Please use rc here: rc = libxl__hw_info_to_libxl_cat_info(...); if (rc) { ... This has the bonus of not losing the error code returned by libxl__hw_info_to_libxl_cat_info. > + libxl_psr_hw_info_list_free(hw_info, *nr); > rc = ERROR_FAIL; > free(ptr); > goto out; > } > - > - ptr[i].cos_max = hw_info.u.xc_cat.cos_max; > - ptr[i].cbm_len = hw_info.u.xc_cat.cbm_len; > - ptr[i].cdp_enabled = hw_info.u.xc_cat.cdp_enabled; > - > - i++; > } > > *info = ptr; > - *nr = i; > + libxl_psr_hw_info_list_free(hw_info, *nr); > out: > - libxl_bitmap_dispose(&socketmap); > GC_FREE; > return rc; > } > @@ -476,15 +465,97 @@ int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, > return ERROR_FAIL; > } > > +static int libxl__xc_hw_info_to_libxl_hw_info( > + libxl_psr_feat_type type, xc_psr_hw_info *xc_info, > + libxl_psr_hw_info *xl_info) > +{ > + switch (type) { > + case LIBXL_PSR_FEAT_TYPE_CAT: > + xl_info->u.cat.cos_max = xc_info->u.xc_cat.cos_max; > + xl_info->u.cat.cbm_len = xc_info->u.xc_cat.cbm_len; > + xl_info->u.cat.cdp_enabled = xc_info->u.xc_cat.cdp_enabled; > + break; > + case LIBXL_PSR_FEAT_TYPE_MBA: > + xl_info->u.mba.cos_max = xc_info->u.xc_mba.cos_max; > + xl_info->u.mba.thrtl_max = xc_info->u.xc_mba.thrtl_max; > + xl_info->u.mba.linear = xc_info->u.xc_mba.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); > + > + xc_type = libxl__feat_type_to_libxc_feat_type(type, lvl); > + if (xc_type == XC_PSR_FEAT_UNKNOWN) { > + LOG(ERROR, "feature type or lvl is wrong"); > + rc = ERROR_FAIL; > + goto out; > + } > + > + rc = libxl__count_physical_sockets(gc, &nr_sockets); > + if (rc) { > + LOG(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"); > + goto out; > + } > + > + ptr = libxl__malloc(NOGC, nr_sockets * sizeof(libxl_psr_hw_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)) { > + rc = ERROR_FAIL; > + free(ptr); > + goto out; > + } > + > + if (libxl__xc_hw_info_to_libxl_hw_info(type, &hw_info, &ptr[i])) { > + LOGE(ERROR, "Input type %d is wrong!\n", type); > + rc = ERROR_FAIL; > + free(ptr); > + goto out; > + } > + i++; > + } > + > + *info = ptr; > + *nr = i; > +out: > + libxl_bitmap_dispose(&socketmap); > + GC_FREE; > + return rc; > } > > void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr) > { > + unsigned int i; > + > + for (i = 0; i < nr; i++) > + libxl_psr_hw_info_dispose(&list[i]); > + free(list); Don't you also need a libxl_psr_cat_info_list_free? Or am I missing something? > } > > /* > -- > 1.9.1 >
On 17-09-19 11:28:18, Roger Pau Monn� wrote: > On Tue, Sep 05, 2017 at 05:32:32PM +0800, Yi Sun wrote: > > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c > > index dd412cc..d534ec2 100644 > > --- a/tools/libxl/libxl_psr.c > > +++ b/tools/libxl/libxl_psr.c > > @@ -382,60 +382,49 @@ static xc_psr_feat_type libxl__feat_type_to_libxc_feat_type( > > return xc_type; > > } > > > > +static int libxl__hw_info_to_libxl_cat_info( > > + 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; > > Since this is an internal libxl function, is there any possible valid > scenario where this function is called with type != > LIBXL_PSR_FEAT_TYPE_CAT? > > If not this should be an assert instead, and the function could return > void. > Thanks for the good suggestion! > > + > > + 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, > > unsigned int *nr, unsigned int lvl) > > { [...] > > - 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__hw_info_to_libxl_cat_info(LIBXL_PSR_FEAT_TYPE_CAT, > > + &hw_info[i], &ptr[i])) { > > Please use rc here: > > rc = libxl__hw_info_to_libxl_cat_info(...); > if (rc) { > ... > > This has the bonus of not losing the error code returned by > libxl__hw_info_to_libxl_cat_info. > Ok. > > + libxl_psr_hw_info_list_free(hw_info, *nr); > > rc = ERROR_FAIL; > > free(ptr); > > goto out; > > } > > - > > - ptr[i].cos_max = hw_info.u.xc_cat.cos_max; > > - ptr[i].cbm_len = hw_info.u.xc_cat.cbm_len; > > - ptr[i].cdp_enabled = hw_info.u.xc_cat.cdp_enabled; > > - > > - i++; > > } > > > > *info = ptr; > > - *nr = i; > > + libxl_psr_hw_info_list_free(hw_info, *nr); > > out: > > - libxl_bitmap_dispose(&socketmap); > > GC_FREE; > > return rc; > > } > > @@ -476,15 +465,97 @@ int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, > > return ERROR_FAIL; > > } > > [...] > > void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr) > > { > > + unsigned int i; > > + > > + for (i = 0; i < nr; i++) > > + libxl_psr_hw_info_dispose(&list[i]); > > + free(list); > > Don't you also need a libxl_psr_cat_info_list_free? Or am I missing > something? > The libxl_psr_cat_info_list_free is called in xl_psr.c which already exists in original codes. > > } > > > > /* > > -- > > 1.9.1 > >
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c index dd412cc..d534ec2 100644 --- a/tools/libxl/libxl_psr.c +++ b/tools/libxl/libxl_psr.c @@ -382,60 +382,49 @@ static xc_psr_feat_type libxl__feat_type_to_libxc_feat_type( return xc_type; } +static int libxl__hw_info_to_libxl_cat_info( + 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, unsigned 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"); + rc = libxl_psr_get_hw_info(ctx, &hw_info, nr, LIBXL_PSR_FEAT_TYPE_CAT, lvl); + if (rc) 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"); - goto out; - } - - xc_type = libxl__feat_type_to_libxc_feat_type(LIBXL_PSR_FEAT_TYPE_CAT, lvl); - if (xc_type == XC_PSR_FEAT_UNKNOWN) { - LOG(ERROR, "feature type or lvl is wrong"); - rc = ERROR_FAIL; - goto out; - } + 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__hw_info_to_libxl_cat_info(LIBXL_PSR_FEAT_TYPE_CAT, + &hw_info[i], &ptr[i])) { + libxl_psr_hw_info_list_free(hw_info, *nr); rc = ERROR_FAIL; free(ptr); goto out; } - - ptr[i].cos_max = hw_info.u.xc_cat.cos_max; - ptr[i].cbm_len = hw_info.u.xc_cat.cbm_len; - ptr[i].cdp_enabled = hw_info.u.xc_cat.cdp_enabled; - - i++; } *info = ptr; - *nr = i; + libxl_psr_hw_info_list_free(hw_info, *nr); out: - libxl_bitmap_dispose(&socketmap); GC_FREE; return rc; } @@ -476,15 +465,97 @@ int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, return ERROR_FAIL; } +static int libxl__xc_hw_info_to_libxl_hw_info( + libxl_psr_feat_type type, xc_psr_hw_info *xc_info, + libxl_psr_hw_info *xl_info) +{ + switch (type) { + case LIBXL_PSR_FEAT_TYPE_CAT: + xl_info->u.cat.cos_max = xc_info->u.xc_cat.cos_max; + xl_info->u.cat.cbm_len = xc_info->u.xc_cat.cbm_len; + xl_info->u.cat.cdp_enabled = xc_info->u.xc_cat.cdp_enabled; + break; + case LIBXL_PSR_FEAT_TYPE_MBA: + xl_info->u.mba.cos_max = xc_info->u.xc_mba.cos_max; + xl_info->u.mba.thrtl_max = xc_info->u.xc_mba.thrtl_max; + xl_info->u.mba.linear = xc_info->u.xc_mba.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); + + xc_type = libxl__feat_type_to_libxc_feat_type(type, lvl); + if (xc_type == XC_PSR_FEAT_UNKNOWN) { + LOG(ERROR, "feature type or lvl is wrong"); + rc = ERROR_FAIL; + goto out; + } + + rc = libxl__count_physical_sockets(gc, &nr_sockets); + if (rc) { + LOG(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"); + goto out; + } + + ptr = libxl__malloc(NOGC, nr_sockets * sizeof(libxl_psr_hw_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)) { + rc = ERROR_FAIL; + free(ptr); + goto out; + } + + if (libxl__xc_hw_info_to_libxl_hw_info(type, &hw_info, &ptr[i])) { + LOGE(ERROR, "Input type %d is wrong!\n", type); + rc = ERROR_FAIL; + free(ptr); + goto out; + } + + i++; + } + + *info = ptr; + *nr = i; +out: + libxl_bitmap_dispose(&socketmap); + GC_FREE; + return rc; } void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr) { + unsigned int i; + + for (i = 0; i < nr; i++) + libxl_psr_hw_info_dispose(&list[i]); + free(list); } /*
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@linux.intel.com> --- v3: - remove casting. (suggested by Roger Pau Monné) - remove inline. (suggested by Roger Pau Monné) - change 'libxc__psr_hw_info_to_libxl_psr_hw_info' to 'libxl__xc_hw_info_to_libxl_hw_info'. (suggested by Roger Pau Monné) - remove '_hw' from parameter names. (suggested by Roger Pau Monné) - change some 'LOGE' to 'LOG'. (suggested by Roger Pau Monné) - check returned 'xc_type' and remove redundant 'lvl' check. (suggested by Roger Pau Monné) 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, 108 insertions(+), 37 deletions(-)