Re: [PATCH 0/5] Remove some races around folio_test_hugetlb

[Matthew Wilcox <willy@infradead.org>]

On Tue, Mar 05, 2024 at 10:10:08AM +0100, David Hildenbrand wrote:
> > The cost of this reliability is that we now consume the word I recently
> > freed in folio->page[1].  I think this is acceptable; we've still gained
> > a completely reliable folio_test_hugetlb() (which we didn't have before
> > I started messing around with the folio dtors).  Non-hugetlb users
> > can use large_id as a pointer to something else entirely, or even as a
> > non-pointer, as long as they can guarantee it can't conflict (ie don't
> > use it as a bitfield).
> 
> That probably means that we have to always set the lowest bit to use it for
> something else, or use another bit.

Yes, that would work.

> I was wondering if
> 
> a) We could move that to another subpage. In hugetlb folios we have plenty
> of space for such things. I guess we'd have be able to detect the folio size
> without holding a reference, to make sure we can touch another subpage.

Yes, that was my concern.  I wanted to put it in page[2] with all the
other hugetlb goop, but I got to thinking about an order-1 compound
page allocated at the end of memmap and got scared.  We could make
folio_test_hugetlb() look at ->flags for the head bit, then look at
->flags_1 for the order and finally at ->hugetlb_id, but now we've looked
at three cachelines to answer a fairly frequent question.  And then what
if the folio got split between looking at ->flags and ->flags_1 and we
get a bogus folio order that makes it look OK?  We can't even look at
->flags, ->flags_1 and recheck ->flags because it might have got split,
freed and reallocated in the meantime.

> b) We could overload _nr_pages_mapped. We'd effectively have to steal one
> bit from _nr_pages_mapped to make this work.
>
> Maybe what works is using the existing mechanism (hugetlb flag), and then
> storing the pointer in __nr_pages_mapped.
> 
> So depending on the hugetlb flag, we can interpret __nr_pages_mapped either
> as the pointer or as the old variant.
> 
> Mostly only folio_large_is_mapped() would need care for now, to ignore
> _nr_pages_mapped if the hugetlb flag is set.

I don't mind that at all.  We wouldn't even need to steal a bit or use the
existing flag; we could just say that -2 means this is a hugetlb folio.
As long as it ends up at the same offset as page->mapping (because that's
always NULL or a pointer possibly with a low bit set so can't ever be a
number between -4095 and -1).

IOW:

word	page0			page1
0	flags			flags
1	lru.next		head
2	lru.prev		entire_mapcount + gap
3	mapping			nr_pages_mapped + gap / hugetlb_id
4	index			pincount + nr_pages
5	private			unused
6	mapcount+refcount	mapcount+refcount(0)
7	memcg_data		-

or on 32-bit

word	page0			page1
0	flags			flags
1	lru.next		head
2	lru.prev		entire_mapcount
3	mapping			nr_pages_mapped / hugetlb_id
4	index			pincount
5	private			unused
6	mapcount		mapcount
7	refcount		refcount
8	memcg_data		-
9+	virtual? last_cpupid? whatever

Does this fit with your plans?