Re: [PATCH] mm, compaction: Skip all pinned pages during scan

[Khalid Aziz]

Hi Matthew,

Thanks for the review.

On 5/12/23 09:53, Matthew Wilcox wrote:
> On Thu, May 11, 2023 at 10:55:16AM -0600, Khalid Aziz wrote:
> > +/*
> > + * Check if this base page should be skipped from isolation because
> > + * it is pinned. This function is called for regular pages only, and not
> > + * for THP or hugetlbfs pages. This code is inspired by similar code
> > + * in migrate_vma_check_page(), can_split_folio() and
> > + * folio_migrate_mapping()
> > + */
> > +static inline bool is_pinned_page(struct page *page)
>
> ... yet another reminder this file hasn't been converted to folios :-(
> This part is particularly hard because we don't have a refcount on the
> page yet, so it may be allocated or freed while we're looking at it
> which means we can't use folios _here_ because the Tail flag may get
> set which would cause the folio code to drop BUGs all over us.
>
> > +{
> > +	unsigned long extra_refs;
> > +
> > +	/* anonymous page can have extra ref from page cache */
> > +	if (page_mapping(page))
>
> We already did the work of calling page_mapping() in the caller.
> Probably best to pass it in here.

That makes sense. I will change that.

> > +		extra_refs = 1 + page_has_private(page);
>
> page_has_private() is wrong.  That's for determining if we need to call
> the release function.  Filesystems don't increment the refcount when
> they set PG_private_2.  This should just be PagePrivate().

I will fix that.

> > +	else
> > +		extra_refs = PageSwapCache(page) ? 1 : 0;
> > +
> > +	/*
> > +	 * This is an admittedly racy check but good enough to determine
> > +	 * if a page should be isolated
> > +	 */
> > +	if ((page_count(page) - extra_refs) > page_mapcount(page))
>
> page_count() includes a hidden call to compound_head(); you probably
> meant page_ref_count() here.

You are right.

> >   		/*
> > -		 * Migration will fail if an anonymous page is pinned in memory,
> > -		 * so avoid taking lru_lock and isolating it unnecessarily in an
> > -		 * admittedly racy check.
> > +		 * Migration will fail if a page is pinned in memory,
> > +		 * so avoid taking lru_lock and isolating it unnecessarily
> >   		 */
> >   		mapping = page_mapping(page);
> > -		if (!mapping && (page_count(page) - 1) > total_mapcount(page))
> > +		if (is_pinned_page(page))
>
> "pinned" now has two meanings when applied to pages, alas.  Better to
> say "If there are extra references to this page beyond those from the
> page/swap cache and page tables".
>
> So it's probably also unwise to call it is_pinned_page().  Maybe
>
> 		if (page_extra_refcounts(page)) ?

I like that better. I will make these modifications.

Thanks,
Khalid