Re: [PATCH] mm: shrink skip folio mapped by an exiting task

[zhiguojiang]

在 2024/7/8 11:54, Barry Song 写道:
> [Some people who received this message don't often get email from baohua@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On Mon, Jul 8, 2024 at 3:40 PM zhiguojiang <justinjiang@xxxxxxxx> wrote:
> > 在 2024/6/30 17:35, Barry Song 写道:
> > > [Some people who received this message don't often get email from baohua@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > >
> > > On Thu, Feb 22, 2024 at 12:49 AM Zhiguo Jiang <justinjiang@xxxxxxxx> wrote:
> > > This is clearly version 3, as you previously sent version 2, correct?
> > Hi Barry,
> >
> > Yes, patchs are as follows:
> > v3:
> > https://lore.kernel.org/linux-mm/20240221114904.1849-1-justinjiang@xxxxxxxx/
> > v2:
> > https://lore.kernel.org/linux-mm/20240202012251.1aa5afbfdf2f8b3a862bced3@xxxxxxxxxxxxxxxxxxxx/
> > v1:
> > https://lore.kernel.org/linux-mm/20240124124308.461-1-justinjiang@xxxxxxxx/
> > > > If an anon folio reclaimed by shrink_inactive_list is mapped by an
> > > > exiting task, this anon folio will be firstly swaped-out into
> > > > swapspace in shrink flow and then this swap folio is freed in task
> > > > exit flow. But if this folio mapped by an exiting task can skip
> > > > shrink and be freed directly in task exiting flow, which will save
> > > > swap-out time and alleviate the load of the tasks exiting process.
> > > > The file folio is also similar.
> > > Could you please describe the specific impact on users, including user
> > > experience and power consumption? How serious is this problem?
> > 1.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.
> > 2.Launching large memory app (for example, starting the camera) in multiple
> > backend scenes may result in the high cpu load of exiting processes.
> > > > And when system is low memory, it more likely to occur, because more
> > > > backend applidatuions will be killed.
> > > Applications?
> > Yes.
> > > > This patch can alleviate the load of the tasks exiting process.
> > > 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.
> > >
> > > > Signed-off-by: Zhiguo Jiang <justinjiang@xxxxxxxx>
> > > > ---
> > > >    mm/rmap.c | 7 +++++++
> > > >    1 file changed, 7 insertions(+)
> > > >    mode change 100644 => 100755 mm/rmap.c
> > > You changed the file mode to 755, which is incorrect.
> > >
> > > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > > index 3746a5531018..146e5f4ec069
> > > > --- a/mm/rmap.c
> > > > +++ b/mm/rmap.c
> > > > @@ -840,6 +840,13 @@ static bool folio_referenced_one(struct folio *folio,
> > > >           int referenced = 0;
> > > >           unsigned long start = address, ptes = 0;
> > > >
> > > > +       /* Skip this folio if it's mapped by an exiting task */
> > > > +       if (unlikely(!atomic_read(&vma->vm_mm->mm_users)) ||
> > > > +               unlikely(test_bit(MMF_OOM_SKIP, &vma->vm_mm->flags))) {
> > > > +               pra->referenced = -1;
> > > Why use -1? Is this meant to simulate lock contention to keep the folio without
> > > activating it?
> > >
> > >           /* rmap lock contention: rotate */
> > >           if (referenced_ptes == -1)
> > >                   return FOLIOREF_KEEP;
> > >
> > > 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.
> > 1.The skiped folios are FOLIOREF_KEEP and added into inactive lru, beacase
> > the folios will be freed soon in the exiting process flow.
> This requires an explicit comment in the code. The expression
> referenced_ptes == -1
> implies "rmap lock contention: rotate", but it appears that this is
> not the case here. You
> should either update the original comment to reflect the current logic
> or use a different
> value.
Ok, this has been updated in patch v5.
https://lore.kernel.org/linux-mm/20240708090413.888-1-justinjiang@xxxxxxxx/
Thanks
> > 2.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/
> >
> > Thanks
> > Zhiguo
> > > > +               return false;
> > > > +       }
> > > > +
> > > >           while (page_vma_mapped_walk(&pvmw)) {
> > > >                   address = pvmw.address;
> > > >
> > > > --
> > > > 2.39.0
> > > Thanks
> > > Barry