Re: [Xen-devel] [PATCH v9 6/6] tools: enable Cache QoS Monitoring feature for libxl/libxc

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-02-19 at 14:32 +0800, Dongxiao Xu wrote:
> +=item B<pqos-attach> [I<qos-type>] [I<domain-id>]
> +
> +Attach certain platform QoS service for a domain.
> +Current supported I<qos-type> is: "cqm".

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 12d6c31..f3d2202 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1105,6 +1105,10 @@ int libxl_flask_getenforce(libxl_ctx *ctx);
>  int libxl_flask_setenforce(libxl_ctx *ctx, int mode);
>  int libxl_flask_loadpolicy(libxl_ctx *ctx, void *policy, uint32_t size);
>  
> +int libxl_pqos_attach(libxl_ctx *ctx, uint32_t domid, const char * qos_type);
> +int libxl_pqos_detach(libxl_ctx *ctx, uint32_t domid, const char * qos_type);

I have a feeling that qos_type should actually be an enum in the IDL.
The xl functions can probably use the autogenerate
libxl_BLAH_from_string() functions to help with parsing.

What other qos types are you envisaging? Is it valid to enable or
disable multiple such things independently?

> +void libxl_map_cqm_buffer(libxl_ctx *ctx, libxl_cqminfo *xlinfo);

So each qos type is going to come with its own map function?

I don't see the LIBXL_HAVE #define which we discussed last time anywhere
here.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 649ce50..43c0f48 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -596,3 +596,10 @@ libxl_event = Struct("event",[
>                                   ])),
>             ("domain_create_console_available", Struct(None, [])),
>             ]))])
> +
> +libxl_cqminfo = Struct("cqminfo", [
> +    ("buffer_virt",    uint64),

An opaque void * masquerading as an integer is not a suitable interface.

This should be a (pointer to a) struct of the appropriate type, or an
Array of such types etc (or possibly several such arrays depending on
what you are returning).

I haven't looked in detail into what is actually in this buffer, but
please try and have libxl bake it into a more consumable form -- e.g. an
array of per-domain properties or something rather than a raw list.

> +    ("size",           uint32),
> +    ("nr_rmids",       uint32),
> +    ("nr_sockets",     uint32),
> +    ])
> [...][
> +static void print_cqm_info(const libxl_cqminfo *info)
> +{
> +    unsigned int i, j, k;
> +    char *domname;
> +    int print_header;
> +    int cqm_domains = 0;
> +    uint16_t *rmid_to_dom;
> +    uint64_t *l3c_data;
> +    uint32_t first_domain = 0;
> +    unsigned int num_domains = 1024;
> +
> +    if (info->nr_rmids == 0) {
> +        printf("System doesn't support CQM.\n");
> +        return;
> +    }
> +
> +    print_header = 1;
> +    l3c_data = (uint64_t *)(info->buffer_virt);
> +    rmid_to_dom = (uint16_t *)(info->buffer_virt +
> +                  info->nr_sockets * info->nr_rmids * sizeof(uint64_t));
> +    for (i = first_domain; i < (first_domain + num_domains); i++) {
> +        for (j = 0; j < info->nr_rmids; j++) {
> +            if (rmid_to_dom[j] != i)
> +                continue;
> +
> +            if (print_header) {
> +                printf("Name                                        ID");
> +                for (k = 0; k < info->nr_sockets; k++)
> +                    printf("\tSocketID\tL3C_Usage");
> +                print_header = 0;
> +            }
> +
> +            domname = libxl_domid_to_name(ctx, i);
> +            printf("\n%-40s %5d", domname, i);
> +            free(domname);
> +            cqm_domains++;
> +
> +            for (k = 0; k < info->nr_sockets; k++)
> +                printf("%10u %16lu     ",
> +                        k, l3c_data[info->nr_rmids * k + j]);
> +        }

This should be transformed into a sensible interface within libxl so
that it can be consumed in a straightforward manner by the users of
libxl, rather than asking them all to reimplement this.

Is the buffer format considered a frozen ABI?

> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index ebe0220..d4af4a9 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -494,6 +494,21 @@ struct cmd_spec cmd_table[] = {
>        "[options]",
>        "-F                      Run in the foreground",
>      },
> +    { "pqos-attach",
> +      &main_pqosattach, 0, 1,
> +      "Allocate and map qos resource",
> +      "<Resource> <Domain>",
> +    },
> +    { "pqos-detach",
> +      &main_pqosdetach, 0, 1,
> +      "Reliquish qos resource",

"Relinquish"

and perhaps "resources" (in both cases)

> +      "<Resource> <Domain>",
> +    },
> +    { "pqos-list",
> +      &main_pqoslist, 0, 0,
> +      "List qos information for all domains",
> +      "<Resource>",
> +    },
>  };
>  
>  int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel