Hello,
On 2023-09-11 at 13:05:30 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/1/2023 1:55 AM, Wieczor-Retman, Maciej wrote:
>> From: Fenghua Yu <[email protected]>
>>
>> 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"
Okay, I'll change it, thanks.
>> 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.
I'll change it.
>>
>> Signed-off-by: Fenghua Yu <[email protected]>
>> Signed-off-by: Wieczor-Retman, Maciej <[email protected]>
>> ---
>> 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.
Okay, I'll go through the patches and unify the names to "bit mask"
rather than "bitmap" etc.
>Reinette
--
Kind regards
Maciej Wiecz?r-Retman