Resctrl provides option to configure events by writing to the interfaces
/sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config or
/sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config when BMEC (Bandwidth
Monitoring Event Configuration) is supported.
Whenever the event configuration is updated, MBM assignments must be
revised across all monitor groups within the impacted domains.
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v10: Code changed completely with domain specific counter assignment.
Rewrite the commit message.
Added few more code comments.
v9: Again patch changed completely based on the comment.
https://lore.kernel.org/lkml/03b278b5-6c15-4d09-9ab7-3317e84a409e@intel.com/
Introduced resctrl_mon_event_config_set to handle IPI.
But sending another IPI inside IPI causes problem. Kernel reports SMP
warning. So, introduced resctrl_arch_update_cntr() to send the command directly.
v8: Patch changed completely.
Updated the assignment on same IPI as the event is updated.
Could not do the way we discussed in the thread.
https://lore.kernel.org/lkml/f77737ac-d3f6-3e4b-3565-564f79c86ca8@amd.com/
Needed to figure out event type to update the configuration.
v7: New patch to update the assignments. Missed it earlier.
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 67 ++++++++++++++++++++++----
include/linux/resctrl.h | 4 +-
2 files changed, 61 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 65b3556978ad..6b5c886b7e99 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1704,26 +1704,26 @@ u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
return INVALID_CONFIG_VALUE;
}
-void resctrl_arch_mon_event_config_set(void *info)
+void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d,
+ enum resctrl_event_id eventid, u32 val)
{
- struct mon_config_info *mon_info = info;
struct rdt_hw_mon_domain *hw_dom;
unsigned int index;
- index = mon_event_config_index_get(mon_info->evtid);
+ index = mon_event_config_index_get(eventid);
if (index == INVALID_CONFIG_INDEX)
return;
- wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
+ wrmsr(MSR_IA32_EVT_CFG_BASE + index, val, 0);
- hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
+ hw_dom = resctrl_to_arch_mon_dom(d);
- switch (mon_info->evtid) {
+ switch (eventid) {
case QOS_L3_MBM_TOTAL_EVENT_ID:
- hw_dom->mbm_total_cfg = mon_info->mon_config;
+ hw_dom->mbm_total_cfg = val;
break;
case QOS_L3_MBM_LOCAL_EVENT_ID:
- hw_dom->mbm_local_cfg = mon_info->mon_config;
+ hw_dom->mbm_local_cfg = val;
break;
default:
break;
@@ -1825,6 +1825,54 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
return 0;
}
+/*
+ * Review the cntr_cfg domain configuration. If a matching assignment is found,
+ * update the counter assignment accordingly. This is within the IPI Context,
+ * so call resctrl_abmc_config_one_amd directly.
+ */
+static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+ enum resctrl_event_id evtid, u32 val)
+{
+ struct cntr_config config;
+ struct rdtgroup *rdtgrp;
+ struct mbm_state *m;
+ u32 cntr_id;
+
+ for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
+ rdtgrp = d->cntr_cfg[cntr_id].rdtgrp;
+ if (rdtgrp && d->cntr_cfg[cntr_id].evtid == evtid) {
+ memset(&config, 0, sizeof(struct cntr_config));
+ config.r = r;
+ config.d = d;
+ config.evtid = evtid;
+ config.rmid = rdtgrp->mon.rmid;
+ config.closid = rdtgrp->closid;
+ config.cntr_id = cntr_id;
+ config.val = val;
+ config.assign = 1;
+
+ resctrl_abmc_config_one_amd(&config);
+
+ m = get_mbm_state(d, rdtgrp->closid, rdtgrp->mon.rmid, evtid);
+ if (m)
+ memset(m, 0, sizeof(struct mbm_state));
+ }
+ }
+}
+
+static void resctrl_mon_event_config_set(void *info)
+{
+ struct mon_config_info *mon_info = info;
+ struct rdt_mon_domain *d = mon_info->d;
+ struct rdt_resource *r = mon_info->r;
+
+ resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config);
+
+ /* Check if assignments needs to be updated */
+ if (resctrl_arch_mbm_cntr_assign_enabled(r))
+ resctrl_arch_update_cntr(r, d, mon_info->evtid,
+ mon_info->mon_config);
+}
static void mbm_config_write_domain(struct rdt_resource *r,
struct rdt_mon_domain *d, u32 evtid, u32 val)
@@ -1840,6 +1888,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
if (config_val == INVALID_CONFIG_VALUE || config_val == val)
return;
+ mon_info.r = r;
mon_info.d = d;
mon_info.evtid = evtid;
mon_info.mon_config = val;
@@ -1851,7 +1900,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
* on one CPU is observed by all the CPUs in the domain.
*/
smp_call_function_any(&d->hdr.cpu_mask,
- resctrl_arch_mon_event_config_set,
+ resctrl_mon_event_config_set,
&mon_info, 1);
/*
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 03c67d9156f3..2bf461179680 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -137,6 +137,7 @@ struct rdt_mon_domain {
* @mon_config: Event configuration value
*/
struct mon_config_info {
+ struct rdt_resource *r;
struct rdt_mon_domain *d;
enum resctrl_event_id evtid;
u32 mon_config;
@@ -376,7 +377,8 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
*/
void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
-void resctrl_arch_mon_event_config_set(void *info);
+void resctrl_arch_mon_event_config_set(struct rdt_mon_domain *d,
+ enum resctrl_event_id eventid, u32 val);
u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
enum resctrl_event_id eventid);
--
2.34.1Hi Babu,
On 12/12/24 12:15 PM, Babu Moger wrote:
> Resctrl provides option to configure events by writing to the interfaces
> /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config or
> /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config when BMEC (Bandwidth
> Monitoring Event Configuration) is supported.
>
> Whenever the event configuration is updated, MBM assignments must be
> revised across all monitor groups within the impacted domains.
This needs imperative tone description of what this patch does.
...
> @@ -1825,6 +1825,54 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
> return 0;
> }
>
> +/*
> + * Review the cntr_cfg domain configuration. If a matching assignment is found,
> + * update the counter assignment accordingly. This is within the IPI Context,
This "Review the cntr_cfg domain configuration. If a matching assignment is found,"
is too vague for me to make sense of what it is trying to do. Can this be made more specific?
> + * so call resctrl_abmc_config_one_amd directly.
> + */
> +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> + enum resctrl_event_id evtid, u32 val)
> +{
> + struct cntr_config config;
> + struct rdtgroup *rdtgrp;
> + struct mbm_state *m;
> + u32 cntr_id;
> +
> + for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
> + rdtgrp = d->cntr_cfg[cntr_id].rdtgrp;
> + if (rdtgrp && d->cntr_cfg[cntr_id].evtid == evtid) {
> + memset(&config, 0, sizeof(struct cntr_config));
> + config.r = r;
> + config.d = d;
> + config.evtid = evtid;
> + config.rmid = rdtgrp->mon.rmid;
> + config.closid = rdtgrp->closid;
> + config.cntr_id = cntr_id;
> + config.val = val;
> + config.assign = 1;
> +
> + resctrl_abmc_config_one_amd(&config);
> +
> + m = get_mbm_state(d, rdtgrp->closid, rdtgrp->mon.rmid, evtid);
> + if (m)
> + memset(m, 0, sizeof(struct mbm_state));
> + }
> + }
> +}
> +
> +static void resctrl_mon_event_config_set(void *info)
> +{
> + struct mon_config_info *mon_info = info;
> + struct rdt_mon_domain *d = mon_info->d;
> + struct rdt_resource *r = mon_info->r;
> +
> + resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config);
> +
> + /* Check if assignments needs to be updated */
> + if (resctrl_arch_mbm_cntr_assign_enabled(r))
> + resctrl_arch_update_cntr(r, d, mon_info->evtid,
> + mon_info->mon_config);
> +}
>
> static void mbm_config_write_domain(struct rdt_resource *r,
> struct rdt_mon_domain *d, u32 evtid, u32 val)
> @@ -1840,6 +1888,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
> if (config_val == INVALID_CONFIG_VALUE || config_val == val)
> return;
>
> + mon_info.r = r;
> mon_info.d = d;
> mon_info.evtid = evtid;
> mon_info.mon_config = val;
> @@ -1851,7 +1900,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
> * on one CPU is observed by all the CPUs in the domain.
> */
> smp_call_function_any(&d->hdr.cpu_mask,
> - resctrl_arch_mon_event_config_set,
> + resctrl_mon_event_config_set,
> &mon_info, 1);
>
> /*
Reinette
Hi Reinette, On 12/19/2024 9:12 PM, Reinette Chatre wrote: > Hi Babu, > > On 12/12/24 12:15 PM, Babu Moger wrote: >> Resctrl provides option to configure events by writing to the interfaces >> /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config or >> /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config when BMEC (Bandwidth >> Monitoring Event Configuration) is supported. >> >> Whenever the event configuration is updated, MBM assignments must be >> revised across all monitor groups within the impacted domains. > > This needs imperative tone description of what this patch does. Sure. > > > ... > >> @@ -1825,6 +1825,54 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of, >> return 0; >> } >> >> +/* >> + * Review the cntr_cfg domain configuration. If a matching assignment is found, >> + * update the counter assignment accordingly. This is within the IPI Context, > > This "Review the cntr_cfg domain configuration. If a matching assignment is found," > is too vague for me to make sense of what it is trying to do. Can this be made more specific? Does this look ok? Check the counter configuration in the domain. If the specific event is configured, then update the assignment with the new event configuration value. This is within the IPI Context, so call resctrl_abmc_config_one_amd directly" Thanks, Babu
Hi Babu,
On 12/21/24 6:59 AM, Moger, Babu wrote:
> Hi Reinette,
>
> On 12/19/2024 9:12 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 12/12/24 12:15 PM, Babu Moger wrote:
>>> Resctrl provides option to configure events by writing to the interfaces
>>> /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config or
>>> /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config when BMEC (Bandwidth
>>> Monitoring Event Configuration) is supported.
>>>
>>> Whenever the event configuration is updated, MBM assignments must be
>>> revised across all monitor groups within the impacted domains.
>>
>> This needs imperative tone description of what this patch does.
>
> Sure.
>
>>
>> ...
>>
>>> @@ -1825,6 +1825,54 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>> return 0;
>>> }
>>> +/*
>>> + * Review the cntr_cfg domain configuration. If a matching assignment is found,
>>> + * update the counter assignment accordingly. This is within the IPI Context,
>>
>> This "Review the cntr_cfg domain configuration. If a matching assignment is found,"
>> is too vague for me to make sense of what it is trying to do. Can this be made more specific?
>
> Does this look ok?
>
> Check the counter configuration in the domain. If the specific event is configured, then update the assignment with the new event configuration value. This is within the IPI Context, so call resctrl_abmc_config_one_amd directly"
I think it will be easier to understand what this function does if the
comment is made more specific. For example:
Update hardware counter configuration after event configuration change.
Walk the hardware counters of domain @d to reconfigure all assigned
counters that are monitoring @evtid with the event's new configuration
@mon_config (or @config_val).
This is run on a CPU belonging to domain @d so call
resctrl_abmc_config_one_amd() directly.
Looking closer at architecture specific resctrl_arch_update_cntr() the
reset of non-arch state (get_mbm_state()->memset()) seems out of place.
There is a resctrl_arch_reset_rmid_all() within mbm_config_write_domain() that
resets all architectural state after the event configuration is changed,
should the non-architectural state not also be reset at that time? It looks
to me like it is something that may be needed for existing event
configuration (but not an issue until Peter's new feature lands) and when done,
the reset done within resctrl_arch_update_cntr() will no longer be necessary.
Something else to consider is the resctrl_arch_reset_rmid() within
resctrl_abmc_config_one_amd() seems redundant on this call path since
it is followed by resctrl_arch_reset_rmid_all(). resctrl_arch_reset_rmid()
does one MSR write and one MSR read for every counter that needs to be
reconfigured and if that is unnecessary it may be worthwhile to optimize
out?
Reinette
Hi Reinette,
On 12/23/24 10:20, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/21/24 6:59 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 12/19/2024 9:12 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 12/12/24 12:15 PM, Babu Moger wrote:
>>>> Resctrl provides option to configure events by writing to the interfaces
>>>> /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config or
>>>> /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config when BMEC (Bandwidth
>>>> Monitoring Event Configuration) is supported.
>>>>
>>>> Whenever the event configuration is updated, MBM assignments must be
>>>> revised across all monitor groups within the impacted domains.
>>>
>>> This needs imperative tone description of what this patch does.
>>
>> Sure.
>>
>>>
>>> ...
>>>
>>>> @@ -1825,6 +1825,54 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>>> return 0;
>>>> }
>>>> +/*
>>>> + * Review the cntr_cfg domain configuration. If a matching assignment is found,
>>>> + * update the counter assignment accordingly. This is within the IPI Context,
>>>
>>> This "Review the cntr_cfg domain configuration. If a matching assignment is found,"
>>> is too vague for me to make sense of what it is trying to do. Can this be made more specific?
>>
>> Does this look ok?
>>
>> Check the counter configuration in the domain. If the specific event is configured, then update the assignment with the new event configuration value. This is within the IPI Context, so call resctrl_abmc_config_one_amd directly"
>
> I think it will be easier to understand what this function does if the
> comment is made more specific. For example:
> Update hardware counter configuration after event configuration change.
>
> Walk the hardware counters of domain @d to reconfigure all assigned
> counters that are monitoring @evtid with the event's new configuration
> @mon_config (or @config_val).
>
> This is run on a CPU belonging to domain @d so call
> resctrl_abmc_config_one_amd() directly.
Looks good. Thanks
>
> Looking closer at architecture specific resctrl_arch_update_cntr() the
> reset of non-arch state (get_mbm_state()->memset()) seems out of place.
> There is a resctrl_arch_reset_rmid_all() within mbm_config_write_domain() that
> resets all architectural state after the event configuration is changed,
> should the non-architectural state not also be reset at that time? It looks
Moved the reset of non-arch state inside mbm_config_write_domain(). It
seems to work fine. Also I can simplify the IPI code further.
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5f5cf9b3a053..ce08fb718e2e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2076,9 +2076,6 @@ static void resctrl_abmc_config_one_amd(void *info)
abmc_cfg.split.bw_type = config->val;
wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
-
- resctrl_arch_reset_rmid(config->r, config->d, config->closid,
- config->rmid, config->evtid);
}
static int mbm_config_show(struct seq_file *s, struct rdt_resource *r,
u32 evtid)
@@ -2153,10 +2150,6 @@ static void resctrl_arch_update_cntr(struct
rdt_resource *r, struct rdt_mon_doma
config.assign = 1;
resctrl_abmc_config_one_amd(&config);
-
- m = get_mbm_state(d, rdtgrp->closid,
rdtgrp->mon.rmid, evtid);
- if (m)
- memset(m, 0, sizeof(struct mbm_state));
}
}
}
@@ -2178,6 +2171,7 @@ static void resctrl_mon_event_config_set(void *info)
static void mbm_config_write_domain(struct rdt_resource *r,
struct rdt_mon_domain *d, u32 evtid,
u32 val)
{
+ u32 idx_limit = resctrl_arch_system_num_rmid_idx();
struct mon_config_info mon_info = {0};
u32 config_val;
@@ -2214,6 +2208,12 @@ static void mbm_config_write_domain(struct
rdt_resource *r,
* mbm_local and mbm_total counts for all the RMIDs.
*/
resctrl_arch_reset_rmid_all(r, d);
+
+ if (is_mbm_total_enabled())
+ memset(d->mbm_total, 0, sizeof(struct mbm_state) * idx_limit);
+
+ if (is_mbm_local_enabled())
+ memset(d->mbm_local, 0, sizeof(struct mbm_state) * idx_limit);
}
static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
> to me like it is something that may be needed for existing event
> configuration (but not an issue until Peter's new feature lands) and when done,
> the reset done within resctrl_arch_update_cntr() will no longer be necessary.
>
> Something else to consider is the resctrl_arch_reset_rmid() within
> resctrl_abmc_config_one_amd() seems redundant on this call path since
> it is followed by resctrl_arch_reset_rmid_all(). resctrl_arch_reset_rmid()
> does one MSR write and one MSR read for every counter that needs to be
> reconfigured and if that is unnecessary it may be worthwhile to optimize
> out?
Yes. Removed the resctrl_arch_reset_rmid() within
resctrl_abmc_config_one_amd().
Tested the code and seems to work fine.
--
Thanks
Babu Moger
© 2016 - 2025 Red Hat, Inc.