|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 13/15] tools: implement new generic get value interface and MBA get value command
On Tue, Sep 05, 2017 at 05:32:35PM +0800, Yi Sun wrote:
> This patch implements generic get value interfaces in libxc and libxl.
> It also refactors the get value flow in xl to make it be suitable for all
> allocation features. Based on that, a new MBA get value command is added in
> xl.
>
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> v3:
> - replace 'libxl_psr_cbm_type' to 'libxl_psr_type' in newly defined
> interfaces.
> (suggested by Roger Pau Monné)
> v2:
> - change 'CAT_INFO'/'MBA_INFO' to 'CAT'/'MBA'. The related structure names
> are changed too.
> (suggested by Chao Peng)
> ---
> tools/libxc/include/xenctrl.h | 7 +-
> tools/libxc/xc_psr.c | 9 +-
> tools/libxl/libxl_psr.c | 59 +++++++++-----
> tools/xl/xl.h | 1 +
> tools/xl/xl_cmdtable.c | 5 ++
> tools/xl/xl_psr.c | 185
> ++++++++++++++++++++++++++++++------------
> 6 files changed, 184 insertions(+), 82 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 63b92d2..eef06be 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2455,6 +2455,7 @@ enum xc_psr_type {
> XC_PSR_CAT_L3_CBM_CODE = 2,
> XC_PSR_CAT_L3_CBM_DATA = 3,
> XC_PSR_CAT_L2_CBM = 4,
> + XC_PSR_MBA_THRTL = 5,
> };
> typedef enum xc_psr_type xc_psr_type;
>
> @@ -2501,9 +2502,9 @@ int xc_psr_cmt_enabled(xc_interface *xch);
> int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid,
> xc_psr_type type, uint32_t target,
> uint64_t data);
> -int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
> - xc_psr_type type, uint32_t target,
> - uint64_t *data);
> +int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
> + xc_psr_type type, uint32_t target,
> + uint64_t *data);
> int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
> xc_psr_feat_type type, xc_psr_hw_info *hw_info);
>
> diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
> index 80642a2..2f0eed9 100644
> --- a/tools/libxc/xc_psr.c
> +++ b/tools/libxc/xc_psr.c
> @@ -283,9 +283,9 @@ int xc_psr_cat_set_domain_data(xc_interface *xch,
> uint32_t domid,
> return do_domctl(xch, &domctl);
> }
>
> -int xc_psr_cat_get_domain_data(xc_interface *xch, uint32_t domid,
> - xc_psr_type type, uint32_t target,
> - uint64_t *data)
> +int xc_psr_get_domain_data(xc_interface *xch, uint32_t domid,
> + xc_psr_type type, uint32_t target,
> + uint64_t *data)
> {
> int rc;
> DECLARE_DOMCTL;
> @@ -305,6 +305,9 @@ int xc_psr_cat_get_domain_data(xc_interface *xch,
> uint32_t domid,
> case XC_PSR_CAT_L2_CBM:
> cmd = XEN_DOMCTL_PSR_ALLOC_GET_L2_CBM;
> break;
> + case XC_PSR_MBA_THRTL:
> + cmd = XEN_DOMCTL_PSR_ALLOC_GET_MBA_THRTL;
> + break;
> default:
> errno = EINVAL;
> return -1;
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index c8d2921..78d5bc5 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -71,16 +71,30 @@ static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int
> err)
> LOGE(ERROR, "%s", msg);
> }
>
> -static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int err)
> +static void libxl__psr_alloc_log_err_msg(libxl__gc *gc,
> + int err,
> + libxl_psr_type type)
> {
> + /*
> + * Index is 'libxl_psr_type' so we set two 'CDP' to correspond to
> + * DATA and CODE.
> + */
> + const char * const feat_name[6] = {
The explicit '6' is not needed.
> + "UNKNOWN",
> + "L3 CAT",
> + "CDP",
> + "CDP",
> + "L2 CAT",
> + "MBA",
I'm not sure whether you want to use designated initializers here, in
case someone decides to change the order of the xc_psr_type enum or
the order here. ie:
feat_name[] = {
[XC_PSR_CAT_L3_CBM] = "L3 CAT",
[XC_PSR_CAT_L3_CBM_CODE..XC_PSR_CAT_L3_CBM_DATA] = "CDP",
...
}
> + };
> char *msg;
>
> switch (err) {
> case ENODEV:
> - msg = "CAT is not supported in this system";
> + msg = "is not supported in this system";
> break;
> case ENOENT:
> - msg = "CAT is not enabled on the socket";
> + msg = "is not enabled on the socket";
> break;
> case EOVERFLOW:
> msg = "no free COS available";
> @@ -106,7 +120,7 @@ static void libxl__psr_cat_log_err_msg(libxl__gc *gc, int
> err)
> return;
> }
>
> - LOGE(ERROR, "%s", msg);
> + LOGE(ERROR, "%s: %s", feat_name[type], msg);
I don't think you should use LOGE here, but rather LOG. LOGE should be
used when errno is set, which I don't think is the case here.
> }
>
> static int libxl__pick_socket_cpu(libxl__gc *gc, uint32_t socketid)
> @@ -303,10 +317,10 @@ out:
> return rc;
> }
>
> -static inline xc_psr_type libxl__psr_cbm_type_to_libxc_psr_type(
> - libxl_psr_cbm_type type)
> +static inline xc_psr_type libxl__psr_type_to_libxc_psr_type(
> + libxl_psr_type type)
> {
> - BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_type));
> + BUILD_BUG_ON(sizeof(libxl_psr_type) != sizeof(xc_psr_type));
> return (xc_psr_type)type;
> }
>
> @@ -330,10 +344,10 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t
> domid,
> if (socketid >= nr_sockets)
> break;
>
> - xc_type = libxl__psr_cbm_type_to_libxc_psr_type(type);
> + xc_type = libxl__psr_type_to_libxc_psr_type(type);
> if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type,
> socketid, cbm)) {
> - libxl__psr_cat_log_err_msg(gc, errno);
> + libxl__psr_alloc_log_err_msg(gc, errno, type);
> rc = ERROR_FAIL;
> }
> }
> @@ -347,18 +361,7 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
> libxl_psr_cbm_type type, uint32_t target,
> uint64_t *cbm_r)
> {
> - GC_INIT(ctx);
> - int rc = 0;
> - xc_psr_type xc_type = libxl__psr_cbm_type_to_libxc_psr_type(type);
> -
> - if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type,
> - target, cbm_r)) {
> - libxl__psr_cat_log_err_msg(gc, errno);
> - rc = ERROR_FAIL;
> - }
> -
> - GC_FREE;
> - return rc;
> + return libxl_psr_get_val(ctx, domid, type, target, cbm_r);
> }
You could even move this to libxl.h as a static function IMHO.
>
> static xc_psr_feat_type libxl__feat_type_to_libxc_feat_type(
> @@ -462,7 +465,19 @@ int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid,
> libxl_psr_type type, unsigned int target,
> uint64_t *val)
> {
> - return ERROR_FAIL;
> + GC_INIT(ctx);
> + int rc = 0;
> +
> + xc_psr_type xc_type = libxl__psr_type_to_libxc_psr_type(type);
> +
> + if (xc_psr_get_domain_data(ctx->xch, domid, xc_type,
> + target, val)) {
> + libxl__psr_alloc_log_err_msg(gc, errno, type);
> + rc = ERROR_FAIL;
> + }
> +
> + GC_FREE;
> + return rc;
> }
>
> static int libxl__xc_hw_info_to_libxl_hw_info(
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index 8d7b957..3389df9 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -204,6 +204,7 @@ int main_psr_cmt_detach(int argc, char **argv);
> int main_psr_cmt_show(int argc, char **argv);
> int main_psr_cat_cbm_set(int argc, char **argv);
> int main_psr_cat_show(int argc, char **argv);
> +int main_psr_mba_show(int argc, char **argv);
> #endif
> int main_qemu_monitor_command(int argc, char **argv);
>
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index a01245d..cdc2349 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -560,6 +560,11 @@ struct cmd_spec cmd_table[] = {
> "[options] <Domain>",
> "-l <level> Specify the cache level to process, otherwise L3
> cache is processed\n"
> },
> + { "psr-mba-show",
> + &main_psr_mba_show, 0, 1,
> + "Show Memory Bandwidth Allocation information",
> + "<Domain>",
> + },
> #endif
> { "usbctrl-attach",
> &main_usbctrl_attach, 0, 1,
> diff --git a/tools/xl/xl_psr.c b/tools/xl/xl_psr.c
> index 40269b4..46b7788 100644
> --- a/tools/xl/xl_psr.c
> +++ b/tools/xl/xl_psr.c
> @@ -327,19 +327,27 @@ out:
> return rc;
> }
>
> -static void psr_cat_print_one_domain_cbm_type(uint32_t domid, uint32_t
> socketid,
> - libxl_psr_cbm_type type)
> +static void psr_print_one_domain_val_type(uint32_t domid,
> + libxl_psr_hw_info *info,
> + libxl_psr_type type)
> {
> - uint64_t cbm;
> + uint64_t val;
>
> - if (!libxl_psr_cat_get_cbm(ctx, domid, type, socketid, &cbm))
> - printf("%#16"PRIx64, cbm);
> + if (!libxl_psr_get_val(ctx, domid, type, info->id, &val))
> + {
> + if (type == LIBXL_PSR_CBM_TYPE_MBA_THRTL && info->u.mba.linear)
> + printf("%16"PRIu64, val);
> + else
> + printf("%#16"PRIx64, val);
> + }
> else
> printf("%16s", "error");
> }
>
> -static void psr_cat_print_one_domain_cbm(uint32_t domid, uint32_t socketid,
> - bool cdp_enabled, unsigned int lvl)
> +static void psr_print_one_domain_val(uint32_t domid,
> + libxl_psr_hw_info *info,
> + libxl_psr_feat_type type,
> + unsigned int lvl)
> {
> char *domain_name;
>
> @@ -347,106 +355,154 @@ static void psr_cat_print_one_domain_cbm(uint32_t
> domid, uint32_t socketid,
> printf("%5d%25s", domid, domain_name);
> free(domain_name);
>
> - switch (lvl) {
> - case 3:
> - if (!cdp_enabled) {
> - psr_cat_print_one_domain_cbm_type(domid, socketid,
> + switch (type) {
> + case LIBXL_PSR_FEAT_TYPE_CAT:
> + switch (lvl) {
> + case 3:
> + if (!info->u.cat.cdp_enabled) {
> + psr_print_one_domain_val_type(domid, info,
> LIBXL_PSR_CBM_TYPE_L3_CBM);
> - } else {
> - psr_cat_print_one_domain_cbm_type(domid, socketid,
> + } else {
> + psr_print_one_domain_val_type(domid, info,
>
> LIBXL_PSR_CBM_TYPE_L3_CBM_CODE);
> - psr_cat_print_one_domain_cbm_type(domid, socketid,
> + psr_print_one_domain_val_type(domid, info,
>
> LIBXL_PSR_CBM_TYPE_L3_CBM_DATA);
> - }
> - break;
> - case 2:
> - psr_cat_print_one_domain_cbm_type(domid, socketid,
> + }
> + break;
> +
> + case 2:
> + psr_print_one_domain_val_type(domid, info,
> LIBXL_PSR_CBM_TYPE_L2_CBM);
> + break;
> +
> + default:
> + printf("Input lvl %d is wrong!", lvl);
> + }
> break;
> - default:
> - printf("Input lvl %d is wrong!", lvl);
> +
> + case LIBXL_PSR_FEAT_TYPE_MBA:
> + psr_print_one_domain_val_type(domid, info,
> + LIBXL_PSR_CBM_TYPE_MBA_THRTL);
> break;
> }
>
> printf("\n");
> }
>
> -static int psr_cat_print_domain_cbm(uint32_t domid, uint32_t socketid,
> - bool cdp_enabled, unsigned int lvl)
> +static int psr_print_domain_val(uint32_t domid,
> + libxl_psr_hw_info *info,
> + libxl_psr_feat_type type,
> + unsigned int lvl)
> {
> int i, nr_domains;
> libxl_dominfo *list;
>
> if (domid != INVALID_DOMID) {
> - psr_cat_print_one_domain_cbm(domid, socketid, cdp_enabled, lvl);
> + psr_print_one_domain_val(domid, info, type, lvl);
> return 0;
> }
>
> if (!(list = libxl_list_domain(ctx, &nr_domains))) {
> - fprintf(stderr, "Failed to get domain list for cbm display\n");
> - return -1;
> + fprintf(stderr, "Failed to get domain list for value display\n");
> + return EXIT_FAILURE;
> }
>
> for (i = 0; i < nr_domains; i++)
> - psr_cat_print_one_domain_cbm(list[i].domid, socketid, cdp_enabled,
> lvl);
> + psr_print_one_domain_val(list[i].domid, info, type, lvl);
> libxl_dominfo_list_free(list, nr_domains);
>
> return 0;
> }
>
> -static int psr_cat_print_socket(uint32_t domid, libxl_psr_cat_info *info,
> - unsigned int lvl)
> +static int psr_print_socket(uint32_t domid,
> + libxl_psr_hw_info *info,
> + libxl_psr_feat_type type,
> + unsigned int lvl)
> {
> - int rc;
> - uint32_t l3_cache_size;
> -
> printf("%-16s: %u\n", "Socket ID", info->id);
>
> - /* So far, CMT only supports L3 cache. */
> - if (lvl == 3) {
> - rc = libxl_psr_cmt_get_l3_cache_size(ctx, info->id, &l3_cache_size);
> - if (rc) {
> - fprintf(stderr, "Failed to get l3 cache size for socket:%d\n",
> - info->id);
> - return -1;
> + switch (type) {
> + case LIBXL_PSR_FEAT_TYPE_CAT:
> + {
> + int rc;
> + uint32_t l3_cache_size;
> +
> + /* So far, CMT only supports L3 cache. */
> + if (lvl == 3) {
Shouldn't you print some kind of error message if lvl != 3? Or is it
expected that this function will be called with lvl != 3 and it should
be ignored?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |