Hi Reinette,
On 12/19/2024 5:32 PM, Reinette Chatre wrote:
Hi Babu,
On 12/12/24 12:15 PM, Babu Moger wrote:
The mbm_cntr_assign mode provides a limited number of hardware counters
that can be assigned to an RMID, event pair to monitor bandwidth while
assigned. If all counters are in use, the kernel will show an error
message: "Out of MBM assignable counters" when a new assignment is
requested. To make space for a new assignment, users must unassign an
already assigned counter.
Introduce an interface that allows for the unassignment of counter IDs
from the domain.
Subject and changelog claims this introduces an interface, there is no new
resctrl interface introduced here. Can this be more specific?
Sure. Let me rewrite the subject and description.
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
---
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 70d2577fc377..f858098dbe4b 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -706,6 +706,8 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
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);
+int rdtgroup_unassign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+ struct rdt_mon_domain *d, enum resctrl_event_id evtid);
(please use consistent parameter ordering)
Sure.
struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
u32 rmid, enum resctrl_event_id evtid);
#endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 1c8694a68cf4..a71a8389b649 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1990,6 +1990,20 @@ static void mbm_cntr_free(struct rdt_resource *r, struct rdt_mon_domain *d,
}
}
+static int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
+ struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
+{
+ int cntr_id;
+
+ for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
+ if (d->cntr_cfg[cntr_id].rdtgrp == rdtgrp &&
+ d->cntr_cfg[cntr_id].evtid == evtid)
+ return cntr_id;
+ }
+
+ return -EINVAL;
This could be -ENOENT?
Sure.
+}
mbm_cntr_get() seems to be essentially a duplicate of mbm_cntr_assigned() that returns
actual counter ID instrad of true/false. Could only one be used?
Yes. We can use mbm_cntr_get() alone.
+
/*
* Assign a hardware counter to event @evtid of group @rdtgrp.
* Counter will be assigned to all the domains if rdt_mon_domain is NULL
@@ -2037,6 +2051,44 @@ int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
return ret;
}
+/*
+ * Unassign a hardware counter associated with @evtid from the domain and
+ * the group. Unassign the counters from all the domains if rdt_mon_domain
+ * is NULL else unassign from the specific domain.
(same comment as previous patch about consistency in referring to function
parameters)
Sure.
+ */
+int rdtgroup_unassign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+ struct rdt_mon_domain *d, enum resctrl_event_id evtid)
+{
+ int cntr_id, ret = 0;
+
+ if (!d) {
+ list_for_each_entry(d, &r->mon_domains, hdr.list) {
+ if (!mbm_cntr_assigned(r, d, rdtgrp, evtid))
+ continue;
+
+ cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
+
It seems unnecessary to loop over array twice here. mbm_cntr_assigned() seems
unnecessary. Return value of mbm_cntr_get() can be used to determine if it
is assigned or not?
Yes. Sure.
+ ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
+ rdtgrp->closid, cntr_id, false);
+ if (!ret)
+ mbm_cntr_free(r, d, rdtgrp, evtid);
... and by providing cntr_id to mbm_cntr_free() another unnecessary loop can be avoided.
Sure.
+ }
+ } else {
+ if (!mbm_cntr_assigned(r, d, rdtgrp, evtid))
+ goto out_done_unassign;
+
+ cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
+
+ ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
+ rdtgrp->closid, cntr_id, false);
+ if (!ret)
+ mbm_cntr_free(r, d, rdtgrp, evtid);
+ }
+
+out_done_unassign:
+ return ret;
+}
+
/* rdtgroup information files for one cache resource. */
static struct rftype res_common_files[] = {
{
Reinette
