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

[Wei Yang <richard.weiyang@gmail.com>]

On Thu, Dec 06, 2018 at 10:26:55AM +0100, David Hildenbrand wrote:
>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.

I think you are right.

>
>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.
> */
>

This looks good to me. Let's see which one others prefer :-)

>
>-- 
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me