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

[Matthew Wilcox <willy@infradead.org>]

On Thu, Mar 07, 2024 at 10:20:15AM +0100, David Hildenbrand wrote:
> > > > 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
> > 
> > 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?
> 
> _entire_mapcount is (still) used for hugetlb folios.

Oh, duh, of course it is.  I thought we used page[0].mapcount for them,
but we don't and shouldn't.  I suppose we could use a magic value for
page[0].mapcount to indicate hugetlb, but that'd make page_mapcount()
more complex.

> With the total mapcount in place, I was thinking about renaming it to
> "_pmd_mapcount" and stop using it for hugetlb folios, just like we'd not be
> using _nr_pages_mapped for hugetlb folios.
> 
> [I also thought about moving the _pmd_mapcount to another subpage, where
> we'd also have a _pud_mapcount in the future; but again, stuff for the
> future]
> 
> Until then, wouldn't _hugetlb_id be problematic here? [I could move
> _entire_mapcount/_pmd_mapcount later I guess]

New idea then, how about simply:

                        unsigned long _flags_1;
                        unsigned long _head_1;
-                       unsigned long _folio_avail;
+                       unsigned long _hugetlb_id;

We have to check the various other users of struct page to see what we
might conflict with.

slab:
                                struct list_head slab_list;
                                        void *freelist;         /* first free object */
                struct rcu_head rcu_head;

ptdesc:
                struct rcu_head pt_rcu_head;
                struct list_head pt_list;
                        pgtable_t pmd_huge_pte;

So it's always used as a pointer (or a pointer in disguise).  That means
that either using a pointer to something hugetlb-related or a value like
(void *)-2 will be unambiguous.  It'll just be something to document in
each of the types split from struct page.