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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 16-09-22 11:09:31, Ian Jackson wrote:
> 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 :-).
> 
> 
Thank you very much for reviewing my patches and provide your comments!

Sorry to make you confused. I will try best to explain my intention ;-).

> > +#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.
> 
>
Thanks! I will remove the macros and just use integer.
 
> > -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.
> 
> 
Thanks! I will add one more HAVE #define in xl.h.

> > 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.
> 
psr_l2_cat_hwinfo() is implemented to get L2 CAT HW info back. Because this
HW info is almost same as L3 CAT, I reuse the libxl_psr_cat_info defined
in libxl_types.idl. If you think this is not good enough, I think I may add
'lvl' in libxl_psr_cat_info to express the level to handle?

> 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 ?
> 
I think your intention is to put more feature details into libxl_psr.c then
the developer of other app will be easy to implement the app, right?

My concerns are below:
1. The application needs user to input the lvl option to know which lvl to
operate anyway. Of course, we can use L3 as default option to be backward
compatible. But to get/set L2, we still need user to input level. That means
the application knows the level concept.
2. From the functional view, print what information out and how to operate
should be decided by application but not the library.

So, I think it is acceptable to let xl know some L2 and L3 details and operate
differently on different levels.

If my thought is not right, please correct me. Thank you!

> > +    } 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 ?
> 
Because '-c'(code) and '-d'(data) are used for CDP and L2 CAT does not
support CDP, they are handled only at L3 level now.

> >      } 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.
> 
Thank you! I will remove this and handle L3 by default because L3 is the only
choice in old interface.

> > @@ -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 ?
> 
Sorry for the confusion.

psr_cat_show() was implemented for L3 CAT before. To support L2, I implement
psr_l2_cat_show(). But I don't want to modify the old function so
psr_cat_show is remained.

If you think this is not good, I can change psr_cat_show to psr_l3_cat_show
to make it clearer.

> > 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.)
> 
Sorry for this. I missed to modify this sentence.

If L3 is the default behavior as above comment, this sentence will be kept.

> 
> Thanks,
> Ian.

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