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

[David Hildenbrand <david@redhat.com>]

On 05.03.24 21:35, Matthew Wilcox wrote:
> 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).

Would hugetlb_id below be 32bit or 64bit on 64-bit?

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

^ In the worst case, I think, nr_pages_mapped with a lot of entire 
mappings could end up matching hugetlb_id. We add a large value to 
nr_pages_mapped every time we add an entire mapping ... (not sure if 
that could currently be a problem with many entire mappings of a large 
folio)


> 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?

For the total mapcount this would do (and it would be better), but the 
layout gets a bit "sparse" on 64bit that way, which will end up being 
problematic for some other stuff I might want to put in there.

Not that we have to resolve that now, just bringing it up, that maybe we 
can do better right away :)


IIUC, we would not be able to reuse the "gap" in "nr_pages_mapped + gap 
/ hugetlb_id", essentially consuming an additional 32bit compared to 
what we do now, correct?

I was thinking of the following, assuming your example above indicated 
on64bit a hugetlb_id that is 64bit:

hugetlb folios set
* compound page
* flag in subpage 1 (like we do)
* nr_pages_mapped in subpage 1 to e.g., -2.

So to check lockless if we have a hugetlb folio
* Check if compund
* Check if the flag in subpage 1 is set
* Check if nr_pages_mapped matches

Would that still be too unreliable?

-- 
Cheers,

David / dhildenb