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

[Muchun Song <muchun.song@linux.dev>]

> On Dec 3, 2024, at 15:46, Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> 
> On 12/3/24 08:59, Muchun Song wrote:
>> 
>> 
>>> On Dec 3, 2024, at 10:32, Anshuman Khandual <anshuman.khandual@arm.com> 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@linux.dev>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Oscar Salvador <osalvador@suse.de>
>>> Cc: linux-mm@kvack.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> 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@arm.com/
>>> 
>>> 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.

Thanks.

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