The "mbm_event" mode allows users to assign a hardware counter ID to an
RMID, event pair and monitor bandwidth usage as long as it is assigned.
The hardware continues to track the assigned counter until it is
explicitly unassigned by the user. Counters are assigned/unassigned at
monitoring domain level.
Manage a monitoring domain's hardware counters using a per monitoring
domain array of struct mbm_cntr_cfg that is indexed by the hardware
counter ID. A hardware counter's configuration contains the MBM event
ID and points to the monitoring group that it is assigned to, with a
NULL pointer meaning that the hardware counter is available for assignment.
There is no direct way to determine which hardware counters are assigned
to a particular monitoring group. Check every entry of every hardware
counter configuration array in every monitoring domain to query which
MBM events of a monitoring group is tracked by hardware. Such queries are
acceptable because of a very small number of assignable counters (32
to 64).
Suggested-by: Peter Newman <peternewman@google.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v14: Updated code documentation and changelog.
Fixed up the indentation in resctrl.h.
Changed subject line to fs/resctrl.
v13: Resolved conflicts caused by the recent FS/ARCH code restructure.
The files monitor.c/rdtgroup.c have been split between FS and ARCH directories.
v12: Fixed the struct mbm_cntr_cfg code documentation.
Removed few strange charactors in changelog.
Added the counter range for better understanding.
Moved the struct mbm_cntr_cfg definition to resctrl/internal.h as
suggested by James.
v11: Refined the change log based on Reinette's feedback.
Fixed few style issues.
v10: Patch changed completely to handle the counters at domain level.
https://lore.kernel.org/lkml/CALPaoCj+zWq1vkHVbXYP0znJbe6Ke3PXPWjtri5AFgD9cQDCUg@mail.gmail.com/
Removed Reviewed-by tag.
Did not see the need to add cntr_id in mbm_state structure. Not used in the code.
v9: Added Reviewed-by tag. No other changes.
v8: Minor commit message changes.
v7: Added check mbm_cntr_assignable for allocating bitmap mbm_cntr_map
v6: New patch to add domain level assignment.
---
fs/resctrl/rdtgroup.c | 8 ++++++++
include/linux/resctrl.h | 19 +++++++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index 967e4df62a19..90b52593ef29 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -4084,6 +4084,7 @@ static void rdtgroup_setup_default(void)
static void domain_destroy_mon_state(struct rdt_mon_domain *d)
{
+ kfree(d->cntr_cfg);
bitmap_free(d->rmid_busy_llc);
for (int i = 0; i < QOS_NUM_L3_MBM_EVENTS; i++) {
kfree(d->mbm_states[i]);
@@ -4167,6 +4168,13 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_mon_domain
goto cleanup;
}
+ if (resctrl_is_mbm_enabled() && r->mon.mbm_cntr_assignable) {
+ tsize = sizeof(*d->cntr_cfg);
+ d->cntr_cfg = kcalloc(r->mon.num_mbm_cntrs, tsize, GFP_KERNEL);
+ if (!d->cntr_cfg)
+ goto cleanup;
+ }
+
return 0;
cleanup:
bitmap_free(d->rmid_busy_llc);
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index f078ef24a8ad..468a4ebabc64 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -156,6 +156,22 @@ struct rdt_ctrl_domain {
u32 *mbps_val;
};
+/**
+ * struct mbm_cntr_cfg - Assignable counter configuration.
+ * @evtid: MBM event to which the counter is assigned. Only valid
+ * if @rdtgroup is not NULL.
+ * @evt_cfg: Event configuration created using the READS_TO_LOCAL_MEM,
+ * READS_TO_REMOTE_MEM, etc. bits that represent the memory
+ * transactions being counted.
+ * @rdtgrp: resctrl group assigned to the counter. NULL if the
+ * counter is free.
+ */
+struct mbm_cntr_cfg {
+ enum resctrl_event_id evtid;
+ u32 evt_cfg;
+ struct rdtgroup *rdtgrp;
+};
+
/**
* struct rdt_mon_domain - group of CPUs sharing a resctrl monitor resource
* @hdr: common header for different domain types
@@ -168,6 +184,8 @@ struct rdt_ctrl_domain {
* @cqm_limbo: worker to periodically read CQM h/w counters
* @mbm_work_cpu: worker CPU for MBM h/w counters
* @cqm_work_cpu: worker CPU for CQM h/w counters
+ * @cntr_cfg: array of assignable counters' configuration (indexed
+ * by counter ID)
*/
struct rdt_mon_domain {
struct rdt_domain_hdr hdr;
@@ -178,6 +196,7 @@ struct rdt_mon_domain {
struct delayed_work cqm_limbo;
int mbm_work_cpu;
int cqm_work_cpu;
+ struct mbm_cntr_cfg *cntr_cfg;
};
/**
--
2.34.1Hi Babu,
On 6/13/25 2:04 PM, Babu Moger wrote:
> The "mbm_event" mode allows users to assign a hardware counter ID to an
"hardware counter ID" -> "hardware counter" (I'll stop pointing these out)
> RMID, event pair and monitor bandwidth usage as long as it is assigned.
> The hardware continues to track the assigned counter until it is
> explicitly unassigned by the user. Counters are assigned/unassigned at
> monitoring domain level.
>
> Manage a monitoring domain's hardware counters using a per monitoring
> domain array of struct mbm_cntr_cfg that is indexed by the hardware
> counter ID. A hardware counter's configuration contains the MBM event
> ID and points to the monitoring group that it is assigned to, with a
> NULL pointer meaning that the hardware counter is available for assignment.
>
> There is no direct way to determine which hardware counters are assigned
> to a particular monitoring group. Check every entry of every hardware
> counter configuration array in every monitoring domain to query which
> MBM events of a monitoring group is tracked by hardware. Such queries are
> acceptable because of a very small number of assignable counters (32
> to 64).
>
> Suggested-by: Peter Newman <peternewman@google.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
...
> ---
> fs/resctrl/rdtgroup.c | 8 ++++++++
> include/linux/resctrl.h | 19 +++++++++++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 967e4df62a19..90b52593ef29 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -4084,6 +4084,7 @@ static void rdtgroup_setup_default(void)
>
> static void domain_destroy_mon_state(struct rdt_mon_domain *d)
> {
> + kfree(d->cntr_cfg);
> bitmap_free(d->rmid_busy_llc);
> for (int i = 0; i < QOS_NUM_L3_MBM_EVENTS; i++) {
> kfree(d->mbm_states[i]);
> @@ -4167,6 +4168,13 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_mon_domain
> goto cleanup;
> }
>
> + if (resctrl_is_mbm_enabled() && r->mon.mbm_cntr_assignable) {
> + tsize = sizeof(*d->cntr_cfg);
> + d->cntr_cfg = kcalloc(r->mon.num_mbm_cntrs, tsize, GFP_KERNEL);
> + if (!d->cntr_cfg)
> + goto cleanup;
> + }
> +
Please see my earlier comment https://lore.kernel.org/lkml/b761e6ec-a874-4d06-8437-a3a717a91abb@intel.com/
Before this addition the "cleanup" goto label can only be called when
(a) idx is guaranteed to be initialized and (b) d->mbm_states[idx] == NULL.
Using that goto label in snippet above cannot guarantee either.
> return 0;
> cleanup:
> bitmap_free(d->rmid_busy_llc);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index f078ef24a8ad..468a4ebabc64 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -156,6 +156,22 @@ struct rdt_ctrl_domain {
> u32 *mbps_val;
> };
>
> +/**
> + * struct mbm_cntr_cfg - Assignable counter configuration.
> + * @evtid: MBM event to which the counter is assigned. Only valid
> + * if @rdtgroup is not NULL.
> + * @evt_cfg: Event configuration created using the READS_TO_LOCAL_MEM,
> + * READS_TO_REMOTE_MEM, etc. bits that represent the memory
> + * transactions being counted.
> + * @rdtgrp: resctrl group assigned to the counter. NULL if the
> + * counter is free.
> + */
> +struct mbm_cntr_cfg {
> + enum resctrl_event_id evtid;
> + u32 evt_cfg;
It is not clear to me why the event configuration needs to be duplicated
between mbm_cntr_cfg::evt_cfg and mon_evt::evt_cfg (done in patch #16).
I think there should be only one "source of truth" and mon_evt::evt_cfg
seems most appropriate since then it can be shared with BMEC.
It also seems unnecessary to make so many copies of the event configuration
if it can just be determined from the event ID.
Looking ahead at how this is used, for example in event_filter_write()
introduced in patch #25:
ret = resctrl_process_configs(buf, &evt_cfg);
if (!ret && mevt->evt_cfg != evt_cfg) {
mevt->evt_cfg = evt_cfg;
resctrl_assign_cntr_allrdtgrp(r, mevt);
}
After user provides new event configuration the mon_evt::evt_cfg is
updated. Since there is this initial check to determine if counters need
to be updated I think it is unnecessary to have a second copy of mbm_cntr_cfg::evt_cfg
that needs to be checked again. The functions called by resctrl_assign_cntr_allrdtgrp(r, mevt)
should just update the counters without any additional comparison.
For example, rdtgroup_assign_cntr() can be simplified to:
rdtgroup_assign_cntr() {
...
list_for_each_entry(d, &r->mon_domains, hdr.list) {
cntr_id = mbm_cntr_get(r, d, rdtgrp, mevt->evtid);
if (cntr_id >= 0)
resctrl_arch_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
rdtgrp->closid, cntr_id, true);
}
}
Reinette
Hi Reinette,
On 6/24/2025 6:31 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 6/13/25 2:04 PM, Babu Moger wrote:
>> The "mbm_event" mode allows users to assign a hardware counter ID to an
>
> "hardware counter ID" -> "hardware counter" (I'll stop pointing these out)
Sure.
>
>> RMID, event pair and monitor bandwidth usage as long as it is assigned.
>> The hardware continues to track the assigned counter until it is
>> explicitly unassigned by the user. Counters are assigned/unassigned at
>> monitoring domain level.
>>
>> Manage a monitoring domain's hardware counters using a per monitoring
>> domain array of struct mbm_cntr_cfg that is indexed by the hardware
>> counter ID. A hardware counter's configuration contains the MBM event
>> ID and points to the monitoring group that it is assigned to, with a
>> NULL pointer meaning that the hardware counter is available for assignment.
>>
>> There is no direct way to determine which hardware counters are assigned
>> to a particular monitoring group. Check every entry of every hardware
>> counter configuration array in every monitoring domain to query which
>> MBM events of a monitoring group is tracked by hardware. Such queries are
>> acceptable because of a very small number of assignable counters (32
>> to 64).
>>
>> Suggested-by: Peter Newman <peternewman@google.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> ...
>
>> ---
>> fs/resctrl/rdtgroup.c | 8 ++++++++
>> include/linux/resctrl.h | 19 +++++++++++++++++++
>> 2 files changed, 27 insertions(+)
>>
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 967e4df62a19..90b52593ef29 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -4084,6 +4084,7 @@ static void rdtgroup_setup_default(void)
>>
>> static void domain_destroy_mon_state(struct rdt_mon_domain *d)
>> {
>> + kfree(d->cntr_cfg);
>> bitmap_free(d->rmid_busy_llc);
>> for (int i = 0; i < QOS_NUM_L3_MBM_EVENTS; i++) {
>> kfree(d->mbm_states[i]);
>> @@ -4167,6 +4168,13 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_mon_domain
>> goto cleanup;
>> }
>>
>> + if (resctrl_is_mbm_enabled() && r->mon.mbm_cntr_assignable) {
>> + tsize = sizeof(*d->cntr_cfg);
>> + d->cntr_cfg = kcalloc(r->mon.num_mbm_cntrs, tsize, GFP_KERNEL);
>> + if (!d->cntr_cfg)
>> + goto cleanup;
>> + }
>> +
>
> Please see my earlier comment https://lore.kernel.org/lkml/b761e6ec-a874-4d06-8437-a3a717a91abb@intel.com/
> Before this addition the "cleanup" goto label can only be called when
> (a) idx is guaranteed to be initialized and (b) d->mbm_states[idx] == NULL.
> Using that goto label in snippet above cannot guarantee either.
Yes. Tony took care of this.
cleanup:
bitmap_free(d->rmid_busy_llc);
for_each_mbm_idx(idx) {
kfree(d->mbm_states[idx]);
d->mbm_states[idx] = NULL;
}
return -ENOMEM;
}
>
>> return 0;
>> cleanup:
>> bitmap_free(d->rmid_busy_llc);
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index f078ef24a8ad..468a4ebabc64 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -156,6 +156,22 @@ struct rdt_ctrl_domain {
>> u32 *mbps_val;
>> };
>>
>> +/**
>> + * struct mbm_cntr_cfg - Assignable counter configuration.
>> + * @evtid: MBM event to which the counter is assigned. Only valid
>> + * if @rdtgroup is not NULL.
>> + * @evt_cfg: Event configuration created using the READS_TO_LOCAL_MEM,
>> + * READS_TO_REMOTE_MEM, etc. bits that represent the memory
>> + * transactions being counted.
>> + * @rdtgrp: resctrl group assigned to the counter. NULL if the
>> + * counter is free.
>> + */
>> +struct mbm_cntr_cfg {
>> + enum resctrl_event_id evtid;
>> + u32 evt_cfg;
>
> It is not clear to me why the event configuration needs to be duplicated
> between mbm_cntr_cfg::evt_cfg and mon_evt::evt_cfg (done in patch #16).
> I think there should be only one "source of truth" and mon_evt::evt_cfg
> seems most appropriate since then it can be shared with BMEC.
>
> It also seems unnecessary to make so many copies of the event configuration
> if it can just be determined from the event ID.
>
> Looking ahead at how this is used, for example in event_filter_write()
> introduced in patch #25:
> ret = resctrl_process_configs(buf, &evt_cfg);
> if (!ret && mevt->evt_cfg != evt_cfg) {
> mevt->evt_cfg = evt_cfg;
> resctrl_assign_cntr_allrdtgrp(r, mevt);
> }
>
> After user provides new event configuration the mon_evt::evt_cfg is
> updated. Since there is this initial check to determine if counters need
> to be updated I think it is unnecessary to have a second copy of mbm_cntr_cfg::evt_cfg
> that needs to be checked again. The functions called by resctrl_assign_cntr_allrdtgrp(r, mevt)
> should just update the counters without any additional comparison.
>
> For example, rdtgroup_assign_cntr() can be simplified to:
> rdtgroup_assign_cntr() {
> ...
> list_for_each_entry(d, &r->mon_domains, hdr.list) {
> cntr_id = mbm_cntr_get(r, d, rdtgrp, mevt->evtid);
> if (cntr_id >= 0)
> resctrl_arch_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
> rdtgrp->closid, cntr_id, true);
> }
> }
>
>
Actually, this interaction works as intended.
It serves as an optimization for cases where the user repeatedly tries
to assign the same event to a group. Since we have no way of knowing
whether the event is up-to-date, this mechanism helps us avoid
unnecessary MSR writes.
For example:
mbm_L3_assignments_write() → resctrl_assign_cntr_event() →
resctrl_alloc_config_cntr() → resctrl_config_cntr() →
resctrl_arch_config_cntr()
resctrl_alloc_config_cntr()
{
..
/*
* Skip reconfiguration if the event setup is current; otherwise,
* update and apply the new configuration to the domain.
*/
if (mevt->evt_cfg != d->cntr_cfg[cntr_id].evt_cfg) {
d->cntr_cfg[cntr_id].evt_cfg = mevt->evt_cfg;
resctrl_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
rdtgrp->closid, cntr_id, true);
}
}
Thanks
Babu
Hi Babu,
On 6/25/25 6:31 PM, Moger, Babu wrote:
> On 6/24/2025 6:31 PM, Reinette Chatre wrote:
>> On 6/13/25 2:04 PM, Babu Moger wrote:
>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index f078ef24a8ad..468a4ebabc64 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -156,6 +156,22 @@ struct rdt_ctrl_domain {
>>> u32 *mbps_val;
>>> };
>>> +/**
>>> + * struct mbm_cntr_cfg - Assignable counter configuration.
>>> + * @evtid: MBM event to which the counter is assigned. Only valid
>>> + * if @rdtgroup is not NULL.
>>> + * @evt_cfg: Event configuration created using the READS_TO_LOCAL_MEM,
>>> + * READS_TO_REMOTE_MEM, etc. bits that represent the memory
>>> + * transactions being counted.
>>> + * @rdtgrp: resctrl group assigned to the counter. NULL if the
>>> + * counter is free.
>>> + */
>>> +struct mbm_cntr_cfg {
>>> + enum resctrl_event_id evtid;
>>> + u32 evt_cfg;
>>
>> It is not clear to me why the event configuration needs to be duplicated
>> between mbm_cntr_cfg::evt_cfg and mon_evt::evt_cfg (done in patch #16).
>> I think there should be only one "source of truth" and mon_evt::evt_cfg
>> seems most appropriate since then it can be shared with BMEC.
>>
>> It also seems unnecessary to make so many copies of the event configuration
>> if it can just be determined from the event ID.
>>
>> Looking ahead at how this is used, for example in event_filter_write()
>> introduced in patch #25:
>> ret = resctrl_process_configs(buf, &evt_cfg);
>> if (!ret && mevt->evt_cfg != evt_cfg) {
>> mevt->evt_cfg = evt_cfg;
>> resctrl_assign_cntr_allrdtgrp(r, mevt);
>> }
>>
>> After user provides new event configuration the mon_evt::evt_cfg is
>> updated. Since there is this initial check to determine if counters need
>> to be updated I think it is unnecessary to have a second copy of mbm_cntr_cfg::evt_cfg
>> that needs to be checked again. The functions called by resctrl_assign_cntr_allrdtgrp(r, mevt)
>> should just update the counters without any additional comparison.
>>
>> For example, rdtgroup_assign_cntr() can be simplified to:
>> rdtgroup_assign_cntr() {
>> ...
>> list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> cntr_id = mbm_cntr_get(r, d, rdtgrp, mevt->evtid);
>> if (cntr_id >= 0)
>> resctrl_arch_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
>> rdtgrp->closid, cntr_id, true);
>> }
>> }
>>
>>
>
> Actually, this interaction works as intended.
>
> It serves as an optimization for cases where the user repeatedly tries to assign the same event to a group. Since we have no way of knowing whether the event is up-to-date, this mechanism helps us avoid unnecessary MSR writes.
>
> For example:
> mbm_L3_assignments_write() → resctrl_assign_cntr_event() → resctrl_alloc_config_cntr() → resctrl_config_cntr() → resctrl_arch_config_cntr()
>
>
> resctrl_alloc_config_cntr()
>
> {
> ..
>
> /*
> * Skip reconfiguration if the event setup is current; otherwise,
> * update and apply the new configuration to the domain.
> */
> if (mevt->evt_cfg != d->cntr_cfg[cntr_id].evt_cfg) {
> d->cntr_cfg[cntr_id].evt_cfg = mevt->evt_cfg;
> resctrl_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
> rdtgrp->closid, cntr_id, true);
> }
> }
This ties in with the feedback to patch #18 where this snippet is
introduced. Please see
https://lore.kernel.org/lkml/77ce3646-2213-4987-a438-a69f6d7c6cfd@intel.com/
It is not clear to me that this reconfiguration should be done, if the
counter is assigned to a group then it should be up to date, no? If there
was any change in configuration after assignment then event_filter_write()
will ensure that all resource groups are updated.
If a user repeatedly assigns the same event to a group then mbm_cntr_get()
will return a valid counter and resctrl_alloc_config_cntr() in above flow
can just return success without doing a reconfigure.
Reinette
Hi Reinette,
On 6/26/25 10:05, Reinette Chatre wrote:
> Hi Babu,
>
> On 6/25/25 6:31 PM, Moger, Babu wrote:
>> On 6/24/2025 6:31 PM, Reinette Chatre wrote:
>>> On 6/13/25 2:04 PM, Babu Moger wrote:
>
>>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>>> index f078ef24a8ad..468a4ebabc64 100644
>>>> --- a/include/linux/resctrl.h
>>>> +++ b/include/linux/resctrl.h
>>>> @@ -156,6 +156,22 @@ struct rdt_ctrl_domain {
>>>> u32 *mbps_val;
>>>> };
>>>> +/**
>>>> + * struct mbm_cntr_cfg - Assignable counter configuration.
>>>> + * @evtid: MBM event to which the counter is assigned. Only valid
>>>> + * if @rdtgroup is not NULL.
>>>> + * @evt_cfg: Event configuration created using the READS_TO_LOCAL_MEM,
>>>> + * READS_TO_REMOTE_MEM, etc. bits that represent the memory
>>>> + * transactions being counted.
>>>> + * @rdtgrp: resctrl group assigned to the counter. NULL if the
>>>> + * counter is free.
>>>> + */
>>>> +struct mbm_cntr_cfg {
>>>> + enum resctrl_event_id evtid;
>>>> + u32 evt_cfg;
>>>
>>> It is not clear to me why the event configuration needs to be duplicated
>>> between mbm_cntr_cfg::evt_cfg and mon_evt::evt_cfg (done in patch #16).
>>> I think there should be only one "source of truth" and mon_evt::evt_cfg
>>> seems most appropriate since then it can be shared with BMEC.
>>>
>>> It also seems unnecessary to make so many copies of the event configuration
>>> if it can just be determined from the event ID.
>>>
>>> Looking ahead at how this is used, for example in event_filter_write()
>>> introduced in patch #25:
>>> ret = resctrl_process_configs(buf, &evt_cfg);
>>> if (!ret && mevt->evt_cfg != evt_cfg) {
>>> mevt->evt_cfg = evt_cfg;
>>> resctrl_assign_cntr_allrdtgrp(r, mevt);
>>> }
>>>
>>> After user provides new event configuration the mon_evt::evt_cfg is
>>> updated. Since there is this initial check to determine if counters need
>>> to be updated I think it is unnecessary to have a second copy of mbm_cntr_cfg::evt_cfg
>>> that needs to be checked again. The functions called by resctrl_assign_cntr_allrdtgrp(r, mevt)
>>> should just update the counters without any additional comparison.
>>>
>>> For example, rdtgroup_assign_cntr() can be simplified to:
>>> rdtgroup_assign_cntr() {
>>> ...
>>> list_for_each_entry(d, &r->mon_domains, hdr.list) {
>>> cntr_id = mbm_cntr_get(r, d, rdtgrp, mevt->evtid);
>>> if (cntr_id >= 0)
>>> resctrl_arch_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
>>> rdtgrp->closid, cntr_id, true);
>>> }
>>> }
>>>
>>>
>>
>> Actually, this interaction works as intended.
>>
>> It serves as an optimization for cases where the user repeatedly tries to assign the same event to a group. Since we have no way of knowing whether the event is up-to-date, this mechanism helps us avoid unnecessary MSR writes.
>>
>> For example:
>> mbm_L3_assignments_write() → resctrl_assign_cntr_event() → resctrl_alloc_config_cntr() → resctrl_config_cntr() → resctrl_arch_config_cntr()
>>
>>
>> resctrl_alloc_config_cntr()
>>
>> {
>> ..
>>
>> /*
>> * Skip reconfiguration if the event setup is current; otherwise,
>> * update and apply the new configuration to the domain.
>> */
>> if (mevt->evt_cfg != d->cntr_cfg[cntr_id].evt_cfg) {
>> d->cntr_cfg[cntr_id].evt_cfg = mevt->evt_cfg;
>> resctrl_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
>> rdtgrp->closid, cntr_id, true);
>> }
>> }
>
> This ties in with the feedback to patch #18 where this snippet is
> introduced. Please see
> https://lore.kernel.org/lkml/77ce3646-2213-4987-a438-a69f6d7c6cfd@intel.com/
>
> It is not clear to me that this reconfiguration should be done, if the
> counter is assigned to a group then it should be up to date, no? If there
> was any change in configuration after assignment then event_filter_write()
> will ensure that all resource groups are updated.
Yes. That is the good point. We can do that. I think we started this code
before we introduced event_filter_write().
>
> If a user repeatedly assigns the same event to a group then mbm_cntr_get()
> will return a valid counter and resctrl_alloc_config_cntr() in above flow
> can just return success without doing a reconfigure.
Sure. We can do that. Will remove evt_cfg from struct mbm_cntr_cfg.
That for pointing this out.
--
Thanks
Babu Moger
© 2016 - 2025 Red Hat, Inc.