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?
>
> 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)
> 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?
> +}
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?
> +
> /*
> * 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)
> + */
> +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?
> + 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.
> + }
> + } 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
