Re: [PATCH] mm/madvise: fix madvise_pageout for private file mappings

[Mark Hemment]

On Wed, 9 Nov 2022 at 05:19, Pavankumar Kondeti
<quic_pkondeti@xxxxxxxxxxx> 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@xxxxxxxxxxx>

Only scanned review the patch; the logic looks good (as does the
reasoning) but a couple of minor comments;


> ---
>  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.


>                 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.


> +       /*
> +        * 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
>
>

Cheers,
Mark