Hi Reinette,
On 4/15/25 11:53, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/15/25 7:20 AM, Moger, Babu wrote:
>> 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.
>
> oh, are you referring to the assignments of different counters across multiple
> domains?
May be I am confusing you here. This is what I meant.
Here is one example.
In a same group,
Configure cntr_id 0, to count reads only (This maps to total event).
Configure cntr_id 1, to count write only (This maps to local event).
Configure cntr_id 2, to count dirty victims.
so on..
so on..
Configure cntr_id 31, to count remote read only.
We have 32 counter ids in a domain. Basically, we can configure all the
counters in a domain to just one group if you want to.
We cannot do that right now because our data structures cannot do that.
We can only configure 2 events(local and total) right now.
My understanding it is same with MPAM also.
>
>>
>>>>
>>>> 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.
>
>>From what I can tell, for example by looking at patch #22, callers of
> resctrl_assign_cntr_event() now need to call mbm_get_assign_config()
> every time before calling resctrl_assign_cntr_event(). Calling
> mbm_get_assign_config() from within resctrl_assign_cntr_event() seems
> simpler to me and that may result in mbm_get_assign_config() moving to
> monitor.c as an extra benefit.
Sure.
>
> ...
>
>>>> +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.
>
> Maybe even resctrl_assign_cntr_event() does not need to call mbm_get_assign_config()
> but only resctrl_alloc_config_cntr() needs to call mbm_get_assign_config(). Doing so
> may avoid more burden on callers while reducing parameters needed throughout.
>
ok. Sure. Will do.
--
Thanks
Babu Moger
