Hi Reinette,
On 11/22/2024 4:07 PM, Reinette Chatre wrote:
Hi Babu,
On 11/22/24 1:04 PM, Moger, Babu wrote:
Hi Reinette,
On 11/21/2024 2:50 PM, Reinette Chatre wrote:
Hi Babu,
On 11/20/24 10:05 AM, Moger, Babu wrote:
Hi Reinette,
On 11/15/24 18:57, Reinette Chatre wrote:
Hi Babu,
On 10/29/24 4:21 PM, Babu Moger wrote:
The mbm_cntr_assign mode offers several hardware counters that can be
assigned to an RMID, event pair and monitor the bandwidth as long as it
is assigned.
Counters are managed at two levels. The global assignment is tracked
using the mbm_cntr_free_map field in the struct resctrl_mon, while
domain-specific assignments are tracked using the mbm_cntr_map field
in the struct rdt_mon_domain. Allocation begins at the global level
and is then applied individually to each domain.
Introduce an interface to allocate these counters and update the
corresponding domains accordingly.
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
...
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 00f7bf60e16a..cb496bd97007 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -717,6 +717,8 @@ unsigned int mon_event_config_index_get(u32 evtid);
int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
enum resctrl_event_id evtid, u32 rmid, u32 closid,
u32 cntr_id, bool assign);
+int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+ struct rdt_mon_domain *d, enum resctrl_event_id evtid);
void rdt_staged_configs_clear(void);
bool closid_allocated(unsigned int closid);
int resctrl_find_cleanest_closid(void);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 1b5529c212f5..bc3752967c44 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1924,6 +1924,93 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
return 0;
}
+/*
+ * Configure the counter for the event, RMID pair for the domain.
+ * Update the bitmap and reset the architectural state.
+ */
+static int resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+ enum resctrl_event_id evtid, u32 rmid, u32 closid,
+ u32 cntr_id, bool assign)
+{
+ int ret;
+
+ ret = resctrl_arch_config_cntr(r, d, evtid, rmid, closid, cntr_id, assign);
+ if (ret)
+ return ret;
+
+ if (assign)
+ __set_bit(cntr_id, d->mbm_cntr_map);
+ else
+ __clear_bit(cntr_id, d->mbm_cntr_map);
+
+ /*
+ * Reset the architectural state so that reading of hardware
+ * counter is not considered as an overflow in next update.
+ */
+ resctrl_arch_reset_rmid(r, d, closid, rmid, evtid);
resctrl_arch_reset_rmid() expects to be run on a CPU that is in the domain
@d ... note that after the architectural state is reset it initializes the
state by reading the event on the current CPU. By running it here it is
run on a random CPU that may not be in the right domain.
Yes. That is correct. We can move this part to our earlier
implementation. We dont need to read the RMID. We just have to reset the
counter.
https://lore.kernel.org/lkml/16d88cc4091cef1999b7ec329364e12dd0dc748d.1728495588.git.babu.moger@xxxxxxx/
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 9fe419d0c536..bc3654ec3a08 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2371,6 +2371,13 @@ int resctrl_arch_config_cntr(struct rdt_resource
*r, struct rdt_mon_domain *d,
smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd,
&abmc_cfg, 1);
+ /*
+ * Reset the architectural state so that reading of hardware
+ * counter is not considered as an overflow in next update.
+ */
+ if (arch_mbm)
+ memset(arch_mbm, 0, sizeof(struct arch_mbm_state));
+
return 0;
}
I am not sure what you envision here. One motivation for the move out of
resctrl_arch_config_cntr() was to avoid architectural state being reset twice. For reference,
mbm_config_write_domain()->resctrl_arch_reset_rmid_all(). Will architectural state
be reset twice again?
That is good point. We don't have to do it twice.
We can move the whole reset(arch_mbm) in resctrl_arch_config_cntr().
This is not clear to me. The architectural state needs to be reset on MBM config write even
when assignable mode is not supported and/or enabled. Moving it to resctrl_arch_config_cntr()
will break this, no?
Yes. The architectural state needs to be reset if ABMC is enabled or not
enabled on MBM config write.
I wonder if it may not simplify things to call resctrl_arch_reset_rmid() from
resctrl_abmc_config_one_amd()?
Yes. That is is an option. I can try.
One thing that I did not notice before is that the non-architectural MBM state is not
reset. Care should be taken to reset this also when considering that there is a plan
to use that MBM state to build a generic rate event for all platforms:
https://lore.kernel.org/all/CALPaoCgFRFgQqG00Uc0GhMHK47bsbtFw6Bxy5O9A_HeYmGa5sA@xxxxxxxxxxxxxx/
Did you mean we should add the following code in resctrl_arch_config_cntr()?
m = get_mbm_state(d, closid, rmid, evtid);
if (m)
memset(m, 0, sizeof(struct mbm_state));
This is not arch code but instead resctrl fs, so resctrl_config_cntr() may be more appropriate?
Sure. We can do that.
Reinette
--
- Babu Moger
