Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Dec 2022 19:47:26 +0530
From:   Pavan Kondeti <quic_pkondeti@quicinc.com>
To:     Mark Hemment <markhemm@googlemail.com>
CC:     Pavankumar Kondeti <quic_pkondeti@quicinc.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        Suren Baghdasaryan <surenb@google.com>,
        Minchan Kim <minchan@kernel.org>,
        Charan Teja Kalla <quic_charante@quicinc.com>,
        Prakash Gupta <quic_guptap@quicinc.com>,
        Divyanand Rangu <quic_drangu@quicinc.com>
Subject: Re: [PATCH] mm/madvise: fix madvise_pageout for private file mappings
Message-ID: <20221201141726.GA18487@hu-pkondeti-hyd.qualcomm.com>
References: <1667971116-12900-1-git-send-email-quic_pkondeti@quicinc.com>
 <CANe_+Uidg=YuZG71VF7YnRhBtUHb-CGRMN45RGgp2PMn+SXGkg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CANe_+Uidg=YuZG71VF7YnRhBtUHb-CGRMN45RGgp2PMn+SXGkg@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

Hi Mark,

On Thu, Dec 01, 2022 at 01:46:36PM +0000, Mark Hemment wrote:
> On Wed, 9 Nov 2022 at 05:19, Pavankumar Kondeti
> <quic_pkondeti@quicinc.com> wrote:
> >
> > When MADV_PAGEOUT is called on a private file mapping VMA region,
> > we bail out early if the process is neither owner nor write capable
> > of the file. However, this VMA may have both private/shared clean
> > pages and private dirty pages. The opportunity of paging out the
> > private dirty pages (Anon pages) is missed. Fix this by caching
> > the file access check and use it later along with PageAnon() during
> > page walk.
> >
> > We observe ~10% improvement in zram usage, thus leaving more available
> > memory on a 4GB RAM system running Android.
> >
> > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com>
> 
> Only scanned review the patch; the logic looks good (as does the
> reasoning) but a couple of minor comments;
> 
Thanks for the review and nice suggestions on how the patch can be improved.

> 
> > ---
> >  mm/madvise.c | 30 +++++++++++++++++++++++-------
> >  1 file changed, 23 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index c7105ec..b6b88e2 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -40,6 +40,7 @@
> >  struct madvise_walk_private {
> >         struct mmu_gather *tlb;
> >         bool pageout;
> > +       bool can_pageout_file;
> >  };
> >
> >  /*
> > @@ -328,6 +329,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> >         struct madvise_walk_private *private = walk->private;
> >         struct mmu_gather *tlb = private->tlb;
> >         bool pageout = private->pageout;
> > +       bool pageout_anon_only = pageout && !private->can_pageout_file;
> >         struct mm_struct *mm = tlb->mm;
> >         struct vm_area_struct *vma = walk->vma;
> >         pte_t *orig_pte, *pte, ptent;
> > @@ -364,6 +366,9 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> >                 if (page_mapcount(page) != 1)
> >                         goto huge_unlock;
> >
> > +               if (pageout_anon_only && !PageAnon(page))
> > +                       goto huge_unlock;
> > +
> >                 if (next - addr != HPAGE_PMD_SIZE) {
> >                         int err;
> >
> > @@ -432,6 +437,8 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> >                 if (PageTransCompound(page)) {
> >                         if (page_mapcount(page) != 1)
> >                                 break;
> > +                       if (pageout_anon_only && !PageAnon(page))
> > +                               break;
> >                         get_page(page);
> >                         if (!trylock_page(page)) {
> >                                 put_page(page);
> > @@ -459,6 +466,9 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> >                 if (!PageLRU(page) || page_mapcount(page) != 1)
> >                         continue;
> >
> > +               if (pageout_anon_only && !PageAnon(page))
> > +                       continue;
> > +
> 
> The added PageAnon()s probably do not have a measurable performance
> impact, but not ideal when walking a large anonymous mapping (as
> '->can_pageout_file' is zero for anon mappings).
> Could the code be re-structured so that PageAnon() is only tested when
> filtering is needed? Say;
>     if (pageout_anon_only_filter && !PageAnon(page)) {
>         continue;
>     }
> where 'pageout_anon_only_filter' is only set for a private named
> mapping when do not have write perms on backing object.  It would not
> be set for anon mappings.
> 
Understood. Like you suggested, PageAnon() check can be eliminated for
an anon mapping. will make the necessary changes.

> 
> >                 VM_BUG_ON_PAGE(PageTransCompound(page), page);
> >
> >                 if (pte_young(ptent)) {
> > @@ -541,11 +551,13 @@ static long madvise_cold(struct vm_area_struct *vma,
> >
> >  static void madvise_pageout_page_range(struct mmu_gather *tlb,
> >                              struct vm_area_struct *vma,
> > -                            unsigned long addr, unsigned long end)
> > +                            unsigned long addr, unsigned long end,
> > +                            bool can_pageout_file)
> >  {
> >         struct madvise_walk_private walk_private = {
> >                 .pageout = true,
> >                 .tlb = tlb,
> > +               .can_pageout_file = can_pageout_file,
> >         };
> >
> >         tlb_start_vma(tlb, vma);
> > @@ -553,10 +565,8 @@ static void madvise_pageout_page_range(struct mmu_gather *tlb,
> >         tlb_end_vma(tlb, vma);
> >  }
> >
> > -static inline bool can_do_pageout(struct vm_area_struct *vma)
> > +static inline bool can_do_file_pageout(struct vm_area_struct *vma)
> >  {
> > -       if (vma_is_anonymous(vma))
> > -               return true;
> >         if (!vma->vm_file)
> >                 return false;
> >         /*
> > @@ -576,17 +586,23 @@ static long madvise_pageout(struct vm_area_struct *vma,
> >  {
> >         struct mm_struct *mm = vma->vm_mm;
> >         struct mmu_gather tlb;
> > +       bool can_pageout_file;
> >
> >         *prev = vma;
> >         if (!can_madv_lru_vma(vma))
> >                 return -EINVAL;
> >
> > -       if (!can_do_pageout(vma))
> > -               return 0;
> 
> The removal of this test results in a process, which cannot get write
> perms for a shared named mapping, performing a 'walk'.  As such a
> mapping cannot have anon pages, this walk will be a no-op.  Not sure
> why a well-behaved program would do a MADV_PAGEOUT on such a mapping,
> but if one does this could be considered a (minor performance)
> regression. As madvise_pageout() can easily filter this case, might be
> worth adding a check.
> 

Got it. we can take care of this edge case by rejecting shared mappings i.e
!!(vma->vm_flags & VM_MAYSHARE) == 1 where the process has no write
permission.

> 
> > +       /*
> > +        * If the VMA belongs to a private file mapping, there can be private
> > +        * dirty pages which can be paged out if even this process is neither
> > +        * owner nor write capable of the file. Cache the file access check
> > +        * here and use it later during page walk.
> > +        */
> > +       can_pageout_file = can_do_file_pageout(vma);
> >
> >         lru_add_drain();
> >         tlb_gather_mmu(&tlb, mm);
> > -       madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
> > +       madvise_pageout_page_range(&tlb, vma, start_addr, end_addr, can_pageout_file);
> >         tlb_finish_mmu(&tlb);
> >
> >         return 0;
> > --
> > 2.7.4
> >
> >
> 
Thanks,
Pavan