On Wed, Jul 10, 2024 at 2:39 PM Zhiguo Jiang <justinjiang@xxxxxxxx> 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@xxxxxxxxxxxxx> > Reviewed-by: David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxx> > Signed-off-by: Zhiguo Jiang <justinjiang@xxxxxxxx> > --- > > 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@xxxxxxxxxx> > 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@xxxxxxxxxxxxx> > 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@xxxxxxxxxx> > 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@xxxxxxxxxx> > 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@xxxxxxxxxx> > 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@xxxxxxxxxxxxx> > 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@xxxxxxxxxx> > 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@xxxxxxxx/ > > 2.David Hildenbrand <david@xxxxxxxxxx> > 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@xxxxxxxxxxxxx> > 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 >
