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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGsJ_4wik+oagod+WKJviavabqKQTDDWxPxUQJ1kS1JSPpA2Qw@mail.gmail.com>
Date: Mon, 8 Jul 2024 15:54:06 +1200
From: Barry Song <baohua@...nel.org>
To: zhiguojiang <justinjiang@...o.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, opensource.kernel@...o.com
Subject: Re: [PATCH] mm: shrink skip folio mapped by an exiting task

On Mon, Jul 8, 2024 at 3:40 PM zhiguojiang <justinjiang@...o.com> wrote:
>
>
>
> 在 2024/6/30 17:35, Barry Song 写道:
> > [Some people who received this message don't often get email from baohua@...nel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > On Thu, Feb 22, 2024 at 12:49 AM Zhiguo Jiang <justinjiang@...o.com> 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@vivo.com/
> v2:
> https://lore.kernel.org/linux-mm/20240202012251.1aa5afbfdf2f8b3a862bced3@linux-foundation.org/
> v1:
> https://lore.kernel.org/linux-mm/20240124124308.461-1-justinjiang@vivo.com/
> >
> >> 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@...o.com>
> >> ---
> >>   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.

> 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@vivo.com/
>
> Thanks
> Zhiguo
> >
> >
> >> +               return false;
> >> +       }
> >> +
> >>          while (page_vma_mapped_walk(&pvmw)) {
> >>                  address = pvmw.address;
> >>
> >> --
> >> 2.39.0
> >>
> > Thanks
> > Barry
>