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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 12 May 2023 11:04:03 -0600
From:   Khalid Aziz <khalid.aziz@...cle.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Steve Sistare <steven.sistare@...cle.com>
Subject: Re: [PATCH] mm, compaction: Skip all pinned pages during scan

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