Hi Babu,
On 8/6/24 3:00 PM, Babu Moger wrote:
The ABMC feature provides an option to the user to assign a hardware
This is about resctrl fs so "The ABMC feature" -> "mbm_cntr_assign mode"
(please check whole series).
counter to an RMID and monitor the bandwidth as long as it is assigned.
The assigned RMID will be tracked by the hardware until the user unassigns
it manually.
Hardware provides only limited number of counters. If the system runs out
of assignable counters, kernel will display an error when a new assignment
is requested. Users need to unassign a already assigned counter to make
space for new assignment.
Provide the interface to unassign the counter ids from the group. Free the
counter if it is not assigned in any of the domains.
The feature details are documented in the APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
Monitoring (ABMC).
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
v6: Removed mbm_cntr_free from this patch.
Added counter test in all the domains and free if it is not assigned to
any domains.
v5: Few name changes to match cntr_id.
Changed the function names to
rdtgroup_unassign_cntr
More comments on commit log.
v4: Added domain specific unassign feature.
Few name changes.
v3: Removed the static from the prototype of rdtgroup_unassign_abmc.
The function is not called directly from user anymore. These
changes are related to global assignment interface.
v2: No changes.
---
arch/x86/kernel/cpu/resctrl/internal.h | 2 +
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 52 ++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 4e8109dee174..cc832955b787 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -689,6 +689,8 @@ int resctrl_arch_assign_cntr(struct rdt_mon_domain *d, enum resctrl_event_id evt
u32 rmid, u32 cntr_id, u32 closid, bool assign);
int rdtgroup_assign_cntr(struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);
int rdtgroup_alloc_cntr(struct rdtgroup *rdtgrp, int index);
+int rdtgroup_unassign_cntr(struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);
+void rdtgroup_free_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp, int index);
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 1ee91a7293a8..0c2215dbd497 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1961,6 +1961,58 @@ int rdtgroup_assign_cntr(struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
return 0;
}
+static int rdtgroup_mbm_cntr_test(struct rdt_resource *r, u32 cntr_id)
Could "test" be replaced with something more specific about what is tested?
for example, "rdtgroup_mbm_cntr_is_assigned()" or something better? The function
looks like a good candidate for returning a bool.
Is this function needed though? (more below)
+{
+ struct rdt_mon_domain *d;
+
+ list_for_each_entry(d, &r->mon_domains, hdr.list)
+ if (test_bit(cntr_id, d->mbm_cntr_map))
+ return 1;
+
+ return 0;
+}
+
+/* Free the counter id after the event is unassigned */
+void rdtgroup_free_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+ int index)
+{
+ /* Update the counter bitmap */
+ if (!rdtgroup_mbm_cntr_test(r, rdtgrp->mon.cntr_id[index])) {
+ mbm_cntr_free(rdtgrp->mon.cntr_id[index]);
+ rdtgrp->mon.cntr_id[index] = MON_CNTR_UNSET;
+ }
+}
+
+/*
+ * Unassign a hardware counter from the group and update all the domains
+ * in the group.
+ */
+int rdtgroup_unassign_cntr(struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
+{
+ struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
+ struct rdt_mon_domain *d;
+ int index;
+
+ index = mon_event_config_index_get(evtid);
+ if (index == INVALID_CONFIG_INDEX)
+ return -EINVAL;
+
+ if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET) {
+ list_for_each_entry(d, &r->mon_domains, hdr.list) {
+ resctrl_arch_assign_cntr(d, evtid, rdtgrp->mon.rmid,
+ rdtgrp->mon.cntr_id[index],
+ rdtgrp->closid, false);
+ clear_bit(rdtgrp->mon.cntr_id[index],
+ d->mbm_cntr_map);
+ }
+
+ /* Free the counter at group level */
+ rdtgroup_free_cntr(r, rdtgrp, index);
rdtgroup_free_cntr() is called right after the counter has been unassigned
from all domains. Will rdtgroup_mbm_cntr_test() thus not always return 0?
It seems unnecessary to have rdtgroup_mbm_cntr_test() and considering that,
rdtgroup_free_cntr() can just be open coded here?
+ }
+
+ return 0;
+}
+
/* rdtgroup information files for one cache resource. */
static struct rftype res_common_files[] = {
{
Reinette
