Re: + mm-hugetlb-make-alloc_gigantic_page-available-for-general-use.patch added to -mm tree

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 10/15/2019 05:06 PM, Michal Hocko wrote:
> On Tue 15-10-19 04:24:33, Matthew Wilcox wrote:
>> On Tue, Oct 15, 2019 at 03:00:49PM +0530, Anshuman Khandual wrote:
>>> On 10/15/2019 01:59 AM, Matthew Wilcox wrote:
>>>> On Mon, Oct 14, 2019 at 02:17:30PM +0200, Michal Hocko wrote:
>>>>> On Fri 11-10-19 13:29:32, Andrew Morton wrote:
>>>>>> alloc_gigantic_page() implements an allocation method where it scans over
>>>>>> various zones looking for a large contiguous memory block which could not
>>>>>> have been allocated through the buddy allocator.  A subsequent patch which
>>>>>> tests arch page table helpers needs such a method to allocate PUD_SIZE
>>>>>> sized memory block.  In the future such methods might have other use cases
>>>>>> as well.  So alloc_gigantic_page() has been split carving out actual
>>>>>> memory allocation method and made available via new
>>>>>> alloc_gigantic_page_order().
>>>>>
>>>>> You are exporting a helper used for hugetlb internally. Is this really
>>>>> what is needed? I haven't followed this patchset but don't you simply
>>>>> need a generic 1GB allocator? If yes then you should be looking at
>>>>> alloc_contig_range.
>>>>
>>>> He actually doesn't need to allocate any memory at all.  All he needs is
>>>> the address of a valid contiguous PUD-sized chunk of memory.
>>>>
>>>
>>> We had already discussed about the benefits of allocated memory over
>>> synthetic pfn potentially derived from a kernel text symbol. More
>>> over we are not adding any new helper or new code for this purpose,
>>> but instead just trying to reuse code which is already present.
>>
>> Yes, and I'm pretty sure you're just wrong.  But I don't know that you're
>> wrong for all architectures.  Re-reading that, I'm still not sure you
>> understood what I was suggesting, so I'll say it again differently.
>>
>> Look up a kernel symbol, something like kernel_init().  This will
>> have a virtual address upon which it is safe to call virt_to_pfn().
>> Let's presume it's in PFN 0x12345678.  Use 0x12345678 as the PFN for
>> your PTE level tests.
>>
>> Then clear the bottom (eg) 9 bits from it and use 0x1234400 for your PMD
>> level tests.  Then clear the bottom 18 bits from it and use 0x12300000
>> for your PUD level tests.
>>
>> I don't think it matters whether there's physical memory at PFN 0x12300000
>> or not.  The various p?d_* functions should work as long as the PFN is
>> in some plausible range.
>>
>> I gave up arguing because you seemed uninterested in this approach,
>> but now that Michal is pointing out that your approach is all wrong,
>> maybe you'll listen.
> 
> Just for the record. I didn't really get to read the patch 2 in this
> series. Matthew is right that I disagree with the current state of the
> "large pages" allocator. If my concerns get resolved then I do not mind
> having it regardless of what patch 2 ends up doing and whether it uses
> it or not.

Sure, will then remove the first patch from here and post it separately
after accommodating all suggestions in the meantime.

> 
> On the other hand, having a testing code that really requires a lot of
> memory to allocate to test page table walkers seems indeed a bit too
> strong of an assumption. Especially when there are ways around that as

I agree it is a strong assumption even though we have fallback of not
running tests when required memory size could not be allocated. There were
some reasons around it, which I am still trying to figure out from our
previous discussions.

> Matthew is suggesting so I would really listen to his suggestions.

Sure, lets evaluate it once more (on the other thread). Current proposal
skips related tests when required size memory could not be allocated, if
we can do without allocating memory, it definitely increases test coverage
on many platforms.