Re: [PATCH 1/2] mm, pageblock: make sure pageblock won't exceed mem_sectioin

[David Hildenbrand <david@redhat.com>]

On 06.12.18 10:21, Wei Yang wrote:
> On Thu, Dec 06, 2018 at 10:00:05AM +0100, David Hildenbrand wrote:
>> On 05.12.18 23:31, Wei Yang wrote:
>>> On Wed, Dec 05, 2018 at 03:37:33PM +0000, Mel Gorman wrote:
>>>> On Wed, Dec 05, 2018 at 12:08:20PM +0000, Wei Yang wrote:
>>>>> On Wed, Dec 05, 2018 at 11:15:13AM +0000, Mel Gorman wrote:
>>>>>> On Wed, Dec 05, 2018 at 05:19:04PM +0800, Wei Yang wrote:
>>>>>>> When SPARSEMEM is used, there is an indication that pageblock is not
>>>>>>> allowed to exceed one mem_section. Current code doesn't have this
>>>>>>> constrain explicitly.
>>>>>>>
>>>>>>> This patch adds this to make sure it won't.
>>>>>>>
>>>>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>>>>>
>>>>>> Is this even possible? This would imply that the section size is smaller
>>>>>> than max order which would be quite a crazy selection for a sparesemem
>>>>>> section size. A lot of assumptions on the validity of PFNs within a
>>>>>> max-order boundary would be broken with such a section size. I'd be
>>>>>> surprised if such a setup could even boot, let alone run.
>>>>>
>>>>> pageblock_order has two definitions.
>>>>>
>>>>>     #define pageblock_order        HUGETLB_PAGE_ORDER
>>>>>
>>>>>     #define pageblock_order        (MAX_ORDER-1)
>>>>>
>>>>> If CONFIG_HUGETLB_PAGE is not enabled, pageblock_order is related to
>>>>> MAX_ORDER, which ensures it is smaller than section size.
>>>>>
>>>>> If CONFIG_HUGETLB_PAGE is enabled, pageblock_order is not related to
>>>>> MAX_ORDER. I don't see HUGETLB_PAGE_ORDER is ensured to be less than
>>>>> section size. Maybe I missed it?
>>>>>
>>>>
>>>> HUGETLB_PAGE_ORDER is less than MAX_ORDER on the basis that normal huge
>>>> pages (not gigantic) pages are served from the buddy allocator which is
>>>> limited by MAX_ORDER.
>>>>
>>>
>>> Maybe I am lost here, I got one possible definition on x86.
>>>
>>> #define pageblock_order		HUGETLB_PAGE_ORDER
>>> #define HUGETLB_PAGE_ORDER	(HPAGE_SHIFT - PAGE_SHIFT)
>>> #define HPAGE_SHIFT		PMD_SHIFT
>>> #define PMD_SHIFT	PUD_SHIFT
>>
>> PMD_SHIFT is usually 21
>>
>> arch/x86/include/asm/pgtable-3level_types.h:#define PMD_SHIFT   21
>> arch/x86/include/asm/pgtable_64_types.h:#define PMD_SHIFT       21
>>
>> Unless CONFIG_PGTABLE_LEVELS <= 2
>>
>> Then include/asm-generic/pgtable-nopmd.h will be used in
>> arch/x86/include/asm/pgtable_types.h
>> 	#define PMD_SHIFT	PUD_SHIFT
>>
>> In that case, also include/asm-generic/pgtable-nopmd.h is uses
>> 	#define PUD_SHIFT	P4D_SHIFT
>>
>> ... include/asm-generic/pgtable-nop4d.h
>> 	#define P4D_SHIFT	PGDIR_SHIFT
>>
>>
>> And that would be
>> arch/x86/include/asm/pgtable-2level_types.h:#define PGDIR_SHIFT 22
>>
>> If I am not wrong.
>>
>> So we would have pageblock_order = (22 - 12) = 10
>>
> 
> Thank, David :-)
> 
> I think current configuration is correct, while all these digits are
> written by programmer.
> 
> My concern and suggestion is to add a compiler check to enforce this. So
> that we would avoid this situation if someone miss this constrain. Just
> as the check on MAX_ORDER and SECION_SIZE.

I am not completely against this, I rather wonder if it is needed
because I assume other things will break horribly in case this is
violated. And at that would only be helpful for somebody developing for
a new architecture/flavor.

As I am a friend of documenting things that are not obvious, I would
rather suggest to add a comment to the
	#define pageblock_order		HUGETLB_PAGE_ORDER
line, stating what we just learned.

/*
 * HUGETLB_PAGE_ORDER will always be smaller than MAX_ORDER, so that
 * huge (not gigantic) pages can be served from the buddy allocator.
 */


-- 

Thanks,

David / dhildenb