fs/inode.c | 3 +- fs/xfs/xfs_buf.c | 3 +- include/linux/swap.h | 17 ++++++++++- mm/slab.c | 3 +- mm/slob.c | 6 ++-- mm/slub.c | 5 ++- mm/vmscan.c | 73 +++++++++++++++++++++++++++++++++----------- 7 files changed, 78 insertions(+), 32 deletions(-)
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. v3 -> v4: - Used global_reclaim() instead of !cgroup_reclaim() in patch 3 to include non-LRU reclaimed pages when writing to memory.reclaim for root (Yu Zhao). - Moved the definition of mm_account_reclaimed_pages() to a static inline in the header file (Dave Chinner). v2 -> v3: - Fixed a compilation problem in patch 2 reported by the bot. - Rebased on top of v6.3-rc2. 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 global_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 | 17 ++++++++++- mm/slab.c | 3 +- mm/slob.c | 6 ++-- mm/slub.c | 5 ++- mm/vmscan.c | 73 +++++++++++++++++++++++++++++++++----------- 7 files changed, 78 insertions(+), 32 deletions(-) -- 2.40.0.348.gf938b09366-goog
On Tue, 4 Apr 2023 00:13:50 +0000 Yosry Ahmed <yosryahmed@google.com> 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.
On Tue, Apr 4, 2023 at 2:38 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 4 Apr 2023 00:13:50 +0000 Yosry Ahmed <yosryahmed@google.com> 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. One thing that I can do is break down patch 2 into two patches, one that adds the flush_reclaim_state() helper, and one that adds the mm_account_reclaimed_pages() helper. The series would be: Patch 1: move set_task_reclaim_state() near other helpers Patch 2: introduce mm_account_reclaimed_pages() Patch 3: introduce flush_reclaim_state() Patch 4: add the one-line change (and the huge comment) to flush_reclaim_state() Backports need only to take patches 3 & 4 (which would be localized to mm/vmscan.c), as patches 1 & 2 would be purely cosmetic with no dependency from patches 3 & 4. For the current series, patch 1 is not needed anyway. So this change would basically save backporters the part of patch 2 that is outside of mm/vmscan.c. If you think this would be useful I can send a v5 with patch 2 broken down into two patches.
On Tue, 4 Apr 2023 14:49:13 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > On Tue, Apr 4, 2023 at 2:38 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Tue, 4 Apr 2023 00:13:50 +0000 Yosry Ahmed <yosryahmed@google.com> 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.
On Tue, Apr 4, 2023 at 2:58 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 4 Apr 2023 14:49:13 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > > On Tue, Apr 4, 2023 at 2:38 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > On Tue, 4 Apr 2023 00:13:50 +0000 Yosry Ahmed <yosryahmed@google.com> 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).
On Tue, 4 Apr 2023 15:00:57 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > ... > > > > > > > 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). -stable maintainers prefer to take something which has already been accepted by Linus. The series could be as simple as simple-two-liner.patch revert-simple-two-liner.patch this-series-as-is.patch simple-two-liner.patch goes into 6.3-rcX and -stable. The other patches into 6.4-rc1.
On Tue, 4 Apr 2023 15:28:16 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Tue, 4 Apr 2023 15:00:57 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > > ... > > > > > > > > > > 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). > > -stable maintainers prefer to take something which has already been > accepted by Linus. > > The series could be as simple as > > simple-two-liner.patch > revert-simple-two-liner.patch > this-series-as-is.patch > > simple-two-liner.patch goes into 6.3-rcX and -stable. The other > patches into 6.4-rc1. But the key question remains: how desirable is a backport? Looking at the changelogs I'm not seeing a clear statement of the impact upon real-world users' real-world workloads. (This is a hint). So I am unable to judge. Please share your thoughts on this.
On Tue, Apr 4, 2023 at 3:31 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 4 Apr 2023 15:28:16 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > > > On Tue, 4 Apr 2023 15:00:57 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > > > > ... > > > > > > > > > > > > > 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). > > > > -stable maintainers prefer to take something which has already been > > accepted by Linus. > > > > The series could be as simple as > > > > simple-two-liner.patch > > revert-simple-two-liner.patch > > this-series-as-is.patch > > > > simple-two-liner.patch goes into 6.3-rcX and -stable. The other > > patches into 6.4-rc1. > > But the key question remains: how desirable is a backport? > > Looking at the changelogs I'm not seeing a clear statement of the > impact upon real-world users' real-world workloads. (This is a hint). > So I am unable to judge. > > Please share your thoughts on this. I think it's nice to have but not really important. It occasionally causes writes to memory.reclaim to report false positives and *might* cause unnecessary retrying when charging memory, but probably too rare to be a practical problem. Personally, I intend to backport to our kernel at Google because it's a simple enough fix and we have occasionally seen test flakiness without it. I have a reworked version of the series that only has 2 patches: - simple-two-liner-patch (actually 5 lines) - one patch including all refactoring squashed (introducing flush_reclaim_state() with the huge comment, introducing mm_account_reclaimed_pages(), and moving set_task_reclaim_state() around). Let me know if you want me to send it as v5, or leave the current v4 if you think backporting is not generally important. >
On Tue, 4 Apr 2023 16:46:30 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > But the key question remains: how desirable is a backport? > > > > Looking at the changelogs I'm not seeing a clear statement of the > > impact upon real-world users' real-world workloads. (This is a hint). > > So I am unable to judge. > > > > Please share your thoughts on this. > > I think it's nice to have but not really important. It occasionally > causes writes to memory.reclaim to report false positives and *might* > cause unnecessary retrying when charging memory, but probably too rare > to be a practical problem. > > Personally, I intend to backport to our kernel at Google because it's > a simple enough fix and we have occasionally seen test flakiness > without it. > > I have a reworked version of the series that only has 2 patches: > - simple-two-liner-patch (actually 5 lines) > - one patch including all refactoring squashed (introducing > flush_reclaim_state() with the huge comment, introducing > mm_account_reclaimed_pages(), and moving set_task_reclaim_state() > around). > > Let me know if you want me to send it as v5, or leave the current v4 > if you think backporting is not generally important. Let's have a look at that v5 and see what people think?
On Wed, Apr 5, 2023 at 11:48 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 4 Apr 2023 16:46:30 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > > > But the key question remains: how desirable is a backport? > > > > > > Looking at the changelogs I'm not seeing a clear statement of the > > > impact upon real-world users' real-world workloads. (This is a hint). > > > So I am unable to judge. > > > > > > Please share your thoughts on this. > > > > I think it's nice to have but not really important. It occasionally > > causes writes to memory.reclaim to report false positives and *might* > > cause unnecessary retrying when charging memory, but probably too rare > > to be a practical problem. > > > > Personally, I intend to backport to our kernel at Google because it's > > a simple enough fix and we have occasionally seen test flakiness > > without it. > > > > I have a reworked version of the series that only has 2 patches: > > - simple-two-liner-patch (actually 5 lines) > > - one patch including all refactoring squashed (introducing > > flush_reclaim_state() with the huge comment, introducing > > mm_account_reclaimed_pages(), and moving set_task_reclaim_state() > > around). > > > > Let me know if you want me to send it as v5, or leave the current v4 > > if you think backporting is not generally important. > > Let's have a look at that v5 and see what people think? Sent v5 [1]. Thanks Andrew! [1]https://lore.kernel.org/lkml/20230405185427.1246289-1-yosryahmed@google.com/
On Tue, Apr 4, 2023 at 3:28 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 4 Apr 2023 15:00:57 -0700 Yosry Ahmed <yosryahmed@google.com> wrote: > > > ... > > > > > > > > > > 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). > > -stable maintainers prefer to take something which has already been > accepted by Linus. > > The series could be as simple as > > simple-two-liner.patch > revert-simple-two-liner.patch > this-series-as-is.patch > > simple-two-liner.patch goes into 6.3-rcX and -stable. The other > patches into 6.4-rc1. Understood, will send a v5 including a simple two-liner for backports.
© 2016 - 2026 Red Hat, Inc.