Hi Reinette,
On 5/22/2025 4:51 PM, Reinette Chatre wrote:
Hi Babu,
On 5/15/25 3:51 PM, Babu Moger wrote:
The ABMC feature provides an option to the user to assign a hardware
counter to an RMID, event pair and monitor the bandwidth as long as it
is assigned. The assigned RMID will be tracked by the hardware until the
user unassigns it manually.
(please review this often repeated snippet to match new design)
Sure.
Implement an architecture-specific handler to assign and unassign the
counter. Configure counters by writing to the L3_QOS_ABMC_CFG MSR,
specifying the counter ID, bandwidth source (RMID), and event
configuration.
...
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index ff4b2abfa044..e31084f7babd 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -448,3 +448,40 @@ inline bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r)
{
return resctrl_to_arch_res(r)->mbm_cntr_assign_enabled;
}
+
+static void resctrl_abmc_config_one_amd(void *info)
+{
+ union l3_qos_abmc_cfg *abmc_cfg = info;
+
+ wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg->full);
+}
+
+/*
+ * Send an IPI to the domain to assign the counter to RMID, event pair.
+ */
+void resctrl_arch_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 rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+ union l3_qos_abmc_cfg abmc_cfg = { 0 };
+ struct arch_mbm_state *am;
+
+ abmc_cfg.split.cfg_en = 1;
+ abmc_cfg.split.cntr_en = assign ? 1 : 0;
+ abmc_cfg.split.cntr_id = cntr_id;
+ abmc_cfg.split.bw_src = rmid;
+ abmc_cfg.split.bw_type = evt_cfg;
Is evt_cfg really needed to be programmed when unassigning a counter? Looking ahead at
patch #14 resctrl_free_config_cntr() needs to go through extra list walk to get this data
but why would hardware need an accurate event configuration to *unassign* a counter?
evt_cfg is not required during unassign. I can remove it.
It seems unnecessary to provide both the event ID *and* the configuration.
resctrl_arch_config_cntr() could drop the "evt_cfg" parameter and instead there
can be a new resctrl utility that architecture can use to query the event's configuration.
Similar to resctrl_is_mon_event_enabled() introduced in
https://lore.kernel.org/lkml/20250521225049.132551-3-tony.luck@xxxxxxxxx/ that exposes an
event property.
Sounds good.
I can add a new function resctrl_get_mon_event_config(evtid) and call it
only during the "assign". It will be called inside
resctrl_arch_config_cntr().
It looks to me as though there are a couple of changes in the telemetry work
that would benefit this work. https://lore.kernel.org/lkml/20250521225049.132551-2-tony.luck@xxxxxxxxx/
switches the monitor events to be maintained in an array indexed by event ID, eliminating the
need for searching the evt_list that this work does in a couple of places. Also note the handy
new for_each_mbm_event() helper (https://lore.kernel.org/lkml/20250521225049.132551-5-tony.luck@xxxxxxxxx/).
Sure. Looking at it now.
+
+ smp_call_function_any(&d->hdr.cpu_mask, resctrl_abmc_config_one_amd, &abmc_cfg, 1);
+
+ /*
+ * The hardware counter is reset (because cfg_en == 1) so there is no
+ * need to record initial non-zero counts.
+ */
+ if (assign) {
+ am = get_arch_mbm_state(hw_dom, rmid, evtid);
+ if (am)
+ memset(am, 0, sizeof(*am));
+ }
+}
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index d77981d1fcb9..59a4fe60ab46 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -559,6 +559,23 @@ void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *
*/
void resctrl_arch_reset_all_ctrls(struct rdt_resource *r);
+/**
+ * resctrl_arch_config_cntr() - Configure the counter id to RMID, event
+ * pair on the domain.
The sentence seem strange, should "Configure the counter" perhaps be
"Assign the counter"? Or if the naming requires "configure" ...
"Configure the counter with its new RMID and event details."? Please feel
free to improve.
Last one looks good.
+ * @r: Resource structure.
+ * @d: Domain that the counter id to be configured.
I am unable to parse description of @d.
The domain in which the counter ID is to be configured.
+ * @evtid: Event type to configure.
+ * @rmid: RMID to configure.
+ * @closid: CLOSID to configure.
+ * @cntr_id: Counter ID to configure.
All four parameters descriptions end with "to configure" ... but it is actually only
the counter that is configured while the rest is the data that the counter is configured with, no?
Will remove "to configure" from all the other fields except the cntr_id.
+ * @evt_cfg: MBM event configuration value representing reads,
+ * writes etc.
Needs definition about what the contents of @evt_cfg means. This is the API ...it
cannot be vague like "reads, write, etc." but should be specific about which bit means
what.
Copying your comment on other patch
https://lore.kernel.org/lkml/14ca1527-ee25-448d-949b-ed8df546c916@xxxxxxxxx/
@evt_cfg: Event configuration created using the READS_TO_LOCAL_MEM,
READS_TO_REMOTE_MEM, etc. bits that represent the memory transactions
being counted.
+ * @assign: Assign or unassign.
"True to assign the counter, false to unassign the counter."
Sure.
Needs some context here about what architecture can expect on how this function will
be called. For example, "Can be called from any CPU."
Sure.
+ */
+void resctrl_arch_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);
+
extern unsigned int resctrl_rmid_realloc_threshold;
extern unsigned int resctrl_rmid_realloc_limit;
Reinette
Thanks
Babu
