Any thoughts on this respin? On Thu, Mar 9, 2023 at 1:31 AM 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. > > v1 -> v2: > - Renamed report_freed_pages() to mm_account_reclaimed_pages(), as > suggested by Dave Chinner. There were discussions about leaving > updating current->reclaim_state open-coded as it's not worth hiding > the current dereferencing to remove one line, but I'd rather have the > logic contained with mm/vmscan.c so that the next person that changes > this logic doesn't have to change 7 different files. > - Renamed add_non_vmscan_reclaimed() to flush_reclaim_state() (Johannes > Weiner). > - Added more context about how this problem was found in the cover > letter (Johannes Weiner). > - Added a patch to move set_task_reclaim_state() below the definition of > cgroup_reclaim(), and added additional helpers in the same position. > This way all the helpers for reclaim_state live together, and there is > no need to declare cgroup_reclaim() early or move its definition > around to call it from flush_reclaim_state(). This should also fix the > build error reported by the bot in !CONFIG_MEMCG. > > RFC -> v1: > - Exported report_freed_pages() in case XFS is built as a module (Matthew > Wilcox). > - Renamed reclaimed_slab to reclaim in previously missed MGLRU code. > - Refactored using reclaim_state to update sc->nr_reclaimed into a > helper and added an XL comment explaining why we ignore > reclaim_state->reclaimed in memcg reclaim (Johannes Weiner). > > Yosry Ahmed (3): > mm: vmscan: move set_task_reclaim_state() after cgroup_reclaim() > mm: vmscan: refactor updating reclaimed pages in reclaim_state > mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim > > fs/inode.c | 3 +- > fs/xfs/xfs_buf.c | 3 +- > include/linux/swap.h | 5 ++- > mm/slab.c | 3 +- > mm/slob.c | 6 +-- > mm/slub.c | 5 +-- > mm/vmscan.c | 88 +++++++++++++++++++++++++++++++++++--------- > 7 files changed, 81 insertions(+), 32 deletions(-) > > -- > 2.40.0.rc0.216.gc4246ad0f0-goog >
