Re: [PATCH 2/3] x86/resctrl: Add sparse_bitmaps file in info

[Reinette Chatre]

Hi Maciej,

On 9/1/2023 1:55 AM, Wieczor-Retman, Maciej wrote:
> From: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> 
> Add the interface in resctrl FS to show if sparse CAT bitmaps are

resctrl is intended to be a generic interface so modifying it
with a vendor specific change is not ok. This is not what the
patch is doing though so the changelog can just be modified to
not create the impression. Perhaps just:
 "sparse CAT bitmaps" -> "sparse cache allocation bit masks"

> supported on the platform. Reading the file returns either a "1" if
> non-contiguous 1s are supported and "0" otherwise. The file path is
> /sys/fs/resctrl/info/{resource}/sparse_bitmaps, where {resource} can be
> either "L2" or "L3" depending on their support in the CAT feature.

No CAT here. L2 and L3 are the hardcoded cache allocation resources
so "depending on their support in the CAT feature" can just be removed.

> 
> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@xxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 725344048f85..4d27354f3f30 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -895,6 +895,17 @@ static int rdt_shareable_bits_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +static int rdt_has_sparse_bitmaps_show(struct kernfs_open_file *of,
> +				       struct seq_file *seq, void *v)
> +{
> +	struct resctrl_schema *s = of->kn->parent->priv;
> +	struct rdt_resource *r = s->res;
> +
> +	seq_printf(seq, "%u\n", r->cache.arch_has_sparse_bitmaps);
> +
> +	return 0;
> +}
> +
>  /**
>   * rdt_bit_usage_show - Display current usage of resources
>   *
> @@ -1839,6 +1850,13 @@ static struct rftype res_common_files[] = {
>  		.seq_show	= rdtgroup_size_show,
>  		.fflags		= RF_CTRL_BASE,
>  	},
> +	{
> +		.name		= "sparse_bitmaps",
> +		.mode		= 0444,
> +		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.seq_show	= rdt_has_sparse_bitmaps_show,
> +		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
> +	},
>  
>  };
>  

I think it is essential to use consistent terminology. To help with
this I reviewed the resctrl documentation and found no mention of
"bitmap" ... yet many instances of bit mask and even a clear official
term of "Cache Bit Masks (CBM)". The user interface is thus already
established and using the term "bit mask". Looking through the
AMD and Intel specs I also only see "bit mask". I think "bitmap"
sneaked in via an Arm contribution as motivated by their spec's
use of the term "cache portion bitmap". Since "bit mask" is already
in the user documentation and also in the interface via "cbm_mask"
I'd prefer that we stick with "bit mask" in user interface instead
of creating fragmentation with a new term.

Considering this I'd like to propose "sparse_masks" to match existing
"cbm_mask".

Please review this series to be consistent in this regard. Note that
patch 2 refers to "bitmaps" and then patch 3 switches to "bitmasks"
... patch 3 already uses the term "sparse bitmasks".

I think that it may also help to add a patch to this series that
renames arch_has_sparse_bitmaps to arch_has_sparse_bitmasks.

Reinette