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:   Fri, 12 May 2023 16:53:02 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Khalid Aziz <khalid.aziz@oracle.com>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Steve Sistare <steven.sistare@oracle.com>
Subject: Re: [PATCH] mm, compaction: Skip all pinned pages during scan
Message-ID: <ZF5g3p+FFc4Gkw69@casper.infradead.org>
References: <20230511165516.77957-1-khalid.aziz@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230511165516.77957-1-khalid.aziz@oracle.com>
Precedence: bulk

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.

> +		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().

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

>  		/*
> -		 * 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)) ?