| Message ID | 20240710023901.1624-1-justinjiang@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v8] mm: shrink skip folio mapped by an exiting process | expand |
On Wed, Jul 10, 2024 at 2:39 PM Zhiguo Jiang <justinjiang@vivo.com> wrote: > > The releasing process of the non-shared anonymous folio mapped solely by > an exiting process may go through two flows: 1) the anonymous folio is > firstly is swaped-out into swapspace and transformed into a swp_entry > in shrink_folio_list; 2) then the swp_entry is released in the process > exiting flow. This will result in the high cpu load of releasing a > non-shared anonymous folio mapped solely by an exiting process. > > When the low system memory and the exiting process exist at the same > time, it will be likely to happen, because the non-shared anonymous > folio mapped solely by an exiting process may be reclaimed by > shrink_folio_list. > > This patch is that shrink skips the non-shared anonymous folio solely > mapped by an exting process and this folio is only released directly in > the process exiting flow, which will save swap-out time and alleviate > the load of the process exiting. > > Reviewed-by: Matthew Wilcox <willy@infradead.org> > Reviewed-by: David Hildenbrand <david@redhat.com> No, this is a disaster. Please ask someone for help before you send it. Neither Willy nor David has ever posted any Reviewed-by tags. Please do get someone to help you. Stop posting like this! > Acked-by: Barry Song <baohua@kernel.org> > Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com> > --- > > Change log: > v7->v8: > 1.Add tags of Reviewed-by and Acked-by. > 2.Add #include <linux/oom.h> to solve compilation issue. > v6->v7: > 1.Modify tab indentation to space indentation of the continuation > lines of the condition. > v5->v6: > 1.Move folio_likely_mapped_shared() under the PTL. > 2.Add check_stable_address_space() to replace MMF_OOM_SKIP. > 3.Remove folio_test_anon(folio). > v4->v5: > 1.Further modify to skip non-shared anonymous folio only. > 2.Update comments for pra->referenced = -1. > v3->v4: > 1.Modify to skip only the non-shared anonymous folio mapped solely > by an exiting process. > v2->v3: > Nothing. > v1->v2: > 1.The VM_EXITING added in v1 patch is removed, because it will fail > to compile in 32-bit system. > > Comments from participants and my responses: > [v7->v8]: > 1.Barry Song <baohua@kernel.org> > You should have collected tags such as reviewed-by, acked-by you got in > v6 while sending v7. > --> > Added in patch v8. > > You didn't even pass the compilation stage because you're missing > 'linux/oom.h'. It's quite disappointing because I believe in your idea, > but you didn't even build it before sending. > --> > Sorry, I overlooked the compilation of folio_likely_mapped_shared() added > in patch v5. Compiled and Updated have been compeleted in patch v8. > > [v6->v7]: > 1.Matthew Wilcox <willy@infradead.org> > You told me you'd fix the indentation. You cannot indent both the > continuation lines of the condition and the body of the if by one tab > each! > --> > Modify tab indentation to space indentation of the continuation > lines of the condition. > > [v5->v6]: > 1.David Hildenbrand <david@redhat.com> > I'm currently working on moving all folio_likely_mapped_shared() under > the PTL, where we are then sure that the folio is actually mapped by > this process (e.g., no concurrent unmapping poisslbe). Can we do the > same here directly? > --> > You are right. we might use page_vma_mapped_walk_done() to bail out. > (Barry Song) > > 2.Barry Song <baohua@kernel.org> > By the way, I am not convinced that using test_bit(MMF_OOM_SKIP, > &vma->vm_mm->flags) is correct (I think it is wrong). And exit_mmap() > automatically has MMF_OOM_SKIP. What is the purpose of this check? > Is there a better way to determine if a process is an OOM target? > What about check_stable_address_space() ? > --> > Sorry, I overlook the situation with if (is_global_init(p)), > MMF_OOM_SKIP is indeed not suitable. It seems feasible for > check_stable_address_space() replacing MMF_OOM_SKIP. > check_stable_address_space() can indicate oom kill, and > !atomic_read(&vma->vm_mm->mm_users) can indicate the normal > process exiting. > > I also think we actually can remove "folio_test_anon(folio)". > --> > Yes, update in patch v6. > > [v4->v5]: > 1.Barry Song <baohua@kernel.org> > I don't think this is correct. folio_likely_mapped_shared() is almost > "correct" but not always. > Please explain why you set pra->referenced = -1. Please address all > comments before you send a new version. > --> > Update in patch v5. > > 2.Matthew Wilcox <willy@infradead.org> > How is the file folio similar? File folios are never written to swap, > and they'll be written back from the page cache whenever the filesystem > decides it's a good time to do so. > --> > What do you mean is that the file folio will not have any relevant > identifier left in memory after it is reclamed in the shrink flow, > and it will not be released again during an exiting process? If that's > the case, I think we only need the anon folio is skipped here. > > [v3->v4]: > 1.Barry Song <baohua@kernel.org> > This is clearly version 3, as you previously sent version 2, correct? > --> > Yes. > > Could you please describe the specific impact on users, including user > experience and power consumption? How serious is this problem? > --> > At present, I do not have a suitable method to accurately measure the > optimization benefit datas of this modifications, but I believe it > theoretically has some benefits. > Launching large memory app (for example, starting the camera) in multiple > backend scenes may result in the high cpu load of the exiting processes. > > Applications? > --> > Yes, when system is low memory, it more likely to occur. > > I'm not completely convinced this patch is correct, but it appears to be > heading in the right direction. Therefore, I expect to see new versions > rather than it being dead. > You changed the file mode to 755, which is incorrect. > --> > Solved. > > Why use -1? Is this meant to simulate lock contention to keep the folio > without activating it? Please do have some comments to explain why. > I'm not convinced this change is appropriate for shared folios. It seems > more suitable for exclusive folios used solely by the exiting process. > --> > The skiped folios are FOLIOREF_KEEP and added into inactive lru, beacase > the folios will be freed soon in the exiting process flow. > Yes, the shared folios can not be simply skipped. I have made relevant > modifications in patch v4 and please help to further review. > https://lore.kernel.org/linux-mm/20240708031517.856-1-justinjiang@vivo.com/ > > 2.David Hildenbrand <david@redhat.com> > but what if it is shared among multiple processes and only one of them > is exiting? > --> > Modify to skip only the non-shared anonymous folio mapped solely > by an exiting process in next version v4. > > [v2->v3:] > Nothing. > > [v1->v2]: > 1.Matthew Wilcox <willy@infradead.org> > What testing have you done of this patch? How often does it happen? > Are there particular workloads that benefit from this? (I'm not sure > what "mutil backed-applications" are?) > And I do mean specifically of this patch, because to my eyes it > shouldn't even compile. Except on 32-bit where it'll say "warning: > integer constant out of range". > --> > Yes, I have tested. When the low system memory and the exiting process > exist at the same time, it will happen. This modification can alleviate > the load of the exiting process. > "mutil backed-applications" means that there are a large number of > the backend applications in the system. > The VM_EXITING added in v1 patch is removed, because it will fail > to compile in 32-bit system. > > mm/rmap.c | 15 +++++++++++++++ > mm/vmscan.c | 7 ++++++- > 2 files changed, 21 insertions(+), 1 deletion(-) > mode change 100644 => 100755 mm/rmap.c > > diff --git a/mm/rmap.c b/mm/rmap.c > index 26806b49a86f..5b92c3dadcc2 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -75,6 +75,7 @@ > #include <linux/memremap.h> > #include <linux/userfaultfd_k.h> > #include <linux/mm_inline.h> > +#include <linux/oom.h> > > #include <asm/tlbflush.h> > > @@ -870,6 +871,20 @@ static bool folio_referenced_one(struct folio *folio, > continue; > } > > + /* > + * Skip the non-shared swapbacked folio mapped solely by > + * the exiting or OOM-reaped process. This avoids redundant > + * swap-out followed by an immediate unmap. > + */ > + if ((!atomic_read(&vma->vm_mm->mm_users) || > + check_stable_address_space(vma->vm_mm)) && > + folio_test_swapbacked(folio) && > + !folio_likely_mapped_shared(folio)) { > + pra->referenced = -1; > + page_vma_mapped_walk_done(&pvmw); > + return false; > + } > + > if (pvmw.pte) { > if (lru_gen_enabled() && > pte_young(ptep_get(pvmw.pte))) { > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 80f9a486cf27..1d5f78a3dbeb 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -863,7 +863,12 @@ static enum folio_references folio_check_references(struct folio *folio, > if (vm_flags & VM_LOCKED) > return FOLIOREF_ACTIVATE; > > - /* rmap lock contention: rotate */ > + /* > + * There are two cases to consider. > + * 1) Rmap lock contention: rotate. > + * 2) Skip the non-shared swapbacked folio mapped solely by > + * the exiting or OOM-reaped process. > + */ > if (referenced_ptes == -1) > return FOLIOREF_KEEP; > > -- > 2.39.0 >
在 2024/7/10 10:41, Barry Song 写道: > On Wed, Jul 10, 2024 at 2:39 PM Zhiguo Jiang <justinjiang@vivo.com> wrote: >> The releasing process of the non-shared anonymous folio mapped solely by >> an exiting process may go through two flows: 1) the anonymous folio is >> firstly is swaped-out into swapspace and transformed into a swp_entry >> in shrink_folio_list; 2) then the swp_entry is released in the process >> exiting flow. This will result in the high cpu load of releasing a >> non-shared anonymous folio mapped solely by an exiting process. >> >> When the low system memory and the exiting process exist at the same >> time, it will be likely to happen, because the non-shared anonymous >> folio mapped solely by an exiting process may be reclaimed by >> shrink_folio_list. >> >> This patch is that shrink skips the non-shared anonymous folio solely >> mapped by an exting process and this folio is only released directly in >> the process exiting flow, which will save swap-out time and alleviate >> the load of the process exiting. >> >> Reviewed-by: Matthew Wilcox <willy@infradead.org> >> Reviewed-by: David Hildenbrand <david@redhat.com> > No, this is a disaster. Please ask someone for help before you send it. > Neither Willy nor David has ever posted any Reviewed-by tags. > Please do get someone to help you. Stop posting like this! Reviewed-by tags information has been updated in v9. Thank you for agreeing my idea and providing strong assistance in improving the subsequent patchs. Thanks Zhiguo > > >> Acked-by: Barry Song <baohua@kernel.org> >> Signed-off-by: Zhiguo Jiang <justinjiang@vivo.com> >> --- >> >> Change log: >> v7->v8: >> 1.Add tags of Reviewed-by and Acked-by. >> 2.Add #include <linux/oom.h> to solve compilation issue. >> v6->v7: >> 1.Modify tab indentation to space indentation of the continuation >> lines of the condition. >> v5->v6: >> 1.Move folio_likely_mapped_shared() under the PTL. >> 2.Add check_stable_address_space() to replace MMF_OOM_SKIP. >> 3.Remove folio_test_anon(folio). >> v4->v5: >> 1.Further modify to skip non-shared anonymous folio only. >> 2.Update comments for pra->referenced = -1. >> v3->v4: >> 1.Modify to skip only the non-shared anonymous folio mapped solely >> by an exiting process. >> v2->v3: >> Nothing. >> v1->v2: >> 1.The VM_EXITING added in v1 patch is removed, because it will fail >> to compile in 32-bit system. >> >> Comments from participants and my responses: >> [v7->v8]: >> 1.Barry Song <baohua@kernel.org> >> You should have collected tags such as reviewed-by, acked-by you got in >> v6 while sending v7. >> --> >> Added in patch v8. >> >> You didn't even pass the compilation stage because you're missing >> 'linux/oom.h'. It's quite disappointing because I believe in your idea, >> but you didn't even build it before sending. >> --> >> Sorry, I overlooked the compilation of folio_likely_mapped_shared() added >> in patch v5. Compiled and Updated have been compeleted in patch v8. >> >> [v6->v7]: >> 1.Matthew Wilcox <willy@infradead.org> >> You told me you'd fix the indentation. You cannot indent both the >> continuation lines of the condition and the body of the if by one tab >> each! >> --> >> Modify tab indentation to space indentation of the continuation >> lines of the condition. >> >> [v5->v6]: >> 1.David Hildenbrand <david@redhat.com> >> I'm currently working on moving all folio_likely_mapped_shared() under >> the PTL, where we are then sure that the folio is actually mapped by >> this process (e.g., no concurrent unmapping poisslbe). Can we do the >> same here directly? >> --> >> You are right. we might use page_vma_mapped_walk_done() to bail out. >> (Barry Song) >> >> 2.Barry Song <baohua@kernel.org> >> By the way, I am not convinced that using test_bit(MMF_OOM_SKIP, >> &vma->vm_mm->flags) is correct (I think it is wrong). And exit_mmap() >> automatically has MMF_OOM_SKIP. What is the purpose of this check? >> Is there a better way to determine if a process is an OOM target? >> What about check_stable_address_space() ? >> --> >> Sorry, I overlook the situation with if (is_global_init(p)), >> MMF_OOM_SKIP is indeed not suitable. It seems feasible for >> check_stable_address_space() replacing MMF_OOM_SKIP. >> check_stable_address_space() can indicate oom kill, and >> !atomic_read(&vma->vm_mm->mm_users) can indicate the normal >> process exiting. >> >> I also think we actually can remove "folio_test_anon(folio)". >> --> >> Yes, update in patch v6. >> >> [v4->v5]: >> 1.Barry Song <baohua@kernel.org> >> I don't think this is correct. folio_likely_mapped_shared() is almost >> "correct" but not always. >> Please explain why you set pra->referenced = -1. Please address all >> comments before you send a new version. >> --> >> Update in patch v5. >> >> 2.Matthew Wilcox <willy@infradead.org> >> How is the file folio similar? File folios are never written to swap, >> and they'll be written back from the page cache whenever the filesystem >> decides it's a good time to do so. >> --> >> What do you mean is that the file folio will not have any relevant >> identifier left in memory after it is reclamed in the shrink flow, >> and it will not be released again during an exiting process? If that's >> the case, I think we only need the anon folio is skipped here. >> >> [v3->v4]: >> 1.Barry Song <baohua@kernel.org> >> This is clearly version 3, as you previously sent version 2, correct? >> --> >> Yes. >> >> Could you please describe the specific impact on users, including user >> experience and power consumption? How serious is this problem? >> --> >> At present, I do not have a suitable method to accurately measure the >> optimization benefit datas of this modifications, but I believe it >> theoretically has some benefits. >> Launching large memory app (for example, starting the camera) in multiple >> backend scenes may result in the high cpu load of the exiting processes. >> >> Applications? >> --> >> Yes, when system is low memory, it more likely to occur. >> >> I'm not completely convinced this patch is correct, but it appears to be >> heading in the right direction. Therefore, I expect to see new versions >> rather than it being dead. >> You changed the file mode to 755, which is incorrect. >> --> >> Solved. >> >> Why use -1? Is this meant to simulate lock contention to keep the folio >> without activating it? Please do have some comments to explain why. >> I'm not convinced this change is appropriate for shared folios. It seems >> more suitable for exclusive folios used solely by the exiting process. >> --> >> The skiped folios are FOLIOREF_KEEP and added into inactive lru, beacase >> the folios will be freed soon in the exiting process flow. >> Yes, the shared folios can not be simply skipped. I have made relevant >> modifications in patch v4 and please help to further review. >> https://lore.kernel.org/linux-mm/20240708031517.856-1-justinjiang@vivo.com/ >> >> 2.David Hildenbrand <david@redhat.com> >> but what if it is shared among multiple processes and only one of them >> is exiting? >> --> >> Modify to skip only the non-shared anonymous folio mapped solely >> by an exiting process in next version v4. >> >> [v2->v3:] >> Nothing. >> >> [v1->v2]: >> 1.Matthew Wilcox <willy@infradead.org> >> What testing have you done of this patch? How often does it happen? >> Are there particular workloads that benefit from this? (I'm not sure >> what "mutil backed-applications" are?) >> And I do mean specifically of this patch, because to my eyes it >> shouldn't even compile. Except on 32-bit where it'll say "warning: >> integer constant out of range". >> --> >> Yes, I have tested. When the low system memory and the exiting process >> exist at the same time, it will happen. This modification can alleviate >> the load of the exiting process. >> "mutil backed-applications" means that there are a large number of >> the backend applications in the system. >> The VM_EXITING added in v1 patch is removed, because it will fail >> to compile in 32-bit system. >> >> mm/rmap.c | 15 +++++++++++++++ >> mm/vmscan.c | 7 ++++++- >> 2 files changed, 21 insertions(+), 1 deletion(-) >> mode change 100644 => 100755 mm/rmap.c >> >> diff --git a/mm/rmap.c b/mm/rmap.c >> index 26806b49a86f..5b92c3dadcc2 100644 >> --- a/mm/rmap.c >> +++ b/mm/rmap.c >> @@ -75,6 +75,7 @@ >> #include <linux/memremap.h> >> #include <linux/userfaultfd_k.h> >> #include <linux/mm_inline.h> >> +#include <linux/oom.h> >> >> #include <asm/tlbflush.h> >> >> @@ -870,6 +871,20 @@ static bool folio_referenced_one(struct folio *folio, >> continue; >> } >> >> + /* >> + * Skip the non-shared swapbacked folio mapped solely by >> + * the exiting or OOM-reaped process. This avoids redundant >> + * swap-out followed by an immediate unmap. >> + */ >> + if ((!atomic_read(&vma->vm_mm->mm_users) || >> + check_stable_address_space(vma->vm_mm)) && >> + folio_test_swapbacked(folio) && >> + !folio_likely_mapped_shared(folio)) { >> + pra->referenced = -1; >> + page_vma_mapped_walk_done(&pvmw); >> + return false; >> + } >> + >> if (pvmw.pte) { >> if (lru_gen_enabled() && >> pte_young(ptep_get(pvmw.pte))) { >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index 80f9a486cf27..1d5f78a3dbeb 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -863,7 +863,12 @@ static enum folio_references folio_check_references(struct folio *folio, >> if (vm_flags & VM_LOCKED) >> return FOLIOREF_ACTIVATE; >> >> - /* rmap lock contention: rotate */ >> + /* >> + * There are two cases to consider. >> + * 1) Rmap lock contention: rotate. >> + * 2) Skip the non-shared swapbacked folio mapped solely by >> + * the exiting or OOM-reaped process. >> + */ >> if (referenced_ptes == -1) >> return FOLIOREF_KEEP; >> >> -- >> 2.39.0 >>
diff --git a/mm/rmap.c b/mm/rmap.c index 26806b49a86f..5b92c3dadcc2 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -75,6 +75,7 @@ #include <linux/memremap.h> #include <linux/userfaultfd_k.h> #include <linux/mm_inline.h> +#include <linux/oom.h> #include <asm/tlbflush.h> @@ -870,6 +871,20 @@ static bool folio_referenced_one(struct folio *folio, continue; } + /* + * Skip the non-shared swapbacked folio mapped solely by + * the exiting or OOM-reaped process. This avoids redundant + * swap-out followed by an immediate unmap. + */ + if ((!atomic_read(&vma->vm_mm->mm_users) || + check_stable_address_space(vma->vm_mm)) && + folio_test_swapbacked(folio) && + !folio_likely_mapped_shared(folio)) { + pra->referenced = -1; + page_vma_mapped_walk_done(&pvmw); + return false; + } + if (pvmw.pte) { if (lru_gen_enabled() && pte_young(ptep_get(pvmw.pte))) { diff --git a/mm/vmscan.c b/mm/vmscan.c index 80f9a486cf27..1d5f78a3dbeb 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -863,7 +863,12 @@ static enum folio_references folio_check_references(struct folio *folio, if (vm_flags & VM_LOCKED) return FOLIOREF_ACTIVATE; - /* rmap lock contention: rotate */ + /* + * There are two cases to consider. + * 1) Rmap lock contention: rotate. + * 2) Skip the non-shared swapbacked folio mapped solely by + * the exiting or OOM-reaped process. + */ if (referenced_ptes == -1) return FOLIOREF_KEEP;