Re: [PATCH v10 11/24] x86/resctrl: Remove MSR reading of event configuration value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Reinette,

On 12/19/2024 4:12 PM, Reinette Chatre wrote:
Hi Babu,

On 12/12/24 12:15 PM, Babu Moger wrote:

@@ -1604,33 +1645,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
  	}
  }
-static void mon_event_config_read(void *info)
-{
-	struct mon_config_info *mon_info = info;
-	unsigned int index;
-	u64 msrval;
-
-	index = mon_event_config_index_get(mon_info->evtid);
-	if (index == INVALID_CONFIG_INDEX) {
-		pr_warn_once("Invalid event id %d\n", mon_info->evtid);
-		return;
-	}
-	rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
-
-	/* Report only the valid event configuration bits */
-	mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
-}
-
-static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
-{
-	smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
-}
-
  static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
  {
-	struct mon_config_info mon_info;
  	struct rdt_mon_domain *dom;
  	bool sep = false;
+	u32 val;

Could this variable name be more descriptive? For example, mon_config, or config_val as
used in mbm_config_write_domain()?

Will change it to config_val.


...

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index f11d6fdfd977..c8ab3d7a0dab 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -118,6 +118,18 @@ struct rdt_mon_domain {
  	int				cqm_work_cpu;
  };
+/**
+ * struct mon_config_info - Monitoring event configuratiin details

configuratiin -> configuration

Sure.


... but actually, the motivation for moving this struct here was
to make it available for an arch to interpret the data passed
via resctrl_arch_mon_event_config_set(). This patch passes data
in this struct but a later patch modifies
resctrl_arch_mon_event_config_set() to not use struct anymore ...
and then leaves struct mon_config_info here.

Even so, considering Boris's preference this is no longer needed.

ok. I will move the "struct mon_config_info" definition where it is used(rdtgroup.c).


+ * @d:			Domain for the event
+ * @evtid:		Event type
+ * @mon_config:		Event configuration value
+ */
+struct mon_config_info {
+	struct rdt_mon_domain *d;
+	enum resctrl_event_id evtid;
+	u32 mon_config;
+};
+
  /**
   * struct resctrl_cache - Cache allocation related data
   * @cbm_len:		Length of the cache bit mask
@@ -352,6 +364,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
   */
  void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
+void resctrl_arch_mon_event_config_set(void *info);
+u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
+				      enum resctrl_event_id eventid);
+

Please move to internal header file instead and consider this for
all changes to include/linux/resctrl.h

Sure. Thanks

Babu





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux