From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH v3 8/8] tools: add tools support for Intel CAT Date: Wed, 1 Apr 2015 15:55:49 +0800 Message-ID: <20150401075549.GA2696@pengc-linux.bj.intel.com> References: <1427373505-9303-1-git-send-email-chao.p.peng@linux.intel.com> <1427373505-9303-9-git-send-email-chao.p.peng@linux.intel.com> <1427819334.2115.194.camel@citrix.com> Reply-To: Chao Peng Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1427819334.2115.194.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: keir@xen.org, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, will.auld@intel.com, JBeulich@suse.com, wei.liu2@citrix.com, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Tue, Mar 31, 2015 at 05:28:54PM +0100, Ian Campbell wrote: > On Thu, 2015-03-26 at 20:38 +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] > > Set cache capacity bitmasks(CBM) for a domain. > > - xl psr-cat-show > > Show Cache Allocation Technology information. > > Please could you show an example of the output from this one. I did put the sample output in the cover letter, but sounds like here is the right place. > > > > > Signed-off-by: Chao Peng > > --- > > + > > +=item B [I] [I] > > + > > +Set cache capacity bitmasks(CBM) for a domain. > > What is the syntax of these bitmaps, and where do I pass them? [I] is missing here. > > I think there is also a bunch of terminology (CBM, COS) which need > explaining, otherwise no one will know how to use it. Perhaps that > belongs in a separate document or the wiki though? Sounds it's worthy to create docs/misc/xl-psr.markdown. > > + > > +#define LIBXL_PSR_TARGET_ALL (~0U) > > +int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, > > + libxl_psr_cat_type type, uint32_t target, > > + uint64_t cbm); > > +int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid, > > + libxl_psr_cat_type type, uint32_t target, > > + uint64_t *cbm_r); > > What are the units of the various cbm* > > If they are now more precisely typed (i.e. not the opaque data from last > time) then is the type parameter still needed? 'type' is still used because in the future the possible value can be L3_CODE_CBM/L3_DATA_CBM or even L2_CBM. We want to keep it common. > > > +int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, uint32_t socket, > > + uint32_t *cos_max_r, uint32_t *cbm_len_r); > > Is there going to be any user documentation regarding what cos and cbm > are and how to interpret them and set them? So I'd like to create docs/misc/xl-psr.markdown to answer all these questions. > > > @@ -247,6 +290,75 @@ out: > > return rc; > > } > > > > +int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid, > > + libxl_psr_cat_type type, uint32_t target, > > + uint64_t cbm) > > +{ > > + GC_INIT(ctx); > > + int rc; > > + > > + uint32_t i, nr_sockets; > > + > > + if (target != LIBXL_PSR_TARGET_ALL) { > > + rc = xc_psr_cat_set_domain_data(ctx->xch, domid, type, target, cbm); > > + if (rc < 0) { > > + libxl__psr_cat_log_err_msg(gc, errno); > > + rc = ERROR_FAIL; > > + } > > + } else { > > + nr_sockets = libxl_count_physical_sockets(ctx); > > + if (nr_sockets == 0) { > > + LOGE(ERROR, "failed to get system socket count"); > > + rc = ERROR_FAIL; > > + goto out; > > + } > > + for (i = 0; i < nr_sockets; i++) { > > + rc = xc_psr_cat_set_domain_data(ctx->xch, domid, type, i, cbm); > > + if (rc < 0) { > > + libxl__psr_cat_log_err_msg(gc, errno); > > + rc = ERROR_FAIL; > > You neither error out nor latch this value, so you only return the > status of the final socket. > > I think you probably want to exit on first error. And depending on the > semantics of this stuff you may also need to unwind any work already > done. That's right, exit on first error is my initial purpose. > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > > index f26a02d..ff963df 100644 > > --- a/tools/libxl/xl_cmdimpl.c > > +++ b/tools/libxl/xl_cmdimpl.c > > @@ -8038,6 +8038,152 @@ int main_psr_cmt_show(int argc, char **argv) > > } > > #endif > > > > +#ifdef LIBXL_HAVE_PSR_CAT > > +static void psr_cat_print_one_domain_cbm(libxl_dominfo *dominfo, > > + uint32_t socket) > > +{ > > + char *domain_name; > > + uint64_t cbm; > > + > > + domain_name = libxl_domid_to_name(ctx, dominfo->domid); > > AFAICT you use dominfo here for domifo->domid, but you looked it up with > libxl_domain_info(,... domid) so you already had that in your hand. > > IOW I think you can just pass the domid to this function, and omit the > call to libxl_domain_info in the callers, or use list->domid in the case > you have that in hand. Agreed. > > > + printf("%5d%25s", dominfo->domid, domain_name); > > + free(domain_name); > > + > > + if (!libxl_psr_cat_get_cbm(ctx, dominfo->domid, > > + LIBXL_PSR_CAT_TYPE_L3_CBM, socket, &cbm)) > > + printf("%#16"PRIx64, cbm); > > + > > + printf("\n"); > > +} > > + > > +static int psr_cat_print_socket(uint32_t domid, uint32_t socket) > > +{ > > + uint32_t l3_cache_size, cos_max, cbm_len; > > + int rc; > > + > > + rc = libxl_psr_cmt_get_l3_cache_size(ctx, socket, &l3_cache_size); > > + if (rc) { > > + fprintf(stderr, "Failed to get l3 cache size for socket:%d\n", socket); > > + return -1; > > + } > > + > > + rc = libxl_psr_cat_get_l3_info(ctx, socket, &cos_max, &cbm_len); > > Would an interface which returns a list with information for all sockets > not be more convenient? If this one returns all sockets but not a specified socket data (which I agreed) and not consider legacy cmt code, then I think I can make libxl_count_physical_sockets() private and move it to libxl/libxl_psr.c. Chao