Re: [PATCH V2] mm/hugetlb: Make __NR_USED_SUBPAGE check conditional

[Anshuman Khandual]

On 12/3/24 13:50, Muchun Song wrote:
> 
> 
>> On Dec 3, 2024, at 15:46, Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:
>>
>>
>> On 12/3/24 08:59, Muchun Song wrote:
>>>
>>>
>>>> On Dec 3, 2024, at 10:32, Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:
>>>>
>>>> The HugeTLB order check against __NR_USED_SUBPAGE is required only when
>>>> HUGETLB_PAGE_OPTIMIZE_VMEMMAP is enabled. Hence BUG_ON() trigger should
>>>> happen only when applicable.
>>>>
>>>> Cc: Muchun Song <muchun.song@xxxxxxxxx>
>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>>> Cc: Oscar Salvador <osalvador@xxxxxxx>
>>>> Cc: linux-mm@xxxxxxxxx
>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>>> ---
>>>> This patch applies on v6.13-rc1
>>>>
>>>> Changes in V2:
>>>>
>>>> - Fixed #ifdef with CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP per Oscar
>>>>
>>>> Changes in V1:
>>>>
>>>> https://lore.kernel.org/all/20241202090728.78935-1-anshuman.khandual@xxxxxxx/
>>>>
>>>> mm/hugetlb.c | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>>> index ea2ed8e301ef..e6a5b21e3578 100644
>>>> --- a/mm/hugetlb.c
>>>> +++ b/mm/hugetlb.c
>>>> @@ -4513,11 +4513,13 @@ void __init hugetlb_add_hstate(unsigned int order)
>>>> 	struct hstate *h;
>>>> 	unsigned long i;
>>>>
>>>> - 	if (size_to_hstate(PAGE_SIZE << order)) {
>>>> + 	if (size_to_hstate(PAGE_SIZE << order))
>>>> 		return;
>>>> - 	}
>>>> +
>>>> 	BUG_ON(hugetlb_max_hstate >= HUGE_MAX_HSTATE);
>>>> +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>>>> 	BUG_ON(order < order_base_2(__NR_USED_SUBPAGE));
>>>
>>> Hi Anshuman,
>>>
>>> __NR_USED_SUBPAGE indicates how many struct pages are used to store
>>> extra metadata for a HugeTLB page. So we need to make sure the order
>>> of a HugeTLB page should be bigger than or equal to order_base_2(__NR_USED_SUBPAGE).
>>> So It is not related to HVO. I don't think the changes make sense.
>>
>> I did think about that but order_base_2(__NR_USED_SUBPAGE) actually
>> turns out to be just 2 as __NR_USED_SUBPAGE is 3. Would not HugeTLB
>> size be always greater than four base pages in reality, thus making
>> this BUG_ON() check just redundant ?
> 
> Currently, there is no architectures supporting hugetlb page smaller
> than 4 base pages. I think the smallest size is 64KB in arm64, but who
> knows whether if certain architectures supports 8KB in the future or
> we want to uses more struct pages to store metadata for increasing
> __NR_USED_SUBPAGE (e.g. change it to 17). So I tend to keep this
> BUG_ON remain to catch unexpected bugs.

Sure, fair enough.