Re: [PATCH v1 10/15] mm/page-flags: reuse PG_slab as PG_anon_exclusive for PageAnon() pages

[David Hildenbrand]

Hi,

On 09.03.22 16:47, Matthew Wilcox wrote:
> On Tue, Mar 08, 2022 at 03:14:32PM +0100, David Hildenbrand wrote:
>> The basic question we would like to have a reliable and efficient answer
>> to is: is this anonymous page exclusive to a single process or might it
>> be shared?
> 
> Is this supposed to be for PAGE_SIZE pages as well, or is it only used
> on pages > PAGE_SIZE?

As documented, simple/ordinary PAGE_SIZE pages as well (unfortunately ,
otherwise we'd have more space :) ).

> 
>> In an ideal world, we'd have a spare pageflag. Unfortunately, pageflags
>> don't grow on trees, so we have to get a little creative for the time
>> being.
> 
> This feels a little _too_ creative to me.  There's now an implicit

It's making the semantics of PG_slab depend on another bit in the head
page. I agree, it's not perfect, but it's not *too* crazy. As raised in
the cover letter, not proud of this, but I didn't really find an
alternative for the time being.

> requirement that SL[AOU]B doesn't use the bottom two bits of

I think only the last bit (0x1)

> ->slab_cache, which is probably OK but would need to be documented.

We'd already have false positive PageAnon() if that wasn't the case. At
least in stable_page_flags() would already indicate something wrong I
think (KPF_ANON). We'd need !PageSlab() checks at a couple of places
already if I'm not wrong.

I had a comment in there, but after the PageSlab cleanups came in, I
dropped it because my assumption was that actually nobody is really
allowed to use the lowest mapping bit for something else. We can
document that, of course.

So at least in that regard, I think this is fine.

> 
> I have plans to get rid of PageError and PagePrivate, but those are going
> to be too late for you.  I don't think mappedtodisk has meaning for anon
> pages, even if they're in the swapcache.  It would need PG_has_hwpoisoned

Are you sure it's not used if the page is in the swapcache? I have no
detailed knowledge how file-back swap targets are handled in that
regard. So fs experience would be highly appreciated :)

> to shift to another bit ... but almost any bit will do for has_hwpoisoned.
> Or have I overlooked something?

Good question, I assume we could use another bit that's not already
defined/check on subpages of a compound page.


Overloading PG_reserved would be an alternative, however, that flag can
also indicate that the remainder of the memmap might be mostly garbage,
so it's not that good of a fit IMHO.

> 
>> @@ -920,6 +976,70 @@ extern bool is_free_buddy_page(struct page *page);
>>  
>>  __PAGEFLAG(Isolated, isolated, PF_ANY);
>>  
>> +static __always_inline bool folio_test_slab(struct folio *folio)
>> +{
>> +	return !folio_test_anon(folio) &&
>> +	       test_bit(PG_slab, folio_flags(folio, FOLIO_PF_NO_TAIL));
>> +}
>> +
>> +static __always_inline int PageSlab(struct page *page)
>> +{
>> +	return !PageAnon(page) &&
>> +		test_bit(PG_slab, &PF_NO_TAIL(page, 0)->flags);
>> +}
> 
> In case we do end up using this, this would be better implemented as
> 
> static __always_inline int PageSlab(struct page *page)
> {
> 	return folio_test_slab(page_folio(page));
> }
> 
> since PageAnon already has a page_folio() call embedded in it.

Agreed, I mainly copied the stubs and extended them.

> 
>> +static __always_inline void __SetPageSlab(struct page *page)
>> +{
>> +	VM_BUG_ON_PGFLAGS(PageAnon(page), page);
>> +	__set_bit(PG_slab, &PF_NO_TAIL(page, 1)->flags);
>> +}
> 
> There's only one caller of __SetPageSlab() left, in kfence.  And that
> code looks ... weird.
> 
>         for (i = 0; i < KFENCE_POOL_SIZE / PAGE_SIZE; i++) {
>                 if (!i || (i % 2))
>                         continue;
> 
>                 /* Verify we do not have a compound head page. */
>                 if (WARN_ON(compound_head(&pages[i]) != &pages[i]))
>                         goto err;
> 
>                 __SetPageSlab(&pages[i]);
> 
> I think the author probably intended WARN_ON(PageCompound(page)) because
> they're actually verifying that it's not a tail page, rather than head
> page.

It's certainly a head-scratcher.

> 
>> +static __always_inline void __ClearPageSlab(struct page *page)
>> +{
>> +	VM_BUG_ON_PGFLAGS(PageAnon(page), page);
>> +	__clear_bit(PG_slab, &PF_NO_TAIL(page, 1)->flags);
>> +}
> 
> There are no remaining callers of __ClearPageSlab().  yay.
> 

Indeed, nice.

Thanks!

-- 
Thanks,

David / dhildenb