| Message ID | 1504603957-5389-15-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:36PM +0800, Yi Sun wrote: > This patch implements new generic set value interfaces in libxc and libxl. > These interfaces are suitable for all allocation features. It also adds a > new MBA set value command in xl. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > v3: > - add 'const' for 'opts[]' in 'main_psr_mba_set'. > (suggested by Roger Pau Monné) > - replace 'libxl_psr_cbm_type' to 'libxl_psr_type' for newly defined > interfaces. > (suggested by Roger Pau Monné) > --- > tools/libxc/include/xenctrl.h | 6 ++--- > tools/libxc/xc_psr.c | 9 ++++--- > tools/libxl/libxl_psr.c | 56 +++++++++++++++++++++---------------------- > tools/xl/xl.h | 1 + > tools/xl/xl_cmdtable.c | 6 +++++ > tools/xl/xl_psr.c | 55 ++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 99 insertions(+), 34 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index eef06be..21dac2f 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -2499,9 +2499,9 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu, > uint64_t *tsc); > 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_set_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); > diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c > index 2f0eed9..e53b5f5 100644 > --- a/tools/libxc/xc_psr.c > +++ b/tools/libxc/xc_psr.c > @@ -248,9 +248,9 @@ int xc_psr_cmt_enabled(xc_interface *xch) > > return 0; > } > -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_set_domain_data(xc_interface *xch, uint32_t domid, > + xc_psr_type type, uint32_t target, > + uint64_t data) > { > DECLARE_DOMCTL; > uint32_t cmd; > @@ -269,6 +269,9 @@ int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid, > case XC_PSR_CAT_L2_CBM: > cmd = XEN_DOMCTL_PSR_ALLOC_SET_L2_CBM; > break; > + case XC_PSR_MBA_THRTL: > + cmd = XEN_DOMCTL_PSR_ALLOC_SET_MBA_THRTL; > + break; > default: > errno = EINVAL; > return -1; > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c > index 78d5bc5..d3c3d42 100644 > --- a/tools/libxl/libxl_psr.c > +++ b/tools/libxl/libxl_psr.c > @@ -328,33 +328,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, > libxl_psr_cbm_type type, libxl_bitmap *target_map, > uint64_t cbm) > { > - GC_INIT(ctx); > - int rc; > - int socketid, nr_sockets; > - > - rc = libxl__count_physical_sockets(gc, &nr_sockets); > - if (rc) { > - LOGED(ERROR, domid, "failed to get system socket count"); > - goto out; > - } > - > - libxl_for_each_set_bit(socketid, *target_map) { > - xc_psr_type xc_type; > - > - if (socketid >= nr_sockets) > - break; > - > - 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_alloc_log_err_msg(gc, errno, type); > - rc = ERROR_FAIL; > - } > - } > - > -out: > - GC_FREE; > - return rc; > + return libxl_psr_set_val(ctx, domid, type, target_map, cbm); > } > > int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid, > @@ -458,7 +432,33 @@ int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, > libxl_psr_type type, libxl_bitmap *target_map, > uint64_t val) > { > - return ERROR_FAIL; > + GC_INIT(ctx); > + int rc; > + int socketid, nr_sockets; You could fit them all in a single line. > + rc = libxl__count_physical_sockets(gc, &nr_sockets); > + if (rc) { > + LOG(ERROR, "failed to get system socket count"); > + goto out; > + } > + > + libxl_for_each_set_bit(socketid, *target_map) { > + xc_psr_type xc_type; > + > + if (socketid >= nr_sockets) > + break; > + > + xc_type = libxl__psr_type_to_libxc_psr_type(type); > + if (xc_psr_set_domain_data(ctx->xch, domid, xc_type, > + socketid, val)) { > + libxl__psr_alloc_log_err_msg(gc, errno, type); > + rc = ERROR_FAIL; > + } > + } > + > +out: > + GC_FREE; > + return rc; > } > > int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, > diff --git a/tools/xl/xl.h b/tools/xl/xl.h > index 3389df9..3f99b6b 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_set(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 cdc2349..9d45d3b 100644 > --- a/tools/xl/xl_cmdtable.c > +++ b/tools/xl/xl_cmdtable.c > @@ -560,6 +560,12 @@ struct cmd_spec cmd_table[] = { > "[options] <Domain>", > "-l <level> Specify the cache level to process, otherwise L3 cache is processed\n" > }, > + { "psr-mba-set", > + &main_psr_mba_set, 0, 1, > + "Set throttling value (THRTL) for a domain", > + "[options] <Domain> <THRTL>", > + "-s <socket> Specify the socket to process, otherwise all sockets are processed\n" > + }, > { "psr-mba-show", > &main_psr_mba_show, 0, 1, > "Show Memory Bandwidth Allocation information", > diff --git a/tools/xl/xl_psr.c b/tools/xl/xl_psr.c > index 46b7788..a648b1a 100644 > --- a/tools/xl/xl_psr.c > +++ b/tools/xl/xl_psr.c > @@ -552,6 +552,61 @@ int main_psr_mba_show(int argc, char **argv) > return psr_val_show(domid, LIBXL_PSR_FEAT_TYPE_MBA, 0); > } > > +int main_psr_mba_set(int argc, char **argv) > +{ > + uint32_t domid; > + libxl_psr_type type; > + uint64_t thrtl; > + int ret, opt = 0; > + libxl_bitmap target_map; > + char *value; > + libxl_string_list socket_list; > + unsigned long start, end; > + unsigned int i, j, len; > + > + static const struct option opts[] = { > + {"socket", 1, 0, 's'}, > + COMMON_LONG_OPTS > + }; > + > + libxl_socket_bitmap_alloc(ctx, &target_map, 0); > + libxl_bitmap_set_none(&target_map); > + > + SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-mba-set", 0) { > + case 's': > + trim(isspace, optarg, &value); > + split_string_into_string_list(value, ",", &socket_list); > + len = libxl_string_list_length(&socket_list); > + for (i = 0; i < len; i++) { > + parse_range(socket_list[i], &start, &end); Indentation. > + for (j = start; j <= end; j++) > + libxl_bitmap_set(&target_map, j); > + } > + > + libxl_string_list_dispose(&socket_list); > + free(value); > + break; > + } > + > + type = LIBXL_PSR_CBM_TYPE_MBA_THRTL; > + > + if (libxl_bitmap_is_empty(&target_map)) > + libxl_bitmap_set_any(&target_map); > + > + if (argc != optind + 2) { > + help("psr-mba-set"); > + return 2; > + } Can you do this check at the beginning of the function? Also why return 2 instead of EXIT_FAILURE? Thanks, Roger.
On 17-09-19 12:30:59, Roger Pau Monn� wrote: > > int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid, > > @@ -458,7 +432,33 @@ int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, > > libxl_psr_type type, libxl_bitmap *target_map, > > uint64_t val) > > { > > - return ERROR_FAIL; > > + GC_INIT(ctx); > > + int rc; > > + int socketid, nr_sockets; > > You could fit them all in a single line. > Yes, thanks. > > +int main_psr_mba_set(int argc, char **argv) > > +{ > > + uint32_t domid; > > + libxl_psr_type type; > > + uint64_t thrtl; > > + int ret, opt = 0; > > + libxl_bitmap target_map; > > + char *value; > > + libxl_string_list socket_list; > > + unsigned long start, end; > > + unsigned int i, j, len; > > + > > + static const struct option opts[] = { > > + {"socket", 1, 0, 's'}, > > + COMMON_LONG_OPTS > > + }; > > + > > + libxl_socket_bitmap_alloc(ctx, &target_map, 0); > > + libxl_bitmap_set_none(&target_map); > > + > > + SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-mba-set", 0) { > > + case 's': > > + trim(isspace, optarg, &value); > > + split_string_into_string_list(value, ",", &socket_list); > > + len = libxl_string_list_length(&socket_list); > > + for (i = 0; i < len; i++) { > > + parse_range(socket_list[i], &start, &end); > > Indentation. > Sorry. > > + for (j = start; j <= end; j++) > > + libxl_bitmap_set(&target_map, j); > > + } > > + > > + libxl_string_list_dispose(&socket_list); > > + free(value); > > + break; > > + } > > + > > + type = LIBXL_PSR_CBM_TYPE_MBA_THRTL; > > + > > + if (libxl_bitmap_is_empty(&target_map)) > > + libxl_bitmap_set_any(&target_map); > > + > > + if (argc != optind + 2) { > > + help("psr-mba-set"); > > + return 2; > > + } > > Can you do this check at the beginning of the function? Also why > return 2 instead of EXIT_FAILURE? > Yes, will move it to the beginning. Will return EXIT_FAILURE. > Thanks, Roger.
On Tue, Sep 19, 2017 at 12:30:59PM +0100, Roger Pau Monné wrote: > > + type = LIBXL_PSR_CBM_TYPE_MBA_THRTL; > > + > > + if (libxl_bitmap_is_empty(&target_map)) > > + libxl_bitmap_set_any(&target_map); > > + > > + if (argc != optind + 2) { > > + help("psr-mba-set"); > > + return 2; > > + } > > Can you do this check at the beginning of the function? Also why > return 2 instead of EXIT_FAILURE? > Returning 2 is used in several places when help string is printed. It isn't really consistent in xl though. I'm not too fussed whether it is 2 or EXIT_FAILURE.
On Wed, 2017-09-20 at 17:10 +0100, Wei Liu wrote: > On Tue, Sep 19, 2017 at 12:30:59PM +0100, Roger Pau Monné wrote: > > > + type = LIBXL_PSR_CBM_TYPE_MBA_THRTL; > > > + > > > + if (libxl_bitmap_is_empty(&target_map)) > > > + libxl_bitmap_set_any(&target_map); > > > + > > > + if (argc != optind + 2) { > > > + help("psr-mba-set"); > > > + return 2; > > > + } > > > > Can you do this check at the beginning of the function? Also why > > return 2 instead of EXIT_FAILURE? > > > > Returning 2 is used in several places when help string is > printed. It > isn't really consistent in xl though. > True. However, there's been efforts for improving that (consistency, I mean) and getting to a situation where only EXIT_SUCCESS or EXIT_FAILURE are used. I know it's not a done deal yet, and that it's being a slow process, but I think it would be nice if new code would help achieving that, instead than making it more difficult. :-) Then... > I'm not too fussed whether it is 2 or EXIT_FAILURE. > ...this is just my opinion (as one of the ones which was part of that effort, and that plans to get back to finish it at some point). You're the maintainer, your rules. :-P :-D Regards, Dario
On Thu, Sep 28, 2017 at 06:23:16PM +0200, Dario Faggioli wrote: > On Wed, 2017-09-20 at 17:10 +0100, Wei Liu wrote: > > On Tue, Sep 19, 2017 at 12:30:59PM +0100, Roger Pau Monné wrote: > > > > + type = LIBXL_PSR_CBM_TYPE_MBA_THRTL; > > > > + > > > > + if (libxl_bitmap_is_empty(&target_map)) > > > > + libxl_bitmap_set_any(&target_map); > > > > + > > > > + if (argc != optind + 2) { > > > > + help("psr-mba-set"); > > > > + return 2; > > > > + } > > > > > > Can you do this check at the beginning of the function? Also why > > > return 2 instead of EXIT_FAILURE? > > > > > > > Returning 2 is used in several places when help string is > > printed. It > > isn't really consistent in xl though. > > > True. However, there's been efforts for improving that (consistency, I > mean) and getting to a situation where only EXIT_SUCCESS or > EXIT_FAILURE are used. > > I know it's not a done deal yet, and that it's being a slow process, > but I think it would be nice if new code would help achieving that, > instead than making it more difficult. :-) > > Then... > > > I'm not too fussed whether it is 2 or EXIT_FAILURE. > > > ...this is just my opinion (as one of the ones which was part of that > effort, and that plans to get back to finish it at some point). You're > the maintainer, your rules. :-P :-D > This makes sense. If Yi doesn't end up changing it to EXIT_FAILURE, I will do that myself.
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index eef06be..21dac2f 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2499,9 +2499,9 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu, uint64_t *tsc); 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_set_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); diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c index 2f0eed9..e53b5f5 100644 --- a/tools/libxc/xc_psr.c +++ b/tools/libxc/xc_psr.c @@ -248,9 +248,9 @@ int xc_psr_cmt_enabled(xc_interface *xch) return 0; } -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_set_domain_data(xc_interface *xch, uint32_t domid, + xc_psr_type type, uint32_t target, + uint64_t data) { DECLARE_DOMCTL; uint32_t cmd; @@ -269,6 +269,9 @@ int xc_psr_cat_set_domain_data(xc_interface *xch, uint32_t domid, case XC_PSR_CAT_L2_CBM: cmd = XEN_DOMCTL_PSR_ALLOC_SET_L2_CBM; break; + case XC_PSR_MBA_THRTL: + cmd = XEN_DOMCTL_PSR_ALLOC_SET_MBA_THRTL; + break; default: errno = EINVAL; return -1; diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c index 78d5bc5..d3c3d42 100644 --- a/tools/libxl/libxl_psr.c +++ b/tools/libxl/libxl_psr.c @@ -328,33 +328,7 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, libxl_psr_cbm_type type, libxl_bitmap *target_map, uint64_t cbm) { - GC_INIT(ctx); - int rc; - int socketid, nr_sockets; - - rc = libxl__count_physical_sockets(gc, &nr_sockets); - if (rc) { - LOGED(ERROR, domid, "failed to get system socket count"); - goto out; - } - - libxl_for_each_set_bit(socketid, *target_map) { - xc_psr_type xc_type; - - if (socketid >= nr_sockets) - break; - - 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_alloc_log_err_msg(gc, errno, type); - rc = ERROR_FAIL; - } - } - -out: - GC_FREE; - return rc; + return libxl_psr_set_val(ctx, domid, type, target_map, cbm); } int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid, @@ -458,7 +432,33 @@ int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, libxl_psr_type type, libxl_bitmap *target_map, uint64_t val) { - return ERROR_FAIL; + GC_INIT(ctx); + int rc; + int socketid, nr_sockets; + + rc = libxl__count_physical_sockets(gc, &nr_sockets); + if (rc) { + LOG(ERROR, "failed to get system socket count"); + goto out; + } + + libxl_for_each_set_bit(socketid, *target_map) { + xc_psr_type xc_type; + + if (socketid >= nr_sockets) + break; + + xc_type = libxl__psr_type_to_libxc_psr_type(type); + if (xc_psr_set_domain_data(ctx->xch, domid, xc_type, + socketid, val)) { + libxl__psr_alloc_log_err_msg(gc, errno, type); + rc = ERROR_FAIL; + } + } + +out: + GC_FREE; + return rc; } int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, diff --git a/tools/xl/xl.h b/tools/xl/xl.h index 3389df9..3f99b6b 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_set(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 cdc2349..9d45d3b 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -560,6 +560,12 @@ struct cmd_spec cmd_table[] = { "[options] <Domain>", "-l <level> Specify the cache level to process, otherwise L3 cache is processed\n" }, + { "psr-mba-set", + &main_psr_mba_set, 0, 1, + "Set throttling value (THRTL) for a domain", + "[options] <Domain> <THRTL>", + "-s <socket> Specify the socket to process, otherwise all sockets are processed\n" + }, { "psr-mba-show", &main_psr_mba_show, 0, 1, "Show Memory Bandwidth Allocation information", diff --git a/tools/xl/xl_psr.c b/tools/xl/xl_psr.c index 46b7788..a648b1a 100644 --- a/tools/xl/xl_psr.c +++ b/tools/xl/xl_psr.c @@ -552,6 +552,61 @@ int main_psr_mba_show(int argc, char **argv) return psr_val_show(domid, LIBXL_PSR_FEAT_TYPE_MBA, 0); } +int main_psr_mba_set(int argc, char **argv) +{ + uint32_t domid; + libxl_psr_type type; + uint64_t thrtl; + int ret, opt = 0; + libxl_bitmap target_map; + char *value; + libxl_string_list socket_list; + unsigned long start, end; + unsigned int i, j, len; + + static const struct option opts[] = { + {"socket", 1, 0, 's'}, + COMMON_LONG_OPTS + }; + + libxl_socket_bitmap_alloc(ctx, &target_map, 0); + libxl_bitmap_set_none(&target_map); + + SWITCH_FOREACH_OPT(opt, "s:", opts, "psr-mba-set", 0) { + case 's': + trim(isspace, optarg, &value); + split_string_into_string_list(value, ",", &socket_list); + len = libxl_string_list_length(&socket_list); + for (i = 0; i < len; i++) { + parse_range(socket_list[i], &start, &end); + for (j = start; j <= end; j++) + libxl_bitmap_set(&target_map, j); + } + + libxl_string_list_dispose(&socket_list); + free(value); + break; + } + + type = LIBXL_PSR_CBM_TYPE_MBA_THRTL; + + if (libxl_bitmap_is_empty(&target_map)) + libxl_bitmap_set_any(&target_map); + + if (argc != optind + 2) { + help("psr-mba-set"); + return 2; + } + + domid = find_domain(argv[optind]); + thrtl = strtoll(argv[optind + 1], NULL , 0); + + ret = libxl_psr_set_val(ctx, domid, type, &target_map, thrtl); + + libxl_bitmap_dispose(&target_map); + return ret; +} + static int psr_mba_hwinfo(void) { int rc;
This patch implements new generic set value interfaces in libxc and libxl. These interfaces are suitable for all allocation features. It also adds a new MBA set value command in xl. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v3: - add 'const' for 'opts[]' in 'main_psr_mba_set'. (suggested by Roger Pau Monné) - replace 'libxl_psr_cbm_type' to 'libxl_psr_type' for newly defined interfaces. (suggested by Roger Pau Monné) --- tools/libxc/include/xenctrl.h | 6 ++--- tools/libxc/xc_psr.c | 9 ++++--- tools/libxl/libxl_psr.c | 56 +++++++++++++++++++++---------------------- tools/xl/xl.h | 1 + tools/xl/xl_cmdtable.c | 6 +++++ tools/xl/xl_psr.c | 55 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 99 insertions(+), 34 deletions(-)