[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 7/7] tools: add tools support for Intel CAT
On Thu, 2015-03-19 at 18:41 +0800, Chao Peng wrote: > This is the xc/xl changes to support Intel Cache Allocation > Technology(CAT). Two commands are introduced: > - xl psr-cat-cbm-set [-s socket] <domain> <cbm> > Set cache capacity bitmasks(CBM) for a domain. > - xl psr-cat-show <domain> > Show Cache Allocation Technology information. > > Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx> > --- > tools/libxc/include/xenctrl.h | 15 +++ > tools/libxc/xc_psr.c | 74 ++++++++++++++ At the libxc level this looks like a reasonable wrapping of the hypercall interface, so if the hypervisor folks are happy with that then I'm happy with this. > tools/libxl/libxl.h | 18 ++++ At the libxl level things seem to be rather opaque to me, i.e. what is domain_data? what does it contain? does it have any more semantics than a 64-bit number? What future new values might there be for libxl_psr_cat_type? > tools/libxl/libxl_psr.c | 107 ++++++++++++++++++-- > tools/libxl/libxl_types.idl | 4 + > tools/libxl/xl.h | 4 + > tools/libxl/xl_cmdimpl.c | 225 > ++++++++++++++++++++++++++++++++++++++++-- > tools/libxl/xl_cmdtable.c | 13 +++ You've missed updating the manpage. > 8 files changed, 445 insertions(+), 15 deletions(-) > > +#ifdef LIBXL_HAVE_PSR_CAT > +int libxl_psr_cat_set_domain_data(libxl_ctx *ctx, uint32_t domid, > + libxl_psr_cat_type type, uint32_t target, > + uint64_t data); > +int libxl_psr_cat_get_domain_data(libxl_ctx *ctx, uint32_t domid, > + libxl_psr_cat_type type, uint32_t target, > + uint64_t *data_r); > +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, uint32_t socket, > + uint32_t *cos_max_r, uint32_t *cbm_len_r); > +#endif > + > [...] > +static uint32_t get_phy_socket_num(void) I think this would be better named "count_physical_sockets" or something, otherwise I might think it takes a lock. > +{ > + int rc; > + uint32_t nr_sockets; uint32_t nr_sockets = 0; > + libxl_physinfo info; > + libxl_physinfo_init(&info); > + rc = libxl_get_physinfo(ctx, &info); Then: if (!rc) nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket; libxl_physinfo_dispose(&info); return nr_sockets; means you only have one return path to do clean up on. Also, does this function need to be in an ifdef, since it isn't called unless LIBXL_HAVE_... is set? (IOW: does this compile for ARM) > @@ -8057,6 +8070,202 @@ int main_psr_cmt_show(int argc, char **argv) > } > #endif > > +#ifdef LIBXL_HAVE_PSR_CAT > +static int psr_cat_l3_cbm_set(uint32_t domid, uint32_t socket, uint64_t cbm) > +{ > + int rc; > + uint32_t i, nr_sockets; > + > + if (socket != ALL_SOCKETS) { > + return libxl_psr_cat_set_domain_data(ctx, domid, > + LIBXL_PSR_CAT_TYPE_L3_CBM, > + socket, cbm); > + } else { > + nr_sockets = get_phy_socket_num(); I wonder if the libxl API ought to allow for an "all sockets" argument and then do all this internally instead of making the callers do it? > + if (nr_sockets == 0) { > + fprintf(stderr, "Failed getting physinfo\n"); > + return -1; > + } > + for (i = 0; i < nr_sockets; i++) { > + rc = libxl_psr_cat_set_domain_data(ctx, domid, > + LIBXL_PSR_CAT_TYPE_L3_CBM, > + i, cbm); > + if (rc < 0) { > + fprintf(stderr, "Failed to set l3 cbm for socket:%d\n", i); > + return -1; Indentation. > + } > + } > + } > + return 0; > +} > + > +struct psr_cat_l3_info > +{ > + uint32_t cos_max; > + uint32_t cbm_len; > +}; Is it every useful to retrieve these independently? Perhaps this struct should be in the libxl API and be what is passed to libxl_psr_cat_get_l3_info? > + > +static void psr_cat_print_domain_info(libxl_dominfo *dominfo, > + struct psr_cat_l3_info *l3_info, > + uint32_t nr_sockets) > +{ > + char *domain_name; > + uint32_t socketid; > + uint64_t cbm; > + > + domain_name = libxl_domid_to_name(ctx, dominfo->domid); > + printf("%-40s %5d", domain_name, dominfo->domid); > + free(domain_name); > + > + for (socketid = 0; socketid < nr_sockets; socketid++) { > + if (l3_info[socketid].cbm_len > 0 && > + !libxl_psr_cat_get_domain_data(ctx, dominfo->domid, > + LIBXL_PSR_CAT_TYPE_L3_CBM, > + socketid, &cbm) ) > + printf("%#16"PRIx64, cbm); Print socket id too? > + > + /* Header */ > + printf("%-40s %5s", "Name", "ID"); > + for (socketid = 0; socketid < nr_sockets; socketid++) > + printf("%14s %d", "Socket", socketid); > + printf("\n"); > + > + /* Total L3 cache size */ > + printf("%-46s", "Total L3 Cache Size"); How wide are these lines going to be? Can we try and keep it to <80 columns? Perhaps you could include an example of the output of each of the show functions in the commit message? > + for (socketid = 0; socketid < nr_sockets; socketid++) { > + rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size); > + if (rc < 0) { > + fprintf(stderr, "Failed to get system l3 cache size for > socket:%d\n", > + socketid); > + return -1; > + } > + printf("%13u KB", l3_cache_size); > + } > + printf("\n"); > + > + /* Max COS and CBM length */ > + l3_info = malloc(sizeof(l3_info) * nr_sockets); > + //if (!l3_info) Leftover. > diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c > index 22ab63b..ffaf4ed 100644 > --- a/tools/libxl/xl_cmdtable.c > +++ b/tools/libxl/xl_cmdtable.c > @@ -542,6 +542,19 @@ struct cmd_spec cmd_table[] = { > "\"total_mem_bandwidth\": Show total memory bandwidth(KB/s)\n" > "\"local_mem_bandwidth\": Show local memory bandwidth(KB/s)\n", > }, > + { "psr-cat-cbm-set", > + &main_psr_cat_cbm_set, 0, 1, > + "Set cache capacity bitmasks(CBM) for a domain", > + "-s <socket> Specify the socket to process, all sockets when not" "Specify the socket to process, otherwise all sockets are processed" Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |