On Tue, Mar 28, 2023 at 06:16:34AM +0000, Yosry Ahmed wrote: > As Johannes notes in [1], stats_flush_lock is currently used to: > (a) Protect updated to stats_flush_threshold. > (b) Protect updates to flush_next_time. > (c) Serializes calls to cgroup_rstat_flush() based on those ratelimits. > > However: > > 1. stats_flush_threshold is already an atomic > > 2. flush_next_time is not atomic. The writer is locked, but the reader > is lockless. If the reader races with a flush, you could see this: > > if (time_after(jiffies, flush_next_time)) > spin_trylock() > flush_next_time = now + delay > flush() > spin_unlock() > spin_trylock() > flush_next_time = now + delay > flush() > spin_unlock() > > which means we already can get flushes at a higher frequency than > FLUSH_TIME during races. But it isn't really a problem. > > The reader could also see garbled partial updates, so it needs at > least READ_ONCE and WRITE_ONCE protection. > > 3. Serializing cgroup_rstat_flush() calls against the ratelimit > factors is currently broken because of the race in 2. But the race > is actually harmless, all we might get is the occasional earlier > flush. If there is no delta, the flush won't do much. And if there > is, the flush is justified. > > So the lock can be removed all together. However, the lock also served > the purpose of preventing a thundering herd problem for concurrent > flushers, see [2]. Use an atomic instead to serve the purpose of > unifying concurrent flushers. > > [1]https://lore.kernel.org/lkml/20230323172732.GE739026@xxxxxxxxxxx/ > [2]https://lore.kernel.org/lkml/20210716212137.1391164-2-shakeelb@xxxxxxxxxx/ > > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx> With Shakeel's suggestion: Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>
