On Thu, Mar 23, 2023 at 09:01:12AM -0700, Yosry Ahmed wrote: > On Thu, Mar 23, 2023 at 8:56 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote: > > > > On Thu, Mar 23, 2023 at 04:00:34AM +0000, Yosry Ahmed wrote: > > > @@ -644,26 +644,26 @@ static void __mem_cgroup_flush_stats(void) > > > return; > > > > > > flush_next_time = jiffies_64 + 2*FLUSH_TIME; > > > - cgroup_rstat_flush(root_mem_cgroup->css.cgroup, false); > > > + cgroup_rstat_flush(root_mem_cgroup->css.cgroup, may_sleep); > > > > How is it safe to call this with may_sleep=true when it's holding the > > stats_flush_lock? > > stats_flush_lock is always called with trylock, it is only used today > so that we can skip flushing if another cpu is already doing a flush > (which is not 100% correct as they may have not finished flushing yet, > but that's orthogonal here). So I think it should be safe to sleep as > no one can be blocked waiting for this spinlock. I see. It still cannot sleep while the lock is held, though, because preemption is disabled. Make sure you have all lock debugging on while testing this. > Perhaps it would be better semantically to replace the spinlock with > an atomic test and set, instead of having a lock that can only be used > with trylock? It could be helpful to clarify what stats_flush_lock is protecting first. Keep in mind that locks should protect data, not code paths. Right now it's doing multiple things: 1. It protects updates to stats_flush_threshold 2. It protects updates to flush_next_time 3. It 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. In summary, it seems to me the lock can be ditched altogether. All the code needs is READ_ONCE/WRITE_ONCE around flush_next_time.