Hi Reinette,
Lots of things in here.
On 2/6/2025 12:42 PM, Reinette Chatre wrote:
Hi Babu,
On 2/6/25 8:15 AM, Moger, Babu wrote:
Hi Reinette,
On 2/5/2025 4:49 PM, Reinette Chatre wrote:
Hi Babu,
On 1/22/25 12:20 PM, Babu Moger wrote:
Add the functionality to enable/disable AMD ABMC feature.
AMD ABMC feature is enabled by setting enabled bit(0) in MSR
L3_QOS_EXT_CFG. When the state of ABMC is changed, the MSR needs
to be updated on all the logical processors in the QOS Domain.
Hardware counters will reset when ABMC state is changed.
I find that the state management in this series is organized better
and easier to understand. I do think that it can be simplified more
and a hint to this is that it is mentioned here but not done in the
code introduced here but instead required from the caller. It seems
simpler to me that the architectural state can just be reset at the
same time as enable/disable of ABMC?
Right now, it is done from mbm_cntr_reset(). It does both arch and non-arch state reset for all the RMIDs in all the domains. It is called in two places.
1 rdtgroup.c resctrl_mbm_assign_mode_write -> mbm_cntr_reset();
Please see my response to this usage in the related patch:
https://lore.kernel.org/lkml/b60b4f72-6245-46db-a126-428fb13b6310@xxxxxxxxx/
In summary, I find mbm_cntr_reset() ended up being a catch-all for random
cleanup and creates confusion with the other mbm_cntr_*() calls.
Yes. It should work. Will respond that comment later.
2 rdtgroup.c rdt_kill_sb()-> mbm_cntr_reset();
Please see my response to this usage in the related patch:
https://lore.kernel.org/lkml/8d04f824-d1cc-461c-9c57-0f26c6aa96e0@xxxxxxxxx/
In summary, it does not solve the problem it originally set out to solve
and it can be eliminated.
Yes. Should be fine. mbm_cntr_reset() can be completely removed.
Will respond that comment later.
I will have to introduce another function to reset RMIDs in all the domains. Also, make sure it is called from both these places.
list_for_each_entry(dom, &r->mon_domains, hdr.list)
resctrl_arch_reset_rmid_all(r, dom);
I do not see need for new functions, except the one I mention in
https://lore.kernel.org/lkml/b60b4f72-6245-46db-a126-428fb13b6310@xxxxxxxxx/
that suggests a new helper for reset of architectural state that does not
exist and ends up being open coded in two places in this series.
With only one place (resctrl_mbm_assign_mode_write()) remaining that needs
all state reset I think it will be easier to understand if the state reset
is open coded within it, replacing mbm_cntr_reset() with:
list_for_each_entry(dom, &r->mon_domains, hdr.list) {
mbm_cntr_free_all()
resctrl_reset_rmid_all() // Just for architectural state
}
You meant "Just for non-architectural state" ?
I would not insist on reset of architectural state within the
architectural helper. I find that it is best for architecture to
maintain its state but I also see there are many precedent for
resctrl explicitly managing the state.
I feel current code is much more cleaner. What do you think?
It is better that previous versions, yes.
The ABMC feature details are documented in APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
Monitoring (ABMC).
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
...
...
+ */
+static void _resctrl_abmc_enable(struct rdt_resource *r, bool enable)
+{
+ struct rdt_mon_domain *d;
+
+ list_for_each_entry(d, &r->mon_domains, hdr.list)
+ on_each_cpu_mask(&d->hdr.cpu_mask,
+ resctrl_abmc_set_one_amd, &enable, 1);
+}
+
+int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
+{
+ struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
+
+ if (r->mon.mbm_cntr_assignable &&
+ hw_res->mbm_cntr_assign_enabled != enable) {
+ _resctrl_abmc_enable(r, enable);
+ hw_res->mbm_cntr_assign_enabled = enable;
Added benefit of resetting architectural state within this if statement
(perhaps simpler to be done within _resctrl_abmc_enable()) is that it will
not be done unnecessarily if ABMC is already in requested state.
It will be
list_for_each_entry(dom, &r->mon_domains, hdr.list)
resctrl_arch_reset_rmid_all(r, dom);
I am not sure if you are actually planning a new loop here ... as
I suggested above this can be added to _resctrl_abmc_enable() where
there is already a loop over all monitor domains and all that is
needed is to add a call to resctrl_arch_reset_rmid_all(r, dom).
Sure.
Even so, as I mentioned above, if after fixing automatic counter
unassignment you still find that resetting architectural and
non-architectural state together then we can go with that to match
the other flows (eg. mbm_config_write_domain()).
Sure.
Thanks
Babu
