Re: [PATCH v7 3/6] cgroup/dmem: Add reclaim callback for lowering max below current usage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Holding `srcu_read_lock` across a potentially unbounded reclaim loop causes `synchronize_srcu()` to hang indefinitely during region unregistration.
--

commit a6f9976e310be028f3b0c1a4f382c0d26ce5f305
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Subject: cgroup/dmem: Add reclaim callback for lowering max below current usage

This commit adds an optional reclaim callback to struct dmem_cgroup_region 
that is invoked when dmem.max is lowered. It introduces an SRCU domain to
protect reclaim callbacks against concurrent region unregistrations.

> diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
> index 373ad04d289d5..93bc9813903c8 100644
> --- a/kernel/cgroup/dmem.c
> +++ b/kernel/cgroup/dmem.c

[ ... ]

> @@ -145,21 +161,52 @@ static void free_cg_pool(struct dmem_cgroup_pool_state *pool)
>  }
>  
>  static void
> -set_resource_max(struct dmem_cgroup_pool_state *pool, u64 val)
> +set_resource_max(struct dmem_cgroup_pool_state *pool, u64 val, bool nonblock)
>  {
> -	page_counter_set_max(&pool->cnt, val);
> +	struct dmem_cgroup_region *region = pool->region;
> +	unsigned long limit = (unsigned long)val;
> +
> +	/* Apply the new limit immediately so concurrent allocations are throttled. */
> +	xchg(&pool->cnt.max, limit);
> +
> +	if (nonblock)
> +		return;
> +
> +	int srcu_idx = srcu_read_lock(&dmemcg_srcu);
> +
> +	if (!READ_ONCE(region->unregistered) && region->ops && region->ops->reclaim) {
> +		for (int retries = DMEM_MAX_RECLAIM_RETRIES; ; ) {
> +			u64 usage = page_counter_read(&pool->cnt);
> +			int ret;
> +
> +			if (usage <= limit)
> +				break;
> +
> +			if (signal_pending(current))
> +				break;
> +
> +			ret = region->ops->reclaim(pool, usage - limit, region->reclaim_priv);
> +
> +			/* -ENOSPC means no progress; other errors are fatal. */
> +			if (ret && (ret != -ENOSPC || !retries--))
> +				break;
> +
> +			cond_resched();
> +		}

[Severity: High]
Can this loop cause synchronize_srcu() in dmem_cgroup_unregister_region() to
hang indefinitely?

If the driver's reclaim() callback repeatedly returns 0 (indicating some
progress) but concurrent allocations keep usage above the limit, this loop
could run for a very long time.

Because srcu_read_lock() is held outside the loop, a concurrent call to
dmem_cgroup_unregister_region() would block in synchronize_srcu() waiting
for this reader to finish.

Would it be safer to scope the SRCU read lock inside the loop iteration?
This would allow synchronize_srcu() to proceed and the loop could safely abort
when it sees region->unregistered become true.

> +	}
> +	srcu_read_unlock(&dmemcg_srcu, srcu_idx);
>  }

[ ... ]

> @@ -488,9 +539,11 @@ void dmem_cgroup_unregister_region(struct dmem_cgroup_region *region)
>  	 * no new pools should be added to the dead region
>  	 * by get_cg_pool_unlocked.
>  	 */
> -	region->unregistered = true;
> +	WRITE_ONCE(region->unregistered, true);
>  	spin_unlock(&dmemcg_lock);
>  
> +	synchronize_srcu(&dmemcg_srcu);
> +
>  	kref_put(&region->ref, dmemcg_free_region);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703130541.2686-1-thomas.hellstrom@linux.intel.com?part=3