Re: [Xen-devel] [v2 3/3] tools & docs: add L2 CAT support in tools and docs.

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Yi Sun writes ("[v2 3/3] tools & docs: add L2 CAT support in tools and docs."):
> This patch is the xl/xc changes to support Intel L2 CAT
> (Cache Allocation Technology).
> 
> The new level option is introduced to original CAT setting
> command in order to set CBM for specified level CAT.

Thanks for this.  I have some comments about the libxl API and xl.

You'll see that I'm somewhat confused.  Please help enlighten me :-).


> +#define L3_CAT 3
> +#define L2_CAT 2
> +int xc_psr_cat_get_info(xc_interface *xch, uint32_t socket, int lvl,
> +                        uint32_t *cos_max, uint32_t *cbm_len,
> +                        bool *cdp_enabled)

I find these defines rather odd.  You have chosen to use an integer
"level" to specify which cache to affect.  It probably wouldn't be
desirable to decouple these integer values from the names, but the
names are just integers with a funny hat on.

If these names are really useful they should be in the IDL.


> -int xc_psr_cat_get_l3_info(xc_interface *xch, uint32_t socket,
> -                           uint32_t *cos_max, uint32_t *cbm_len,
> -                           bool *cdp_enabled);
> +int xc_psr_cat_get_info(xc_interface *xch, uint32_t socket, int lvl,
> +                        uint32_t *cos_max, uint32_t *cbm_len,
> +                        bool *cdp_enabled);

This needs a new HAVE #define.


> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 99733f6..861d5a8 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
..
> +static int psr_l2_cat_hwinfo(void)
> +{
> +    int rc;
> +    int i, nr;
> +    libxl_psr_cat_info *info;
...
> +    for (i = 0; i < nr; i++) {
> +        /* There is no CMT on L2 cache so far. */
> +        printf("%-16s: %u\n", "Socket ID", info[i].id);
> +        printf("%-16s: %u\n", "Maximum COS", info[i].cos_max);
> +        printf("%-16s: %u\n", "CBM length", info[i].cbm_len);
> +        printf("%-16s: %#llx\n", "Default CBM",
> +               (1ull << info[i].cbm_len) - 1);

I find this code confusing.

I don't understand why libxl needs to know about the properties of L2
vs L3 CAT.  If it does need to know these properties then probably the
IDL needs to know about them too, but the IDL is completely agnostic.

Perhaps libxl ought probably to be "thinner", and simply present whats
in the IDL ?

For example:

> +    if (lvl == 2) {
> +        if (opt_data || opt_code) {
> +            fprintf(stderr, "L2 CAT does not support CDP yet.\n");
> +            return -1;

This should perhaps be done by calling libxl and getting a
notimplemented error ?

> +    } else if (lvl == 3) {
> +        if (opt_data && opt_code) {
> +            fprintf(stderr, "Cannot handle -c and -d at the same time\n");
> +            return -1;
> +        } else if (opt_data) {
> +            type = LIBXL_PSR_CBM_TYPE_L3_CBM_DATA;
> +        } else if (opt_code) {
> +            type = LIBXL_PSR_CBM_TYPE_L3_CBM_CODE;
> +        } else {
> +            type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> +        }

Is this inability to do -c and -d really contingent on the cache
level ?

>      } else {
> -        type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> +        fprintf(stderr, "'xl %s' requires to input cache level -l%d or 
> -l%d.\n",
> +                "psr-cat-cbm-set", 2, 3);

I think you have made the old calling pattern break, here: that is, if
you pass no -l, it takes this branch and bombs out.

Ie, a non-backwards-compatible change to the xl command line
interface.  I'm afraid that's not allowed.

> @@ -9503,7 +9624,15 @@ int main_psr_cat_show(int argc, char **argv)
>          return 2;
>      }
>  
> -    return psr_cat_show(domid);
> +    if (3 == lvl)
> +        rc = psr_cat_show(domid);
> +    else if (2 == lvl)
> +        rc = psr_l2_cat_show(domid);

This code is confusing to me.  Surely psr_cat_show should be changed
to take a level argument ?  Or if psr_cat_show can only show L3, why
does it not have l3 in its name ?

> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 78786fe..5a2676e 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
...
> +      "-l <level>        Specify the cache level to process, otherwise L3 
> cache is processed\n"

Maybe I have misread the option parsing but is this actually true that
it defaults to L3 ?  (See above.)


Thanks,
Ian.

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