Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 28 Mar 2020 03:39:20 +0300
From:   "Kirill A. Shutemov" <kirill@shutemov.name>
To:     Zi Yan <ziy@nvidia.com>
Cc:     akpm@linux-foundation.org, Andrea Arcangeli <aarcange@redhat.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH 5/7] khugepaged: Allow to collapse PTE-mapped compound
 pages
Message-ID: <20200328003920.xvkt3hp65uccsq7b@box>
References: <20200327170601.18563-1-kirill.shutemov@linux.intel.com>
 <20200327170601.18563-6-kirill.shutemov@linux.intel.com>
 <D5721ED6-774B-4CD3-8533-4BF9BDB2401E@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <D5721ED6-774B-4CD3-8533-4BF9BDB2401E@nvidia.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 27, 2020 at 02:55:06PM -0400, Zi Yan wrote:
> On 27 Mar 2020, at 13:05, Kirill A. Shutemov wrote:
> 
> > We can collapse PTE-mapped compound pages. We only need to avoid
> > handling them more than once: lock/unlock page only once if it's present
> > in the PMD range multiple times as it handled on compound level. The
> > same goes for LRU isolation and putpack.
> 
> s/putpack/putback/
> 
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  mm/khugepaged.c | 41 +++++++++++++++++++++++++++++++----------
> >  1 file changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index b47edfe57f7b..c8c2c463095c 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -515,6 +515,17 @@ void __khugepaged_exit(struct mm_struct *mm)
> >
> >  static void release_pte_page(struct page *page)
> >  {
> > +	/*
> > +	 * We need to unlock and put compound page on LRU only once.
> > +	 * The rest of the pages have to be locked and not on LRU here.
> > +	 */
> > +	VM_BUG_ON_PAGE(!PageCompound(page) &&
> > +			(!PageLocked(page) && PageLRU(page)), page);
> It only checks base pages.

That's the point.

> Add
> 
> VM_BUG_ON_PAGE(PageCompound(page) && PageLocked(page) && PageLRU(page), page);
> 
> to check for compound pages.

The compound page may be locked here if the function called for the first
time for the page and not locked after that (becouse we've unlocked it we
saw it the first time). The same with LRU.

> > +
> > +	if (!PageLocked(page))
> > +		return;
> > +
> > +	page = compound_head(page);
> >  	dec_node_page_state(page, NR_ISOLATED_ANON + page_is_file_cache(page));
> >  	unlock_page(page);
> >  	putback_lru_page(page);
> > @@ -537,6 +548,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> >  	pte_t *_pte;
> >  	int none_or_zero = 0, result = 0, referenced = 0;
> >  	bool writable = false;
> > +	LIST_HEAD(compound_pagelist);
> >
> >  	for (_pte = pte; _pte < pte+HPAGE_PMD_NR;
> >  	     _pte++, address += PAGE_SIZE) {
> > @@ -561,13 +573,23 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> >  			goto out;
> >  		}
> >
> > -		/* TODO: teach khugepaged to collapse THP mapped with pte */
> > +		VM_BUG_ON_PAGE(!PageAnon(page), page);
> > +
> >  		if (PageCompound(page)) {
> > -			result = SCAN_PAGE_COMPOUND;
> > -			goto out;
> > -		}
> > +			struct page *p;
> > +			page = compound_head(page);
> >
> > -		VM_BUG_ON_PAGE(!PageAnon(page), page);
> > +			/*
> > +			 * Check if we have dealt with the compount page
> 
> s/compount/compound/
> 

Thanks.

> > +			 * already
> > +			 */
> > +			list_for_each_entry(p, &compound_pagelist, lru) {
> > +				if (page ==  p)
> > +					break;
> > +			}
> > +			if (page ==  p)
> > +				continue;
> > +		}
> >
> >  		/*
> >  		 * We can do it before isolate_lru_page because the
> > @@ -640,6 +662,9 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> >  		    page_is_young(page) || PageReferenced(page) ||
> >  		    mmu_notifier_test_young(vma->vm_mm, address))
> >  			referenced++;
> > +
> > +		if (PageCompound(page))
> > +			list_add_tail(&page->lru, &compound_pagelist);
> >  	}
> >  	if (likely(writable)) {
> >  		if (likely(referenced)) {
> 
> Do we need a list here? There should be at most one compound page we will see here, right?

Um? It's outside the pte loop. We get here once per PMD range.

'page' argument to trace_mm_collapse_huge_page_isolate() is misleading:
it's just the last page handled in the loop.

> 
> If a compound page is seen here, can we bail out the loop early? I guess not,
> because we can a partially mapped compound page at the beginning or the end of a VMA, right?
> 
> > @@ -1185,11 +1210,7 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
> >  			goto out_unmap;
> >  		}
> >
> > -		/* TODO: teach khugepaged to collapse THP mapped with pte */
> > -		if (PageCompound(page)) {
> > -			result = SCAN_PAGE_COMPOUND;
> > -			goto out_unmap;
> > -		}
> > +		page = compound_head(page);
> >
> >  		/*
> >  		 * Record which node the original page is from and save this
> > -- 
> > 2.26.0
> 
> 
> —
> Best Regards,
> Yan Zi


-- 
 Kirill A. Shutemov