Re: [PATCH v5 01/24] docs: create L2 Cache Allocation Technology (CAT) feature document

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

On 17-01-18 10:11:15, Dario Faggioli wrote:
> On Wed, 2017-01-18 at 10:02 +0800, Yi Sun wrote:
> > This patch creates L2 CAT feature document in doc/features/.
> > It describes details of L2 CAT.
> > 
> > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > ---
> >
> Hey,
> 
> it is very very useful to put _RIGHT_HERE_ a summary of what changed,
> within, this patch, wrt the previous version.
> 
> That helps reviewers quite a bit, especially in making sure that you
> hav considered and addressed comments made during the previous
> iterations.
> 
I have resend the patches with summary of what changed. Please check.
Thanks!

Your comments for these patches will be recorded.

> >  docs/features/intel_psr_l2_cat.pandoc | 347
> > ++++++++++++++++++++++++++++++++++
> >  1 file changed, 347 insertions(+)
> >  create mode 100644 docs/features/intel_psr_l2_cat.pandoc
> 
> > --- /dev/null
> > +++ b/docs/features/intel_psr_l2_cat.pandoc
> > +
> > +# User details
> > +
> > +* 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`: Show cbm for L2 cache.
> > +     `-l3`: Show cbm for L3 cache.
> > +
> > +     If neither `-l2` nor `-l3` is given, show both of them. If any
> > one
> > +     is not supported, will print error info.
> > +
> I actually think the best behavior would be:
>  - if -lX is specified, and LX is not supported ==> print error;

Will return error for this case. But the error log is not accurate so far.
Will refine this. Thanks!

>  - if no -l is specified ==> print info about the supported levels.
> 
That is my bad. In fact, the l3 info will be shown if no -l is specified.

> (See comment on patch 21.)
> 
> > +## The relationship between L2 CAT and L3 CAT/CDP
> > +
> > +L2 CAT is independent of L3 CAT/CDP, which means L2 CAT would be
> > enabled
> > +while L3 CAT/CDP is disabled, or L2 CAT and L3 CAT/CDP are all
> > enabled.
> > +
> 
> I find 'would be enabled' and 'are all enabled' a bit confusing.
> 
> Maybe: "which means L2 cat can be enabled while L3 CAT/CDP is disabled,
> or L2 CAT and L3 CAT/CDP can be all enabled"
> 
> ?
Agree, thank you!

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



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