linux-kernel - Re: [PATCH 2/3] x86/resctrl: Add sparse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <w73nwlw6q5drlarcah56rhyknzaktu32iwp3bm7x7idgxdbmtt@m5xf44glqkf6>
Date:   Tue, 12 Sep 2023 08:57:17 +0200
From:   Maciej Wieczór-Retman 
        <maciej.wieczor-retman@...el.com>
To:     Reinette Chatre <reinette.chatre@...el.com>
CC:     Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] x86/resctrl: Add sparse_bitmaps file in info

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 <fenghua.yu@...el.com>
>> 
>> 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 <fenghua.yu@...el.com>
>> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@...el.com>
>> ---
>>  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