[PATCH v5 03/20] x86/resctrl: Consolidate monitoring related data from rdt_resource

Babu Moger posted 20 patches 1 year, 7 months ago
There is a newer version of this series
[PATCH v5 03/20] x86/resctrl: Consolidate monitoring related data from rdt_resource
Posted by Babu Moger 1 year, 7 months ago
The cache allocation and memory bandwidth allocation feature properties
are consolidated into cache and membw structures respectively. In
preparation for more monitoring properties that will clobber the existing
resource struct more, re-organize the monitoring specific properties into
separate structure.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v5: Commit message update.
    Also changes related to data structure updates does to SNC support.

v4: New patch.
---
 arch/x86/kernel/cpu/resctrl/core.c     |  2 +-
 arch/x86/kernel/cpu/resctrl/monitor.c  | 18 +++++++++---------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |  8 ++++----
 include/linux/resctrl.h                | 13 +++++++++++--
 4 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 9417d8bb7029..4a2d0955ccdc 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -617,7 +617,7 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
 
 	arch_mon_domain_online(r, d);
 
-	if (arch_domain_mbm_alloc(r->num_rmid, hw_dom)) {
+	if (arch_domain_mbm_alloc(r->mon.num_rmid, hw_dom)) {
 		mon_domain_free(hw_dom);
 		return;
 	}
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 851b561850e0..795fe91a8feb 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -222,7 +222,7 @@ static int logical_rmid_to_physical_rmid(int cpu, int lrmid)
 	if (snc_nodes_per_l3_cache == 1)
 		return lrmid;
 
-	return lrmid + (cpu_to_node(cpu) % snc_nodes_per_l3_cache) * r->num_rmid;
+	return lrmid + (cpu_to_node(cpu) % snc_nodes_per_l3_cache) * r->mon.num_rmid;
 }
 
 static int __rmid_read_phys(u32 prmid, enum resctrl_event_id eventid, u64 *val)
@@ -297,11 +297,11 @@ void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *
 
 	if (is_mbm_total_enabled())
 		memset(hw_dom->arch_mbm_total, 0,
-		       sizeof(*hw_dom->arch_mbm_total) * r->num_rmid);
+		       sizeof(*hw_dom->arch_mbm_total) * r->mon.num_rmid);
 
 	if (is_mbm_local_enabled())
 		memset(hw_dom->arch_mbm_local, 0,
-		       sizeof(*hw_dom->arch_mbm_local) * r->num_rmid);
+		       sizeof(*hw_dom->arch_mbm_local) * r->mon.num_rmid);
 }
 
 static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
@@ -1083,14 +1083,14 @@ static struct mon_evt mbm_local_event = {
  */
 static void l3_mon_evt_init(struct rdt_resource *r)
 {
-	INIT_LIST_HEAD(&r->evt_list);
+	INIT_LIST_HEAD(&r->mon.evt_list);
 
 	if (is_llc_occupancy_enabled())
-		list_add_tail(&llc_occupancy_event.list, &r->evt_list);
+		list_add_tail(&llc_occupancy_event.list, &r->mon.evt_list);
 	if (is_mbm_total_enabled())
-		list_add_tail(&mbm_total_event.list, &r->evt_list);
+		list_add_tail(&mbm_total_event.list, &r->mon.evt_list);
 	if (is_mbm_local_enabled())
-		list_add_tail(&mbm_local_event.list, &r->evt_list);
+		list_add_tail(&mbm_local_event.list, &r->mon.evt_list);
 }
 
 /*
@@ -1186,7 +1186,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 
 	resctrl_rmid_realloc_limit = boot_cpu_data.x86_cache_size * 1024;
 	hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale / snc_nodes_per_l3_cache;
-	r->num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;
+	r->mon.num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;
 	hw_res->mbm_width = MBM_CNTR_WIDTH_BASE;
 
 	if (mbm_offset > 0 && mbm_offset <= MBM_CNTR_WIDTH_OFFSET_MAX)
@@ -1201,7 +1201,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 	 *
 	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
 	 */
-	threshold = resctrl_rmid_realloc_limit / r->num_rmid;
+	threshold = resctrl_rmid_realloc_limit / r->mon.num_rmid;
 
 	/*
 	 * Because num_rmid may not be a power of two, round the value
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index d7163b764c62..f9f3b5db1987 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1097,7 +1097,7 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
 {
 	struct rdt_resource *r = of->kn->parent->priv;
 
-	seq_printf(seq, "%d\n", r->num_rmid);
+	seq_printf(seq, "%d\n", r->mon.num_rmid);
 
 	return 0;
 }
@@ -1108,7 +1108,7 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
 	struct rdt_resource *r = of->kn->parent->priv;
 	struct mon_evt *mevt;
 
-	list_for_each_entry(mevt, &r->evt_list, list) {
+	list_for_each_entry(mevt, &r->mon.evt_list, list) {
 		seq_printf(seq, "%s\n", mevt->name);
 		if (mevt->configurable)
 			seq_printf(seq, "%s_config\n", mevt->name);
@@ -3057,13 +3057,13 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
 	struct mon_evt *mevt;
 	int ret;
 
-	if (WARN_ON(list_empty(&r->evt_list)))
+	if (WARN_ON(list_empty(&r->mon.evt_list)))
 		return -EPERM;
 
 	priv.u.rid = r->rid;
 	priv.u.domid = do_sum ? d->ci->id : d->hdr.id;
 	priv.u.sum = do_sum;
-	list_for_each_entry(mevt, &r->evt_list, list) {
+	list_for_each_entry(mevt, &r->mon.evt_list, list) {
 		priv.u.evtid = mevt->evtid;
 		ret = mon_addfile(kn, mevt->name, priv.priv);
 		if (ret)
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index b0875b99e811..e43fc5bb5a3a 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -182,6 +182,16 @@ enum resctrl_scope {
 	RESCTRL_L3_NODE,
 };
 
+/**
+ * struct resctrl_mon - Monitoring related data
+ * @num_rmid:		Number of RMIDs available
+ * @evt_list:		List of monitoring events
+ */
+struct resctrl_mon {
+	int			num_rmid;
+	struct list_head	evt_list;
+};
+
 /**
  * struct rdt_resource - attributes of a resctrl resource
  * @rid:		The index of the resource
@@ -207,11 +217,11 @@ struct rdt_resource {
 	int			rid;
 	bool			alloc_capable;
 	bool			mon_capable;
-	int			num_rmid;
 	enum resctrl_scope	ctrl_scope;
 	enum resctrl_scope	mon_scope;
 	struct resctrl_cache	cache;
 	struct resctrl_membw	membw;
+	struct resctrl_mon	mon;
 	struct list_head	ctrl_domains;
 	struct list_head	mon_domains;
 	char			*name;
@@ -221,7 +231,6 @@ struct rdt_resource {
 	int			(*parse_ctrlval)(struct rdt_parse_data *data,
 						 struct resctrl_schema *s,
 						 struct rdt_ctrl_domain *d);
-	struct list_head	evt_list;
 	unsigned long		fflags;
 	bool			cdp_capable;
 };
-- 
2.34.1
Re: [PATCH v5 03/20] x86/resctrl: Consolidate monitoring related data from rdt_resource
Posted by Reinette Chatre 1 year, 7 months ago
Hi Babu,

On 7/3/24 2:48 PM, Babu Moger wrote:
> The cache allocation and memory bandwidth allocation feature properties
> are consolidated into cache and membw structures respectively. In

Let "In preparation ... " start a new paragraph.

Quoting Documentation/process/maintainer-tip.rst:
	It's also useful to structure the changelog into several paragraphs
	and not lump everything together into a single one. A good structure
	is to explain the context, the problem and the solution in separate
	paragraphs and this order.

> preparation for more monitoring properties that will clobber the existing
> resource struct more, re-organize the monitoring specific properties into
> separate structure.

"re-organize the monitoring specific properties into separate structure" ->
"re-organize the monitoring specific properties to also be in a separate structure."

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index b0875b99e811..e43fc5bb5a3a 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -182,6 +182,16 @@ enum resctrl_scope {
>   	RESCTRL_L3_NODE,
>   };
>   
> +/**
> + * struct resctrl_mon - Monitoring related data
> + * @num_rmid:		Number of RMIDs available
> + * @evt_list:		List of monitoring events
> + */
> +struct resctrl_mon {
> +	int			num_rmid;
> +	struct list_head	evt_list;
> +};
> +
>   /**
>    * struct rdt_resource - attributes of a resctrl resource
>    * @rid:		The index of the resource
> @@ -207,11 +217,11 @@ struct rdt_resource {
>   	int			rid;
>   	bool			alloc_capable;
>   	bool			mon_capable;
> -	int			num_rmid;
>   	enum resctrl_scope	ctrl_scope;
>   	enum resctrl_scope	mon_scope;
>   	struct resctrl_cache	cache;
>   	struct resctrl_membw	membw;
> +	struct resctrl_mon	mon;
>   	struct list_head	ctrl_domains;
>   	struct list_head	mon_domains;
>   	char			*name;
> @@ -221,7 +231,6 @@ struct rdt_resource {
>   	int			(*parse_ctrlval)(struct rdt_parse_data *data,
>   						 struct resctrl_schema *s,
>   						 struct rdt_ctrl_domain *d);
> -	struct list_head	evt_list;
>   	unsigned long		fflags;
>   	bool			cdp_capable;
>   };

struct rdt_resource's kernel-doc still refers to the members
removed in this patch. Its kernel-doc also needs an update for the new
member added.

Reinette
Re: [PATCH v5 03/20] x86/resctrl: Consolidate monitoring related data from rdt_resource
Posted by Moger, Babu 1 year, 6 months ago
Hi Reinette,

On 7/12/24 16:57, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/3/24 2:48 PM, Babu Moger wrote:
>> The cache allocation and memory bandwidth allocation feature properties
>> are consolidated into cache and membw structures respectively. In
> 
> Let "In preparation ... " start a new paragraph.
> 
> Quoting Documentation/process/maintainer-tip.rst:
>     It's also useful to structure the changelog into several paragraphs
>     and not lump everything together into a single one. A good structure
>     is to explain the context, the problem and the solution in separate
>     paragraphs and this order.

Ok. Sure,
> 
>> preparation for more monitoring properties that will clobber the existing
>> resource struct more, re-organize the monitoring specific properties into
>> separate structure.
> 
> "re-organize the monitoring specific properties into separate structure" ->
> "re-organize the monitoring specific properties to also be in a separate
> structure."

Sure.
> 
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index b0875b99e811..e43fc5bb5a3a 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -182,6 +182,16 @@ enum resctrl_scope {
>>       RESCTRL_L3_NODE,
>>   };
>>   +/**
>> + * struct resctrl_mon - Monitoring related data
>> + * @num_rmid:        Number of RMIDs available
>> + * @evt_list:        List of monitoring events
>> + */
>> +struct resctrl_mon {
>> +    int            num_rmid;
>> +    struct list_head    evt_list;
>> +};
>> +
>>   /**
>>    * struct rdt_resource - attributes of a resctrl resource
>>    * @rid:        The index of the resource
>> @@ -207,11 +217,11 @@ struct rdt_resource {
>>       int            rid;
>>       bool            alloc_capable;
>>       bool            mon_capable;
>> -    int            num_rmid;
>>       enum resctrl_scope    ctrl_scope;
>>       enum resctrl_scope    mon_scope;
>>       struct resctrl_cache    cache;
>>       struct resctrl_membw    membw;
>> +    struct resctrl_mon    mon;
>>       struct list_head    ctrl_domains;
>>       struct list_head    mon_domains;
>>       char            *name;
>> @@ -221,7 +231,6 @@ struct rdt_resource {
>>       int            (*parse_ctrlval)(struct rdt_parse_data *data,
>>                            struct resctrl_schema *s,
>>                            struct rdt_ctrl_domain *d);
>> -    struct list_head    evt_list;
>>       unsigned long        fflags;
>>       bool            cdp_capable;
>>   };
> 
> struct rdt_resource's kernel-doc still refers to the members
> removed in this patch. Its kernel-doc also needs an update for the new
> member added.

Yea. My bad. Will correct it.

-- 
Thanks
Babu Moger