Message-ID: <9a4410c5-c72f-4a8b-ab64-9737e082c5b1@intel.com>
Date: Thu, 17 Jul 2025 20:53:15 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <corbet@....net>, <tony.luck@...el.com>,
	<james.morse@....com>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>
CC: <Dave.Martin@....com>, <x86@...nel.org>, <hpa@...or.com>,
	<akpm@...ux-foundation.org>, <paulmck@...nel.org>, <rostedt@...dmis.org>,
	<Neeraj.Upadhyay@....com>, <david@...hat.com>, <arnd@...db.de>,
	<fvdl@...gle.com>, <seanjc@...gle.com>, <jpoimboe@...nel.org>,
	<pawan.kumar.gupta@...ux.intel.com>, <xin@...or.com>,
	<manali.shukla@....com>, <tao1.su@...ux.intel.com>, <sohil.mehta@...el.com>,
	<kai.huang@...el.com>, <xiaoyao.li@...el.com>, <peterz@...radead.org>,
	<xin3.li@...el.com>, <kan.liang@...ux.intel.com>,
	<mario.limonciello@....com>, <thomas.lendacky@....com>, <perry.yuan@....com>,
	<gautham.shenoy@....com>, <chang.seok.bae@...el.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<peternewman@...gle.com>, <eranian@...gle.com>
Subject: Re: [PATCH v15 23/34] fs/resctrl: Support counter read/reset with
 mbm_event assignment mode

Hi Babu,

On 7/8/25 3:17 PM, Babu Moger wrote:
> When "mbm_event" counter assignment mode is enabled, the architecture
> requires a counter ID to read the event data.
> 
> Introduce an is_cntr field in struct rmid_read to indicate whether counter
> assignment mode is in use.
> 
> Update the logic to call resctrl_arch_cntr_read() and
> resctrl_arch_reset_cntr() when the assignment mode is active.
> 
> Declare mbm_cntr_get() in fs/resctrl/internal.h to make it accessible to
> other functions within fs/resctrl.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@...el.com>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> v15: New patch to add is_cntr in rmid_read as discussed in
>      https://lore.kernel.org/lkml/b4b14670-9cb0-4f65-abd5-39db996e8da9@intel.com/
> ---
>  fs/resctrl/ctrlmondata.c |  8 ++++++++
>  fs/resctrl/internal.h    |  5 +++++
>  fs/resctrl/monitor.c     | 42 +++++++++++++++++++++++++++++++++-------
>  3 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index ad7ffc6acf13..ce766b2cdfc1 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -569,6 +569,14 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  		return;
>  	}
>  
> +	if (resctrl_arch_mbm_cntr_assign_enabled(r) && resctrl_is_mbm_event(evtid)) {
> +		if (mbm_cntr_get(r, d, rdtgrp, evtid) < 0) {
> +			rr->err = -ENOENT;

This introduces the new -ENOENT error code but rdtgroup_mondata_show() is not enabled to
handle it. Looks like the next patch needs to be squashed into this one.

> +			return;

Looking at this closer this does not seem the right place for this check. This is the
top level mon_event_read() when the user reads an event file. If this is an event
associated with a CTRL_MON group then resctrl should sum the data of the CTRL_MON group
and all of its MON groups.
It may be that the CTRL_MON group's event does not have a counter assigned, but one
or more of the MON groups do. In this case I do not think resctrl should fail reading
the CTRL_MON's event early as is done above but still provide the sum of the events that
can be counted in the MON groups that do have counters assigned. With that, only return
"Unassigned" if no counter is assigned in any CTRL_MON or MON group?


There also appears to be potential for a leak here with the early exit without calling
resctrl_arch_mon_ctx_free(). As mentioned earlier in series I do not think that
arch_mon_ctx is relevant to this counter assignment so I think mbm_update_one_event()
as well as mon_event_read() should make calling resctrl_arch_mon_ctx_alloc()
conditional on is_cntr/is_mbm_cntr. 


> +		}
> +		rr->is_cntr = true;
> +	}
> +
>  	cpu = cpumask_any_housekeeping(cpumask, RESCTRL_PICK_ANY_CPU);
>  
>  	/*
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 647a0466ffa0..fb4fec4a4cdc 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -110,6 +110,8 @@ struct mon_data {
>   *	   domains in @r sharing L3 @ci.id
>   * @evtid: Which monitor event to read.
>   * @first: Initialize MBM counter when true.
> + * @is_cntr: Is the counter valid? true if "mbm_event" counter assignment mode is
> + *	   enabled and it is an MBM event.
>   * @ci_id: Cacheinfo id 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,
> @@ -124,6 +126,7 @@ struct rmid_read {
>  	struct rdt_mon_domain	*d;
>  	enum resctrl_event_id	evtid;
>  	bool			first;
> +	bool			is_cntr;

After seeing how this all comes together the "is_cntr" is very generic and not
matching the often used "mbm_cntr". What do you think of "is_mbm_cntr"?

>  	unsigned int		ci_id;
>  	int			err;
>  	u64			val;
> @@ -391,6 +394,8 @@ int rdtgroup_assign_cntr_event(struct rdt_mon_domain *d, struct rdtgroup *rdtgrp
>  			       struct mon_evt *mevt);
>  void rdtgroup_unassign_cntr_event(struct rdt_mon_domain *d, struct rdtgroup *rdtgrp,
>  				  struct mon_evt *mevt);
> +int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> +		 struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);
>  
>  #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
>  int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 73f507942b6d..35faca7ff3b1 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -366,9 +366,21 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  	struct mbm_state *m;
>  	int err, ret;
>  	u64 tval = 0;
> +	int cntr_id;

I've tried a couple of checkers and not all can pick up that cntr_id is always
assigned when it is used below. While it is not necessary it will be helpful to
initialize cntr_id here (-ENOENT?) just to avoid needing to prove that hypothetical
reports from these checkers are false positives.

> +
> +	if (rr->is_cntr) {
> +		cntr_id = mbm_cntr_get(rr->r, rr->d, rdtgrp, rr->evtid);
> +		if (cntr_id < 0) {
> +			rr->err = -ENOENT;
> +			return -EINVAL;
> +		}

This looks to be the right place for the mbm_cntr_get() check. Just keeping this one
avoids the duplication while also makeing the code match existing user ABI.
What do you think?

> +	}
>  
>  	if (rr->first) {
> -		resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evtid);
> +		if (rr->is_cntr)
> +			resctrl_arch_reset_cntr(rr->r, rr->d, closid, rmid, cntr_id, rr->evtid);
> +		else
> +			resctrl_arch_reset_rmid(rr->r, rr->d, closid, rmid, rr->evtid);
>  		m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
>  		if (m)
>  			memset(m, 0, sizeof(struct mbm_state));
> @@ -379,8 +391,12 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  		/* Reading a single domain, must be on a CPU in that domain. */
>  		if (!cpumask_test_cpu(cpu, &rr->d->hdr.cpu_mask))
>  			return -EINVAL;
> -		rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
> -						 rr->evtid, &tval, rr->arch_mon_ctx);
> +		if (rr->is_cntr)
> +			rr->err = resctrl_arch_cntr_read(rr->r, rr->d, closid, rmid, cntr_id,
> +							 rr->evtid, &tval, rr->arch_mon_ctx);
> +		else
> +			rr->err = resctrl_arch_rmid_read(rr->r, rr->d, closid, rmid,
> +							 rr->evtid, &tval, rr->arch_mon_ctx);
>  		if (rr->err)
>  			return rr->err;
>  
> @@ -405,8 +421,12 @@ static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  	list_for_each_entry(d, &rr->r->mon_domains, hdr.list) {
>  		if (d->ci_id != rr->ci_id)
>  			continue;
> -		err = resctrl_arch_rmid_read(rr->r, d, closid, rmid,
> -					     rr->evtid, &tval, rr->arch_mon_ctx);
> +		if (rr->is_cntr)
> +			err = resctrl_arch_cntr_read(rr->r, d, closid, rmid, cntr_id,
> +						     rr->evtid, &tval, rr->arch_mon_ctx);
> +		else
> +			err = resctrl_arch_rmid_read(rr->r, d, closid, rmid,
> +						     rr->evtid, &tval, rr->arch_mon_ctx);
>  		if (!err) {
>  			rr->val += tval;
>  			ret = 0;
> @@ -620,6 +640,14 @@ static void mbm_update_one_event(struct rdt_resource *r, struct rdt_mon_domain *
>  		return;
>  	}
>  
> +	if (resctrl_arch_mbm_cntr_assign_enabled(r) && resctrl_is_mbm_event(evtid)) {
> +		if (mbm_cntr_get(r, d, rdtgrp, evtid) < 0) {
> +			rr.err = -ENOENT;
> +			return;
> +		}

This can be dropped also? Only is_cntr/is_mbm_cntr needs to be set here, __mon_event_count()
called below duplicates above snippet and will set rr->err

Same comment here about missing call to resctrl_arch_mon_ctx_free().


> +		rr.is_cntr = true;
> +	}
> +
>  	__mon_event_count(rdtgrp, &rr);
>  
>  	/*
> @@ -982,8 +1010,8 @@ static void resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d
>   * Return:
>   * Valid counter ID on success, or -ENOENT on failure.
>   */
> -static int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> -			struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
> +int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> +		 struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
>  {
>  	int cntr_id;
>  

Reinette

Powered by blists - more mailing lists