On Tue, Apr 4, 2023 at 2:58 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Tue, 4 Apr 2023 14:49:13 -0700 Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > > On Tue, Apr 4, 2023 at 2:38 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > On Tue, 4 Apr 2023 00:13:50 +0000 Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > > > > > > Upon running some proactive reclaim tests using memory.reclaim, we > > > > noticed some tests flaking where writing to memory.reclaim would be > > > > successful even though we did not reclaim the requested amount fully. > > > > Looking further into it, I discovered that *sometimes* we over-report > > > > the number of reclaimed pages in memcg reclaim. > > > > > > > > Reclaimed pages through other means than LRU-based reclaim are tracked > > > > through reclaim_state in struct scan_control, which is stashed in > > > > current task_struct. These pages are added to the number of reclaimed > > > > pages through LRUs. For memcg reclaim, these pages generally cannot be > > > > linked to the memcg under reclaim and can cause an overestimated count > > > > of reclaimed pages. This short series tries to address that. > > > > > > > > Patches 1-2 are just refactoring, they add helpers that wrap some > > > > operations on current->reclaim_state, and rename > > > > reclaim_state->reclaimed_slab to reclaim_state->reclaimed. > > > > > > > > Patch 3 ignores pages reclaimed outside of LRU reclaim in memcg reclaim. > > > > The pages are uncharged anyway, so even if we end up under-reporting > > > > reclaimed pages we will still succeed in making progress during > > > > charging. > > > > > > > > Do not let the diff stat deceive you, the core of this series is patch 3, > > > > which has one line of code change. All the rest is refactoring and one > > > > huge comment. > > > > > > > > > > Wouldn't it be better to do this as a single one-line patch for > > > backportability? Then all the refactoring etcetera can be added on > > > later. > > > > Without refactoring the code that adds reclaim_state->reclaimed to > > scan_control->nr_reclaimed into a helper (flush_reclaim_state()), the > > change would need to be done in two places instead of one, and I > > wouldn't know where to put the huge comment. > > Well, all depends on how desirable it it that we backport. If "not > desirable" then leave things as-is. If at least "possibly desirable" > then a simple patch with the two changes and no elaborate comment will > suit. > I would rather leave the current series as-is with an elaborate comment. I can send a separate single patch as a backport to stable if this is something that we usually do (though I am not sure how to format such patch).
