| 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
| ||
|
Message-ID: <04132d34-c69e-62a0-35ba-bafa44210c0a@amd.com>
Date: Mon, 19 Sep 2022 10:46:46 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
tglx@...utronix.de, mingo@...hat.com, bp@...en8.de
Cc: fenghua.yu@...el.com, dave.hansen@...ux.intel.com, x86@...nel.org,
hpa@...or.com, paulmck@...nel.org, akpm@...ux-foundation.org,
quic_neeraju@...cinc.com, rdunlap@...radead.org,
damien.lemoal@...nsource.wdc.com, songmuchun@...edance.com,
peterz@...radead.org, jpoimboe@...nel.org, pbonzini@...hat.com,
chang.seok.bae@...el.com, pawan.kumar.gupta@...ux.intel.com,
jmattson@...gle.com, daniel.sneddon@...ux.intel.com,
sandipan.das@....com, tony.luck@...el.com, james.morse@....com,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
bagasdotme@...il.com, eranian@...gle.com
Subject: Re: [PATCH v4 09/13] x86/resctrl: Add sysfs interface files to
read/write event configuration
Hi Reinette,
On 9/16/22 10:58, Reinette Chatre wrote:
> Hi Babu,
>
> On 9/7/2022 11:01 AM, Babu Moger wrote:
>> Add two new sysfs files to read/write the event configuration if
>> the feature Bandwidth Monitoring Event Configuration (BMEC) is
>> supported. The file mbm_local_config is for the configuration
>> of the event mbm_local_bytes and the file mbm_total_config is
>> for the configuration of mbm_total_bytes.
>>
>> $ls /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local*
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_config
>>
>> $ls /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total*
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_config
>>
> This patch makes the mbm*config files per monitor group. Looking
> ahead at later patches how the configuration is set it is not clear
> to me that this is the right place for these configuration files.
>
> Looking ahead to patch 10 there is neither rmid nor closid within
> the (MSR_IA32_EVT_CFG_BASE + index) register - it only takes
> the bits indicating what access types needs to be counted. Also
> in patch 10 I understand that the scope of this register is per L3 cache
> domain.
Yes. Scope of MSR_IA32_EVT_CFG_BASE per L3 domain.
>
> Considering this, why is the sysfs file associated with each
> monitor group?
Please see the response below.
>
> For example, consider the following scenario:
> # cd /sys/fs/resctrl
> # mkdir g2
> # mkdir mon_groups/m1
> # mkdir mon_groups/m2
> # find . | grep mbm_local_config
> ./mon_data/mon_L3_00/mbm_local_config
> ./mon_data/mon_L3_01/mbm_local_config
> ./g2/mon_data/mon_L3_00/mbm_local_config
> ./g2/mon_data/mon_L3_01/mbm_local_config
> ./mon_groups/m2/mon_data/mon_L3_00/mbm_local_config
> ./mon_groups/m2/mon_data/mon_L3_01/mbm_local_config
> ./mon_groups/m1/mon_data/mon_L3_00/mbm_local_config
> ./mon_groups/m1/mon_data/mon_L3_01/mbm_local_config
>
>
> From what I understand, the following sysfs files are
> associated with cache domain #0 and thus writing to any of these
> files would change the same configuration:
> ./mon_data/mon_L3_00/mbm_local_config
> ./g2/mon_data/mon_L3_00/mbm_local_config
> ./mon_groups/m2/mon_data/mon_L3_00/mbm_local_config
> ./mon_groups/m1/mon_data/mon_L3_00/mbm_local_config
>
> Could you please correct me where I am wrong?
For example, we have CPUs 0-7 in domain 0. We have two counters which are
configurable.
Lets consider same example as your mentioned about.
g2 is a control group.
m1 and m2 are monitor group.
We can have control group g2 with CPUs 0-7 to limit the L3 bandwidth (or
memory bandwidth with required schemata setting).
We can have mon group m1 with cpus 0-3 to monitor mbm_local_bytes.
We can have mon group m2 with cpus 4-7 to monitor mbm_total_bytes.
Each group is independently, monitoring two separate thing. Without having
sysfs file (mbm_local_config and mbm_total_config) in each monitor group,
we wont be able to configure the above configuration.
>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 40 ++++++++++++++++++++++++--------
>> 1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index f55a693fa958..da11fdad204d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -254,6 +254,10 @@ static const struct kernfs_ops kf_mondata_ops = {
>> .seq_show = rdtgroup_mondata_show,
>> };
>>
>> +static const struct kernfs_ops kf_mondata_config_ops = {
>> + .atomic_write_len = PAGE_SIZE,
>> +};
>> +
> Please use coding style (tabs vs spaces) that is consistent with area
> you are contributing to.
Sure
>
>> static bool is_cpu_list(struct kernfs_open_file *of)
>> {
>> struct rftype *rft = of->kn->priv;
>> @@ -2478,24 +2482,40 @@ static struct file_system_type rdt_fs_type = {
>> .kill_sb = rdt_kill_sb,
>> };
>>
>> -static int mon_addfile(struct kernfs_node *parent_kn, const char *name,
>> +static int mon_addfile(struct kernfs_node *parent_kn, struct mon_evt *mevt,
>> void *priv)
>> {
>> - struct kernfs_node *kn;
>> + struct kernfs_node *kn_evt, *kn_evt_config;
>> int ret = 0;
>>
>> - kn = __kernfs_create_file(parent_kn, name, 0444,
>> - GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
>> - &kf_mondata_ops, priv, NULL, NULL);
>> - if (IS_ERR(kn))
>> - return PTR_ERR(kn);
>> + kn_evt = __kernfs_create_file(parent_kn, mevt->name, 0444,
>> + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
>> + &kf_mondata_ops, priv, NULL, NULL);
> Please run your series through checkpatch (alignment issue above)
Sure
>
>> + if (IS_ERR(kn_evt))
>> + return PTR_ERR(kn_evt);
>>
>> - ret = rdtgroup_kn_set_ugid(kn);
>> + ret = rdtgroup_kn_set_ugid(kn_evt);
>> if (ret) {
>> - kernfs_remove(kn);
>> + kernfs_remove(kn_evt);
>> return ret;
>> }
>>
>> + if (mevt->configurable) {
>> + kn_evt_config = __kernfs_create_file(parent_kn,
>> + mevt->config_name, 0644,
>> + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
>> + &kf_mondata_config_ops, priv, NULL, NULL);
>> + if (IS_ERR(kn_evt_config))
>> + return PTR_ERR(kn_evt_config);
>> +
> Since an error is returned here it seems that some cleanup (kn_evt) is missing?
Yes. That is correct. Will fix it.
Thanks
Babu
>
>
>> + ret = rdtgroup_kn_set_ugid(kn_evt_config);
>> + if (ret) {
>> + kernfs_remove(kn_evt_config);
>> + kernfs_remove(kn_evt);
>> + return ret;
>> + }
>> + }
>> +
>> return ret;
>> }
>>
> Reinette
--
Thanks
Babu Moger
Powered by blists - more mailing lists