When a user reads a monitor file rdtgroup_mondata_show() calls
mon_event_read() to package up all the required details into an rmid_read
structure which is passed across the smp_call*() infrastructure to code
that will read data from hardware and return the value (or error status)
in the rmid_read structure.

Sub-NUMA Cluster (SNC) mode adds files with new semantics. These require
the smp_call-ed code to sum event data from all domains that share an
L3 cache.

Add a pointer to the L3 "cacheinfo" structure to struct rmid_read
for the data collection routines to use to pick the domains to be
summed.

Initialize all on-stack declarations of struct rmid_read:
	rdtgroup_mondata_show()
	mbm_update()
	mkdir_mondata_subdir()
to ensure that garbage values from the stack are not passed down
to other functions.

Remove redundant rr->val = 0; from mon_event_read() and rr.first = false;
from mbm_update() since the rmid_read structure is cleared by compiler.

Reinette suggested that the rmid_read structure has become complex enough
to warrant documentation of each of its fields and provided the kerneldoc
documentation for struct rmid_read.

Co-developed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h    | 19 +++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  3 +--
 arch/x86/kernel/cpu/resctrl/monitor.c     |  3 +--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  2 +-
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 135190e0711c..681b5bdcd2f9 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -144,12 +144,31 @@ union mon_data_bits {
 	} u;
 };
 
+/**
+ * struct rmid_read - Data passed across smp_call*() to read event count.
+ * @rgrp:  Resource group for which the counter is being read. If it is a parent
+ *	   resource group then its event count is summed with the count from all
+ *	   its child resource groups.
+ * @r:	   Resource describing the properties of the event being read.
+ * @d:	   Domain that the counter should be read from. If NULL then sum all
+ *	   domains in @r sharing L3 @ci.id
+ * @evtid: Which monitor event to read.
+ * @first: Initialize MBM counter when true.
+ * @ci:    Cacheinfo for L3. Only set when @d is NULL. Used when summing domains.
+ * @err:   Error encountered when reading counter.
+ * @val:   Returned value of event counter. If @rgrp is a parent resource group,
+ *	   @val includes the sum of event counts from its child resource groups.
+ *	   If @d is NULL, @val includes the sum of all domains in @r sharing @ci.id,
+ *	   (summed across child resource groups if @rgrp is a parent resource group).
+ * @arch_mon_ctx: Hardware monitor allocated for this read request (MPAM only).
+ */
 struct rmid_read {
 	struct rdtgroup		*rgrp;
 	struct rdt_resource	*r;
 	struct rdt_mon_domain	*d;
 	enum resctrl_event_id	evtid;
 	bool			first;
+	struct cacheinfo	*ci;
 	int			err;
 	u64			val;
 	void			*arch_mon_ctx;
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 3b9383612c35..4d76ff31a9e0 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -529,7 +529,6 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
 	rr->evtid = evtid;
 	rr->r = r;
 	rr->d = d;
-	rr->val = 0;
 	rr->first = first;
 	rr->arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, evtid);
 	if (IS_ERR(rr->arch_mon_ctx)) {
@@ -557,12 +556,12 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
 {
 	struct kernfs_open_file *of = m->private;
 	struct rdt_domain_hdr *hdr;
+	struct rmid_read rr = {0};
 	struct rdt_mon_domain *d;
 	u32 resid, evtid, domid;
 	struct rdtgroup *rdtgrp;
 	struct rdt_resource *r;
 	union mon_data_bits md;
-	struct rmid_read rr;
 	int ret = 0;
 
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index ff4e74594a19..ca309c93a56b 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -780,9 +780,8 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
 static void mbm_update(struct rdt_resource *r, struct rdt_mon_domain *d,
 		       u32 closid, u32 rmid)
 {
-	struct rmid_read rr;
+	struct rmid_read rr = {0};
 
-	rr.first = false;
 	rr.r = r;
 	rr.d = d;
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 70d41a8fd788..d0443589cd86 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3029,10 +3029,10 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
 				struct rdt_mon_domain *d,
 				struct rdt_resource *r, struct rdtgroup *prgrp)
 {
+	struct rmid_read rr = {0};
 	union mon_data_bits priv;
 	struct kernfs_node *kn;
 	struct mon_evt *mevt;
-	struct rmid_read rr;
 	char name[32];
 	int ret;
 
-- 
2.45.2

Hi Tony,

On 6/27/24 1:38 PM, Tony Luck wrote:
> When a user reads a monitor file rdtgroup_mondata_show() calls
> mon_event_read() to package up all the required details into an rmid_read
> structure which is passed across the smp_call*() infrastructure to code
> that will read data from hardware and return the value (or error status)
> in the rmid_read structure.
> 
> Sub-NUMA Cluster (SNC) mode adds files with new semantics. These require
> the smp_call-ed code to sum event data from all domains that share an
> L3 cache.
> 
> Add a pointer to the L3 "cacheinfo" structure to struct rmid_read
> for the data collection routines to use to pick the domains to be
> summed.

This patch has evolved to a point asking for a split. Everything described below
would be a good fit for its own patch. I do not know if a single patch like this
one will be acceptable but at minimum I would propose that you motivate the
additional changes as a result of the semantic changes related to this struct.
For example, something like below inserted at this point in changelog:

	The new semantics rely on some struct rmid_read members having
	NULL values to distinguish between the SNC and non-SNC scenarios.
	resctrl can thus no longer rely on this struct not being initialized
	properly.

> 
> Initialize all on-stack declarations of struct rmid_read:
> 	rdtgroup_mondata_show()
> 	mbm_update()
> 	mkdir_mondata_subdir()
> to ensure that garbage values from the stack are not passed down
> to other functions.
> 
> Remove redundant rr->val = 0; from mon_event_read() and rr.first = false;
> from mbm_update() since the rmid_read structure is cleared by compiler.
> 
> Reinette suggested that the rmid_read structure has become complex enough
> to warrant documentation of each of its fields and provided the kerneldoc
> documentation for struct rmid_read.
> 
> Co-developed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---

With patch either split or its changes motivated to be logically connected:

| Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

> For example, something like below inserted at this point in changelog:
>
>	The new semantics rely on some struct rmid_read members having
>	NULL values to distinguish between the SNC and non-SNC scenarios.
>	resctrl can thus no longer rely on this struct not being initialized
>	properly.

Just checking whether "resctrl" should be capitalized at the beginning of
a sentence. Digging through logs there are half-a-dozen places where it
got a "R" and three dozen where it didn't.

-Tony

Hi Tony,

On 6/28/24 2:35 PM, Luck, Tony wrote:
>> For example, something like below inserted at this point in changelog:
>>
>> 	The new semantics rely on some struct rmid_read members having
>> 	NULL values to distinguish between the SNC and non-SNC scenarios.
>> 	resctrl can thus no longer rely on this struct not being initialized
>> 	properly.
> 
> Just checking whether "resctrl" should be capitalized at the beginning of
> a sentence. Digging through logs there are half-a-dozen places where it
> got a "R" and three dozen where it didn't.

I am not familiar enough with language rules to know what is correct here.
Either should be fine. If I have to pick I'd vote for consistently
using resctrl without capitalization.

Reinette