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

[Matthew Wilcox <willy@infradead.org>]

On Wed, Mar 06, 2024 at 04:18:45PM +0100, David Hildenbrand wrote:
> 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?

64-bit, so when it's reused by page->mapping after a split, it isn't
ambiguous.

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

My understanding was that nr_pages_mapped was incremented by one for
each page which has a non-zero mapcount.  It was also incremented by
ENTIRELY_MAPPED the first time that we increment ->entire_mapcount.
As such, I don't think entire_mapcount can get the top bit set.

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

How about this layout?

@@ -350,8 +350,13 @@ struct folio {
                        unsigned long _head_1;
                        unsigned long _folio_avail;
        /* public: */
-                       atomic_t _entire_mapcount;
-                       atomic_t _nr_pages_mapped;
+                       union {
+                               unsigned long _hugetlb_id;
+                               struct {
+                                       atomic_t _entire_mapcount;
+                                       atomic_t _nr_pages_mapped;
+                               };
+                       };
                        atomic_t _pincount;
 #ifdef CONFIG_64BIT
                        unsigned int _folio_nr_pages;

That keeps _folio_avail as, well, available.  It puts _hugetlb_id in
the same bits as ->mapping.  It continues to leave ->private unused
on 64-bit.  I think this does everything we want?