Hi Reinette,
On 11/18/2024 1:43 PM, Reinette Chatre wrote:
Hi Babu,
On 10/29/24 4:21 PM, Babu Moger wrote:
Users can modify the configuration of assignable events. Whenever the
event configuration is updated, MBM assignments must be revised across
all monitor groups within the impacted domains.
Please revisit the "Changelog" section in
Documentation/process/maintainer-tip.rst
ok.
Imperative mood, context, problem and solution.
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
v9: Again patch changed completely based on the comment.
https://lore.kernel.org/lkml/03b278b5-6c15-4d09-9ab7-3317e84a409e@xxxxxxxxx/
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.
I see ... the WARN is because there is a check whether IRQs are disabled before
the check whether the function can be run locally.
ok
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@xxxxxxx/
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 | 86 +++++++++++++++++++++++---
include/linux/resctrl.h | 3 +-
2 files changed, 79 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5b8bb8bd913c..7646d67ea10e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1710,6 +1710,7 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
}
struct mon_config_info {
+ struct rdt_resource *r;
struct rdt_mon_domain *d;
u32 evtid;
u32 mon_config;
@@ -1735,26 +1736,28 @@ 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;
}
}
@@ -1826,6 +1829,70 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
return 0;
}
+static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_index(int cntr_id, unsigned int index)
+{
+ struct rdtgroup *prgrp, *crgrp;
+
+ /* Check if the cntr_id is associated to the event type updated */
+ list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
+ if (prgrp->mon.cntr_id[index] == cntr_id)
+ return prgrp;
+
+ list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
+ if (crgrp->mon.cntr_id[index] == cntr_id)
+ return crgrp;
+ }
+ }
+
+ return NULL;
+}
+
+static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+ enum resctrl_event_id evtid, u32 rmid,
+ u32 closid, u32 cntr_id, u32 val)
+{
+ union l3_qos_abmc_cfg abmc_cfg = { 0 };
+
+ abmc_cfg.split.cfg_en = 1;
+ abmc_cfg.split.cntr_en = 1;
+ abmc_cfg.split.cntr_id = cntr_id;
+ abmc_cfg.split.bw_src = rmid;
+ abmc_cfg.split.bw_type = val;
+
+ wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full);
Is it needed to create an almost duplicate function? What if instead
only resctrl_arch_config_cntr() exists and it uses parameter to decide
whether to call resctrl_abmc_config_one_amd() directly or via
smp_call_function_any()? I think that should help to make clear how
the code flows.
Also note that this is an almost identical arch callback with no
error return. I expect that building on existing resctrl_arch_config_cntr()
will make things easier to understand.
It can be done. But it takes another parameter to the function.
It has 7 parameters already. This will be 8th.
Will change it if that is ok.
+}
+
+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;
Note that local variable r is created here while the function is inconsistent by
switching between using r and mon_info->r.
Sure. Got it.
+ struct rdtgroup *rdtgrp;
+ unsigned int index;
+ u32 cntr_id;
+
+ resctrl_arch_mon_event_config_set(d, mon_info->evtid, mon_info->mon_config);
+
+ if (!resctrl_arch_mbm_cntr_assign_enabled(r))
+ return;
+
+ index = mon_event_config_index_get(mon_info->evtid);
This is an AMD arch specific helper to know which offset of an MSR to use. It should
not be used directly in resctrl fs code, this is what MBM_EVENT_ARRAY_INDEX was created for.
Sure.
Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used,
within rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name.
How about ?
static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_event(int cntr_id,
enum resctrl_event_id evtid)
Will move the macro MBM_EVENT_ARRAY_INDEX inside the function.
+ if (index == INVALID_CONFIG_INDEX)
+ return;
+
+ for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
+ if (test_bit(cntr_id, d->mbm_cntr_map)) {
+ rdtgrp = rdtgroup_find_grp_by_cntr_id_index(cntr_id, index);
+ if (rdtgrp)
+ resctrl_arch_update_cntr(mon_info->r, d,
+ mon_info->evtid,
+ rdtgrp->mon.rmid,
+ rdtgrp->closid,
+ cntr_id,
+ mon_info->mon_config);
+ }
+ }
+}
Could you please add some function comments to explain the flow here? For example,
what should reader consider if there is to rdtgroup found?
Sure.
--
- Babu Moger
