Hi Reinette,
On 4/11/25 16:04, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/3/25 5:18 PM, Babu Moger wrote:
>> The mbm_cntr_assign mode offers "num_mbm_cntrs" number of counters that
>> can be assigned to an RMID, event pair and monitor the bandwidth as long
>> as it is assigned.
>
> Above makes it sound as though multiple counters can be assigned to
> an RMID, event pair.
>
Yes. Multiple counter-ids can be assigned to RMID, event pair.
>>
>> Add the functionality to allocate and assign the counters to RMID, event
>> pair in the domain.
>
> "assign *a* counter to an RMID, event pair"?
Sure.
>
>>
>> If all the counters are in use, the kernel will log the error message
>> "Unable to allocate counter in domain" in /sys/fs/resctrl/info/
>> last_cmd_status when a new assignment is requested. Exit on the first
>> failure when assigning counters across all the domains.
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
>
> ...
>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 2 +
>> arch/x86/kernel/cpu/resctrl/monitor.c | 124 +++++++++++++++++++++++++
>> 2 files changed, 126 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 0b73ec451d2c..1a8ac511241a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -574,6 +574,8 @@ bool closid_allocated(unsigned int closid);
>> int resctrl_find_cleanest_closid(void);
>> void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>> unsigned int mon_event_config_index_get(u32 evtid);
>> +int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
>> + struct rdtgroup *rdtgrp, enum resctrl_event_id evtid, u32 evt_cfg);
>
> This is internal to resctrl fs. Why is it needed to provide both the event id
> and the event configuration? Event configuration can be determined from event ID?
Yes. It can be done. Then I have to export the functions like
mbm_get_assign_config() into monitor.c. To avoid that I passed it from
here which I felt much more cleaner.
>
>>
>> #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
>> int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 77f8662dc50b..ff55a4fe044f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -1469,3 +1469,127 @@ 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. Reset the
>> + * non-architectural state to clear all the event counters.
>> + */
>> +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, u32 evt_cfg, bool assign)
>> +{
>> + struct mbm_state *m;
>> + int ret;
>> +
>> + ret = resctrl_arch_config_cntr(r, d, evtid, rmid, closid, cntr_id, evt_cfg, assign);
>> + if (ret)
>> + return ret;
>
> I understood previous discussion to conclude that resctrl_arch_config_cntr() cannot fail
> and thus I expect it to return void and not need any error checking from caller.
> By extension this will result in resctrl_config_cntr() returning void and should simplify
> a few flows. For example, it will make it clear that re-configuring an existing counter
> cannot result in that counter being freed.
Yea. I missed it. Will take care of it next version.
>
>> +
>> + m = get_mbm_state(d, closid, rmid, evtid);
>> + if (m)
>> + memset(m, 0, sizeof(struct mbm_state));
>> +
>> + return ret;
>> +}
>> +
>
> Could you please add comments to these mbm_cntr* functions to provide information
> on how the cntr_cfg data structure is used? Please also include details on
> callers since it seems to me as though these functions are called
> from paths where assignable counters are not supported (mon_event_read()).
Sure. Will add details about these functions.
>
>> +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 -ENOENT;
>> +}
>> +
>> +static int mbm_cntr_alloc(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) {
>> + d->cntr_cfg[cntr_id].rdtgrp = rdtgrp;
>> + d->cntr_cfg[cntr_id].evtid = evtid;
>> + return cntr_id;
>> + }
>> + }
>> +
>> + return -ENOSPC;
>> +}
>> +
>> +static void mbm_cntr_free(struct rdt_mon_domain *d, int cntr_id)
>> +{
>> + memset(&d->cntr_cfg[cntr_id], 0, sizeof(struct mbm_cntr_cfg));
>> +}
>> +
>> +/*
>> + * Allocate a fresh counter and configure the event if not assigned already.
>> + */
>> +static int resctrl_alloc_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> + struct rdtgroup *rdtgrp, enum resctrl_event_id evtid,
>> + u32 evt_cfg)
>
> Same here, why are both evtid and evt_cfg provided as arguments?
Yes. It can be done. Then I have to export the functions like
mbm_get_assign_config() into monitor.c. To avoid that I passed it from
here which I felt much more cleaner.
>
>> +{
>> + int cntr_id, ret = 0;
>> +
>> + /*
>> + * No need to allocate or configure if the counter is already assigned
>> + * and the event configuration is up to date.
>> + */
>> + cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
>> + if (cntr_id >= 0) {
>> + if (d->cntr_cfg[cntr_id].evt_cfg == evt_cfg)
>> + return 0;
>> +
>> + goto cntr_configure;
>> + }
>> +
>> + cntr_id = mbm_cntr_alloc(r, d, rdtgrp, evtid);
>> + if (cntr_id < 0) {
>> + rdt_last_cmd_printf("Unable to allocate counter in domain %d\n",
>> + d->hdr.id);
>
> Please print resource name also.
Sure. We can print r->name.
>
>> + return cntr_id;
>> + }
>> +
>> +cntr_configure:
>> + /* Update and configure the domain with the new event configuration value */
>> + d->cntr_cfg[cntr_id].evt_cfg = evt_cfg;
>> +
>> + ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid, rdtgrp->closid,
>> + cntr_id, evt_cfg, true);
>> + if (ret) {
>> + rdt_last_cmd_printf("Assignment of event %d failed on domain %d\n",
>> + evtid, d->hdr.id);
>
> How is user expected to interpret the event ID (especially when looking forward
> where events can be dynamic)? This should rather be the event name.
Sure. We can do that.
>
>> + mbm_cntr_free(d, cntr_id);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Assign a hardware counter to event @evtid of group @rdtgrp. Counter will be
>> + * assigned to all the domains if @d is NULL else the counter will be assigned
>> + * to @d.
>> + */
>> +int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
>> + struct rdtgroup *rdtgrp, enum resctrl_event_id evtid,
>> + u32 evt_cfg)
>> +{
>> + int ret = 0;
>> +
>> + if (!d) {
>> + list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> + ret = resctrl_alloc_config_cntr(r, d, rdtgrp, evtid, evt_cfg);
>> + if (ret)
>> + return ret;
>> + }
>> + } else {
>> + ret = resctrl_alloc_config_cntr(r, d, rdtgrp, evtid, evt_cfg);
>> + }
>> +
>> + return ret;
>> +}
>
> Reinette
>
--
Thanks
Babu Moger
