Re: [Xen-devel] [v2 3/3] tools & docs: add L2 CAT support in tools and docs.
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
Re: [Xen-devel] [v2 3/3] tools & docs: add L2 CAT support in tools and docs.
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
Re: [Xen-devel] [v2 3/3] tools & docs: add L2 CAT support in tools and docs.
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 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@lists.xen.org
https://lists.xen.org/xen-devel
