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:   Wed, 30 Nov 2022 15:17:39 -0800
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Pavankumar Kondeti <quic_pkondeti@quicinc.com>
Cc:     <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: <20221130151739.a7771037aa83ff536ac2edd5@linux-foundation.org>
In-Reply-To: <1667971116-12900-1-git-send-email-quic_pkondeti@quicinc.com>
References: <1667971116-12900-1-git-send-email-quic_pkondeti@quicinc.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk


On Wed, 9 Nov 2022 10:48:36 +0530 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.
> 

Could we please have some reviewer input on this?

Thanks.

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