| 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: <876401ba-835f-471b-b059-60dad65abd54@amd.com>
Date: Thu, 29 May 2025 17:35:38 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
Babu Moger <babu.moger@....com>, corbet@....net, tony.luck@...el.com,
tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com
Cc: james.morse@....com, dave.martin@....com, fenghuay@...dia.com,
x86@...nel.org, hpa@...or.com, paulmck@...nel.org,
akpm@...ux-foundation.org, thuth@...hat.com, rostedt@...dmis.org,
ardb@...nel.org, gregkh@...uxfoundation.org, daniel.sneddon@...ux.intel.com,
jpoimboe@...nel.org, alexandre.chartre@...cle.com,
pawan.kumar.gupta@...ux.intel.com, thomas.lendacky@....com,
perry.yuan@....com, seanjc@...gle.com, kai.huang@...el.com,
xiaoyao.li@...el.com, kan.liang@...ux.intel.com, xin3.li@...el.com,
ebiggers@...gle.com, xin@...or.com, sohil.mehta@...el.com,
andrew.cooper3@...rix.com, mario.limonciello@....com,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
peternewman@...gle.com, maciej.wieczor-retman@...el.com, eranian@...gle.com,
Xiaojian.Du@....com, gautham.shenoy@....com
Subject: Re: [PATCH v13 20/27] x86/resctrl: Provide interface to update the
event configurations
Hi Reinette,
On 5/22/2025 11:45 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 5/15/25 3:52 PM, Babu Moger wrote:
>> Users can modify the event configuration by writing to the event_filter
>> interface file. The event configurations for mbm_cntr_assign mode are
>> located in /sys/fs/resctrl/info/event_configs/.
>
> heh ... looks like you also started thinking that "event_configs"
> is a better name (also missing L3_MON).
Interesting... 'event_configs' seems to have made its way in somehow.
Alright, I’ll go ahead and add L3_MON."
>
>>
>> Update the assignments of all groups when the event configuration is
>> modified.
>>
>> Example:
>> $ cd /sys/fs/resctrl/
>>
>> $ cat info/L3_MON/counter_configs/mbm_local_bytes/event_filter
>> local_reads,local_non_temporal_writes,local_reads_slow_memory
>>
>> $ echo "local_reads,local_non_temporal_writes" >
>> info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>>
>> $ cat info/L3_MON/counter_configs/mbm_total_bytes/event_filter
>> local_reads,local_non_temporal_writes
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v13: Updated changelog for imperative mode.
>> Added function description in the prototype.
>> Updated the user doc resctrl.rst to address few feedback.
>> Resolved conflicts caused by the recent FS/ARCH code restructure.
>> The rdtgroup.c/monitor.c file has now been split between the FS and ARCH directories.
>>
>> v12: New patch to modify event configurations.
>> ---
>> Documentation/filesystems/resctrl.rst | 12 +++
>> fs/resctrl/rdtgroup.c | 120 +++++++++++++++++++++++++-
>> 2 files changed, 131 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index 4eb9f007ba3d..9923276826db 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>
> ...
>
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index cf84e3a382ac..8c498b41be5d 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1930,6 +1930,123 @@ static int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq,
>> return 0;
>> }
>>
>> +/**
>> + * resctrl_group_assign - Update the counter assignments for the event in
>> + * a group.
>
> This name is very generic with an unexpected namespace. "rdtgroup_" prefix
> is often used for a function that operates on a rdtgroup. This can thus be
> "rdtgroup_assign_cntr()".
Sure.
>
>> + * @r: Resource to which update needs to be done.
>> + * @rdtgrp: Resctrl group.
>> + * @evtid: Event ID.
>> + * @evt_cfg: Event configuration value.
>> + */
>> +static int resctrl_group_assign(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> + enum resctrl_event_id evtid, u32 evt_cfg)
>> +{
>> + struct rdt_mon_domain *d;
>> + int cntr_id;
>> +
>> + list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> + cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
>> + if (cntr_id >= 0 && d->cntr_cfg[cntr_id].evt_cfg != evt_cfg) {
>> + d->cntr_cfg[cntr_id].evt_cfg = evt_cfg;
>> + resctrl_arch_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> + rdtgrp->closid, cntr_id, evt_cfg, true);
>> + }
>> + }
>> +
>> + return 0;
>
> Can just return void?
sure.
>
>> +}
>> +
>> +/**
>> + * resctrl_update_assign - Update the counter assignments for the event for all
>> + * the groups.
>
> Again very generic with "update" and "assign" that seem redundant? How about
> "resctrl_assign_cntr_allrdtgrp()"?
Yes.
>
>> + * @r: Resource to which update needs to be done.
>> + * @evtid: Event ID.
>> + * @evt_cfg: Event configuration value.
>
> Why are both event ID and evt_cfg needed? Could just passing mon_evt simplify this?
>
>> + */
>> +static int resctrl_update_assign(struct rdt_resource *r, enum resctrl_event_id evtid,
>> + u32 evt_cfg)
>> +{
>> + struct rdtgroup *prgrp, *crgrp;
>> +
>> + /* Check if the cntr_id is associated to the event type updated */
>
> Comment does not match code.
Will correct it.
>
>> + list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
>> + resctrl_group_assign(r, prgrp, evtid, evt_cfg);
>> +
>> + list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
>> + resctrl_group_assign(r, crgrp, evtid, evt_cfg);
>> + }
>
> Unnecessary braces?
Yes.
>
>> + }
>> +
>> + return 0;
>
> return void?
>
Sure.
>> +}
>> +
>> +static int resctrl_process_configs(char *tok, u32 *val)
>> +{
>> + char *evt_str;
>> + bool found;
>> + int i;
>> +
>> +next_config:
>> + if (!tok || tok[0] == '\0')
>> + return 0;
>> +
>> + /* Start processing the strings for each event type */
>
> Does comment intend to describe one iteration or all iterations?
> Also, "event type" -> "memory transaction"?
Sure.
>
>> + evt_str = strim(strsep(&tok, ","));
>> + found = false;
>> + for (i = 0; i < NUM_MBM_EVT_VALUES; i++) {
>> + if (!strcmp(mbm_evt_values[i].evt_name, evt_str)) {
>> + *val |= mbm_evt_values[i].evt_val;
>
> check spacing.
ok.
>
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + if (!found) {
>> + rdt_last_cmd_printf("Invalid event type %s\n", evt_str);
>> + return -EINVAL;
>
> Looks like this will return partially initialized data. Please use a local
> variable in which to gather the new configuration and only assign that
> to provided pointer on success.
Yes. Makes sense.
>
>> + }
>> +
>> + goto next_config;
>> +}
>> +
>> +static ssize_t event_filter_write(struct kernfs_open_file *of, char *buf,
>> + size_t nbytes, loff_t off)
>> +{
>> + struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>> + struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>> + u32 evt_cfg = 0;
>> + int ret = 0;
>> +
>> + /* Valid input requires a trailing newline */
>> + if (nbytes == 0 || buf[nbytes - 1] != '\n')
>> + return -EINVAL;
>> +
>> + buf[nbytes - 1] = '\0';
>> +
>> + cpus_read_lock();
>> + mutex_lock(&rdtgroup_mutex);
>> +
>> + rdt_last_cmd_clear();
>> +
>> + if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> + rdt_last_cmd_puts("mbm_cntr_assign mode is not enabled\n");
>> + ret = -EINVAL;
>> + goto unlock_out;
>
> "grep goto fs/resctrl/rdtgroup.c"
Will change it to "out_unlock".
>
>> + }
>> +
>> + ret = resctrl_process_configs(buf, &evt_cfg);
>> + if (!ret && mevt->evt_val != evt_cfg) {
>> + mevt->evt_val = evt_cfg;
>
> ah ... here it is. hmmm ... but it is mon_evt::evt_cfg, no? ah,
> fixed in next patch.
Yea. My bad.
>
> I still seem to be missing something because I expected mon_evt::evt_cfg
> of mbm_total_bytes and mbm_local_bytes to be initialized with a starting
> default. I missed where this is done in this series.
Its not done in this series. Will do in next revision.
>
>> + resctrl_update_assign(r, mevt->evtid, evt_cfg);
>> + }
>> +
>> +unlock_out:
>> + mutex_unlock(&rdtgroup_mutex);
>> + cpus_read_unlock();
>> +
>> + return ret ?: nbytes;
>> +}
>> +
>> /* rdtgroup information files for one cache resource. */
>> static struct rftype res_common_files[] = {
>> {
>> @@ -2056,9 +2173,10 @@ static struct rftype res_common_files[] = {
>> },
>> {
>> .name = "event_filter",
>> - .mode = 0444,
>> + .mode = 0644,
>> .kf_ops = &rdtgroup_kf_single_ops,
>> .seq_show = event_filter_show,
>> + .write = event_filter_write,
>> },
>> {
>> .name = "mbm_assign_mode",
>
> Reinette
>
Powered by blists - more mailing lists