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

[David Hildenbrand <david@redhat.com>]

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

Right, I misremembered!

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

_entire_mapcount is (still) used for hugetlb folios.

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]

-- 
Cheers,

David / dhildenb