On Tue, Mar 28, 2023 at 11:45:19AM -0700, Yosry Ahmed wrote: > On Tue, Mar 28, 2023 at 11:35 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote: > > On Tue, Mar 28, 2023 at 06:16:35AM +0000, Yosry Ahmed wrote: > > > void mem_cgroup_flush_stats_ratelimited(void) > > > { > > > if (time_after64(jiffies_64, READ_ONCE(flush_next_time))) > > > - mem_cgroup_flush_stats(); > > > + mem_cgroup_flush_stats_atomic(); > > > +} > > > > This should probably be mem_cgroup_flush_stats_atomic_ratelimited(). > > > > (Whee, kinda long, but that's alright. Very specialized caller...) > > It should, but the following patch makes it non-atomic anyway, so I > thought I wouldn't clutter the diff by renaming it here and then > reverting it back in the next patch. > > There is an argument for maintaining a clean history tho in case the > next patch is reverted separately (which is the reason I put it in a > separate patch to begin with) -- so perhaps I should rename it here to > mem_cgroup_flush_stats_atomic_ratelimited () and back to > mem_cgroup_flush_stats_ratelimited() in the next patch, just for > consistency? Sounds good to me. It's pretty minor churn. > > Btw, can you guys think of a reason against moving the threshold check > > into the common function? It would then apply to the time-limited > > flushes as well, but that shouldn't hurt anything. This would make the > > code even simpler: > > I think the point of having the threshold check outside the common > function is that the periodic flusher always flushes, regardless of > the threshold, to keep rstat flushing from critical contexts as cheap > as possible. Good point. Yeah, let's keep it separate then. > > > @@ -2845,7 +2845,7 @@ static void prepare_scan_count(pg_data_t *pgdat, struct scan_control *sc) > > > * Flush the memory cgroup stats, so that we read accurate per-memcg > > > * lruvec stats for heuristics. > > > */ > > > - mem_cgroup_flush_stats(); > > > + mem_cgroup_flush_stats_atomic(); > > > > I'm thinking this one could be non-atomic as well. It's called fairly > > high up in reclaim without any locks held. > > A later patch does exactly that. I put making the reclaim and refault > paths non-atomic in separate patches to easily revert them if we see a > regression. Let me know if this is too defensive and if you'd rather > have them squashed. No, good call. I should have just looked ahead first :-)