Re: [PATCH v3 01/15] docs: L2 Cache Allocation Technology (CAT) feature document.

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

On 16-11-25 18:39:41, Dario Faggioli wrote:
> A couple of more things about this document...
> 
> On Tue, 2016-10-25 at 11:40 +0800, Yi Sun wrote:
> > diff --git a/docs/features/l2_cat.pandoc 
> >
> The name of the file could be a bit more descriptive. What about
> something like:
> 
> intel_psr_l2_cat.pandoc
> 
Thanks for your suggestion!

> > +## Implementation Description
> > +
> > +* Hypervisor interfaces:
> > +
> > +  1. Ext: Boot line parameter "psr=cat" now will enable L2 CAT and
> > L3
> > +          CAT if hardware supported.
> > +
> > +  2. New: SYSCTL:
> > +          - XEN_SYSCTL_PSR_CAT_get_l2_info: Get L2 CAT information.
> > +
> > +  3. New: DOMCTL:
> > +          - XEN_DOMCTL_PSR_CAT_OP_GET_L2_CBM: Get L2 CBM for a
> > domain.
> > +          - XEN_DOMCTL_PSR_CAT_OP_SET_L2_CBM: Set L2 CBM for a
> > domain.
> > +
> What do these 'Ext' and 'New' prefixes mean? You perhaps what to
> communicate whether you are extending something that was existing
> already, or introducing something new... Well, I don't think that is a
> very valuable piece of information.
> 
> After all, we want to know that there is a relevant boot parameter, a
> sysctl and a domctl interface, no matter whether they're new or not
> (especially considering that this is the first and for now only feature
> document for PSR).
> 
> I'd just kill them.
> 
Yes, your understand is correct. Thanks for your comments! I will kill them.

> > +* xl interfaces:
> > +
> > +  1. Ext: psr-cat-show -l2 domain-id
> > +          Show L2 cbm for a domain.
> > +          => XEN_SYSCTL_PSR_CAT_get_l2_info /
> > +             XEN_DOMCTL_PSR_CAT_OP_GET_L2_CBM
> > +
> > +  2. Ext: psr-mba-set -l2 domain-id cbm
> > +          Set L2 cbm for a domain.
> > +          => XEN_DOMCTL_PSR_CAT_OP_SET_L2_CBM
> > +
> > +  3. Ext: psr-hwinfo
> > +          Show PSR HW information, including L2 CAT
> > +          => XEN_SYSCTL_PSR_CAT_get_l2_info
> > +
> Same here.
> 
Thank you!

> > +# User information
> > +
> > +* Feature Enabling:
> > +
> > +  Add "psr=cat" to boot line parameter to enable all supported level
> > CAT
> > +  features.
> > +
> > +* xl interfaces:
> > +
> > +  1. `psr-cat-show [OPTIONS] domain-id`:
> > +
> > +     Show domain L2 or L3 CAT CBM.
> > +
> > +     New option `-l` is added.
> > +     `-l2`: Specify cbm for L2 cache.
> > +     `-l3`: Specify cbm for L3 cache.
> > +
> > +     If neither `-l2` nor `-l3` is given, level 3 is the default
> > option.
> > +
> Sorry for saying this only now, but wouldn't it be more natural, if
> neither -l2 not -l3 is specified, to show both (or, in general, all
> that is supported)?
> 
This is for backward compatibility. The original command only supports
L3 CAT and it does not have '-l' option.

But for show command, your suggestion is good. We can show both or
prompt user if any one is not supported.

> > +  2. `psr-cat-cbm-set [OPTIONS] domain-id cbm`:
> > +
> > +     Set domain L2 or L3 CBM.
> > +
> > +     New option `-l` is added.
> > +     `-l2`: Specify cbm for L2 cache.
> > +     `-l3`: Specify cbm for L3 cache.
> > +
> > +     If neither `-l2` nor `-l3` is given, level 3 is the default
> > option.
> > +
> > +  3. `psr-hwinfo [OPTIONS]`:
> > +
> > +     Show L2 & L3 CAT HW informations on every socket.
> 
> > +------------------------------------------------------------------
> > ------
> > +Date       Revision Version  Notes
> > +---------- -------- -------- -------------------------------------
> > ------
> > +2016-08-12 1.0      Xen 4.7  Initial design
> > +2016-09-21 2.0      Xen 4.7  Changes according to review comments.
> > +                             1. L2 CBM bits should be continuous
> > too.
> > +                             2. Describe 'struct feat_info'
> > +                             3. Update 'struct feat_list"
> > description.
> > +                             4. Update 'struct feat_ops"
> > description.
> > +                             5. Update `psr-cat-show` description.
> > +                             6. Update `psr-cat-cbm-set`
> > description.
> > +                             7. Add volume and chapter info in
> > References.
> >
> If you want to keep track of this uncommited version here, I think it's
> ok. But I'd just cur it short to a quick 1-liner summary and kill the
> detailed list of changes (which could then be moved in the patch
> changelog, but below the usual '---' so it won't get commited).
> 
> Personally, I'd also, avoid specifying any Xen version in for these,
> and do that starting from the line corresponding to the first version
> of the document that hits the tree. Like this, it somehow gives the
> impression that the feature is present in Xen 4.7, which is not true.
> 
> Regards,
> Dario

Thanks a lot for the suggestion! I will remove Xen version and change
log here.

BRs,
Sun Yi

> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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