| Message ID | 20230404001353.468224-1-yosryahmed@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Ignore non-LRU-based reclaim in memcg reclaim | expand |
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, 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.
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/