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

[Anshuman Khandual]

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 ?

> 
> Thanks.
> 
>> +#endif
>> 	h = &hstates[hugetlb_max_hstate++];
>> 	__mutex_init(&h->resize_lock, "resize mutex", &h->resize_key);
>> 	h->order = order;
>> -- 
>> 2.30.2
>>
>