|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v18 10/10] tools: CMDs and APIs for Cache Monitoring Technology
On Thu, Oct 02, 2014 at 10:57:50AM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 30, 2014 at 06:49:12PM +0800, Chao Peng wrote:
> > Introduced some new xl commands to enable/disable Cache Monitoring
> > Technology(CMT) feature.
> >
> > The following two commands is to attach/detach the CMT feature
> > to/from a certain domain.
> >
> > $ xl psr-cmt-attach domid
> > $ xl psr-cmt-detach domid
> >
> > This command is to display the CMT information, such as L3 cache
> > occupancy.
> >
> > $ xl psr-cmt-show cache_occupancy <domid>
>
> You need to CC the toolstack maintainers. You can
> use scripts/get_maintainers.pl file to find them.
>
> I am CC-ing them for you.
>
Thanks Konrad.
> >
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> > ---
> > docs/man/xl.pod.1 | 25 +++++
> > tools/libxc/Makefile | 1 +
> > tools/libxc/xc_msr_x86.h | 36 ++++++++
> > tools/libxc/xc_psr.c | 214
> > +++++++++++++++++++++++++++++++++++++++++++
> > tools/libxc/xenctrl.h | 17 ++++
> > tools/libxl/Makefile | 2 +-
> > tools/libxl/libxl.h | 19 ++++
> > tools/libxl/libxl_psr.c | 184 +++++++++++++++++++++++++++++++++++++
> > tools/libxl/libxl_types.idl | 4 +
> > tools/libxl/libxl_utils.c | 28 ++++++
> > tools/libxl/xl.h | 3 +
> > tools/libxl/xl_cmdimpl.c | 132 ++++++++++++++++++++++++++
> > tools/libxl/xl_cmdtable.c | 17 ++++
> > 13 files changed, 681 insertions(+), 1 deletion(-)
> > create mode 100644 tools/libxc/xc_msr_x86.h
> > create mode 100644 tools/libxc/xc_psr.c
> > create mode 100644 tools/libxl/libxl_psr.c
> >
> > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> > index f1e95db..ef8cd24 100644
> > --- a/docs/man/xl.pod.1
> > +++ b/docs/man/xl.pod.1
> > @@ -1387,6 +1387,31 @@ Load FLASK policy from the given policy file. The
> > initial policy is provided to
> > the hypervisor as a multiboot module; this command allows runtime updates
> > to the
> > policy. Loading new security policy will reset runtime changes to device
> > labels.
> >
> > +=head1 Cache Monitoring Technology
> > +
> > +Some new hardware may offer monitoring capability in each logical
> > processor to
You've specified some models in other patches, can you list them here as
well?
> > +measure specific platform shared resource metric, for example, L3 cache
> > +occupancy. In Xen implementation, the monitoring granularity is domain
> > level.
> > +To monitor a specific domain, just attach the domain id with the monitoring
> > +service. When the domain doesn't need to be monitored any more, detach the
> > +domain id from the monitoring service.
> > +
> > +=over 4
> > +
> > +=item B<psr-cmt-attach> [I<domain-id>]
> > +
> > +attach: Attach the platform shared resource monitoring service to a domain.
> > +
> > +=item B<psr-cmt-detach> [I<domain-id>]
> > +
> > +detach: Detach the platform shared resource monitoring service from a
> > domain.
> > +
> > +=item B<psr-cmt-show> [I<psr-monitor-type>] [I<domain-id>]
> > +
> > +Show monitoring data for a certain domain or all domains. Current supported
> > +monitor types are:
> > + - "cache-occupancy": showing the L3 cache occupancy.
> > +
> > =back
> >
> > =head1 TO BE DOCUMENTED
> > diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> > index dde6109..d8cc21b 100644
> > --- a/tools/libxc/Makefile
> > +++ b/tools/libxc/Makefile
> > @@ -35,6 +35,7 @@ CTRL_SRCS-y += xc_kexec.c
> > CTRL_SRCS-y += xtl_core.c
> > CTRL_SRCS-y += xtl_logger_stdio.c
> > CTRL_SRCS-y += xc_resource.c
> > +CTRL_SRCS-y += xc_psr.c
Shouldn't this be CTRL_SRCS-$(CONFIG_X86)?
Since you've done it for libxl changes I'm assuming it is.
> > CTRL_SRCS-$(CONFIG_X86) += xc_pagetab.c
> > CTRL_SRCS-$(CONFIG_Linux) += xc_linux.c xc_linux_osdep.c
> > CTRL_SRCS-$(CONFIG_FreeBSD) += xc_freebsd.c xc_freebsd_osdep.c
> > diff --git a/tools/libxc/xc_msr_x86.h b/tools/libxc/xc_msr_x86.h
> > new file mode 100644
> > index 0000000..1e0ee99
> > --- /dev/null
> > +++ b/tools/libxc/xc_msr_x86.h
> > @@ -0,0 +1,36 @@
> > +/*
> > + * xc_msr_x86.h
> > + *
> > + * MSR definition macros
> > + *
> > + * Copyright (C) 2014 Intel Corporation
> > + * Author Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU Lesser General Public License as published
> > + * by the Free Software Foundation; version 2.1 only. with the special
> > + * exception on linking described in file LICENSE.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU Lesser General Public License for more details.
> > + */
> > +
> > +#ifndef XC_MSR_X86_H
> > +#define XC_MSR_X86_H
> > +
> > +#define MSR_IA32_QOSEVTSEL 0x00000c8d
> > +#define MSR_IA32_QMC 0x00000c8e
> > +
> > +#endif
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
> > new file mode 100644
> > index 0000000..182ddd4
> > --- /dev/null
> > +++ b/tools/libxc/xc_psr.c
> > @@ -0,0 +1,214 @@
> > +/*
> > + * xc_psr.c
> > + *
Assuming this file contains mostly plumbing code and the hypervisor
interface is agreed upon, I just skim this file and don't spot obvious
problem.
> > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> > index 6edb738..b0ab6f4 100644
> > --- a/tools/libxc/xenctrl.h
> > +++ b/tools/libxc/xenctrl.h
> > @@ -2672,6 +2672,23 @@ struct xc_resource_op {
> > typedef struct xc_resource_op xc_resource_op_t;
> > int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t
> > *ops);
> >
> > +enum xc_psr_cmt_type {
> > + XC_PSR_CMT_L3_OCCUPANCY,
> > +};
> > +typedef enum xc_psr_cmt_type xc_psr_cmt_type;
> > +int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid);
> > +int xc_psr_cmt_detach(xc_interface *xch, uint32_t domid);
> > +int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
> > + uint32_t *rmid);
> > +int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
> > +int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
> > + uint32_t *upscaling_factor);
> > +int xc_psr_cmt_get_l3_cache_size(xc_interface *xch,
> > + uint32_t *l3_cache_size);
> > +int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
> > + uint32_t cpu, uint32_t psr_cmt_type, uint64_t *monitor_data);
> > +int xc_psr_cmt_enabled(xc_interface *xch);
> > +
Make them x86 only?
> > #endif /* XENCTRL_H */
> >
> > /*
> > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> > index 990414b..fd9ae28 100644
> > --- a/tools/libxl/Makefile
> > +++ b/tools/libxl/Makefile
> > @@ -43,7 +43,7 @@ LIBXL_OBJS-y += libxl_blktap2.o
> > else
> > LIBXL_OBJS-y += libxl_noblktap2.o
> > endif
> > -LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o
> > +LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
> > LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o
> >
> > ifeq ($(CONFIG_NetBSD),y)
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index bc68cac..5acc933 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -640,6 +640,13 @@ typedef uint8_t libxl_mac[6];
> > #define LIBXL_MAC_BYTES(mac) mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]
> > void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
> >
> > +/*
> > + * LIBXL_HAVE_PSR_CMT
> > + *
> > + * If this is defined, the Cache Monitoring Technology feature is
> > supported.
> > + */
> > +#define LIBXL_HAVE_PSR_CMT 1
> > +
This should also be made x86 only.
> > typedef char **libxl_string_list;
> > void libxl_string_list_dispose(libxl_string_list *sl);
> > int libxl_string_list_length(const libxl_string_list *sl);
> > @@ -1380,6 +1387,18 @@ bool libxl_ms_vm_genid_is_zero(const
> > libxl_ms_vm_genid *id);
> > void libxl_ms_vm_genid_copy(libxl_ctx *ctx, libxl_ms_vm_genid *dst,
> > libxl_ms_vm_genid *src);
> >
> > +int libxl_get_socket_cpu(libxl_ctx *ctx, uint32_t socketid);
> > +
This can be made internal in libxl_internal.{c,h}. Have a look at other
internal functions.
> > +int libxl_psr_cmt_attach(libxl_ctx *ctx, uint32_t domid);
> > +int libxl_psr_cmt_detach(libxl_ctx *ctx, uint32_t domid);
> > +int libxl_psr_cmt_domain_attached(libxl_ctx *ctx, uint32_t domid);
> > +int libxl_psr_cmt_enabled(libxl_ctx *ctx);
> > +int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid);
> > +int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx,
> > + uint32_t *l3_cache_size);
> > +int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
> > + uint32_t socketid, uint32_t *l3_cache_occupancy);
> > +
> > /* misc */
> >
> > /* Each of these sets or clears the flag according to whether the
> > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > new file mode 100644
> > index 0000000..5551be8
> > --- /dev/null
> > +++ b/tools/libxl/libxl_psr.c
> > @@ -0,0 +1,184 @@
> > +/*
> > + * Copyright (C) 2014 Intel Corporation
> > + * Author Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU Lesser General Public License as published
> > + * by the Free Software Foundation; version 2.1 only. with the special
> > + * exception on linking described in file LICENSE.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU Lesser General Public License for more details.
> > + */
> > +
> > +#include "libxl_osdeps.h" /* must come before any other headers */
> > +#include "libxl_internal.h"
> > +
> > +
> > +#define IA32_QM_CTR_ERROR_MASK (0x3ul << 62)
> > +
> > +static void libxl_psr_cmt_err_msg(libxl_ctx *ctx, int err)
Internal functions are normally named libxl__FOO and just take a gc.
> > +{
> > + GC_INIT(ctx);
Redundant as this is never used.
> > +
> > + char *msg;
> > +
> > + switch (err) {
> > + case ENOSYS:
> > + msg = "unsupported operation";
> > + break;
> > + case ENODEV:
> > + msg = "Cache Monitoring Technology is not supported in this
> > system";
> > + break;
> > + case EEXIST:
> > + msg = "Cache Monitoring Technology is already attached to this
> > domain";
> > + break;
> > + case ENOENT:
> > + msg = "Cache Monitoring Technology is not attached to this domain";
> > + break;
> > + case EUSERS:
> > + msg = "there is no free RMID available";
> > + break;
> > + case ESRCH:
> > + msg = "is this Domain ID valid?";
Just use "Domain ID not valid".
> > + break;
> > + case EFAULT:
> > + msg = "failed to exchange data with Xen";
> > + break;
> > + default:
> > + msg = "unknown error";
> > + break;
> > + }
> > +
> > + LOGE(ERROR, "%s", msg);
> > +
If you have to do the logging here, this function should be renamed to
libxl__psc_cmt_log_err_msg.
> > + GC_FREE;
> > +}
> > +
[...]
> > +int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
> > + uint32_t socketid, uint32_t *l3_cache_occupancy)
> > +{
> > + GC_INIT(ctx);
> > +
> > + unsigned int rmid;
> > + uint32_t upscaling_factor;
> > + uint64_t monitor_data;
> > + int cpu, rc;
> > + xc_psr_cmt_type type;
> > +
> > + rc = xc_psr_cmt_get_domain_rmid(ctx->xch, domid, &rmid);
> > + if (rc < 0 || rmid == 0) {
> > + LOGE(ERROR, "fail to get the domain rmid, "
> > + "or domain is not attached with platform QoS monitoring
> > service");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + cpu = libxl_get_socket_cpu(ctx, socketid);
> > + if (cpu < 0) {
> > + LOGE(ERROR, "failed to get socket cpu");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + type = XC_PSR_CMT_L3_OCCUPANCY;
> > + rc = xc_psr_cmt_get_data(ctx->xch, rmid, cpu, type, &monitor_data);
> > + if (rc < 0) {
> > + LOGE(ERROR, "failed to get monitoring data");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + rc = xc_psr_cmt_get_l3_upscaling_factor(ctx->xch, &upscaling_factor);
> > + if (rc < 0) {
> > + LOGE(ERROR, "failed to get L3 upscaling factor");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + *l3_cache_occupancy = upscaling_factor * monitor_data / 1024;
> > + rc = 0;
> > +out:
> > + GC_FREE;
> > + return 0;
return rc;
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index f1fcbc3..27a5022 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -635,3 +635,7 @@ libxl_event = Struct("event",[
> > ])),
> > ("domain_create_console_available", None),
> > ]))])
> > +
> > +libxl_psr_cmt_type = Enumeration("psr_cmt_type", [
> > + (1, "CACHE_OCCUPANCY"),
> > + ])
> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > index 58df4f3..318a996 100644
> > --- a/tools/libxl/libxl_utils.c
> > +++ b/tools/libxl/libxl_utils.c
> > @@ -1065,6 +1065,34 @@ int libxl__random_bytes(libxl__gc *gc, uint8_t *buf,
> > size_t len)
> > return ret;
> > }
> >
> > +int libxl_get_socket_cpu(libxl_ctx *ctx, uint32_t socketid)
To be honest from look of its name I cannot tell immediately what this
function does.
Also reading the code it does some kind of balancing that needs to be
explained.
Maybe you can tell us the purpose of this function so that we can either
have better code comment or better name.
> > +{
> > + int i, j, cpu, nr_cpus;
> > + libxl_cputopology *topology;
> > + int *socket_cpus;
> > +
> > + topology = libxl_get_cpu_topology(ctx, &nr_cpus);
You might also want to check libxl.h for paradigm on how to use a type.
Normally that means _init at the beginning then _dispose at the end.
> > + if (!topology)
> > + return ERROR_FAIL;
> > +
> > + socket_cpus = malloc(sizeof(int) * nr_cpus);
Please use libxl__malloc(gc...) and friends for all memory allocations.
> > + if (!socket_cpus) {
> > + libxl_cputopology_list_free(topology, nr_cpus);
> > + return ERROR_FAIL;
> > + }
... then you can remove these three lines.
> > +
> > + for (i = 0, j = 0; i < nr_cpus; i++)
> > + if (topology[i].socket == socketid)
> > + socket_cpus[j++] = i;
> > +
> > + /* load balance among cpus in the same socket. */
> > + cpu = socket_cpus[rand() % j];
> > +
> > + free(socket_cpus);
And remove this as well..
> > + libxl_cputopology_list_free(topology, nr_cpus);
> > + return cpu;
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> > index 10a2e66..abb1fac 100644
> > --- a/tools/libxl/xl.h
> > +++ b/tools/libxl/xl.h
> > @@ -110,6 +110,9 @@ int main_loadpolicy(int argc, char **argv);
> > int main_remus(int argc, char **argv);
> > #endif
> > int main_devd(int argc, char **argv);
> > +int main_psr_cmt_attach(int argc, char **argv);
> > +int main_psr_cmt_detach(int argc, char **argv);
> > +int main_psr_cmt_show(int argc, char **argv);
> >
> > void help(const char *command);
> >
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 698b3bc..77f19d7 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -7428,6 +7428,138 @@ out:
> > return ret;
> > }
> >
> > +static int psr_cmt_show_cache_occupancy(uint32_t domid)
> > +{
> > + uint32_t i, socketid, nr_sockets, total_rmid;
> > + uint32_t l3_cache_size, l3_cache_occupancy;
> > + libxl_physinfo info;
> > + char *domain_name;
> > + int rc, print_header, nr_domains;
> > + libxl_dominfo *dominfo;
> > +
> > + if (!libxl_psr_cmt_enabled(ctx)) {
> > + printf("CMT is disabled in the system\n");
> > + return -1;
> > + }
Failure message should go to stderr, please use
fprintf(stderr, BLAH);
> > +
> > + rc = libxl_get_physinfo(ctx, &info);
Again, check libxl.h for paradigm of using a type.
Or have a look at libxl_domains() as an example.
> > + if (rc < 0) {
> > + printf("Failed getting physinfo, rc: %d\n", rc);
> > + return -1;
> > + }
> > + nr_sockets = info.nr_cpus / info.threads_per_core /
> > info.cores_per_socket;
> > +
> > + rc = libxl_psr_cmt_get_total_rmid(ctx, &total_rmid);
> > + if (rc < 0) {
> > + printf("Failed to get max RMID value\n");
fprintf(stderr, BLAH);
> > + return -1;
> > + }
> > +
> > + rc = libxl_psr_cmt_get_l3_cache_size(ctx, &l3_cache_size);
> > + if (rc < 0) {
> > + printf("Failed to get system l3 cache size\n");
fprintf(stderr, BLAH);
> > + return -1;
> > + }
> > +
> > + printf("Total RMID: %d\n", total_rmid);
> > + printf("Per-Socket L3 Cache Size: %d KB\n", l3_cache_size);
> > +
> > + print_header = 1;
> > + if (!(dominfo = libxl_list_domain(ctx, &nr_domains))) {
> > + fprintf(stderr, "libxl_list_domain failed.\n");
> > + return -1;
> > + }
As far as I can tell, this function can be used to either list all
domains or a single one.
In the latter case you still ask for information for every living domain
on this host. This is not optimal.
> > + for (i = 0; i < nr_domains; i++) {
> > + if (domid != ~0 && dominfo[i].domid != domid)
What is this domid != ~0 for? Need a comment on this.
> > + continue;
> > + if (!libxl_psr_cmt_domain_attached(ctx, dominfo[i].domid))
> > + continue;
> > + if (print_header) {
> > + printf("%-40s %5s", "Name", "ID");
> > + for (socketid = 0; socketid < nr_sockets; socketid++)
> > + printf("%14s %d", "Socket", socketid);
> > + printf("\n");
> > + print_header = 0;
> > + }
> > + domain_name = libxl_domid_to_name(ctx, dominfo[i].domid);
> > + printf("%-40s %5d", domain_name, dominfo[i].domid);
> > + free(domain_name);
> > + for (socketid = 0; socketid < nr_sockets; socketid++) {
> > + rc = libxl_psr_cmt_get_cache_occupancy(ctx, dominfo[i].domid,
> > + socketid, &l3_cache_occupancy);
> > + if ( !rc )
Coding style. Please remove extra spaces.
> > + printf("%13u KB", l3_cache_occupancy);
> > + }
> > + printf("\n");
> > + }
> > + libxl_dominfo_list_free(dominfo, nr_domains);
> > +
> > + return 0;
> > +}
> > +
> > +int main_psr_cmt_attach(int argc, char **argv)
[...]
> > +
> > +int main_psr_cmt_detach(int argc, char **argv)
[...]
> > +int main_psr_cmt_show(int argc, char **argv)
[...]
Assuming they are mostly copy-n-paste of existing commands they look
good to me.
Wei.
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |