Re: [PATCH v5 RESEND 4/7] fs/resctrl: Stop the limbo handler while resctrl is unmounted

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Jinjie,

On 11/10/26 5:39 AM, Jinjie Ruan wrote:
> A group's RMID is not returned to the free list when the group is
> removed. It stays in limbo until the cache lines tagged with it are
> evicted, and the limbo handler reads its occupancy counter in the

hmmm ... technically all cache lines associated with an RMID need not
be evicted but instead the number of cache lines tagged with the RMID
needs to be below a threshold.

> meantime. The limbo handler keeps running after resctrl is unmounted,
> while the MBM overflow handler stops once resctrl_mounted is cleared.
> 
> Reading the RMIDs in limbo while resctrl is unmounted is not safe on
> every architecture. On MPAM the counter of an RMID is selected by the
> PARTIDs of the CLOSID that owned it, and the CLOSID to PARTID mapping
> depends on the CDP state. The unmount clears the CDP state, so the
> handler that keeps running reads the RMIDs freed under CDP through the
> PARTIDs of the cleared state.

Can the jumping between CLOSID and PARTID be avoided to reduce confusion?
For example, could above paragraph be simplified to something like below:

	Reading the RMIDs in limbo while resctrl is unmounted is not safe on
	every architecture. On MPAM the counter of an RMID is selected by the
	CLOSID that owns it, and the CLOSID depends on the CDP state. Unmount
	clears the CDP state, so the limbo handler that keeps running reads
	the RMIDs freed under CDP through the CLOSIDs of the cleared state.

> 
> Stop the limbo handler while resctrl is unmounted, as the overflow
> handler does, and restart it after a mount on every domain that still
> has RMIDs in limbo. The worker can stay queued across the unmount, so
> cancel it before queuing it again. This keeps the limbo handler
> lifetime the same on every architecture.
> 
> On x86 the RMIDs in limbo are no longer drained while resctrl is
> unmounted. Nothing needs them until the mount that follows the
> unmount, after which the handler frees them as before.

Since this change impacts all architectures the motivation why this is
ok should be stated prominently and not left as what seems like an
addendum to the changelog. What do you think of dropping the above paragraph
and instead add the following at the beginning of the changelog:

	A monitoring group's RMID is not returned to the free list when the
	group is removed. It stays in limbo until the cache occupancy still
	counted	against it has dropped low enough for the RMID to be reused,
	which the limbo handler checks once a second.

	The limbo handler keeps running after resctrl is unmounted, where
	there is nothing for it to achieve. An RMID is only allocated when a
	group is created, which needs resctrl to be mounted, so no RMID the
	limbo handler returns to the free list can be used before the next mount,
	and that mount can check them itself. The MBM overflow handler already
	stops for this reason once resctrl_mounted is cleared.

	Reading the RMIDs in limbo while resctrl is unmounted is not safe ...

	Stop the limbo handler while resctrl is unmounted ...

> 
> Fixes: 6789fb99282c ("arm_mpam: resctrl: Add CDP emulation")
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Cc: <stable@vger.kernel.org> # 7.1+

Looks like 6789fb99282c landed in 7.1 so this stable constraint looks unnecessary?

> ---
>  fs/resctrl/internal.h |  1 +
>  fs/resctrl/monitor.c  | 43 +++++++++++++++++++++++++++++++++++++++++--
>  fs/resctrl/rdtgroup.c |  2 ++
>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 1c85e06f09bf..c95fa6ce62e6 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -373,6 +373,7 @@ void closid_free(int closid);
>  int setup_rmid_lru_list(void);
>  
>  void free_rmid_lru_list(void);
> +void resctrl_limbo_resume(void);

Could this new limbo handler helper please be placed with the other limbo
handler related declarations and also match their naming? For example,
resctrl already has "cqm_setup_limbo_handler()", so perhaps this can be
"cqm_resume_limbo_handler()" to make it obvious that there is a connection
between them?

>  
>  int alloc_rmid(u32 closid);
>  
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index ae9ddd380515..f56d0e32fd06 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -852,6 +852,13 @@ void cqm_handle_limbo(struct work_struct *work)
>  	 */
>  	mutex_lock(&rdtgroup_mutex);
>  
> +	/*
> +	 * If the filesystem has been unmounted this work no longer needs to
> +	 * run. resctrl_limbo_resume() restarts it on the next mount.
> +	 */
> +	if (!resctrl_mounted)
> +		goto out_unlock;
> +
>  	/*
>  	 * Ensure the worker is dedicated to a CPU as intended and not
>  	 * relocated by workqueue subsystem as part of CPU going offline.
> @@ -1001,8 +1008,8 @@ int setup_rmid_lru_list(void)
>  	/*
>  	 * Called on every mount, but the number of RMIDs cannot change
>  	 * after the first mount, so keep using the same set of rmid_ptrs[]
> -	 * until resctrl_exit(). Note that the limbo handler continues to
> -	 * access rmid_ptrs[] after resctrl is unmounted.
> +	 * until resctrl_exit(). Note that the RMIDs in limbo stay in limbo
> +	 * across an unmount and are checked again after the next mount.
>  	 */
>  	if (rmid_ptrs)
>  		return 0;
> @@ -1033,6 +1040,38 @@ int setup_rmid_lru_list(void)
>  	return 0;
>  }
>  
> +/**
> + * resctrl_limbo_resume() - Restart the limbo handler after a mount.
> + *
> + * The limbo handler stops while resctrl is unmounted. Restart it on every
> + * domain that still has RMIDs in limbo.
> + */
> +void resctrl_limbo_resume(void)
> +{
> +	struct rdt_l3_mon_domain *d;
> +	struct rdt_resource *r;
> +
> +	lockdep_assert_cpus_held();
> +	lockdep_assert_held(&rdtgroup_mutex);
> +

Could you also please gate this flow on rdt_resource::mon_capable? This matches
the custom of capability check. resctrl filesystem does not enable the event and,
for example, x86 sets monitoring capability after enabling the event. Having the
monitoring capability consistently used as gate for monitoring flow supports
a robust and consistent flow.

> +	if (!resctrl_is_mon_event_enabled(QOS_L3_OCCUP_EVENT_ID))
> +		return;
> +
> +	r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> +	list_for_each_entry_rcu(d, &r->mon_domains, hdr.list,
> +				lockdep_is_cpus_held()) {
> +		if (!has_busy_rmid(d))
> +			continue;
> +
> +		/*
> +		 * The unmount may have left the worker queued. Cancel it so
> +		 * it is queued again on a CPU picked now.
> +		 */
> +		cancel_delayed_work(&d->cqm_limbo);
> +		cqm_setup_limbo_handler(d, 0, RESCTRL_PICK_ANY_CPU);
> +	}
> +}
> +
>  void free_rmid_lru_list(void)
>  {
>  	if (!resctrl_arch_mon_capable())
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 3312637fb514..b702fe6dbbec 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -3267,6 +3267,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  						   RESCTRL_PICK_ANY_CPU);
>  	}
>  
> +	resctrl_limbo_resume();
> +
>  	/*
>  	 * Ensure root remains accessible after mutex is unlocked so that
>  	 * kernfs_kill_sb() can run safely if called by kernfs_get_tree()'s

Reinette