Re: [PATCH v5 3/3] tools & docs: add tools and docs support for Intel CDP

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-09-28 at 16:29 +0800, He Chen wrote:
> This is the xl/xc changes to support Intel Code/Data Prioritization.
> CAT xl commands to set/get CBMs are extended to support CDP.
> Add new CDP options with CAT commands in xl interface man page.
> Add description of CDP in xl-psr.markdown.
> 
> Signed-off-by: He Chen <he.chen@linux.intel.com>
> ---
> Changes in v5:
> * merge tools and docs patches
> * replace EINVAL with ENXIO in libxl__psr_cat_log_err_msg
> * revise options parsing in psr-cat-cbm-set and invalidate passing -c
>   and -d at the same time
> * refine CDP status output codes in psr_cat_hwinfo
> * adjust CBM output format in command xl psr-cat-show
> * docs revision
> 
> Example of new output format for command xl psr-cat-show:
> 
> CAT-only:
> 
> Socket ID       : 0
> L3 Cache        : 56320KB
> Default CBM     : 0xfffff
>    ID                     NAME                   CBM
>     0                 Domain-0               0xfffff
>     1               centos.hvm               0xfffff
> 
> CDP enabled:
> 
> Socket ID       : 0
> L3 Cache        : 56320KB
> Default CBM     : 0xfffff
>    ID                     NAME                   CBM
>     0                 Domain-0     code: 0xfffff     data: 0xfffff
>     1               centos.hvm     code: 0xfffff     data: 0xfffff

The CBM column header seems a bit out of place.

Perhaps have separate headings, e.g. "CBM (code)" and "CBD (data)"?

diff --git a/docs/misc/xl-psr.markdown b/docs/misc/xl-psr.markdown

> index 3545912..0527211 100644
> --- a/docs/misc/xl-psr.markdown
> +++ b/docs/misc/xl-psr.markdown
> @@ -14,7 +14,7 @@ tracks cache utilization of memory accesses according
> to the RMID and reports
>  monitored data via a counter register.
>  
>  For more detailed information please refer to Intel SDM chapter
> -"17.14 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
> +"Platform Shared Resource Monitoring: Cache Monitoring Technology".

I think these will clash with Cheng Pao's PSR updates, which have already
been applied.
 
> 
> +Code/Data Prioritization (CDP) Technology is an extension of CAT, which is

Other bits of documentation added here seem to suggest it is actually an
extension of CBM, is that right?

> diff --git a/tools/libxc/include/xenctrl.h
> b/tools/libxc/include/xenctrl.h
> index 2000f12..8f4acec 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2794,7 +2794,9 @@ enum xc_psr_cmt_type {
>  typedef enum xc_psr_cmt_type xc_psr_cmt_type;
>  
>  enum xc_psr_cat_type {
> -    XC_PSR_CAT_L3_CBM = 1,
> +    XC_PSR_CAT_L3_CBM  = 1,
> +    XC_PSR_CAT_L3_CODE = 2,
> +    XC_PSR_CAT_L3_DATA = 3,

If CDP is an extension of CBM, then would CBM_CODE + CBM_DATA be better
names (more important in the libxl API than here)

> +    if (opt_data && opt_code) {
> +        fprintf(stderr, "Invalid to pass both -c and -d at the same time\n");

To be correct this would need to say "It is invalid..." but a more natural
sounding (to me) way to express this would be

"Cannot handle -c and -d at the same time"

I think.