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

[Yi Sun <yi.y.sun@linux.intel.com>]

Hi, Ian,

Any comments? That would be very appreciated. Thanks!

BRs,
Sun Yi

On 16-09-23 15:24:57, Yi Sun wrote:
> 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@lists.xen.org
> https://lists.xen.org/xen-devel

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