Re: [Xen-devel] [PATCH v12 9/9] tools: CMDs and APIs for Platform QoS Monitoring

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx
> [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Ian Campbell
> Sent: Thursday, July 10, 2014 11:50 PM
> To: Xu, Dongxiao
> Cc: keir@xxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> George.Dunlap@xxxxxxxxxxxxx; andrew.cooper3@xxxxxxxxxx;
> Ian.Jackson@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; JBeulich@xxxxxxxx;
> dgdegra@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v12 9/9] tools: CMDs and APIs for Platform QoS
> Monitoring

Hi Ian,

Thanks for the comments, some answers below.

> 
> On Fri, 2014-07-04 at 16:34 +0800, Dongxiao Xu wrote:
> > Introduced some new xl commands to handle the platform QoS monitoring
> > related services.
> >
> > The following two commands is to attach/detach the QoS monitoring
> > service to/from a certain domain.
> 
> Is there a literal "service" here to attach to? Or are these operations
> more like enable/disable?

It's more like enable/disable, so I will change the service word...

> 
> > diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
> > index b8b28c1..91e65c4 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_pqos.c
> 
> CONFIG_X86?

For CQM xl commands and libxl APIs are not architectural specific, if we define 
libxc functions to be x86 specific, will it be a problem when compiling on 
non-x86 platform?

> 
> >  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
> 
> > +int xc_pqos_monitor_get_domain_rmid(xc_interface *xch, uint32_t domid,
> > +                                    uint32_t *rmid)
> > +{
> > +    int rc;
> > +    DECLARE_DOMCTL;
> > +
> > +    domctl.cmd = XEN_DOMCTL_pqos_monitor_op;
> > +    domctl.domain = (domid_t)domid;
> > +    domctl.u.pqos_monitor_op.cmd =
> XEN_DOMCTL_PQOS_MONITOR_OP_QUERY_RMID;
> > +
> > +    rc = do_domctl(xch, &domctl);
> > +
> > +    *rmid = domctl.u.pqos_monitor_op.data;
> 
> Only if rc indicates success.

Okay.

> 
> > +
> > +    return rc;
> > +}
> > +
> > +int xc_pqos_monitor_get_total_rmid(xc_interface *xch, uint32_t
> *total_rmid)
> > +{
> > +    static int val = 0;
> > +    int rc;
> > +    DECLARE_SYSCTL;
> > +
> > +    if ( val )
> > +    {
> > +        *total_rmid = val;
> 
> This can never change dynamically?
> 
> Likewise for several more similar things further down.

The total_rmid, L3 scaling factor, and L3 cache total size are platform 
characteristic, and will not change in runtime.

> 
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 69ceac8..f510ade 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -533,6 +533,13 @@ typedef uint8_t libxl_mac[6];
> >  #define LIBXL_MAC_FMTLEN ((2*6)+5) /* 6 hex bytes plus 5 colons */
> >  #define LIBXL_MAC_BYTES(mac) mac[0], mac[1], mac[2], mac[3], mac[4],
> mac[5]
> >
> > +/*
> > + * LIBXL_HAVE_PQOS_MONITOR_CQM
> > + *
> > + * If this is defined, the cache QoS monitoring feature is suported.
> 
> "supported"

Thanks!

> 
> > +int libxl_pqos_monitor_get_l3_cache_size(libxl_ctx *ctx,
> > +    uint32_t *l3_cache_size);
> > +int libxl_pqos_monitor_get_cache_occupancy(libxl_ctx *ctx, uint32_t
> > domid,
> > +    uint32_t socketid, uint64_t *l3_cache_occupancy);
> 
> What are the units of the outputs of these functions?

The l3_cache_occupancy unit is Byte, while l3_cache_size is K-Byte.

> 
> Why is cache size 32 bits but occupancy is 64 ?

Total cache size is derived from boot_cpu->x86_cache_size, which is a 32bit 
integer type variable.
l3_cache_occupancy is derived from a multiply of two 32-bit variables, so I set 
l3_cache_occupancy to 64 bit variable.

I will set them to be both 32bit and use K-Byte as the unit.

> 
> > diff --git a/tools/libxl/libxl_pqos.c b/tools/libxl/libxl_pqos.c
> > new file mode 100644
> > index 0000000..8c56e67
> > --- /dev/null
> > +++ b/tools/libxl/libxl_pqos.c
> > @@ -0,0 +1,171 @@
> > +/*
> > + * 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 const char * const msg[] = {
> 
> Could this be local to the error printing function?

Okay.

> 
> > +    [ENOSYS] = "unsupported operation",
> > +    [ENODEV] = "Platform QoS Monitoring is not supported in this system",
> > +    [EEXIST] = "Platform QoS Monitoring is already attached to this 
> > domain",
> > +    [ENOENT] = "Platform QoS Monitoring is not attached to this domain",
> > +    [EUSERS] = "there is no free RMID available",
> > +    [ESRCH]  = "is this Domain ID valid?",
> > +    [EFAULT] = "cannot find a valid CPU belonging to this socket",
> > +};
> > +
> > +static void libxl_pqos_monitor_err_msg(libxl_ctx *ctx, int err)
> > +{
> > +    GC_INIT(ctx);
> > +
> > +    switch (err) {
> > +    case EINVAL:
> > +    case ENODEV:
> > +    case EEXIST:
> > +    case EUSERS:
> > +    case ESRCH:
> > +    case ENOENT:
> 
> This enumeration of the valid errnos is bound to get out of sync I
> think.
> 
> You can compare err to ARRAY_SIZE(msg) and if it is within bounds check
> it for NULL.
> 
> or  perhaps
> case EINVAL: msg = "unsupported operation"; break;
> etc

Okay.

> 
> > +
> > +int libxl_pqos_monitor_domain_attached(libxl_ctx *ctx, uint32_t domid)
> > +{
> > +    int rc;
> > +    uint32_t rmid;
> > +
> > +    rc = xc_pqos_monitor_get_domain_rmid(ctx->xch, domid, &rmid);
> > +    if (rc < 0)
> > +        return 0;
> 
> don't you mean return rc here?

Here 0 means "not" attached, while 1 means attached...

> 
> > +int libxl_pqos_monitor_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
> > +    uint32_t socketid, uint64_t *l3_cache_occupancy)
> > +{
> > +    GC_INIT(ctx);
> > +
> > +    unsigned int rmid;
> > +    uint32_t upscaling_factor;
> > +    uint64_t monitor_data;
> > +    int cpu, rc;
> > +    xc_pqos_monitor_type type;
> > +
> > +    rc = xc_pqos_monitor_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");
> > +        return ERROR_FAIL;
> 
> All of these error paths fail to GC_FREE. You should follow the
>     if (error thing) {
>         rc = ERROR_FAIL;
>         goto out;
>     }
> 
>     ...
> 
>     rc = 0
> out:
>     GC_FREE;
>     return 0
> idiom.

Okay.

> 
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 1018142..2e7668b 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -604,3 +604,7 @@ libxl_event = Struct("event",[
> >                                   ])),
> >             ("domain_create_console_available", None),
> >             ]))])
> > +
> > +libxl_pqos_monitor_type = Enumeration("pqos_monitor_type", [
> > +    (1, "CACHE_OCCUPANCY"),
> > +    ])
> 
> This is used purely for the libxl_pqos_monitor_type_from_string helper
> in xl and never in the libxl API, is that correct?
> 
> I'm not sure we want to expose this in the libxl api just for stat.
> maybe just strcmp for now and if this list grows unwieldy we can
> revisit.

I suggest we use this API since we will soon have patches to implement "local 
memory bandwidth", "total memory bandwidth", and so on...

> 
> > +    print_header = 1;
> > +    for (domid = first_domain; domid < first_domain + nr_domains; domid++)
> {
> > +        if (!libxl_pqos_monitor_domain_attached(ctx, domid)) {
> > +            if (!ignore_domain_err)
> > +                printf("No CQM info for Domain %d.\n", domid);
> 
> There might be an argument for printing a line anyway with "Unknown".

If we use libxl_list_domain() to get the exact domain list as you suggested, 
then we can get avoid of this "ignore_domain_err" stuff.

> 
> > +            continue;
> > +        }
> > +        if (print_header) {
> > +            printf("Name                                        ID");
> 
> printf("%-40s %5s", "Name", "ID") would be easier to relate to the list
> below.

Okay.

> 
> > +            for (socketid = 0; socketid < nr_sockets; socketid++)
> > +                printf("\tSocketID\tL3C_Usage");
> > +            printf("\n");
> > +            print_header = 0;
> > +        }
> 
> So the output on a two socket system would be:
> Name                                      ID  SocketID        L3CUsage
>       SocketID        L3CUsage
> Blahblah                                   1           0
> 123KB              1              456KB
> 
> ?
> 
> The socketid column seems weird to me. Why not:
> 
> Name                                      ID        Socket 0  Socket 1
> Blahblah                                   1           123KB
> 456KB
> 

Okay, this is fine to me. :)

> 
> Or
> 
> Name                                      ID        Socket    L3C
> Usage
> Blahblah                                   1             0      456KB
>                                                          1      123KB
> 
> ...
> 
> "xl list" takes a bunch of flags to include extra info. Maybe you want
> to think about listing this stuff there instead?

I ever thought about integrating the pqos stuff into xl list, however it is 
difficult for me to make the output looks elegant...
Can we firstly implement it still in a specific command line?

> 
> > +        domain_name = libxl_domid_to_name(ctx, domid);
> > +        printf("%-40s %5d", domain_name, domid);
> > +        free(domain_name);
> > +        for (socketid = 0; socketid < nr_sockets; socketid++) {
> > +            rc = libxl_pqos_monitor_get_cache_occupancy(ctx, domid,
> socketid,
> > +
> &l3_cache_occupancy);
> > +            printf("%10u %13lu KB     ", socketid,
> l3_cache_occupancy/1024);
> > +        }
> > +        printf("\n");
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> > +int main_pqos_monitor_show(int argc, char **argv)
> > +{
> > +    int opt, ret = 0;
> > +    uint32_t first_domain, nr_domains;
> > +    libxl_pqos_monitor_type type;
> > +
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "pqos-monitor-show", 1) {
> > +        /* No options */
> > +    }
> > +
> > +    libxl_pqos_monitor_type_from_string(argv[optind], &type);
> > +
> > +    if (optind + 1 >= argc) {
> > +        first_domain = 0;
> > +        nr_domains = 1024;
> 
> 
> When an explicit domain is not given please use libxl_list_domain() and
> print the status for all of them instead of hardcoding the first 1024
> domains.

Okay.

Thanks,
Dongxiao

> 
> 
> > +    } else if (optind + 1 == argc - 1){
> > +        first_domain = find_domain(argv[optind + 1]);
> > +        nr_domains = 1;
> 
> Ian.
> 
> 
> _______________________________________________
> 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