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 <[email protected]> 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(®ion->ref, dmemcg_free_region); > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
