Re: [PATCH v1 2/2] mm: mTHP stats for pagecache folio allocations

[Ryan Roberts <ryan.roberts@arm.com>]

On 17/07/2024 09:44, David Hildenbrand wrote:
>>>> I guess the real supported orders are:
>>>>
>>>>     anon:
>>>>       min order: 2
>>>>       max order: PMD_ORDER
>>>>     anon-shmem:
>>>>       min order: 1
>>>>       max order: MAX_PAGECACHE_ORDER
>>>>     tmpfs-shmem:
>>>>       min order: PMD_ORDER <= 11 ? PMD_ORDER : NONE
>>>>       max order: PMD_ORDER <= 11 ? PMD_ORDER : NONE
>>>>     file:
>>>>       min order: 1
>>>>       max order: MAX_PAGECACHE_ORDER
>>>
>>> That's my understanding. But not sure about anon-shmem really supporting
>>> order-1, maybe we do.
>>
>> Oh, I thought we only had the restriction for anon folios now (due to deferred
>> split queue), so assumed it would just work. With Gavin's
>> THP_ORDERS_ALL_FILE_DEFAULT change, that certainly implies that shmem must
>> support order-1. If it doesn't then we we might want to tidy that further.
>>
>> Baolin, perhaps you can confirm either way?
> 
> Currently there would not have been a way to enable it, right? (maybe I'm wrong)

__thp_vma_allowable_orders() doesn't do anything special for shmem if TVA_IN_PF
is set, so I guess it could concievably return order-1 in that path. Not sure if
it ever gets called that way for shmem though - I don't think so. But agree that
shmem_allowable_huge_orders() will not currently return order-1.

> 
>>
>>>
>>>>
>>>> But today, controls and stats are exposed for:
>>>>
>>>>     anon:
>>>>       min order: 2
>>>>       max order: PMD_ORDER
>>>>     anon-shmem:
>>>>       min order: 2
>>>>       max order: PMD_ORDER
>>>>     tmpfs-shmem:
>>>>       min order: PMD_ORDER
>>>>       max order: PMD_ORDER
>>>>     file:
>>>>       min order: Nothing yet (this patch proposes 1)
>>>>       max order: Nothing yet (this patch proposes MAX_PAGECACHE_ORDER)
>>>>
>>>> So I think there is definitely a bug for shmem where the minimum order control
>>>> should be order-1 but its currently order-2.
>>>
>>> Maybe, did not play with that yet. Likely order-1 will work. (although probably
>>> of questionable use :) )
>>
>> You might have to expand on why its of "questionable use". I'd assume it has the
>> same amount of value as using order-1 for regular page cache pages? i.e. half
>> the number of objects to manage for the same amount of memory.
> 
> order-1 was recently added for the pagecache to get some device setups running
> (IIRC, where we cannot use order-0, because device blocksize > PAGE_SIZE).
> 
> You might be right about "half the number of objects", but likely just going for
> order-2, order-3, order-4 ... for shmem might be even better. And simply falling
> back to order-0 when you cannot get the larger orders.

Sure, but then you're into the territory of baking in policy. Remember that
originally I was only interested in 64K but the concensus was to expose all the
sizes. Same argument applies to 8K; expose it and let others decide policy.

> 
> I could have sworn you mentioned something like that in your "configurable
> orders for pagecache" RFC that I only briefly skimmed so far :P

I'm exposing the 8K control for pagecache in that series.

> 
> ... only enabling "order-1" and none of the other orders for shmem sounds rather
> "interesting".
> 
> But yeah, maybe there is valid use for it, so I'm all for allowing it if it can
> be done.
> 
>>
>>>
>>>>
>>>> I also wonder about PUD-order for DAX? We don't currently have a stat/control.
>>>> If we wanted to add it in future, if we take the "expose all stats/controls for
>>>> all orders" approach, we would end up extending all the way to PUD-order and
>>>> all
>>>> the orders between PMD and PUD would be dummy for all memory types. That really
>>>> starts to feel odd, so I still favour only populating what's really supported.
>>>
>>> I would go further and say that calling the fsdax thing a THP is borderline
>>> wrong and we should not expose any new toggles for it that way.
>>>
>>> It really behaves much more like hugetlb folios that can be PTE-mapped ... we
>>> cannot split these things, and they are not allocated from the buddy. So I
>>> wouldn't worry about fsdax for now.
>>>
>>> fsdax support for compound pages (now large folios) probably never should have
>>> been glued to any THP toggle.
>>
>> Yeah fair enough. I wasn't really arguing for adding any dax controls; I was
>> just trying to think of examples as to why adding dummy controls might be a bad
>> idea.
> 
> Yes.
> 
>>>
>>>>
>>>> I propose to fix shmem (extend down to 1, stop at MAX_PAGECACHE_ORDER) and
>>>> continue with the approach of "indicating only what really exists" for v2.
>>>>
>>>> Shout if you disagree.
>>>
>>> Makes sense.
>>
>> Excellent. I posted v2, which has these changes, yesterday afternoon. :)
> 
> Yes, still digging through mails ... in-between having roughly 1000 meetings a
> day :)

No problem. You're in-demand. I can wait. :)