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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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

> +## 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.

> +* 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.

> +# 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)?

> +  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
-- 
<<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)

Attachment: signature.asc
Description: This is a digitally signed message part

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