Re: [PATCH v1] mm/memory_hotplug: Easier calculation to get pages to next section boundary

[David Hildenbrand]

On 06.02.20 05:39, Baoquan He wrote:
> On 02/06/20 at 04:34am, Wei Yang wrote:
>> On Thu, Feb 06, 2020 at 10:48:16AM +0800, Baoquan He wrote:
>>> On 02/06/20 at 02:26am, Wei Yang wrote:
>>>> On Thu, Feb 06, 2020 at 08:37:36AM +0800, Baoquan He wrote:
>>>>> On 02/06/20 at 08:13am, Baoquan He wrote:
>>>>>> On 02/06/20 at 07:50am, Wei Yang wrote:
>>>>>>> On Thu, Feb 06, 2020 at 07:19:45AM +0800, Wei Yang wrote:
>>>>>>>> On Wed, Feb 05, 2020 at 02:52:51PM +0100, David Hildenbrand wrote:
>>>>>>>>> Let's use a calculation that's easier to understand and calculates the
>>>>>>>>> same result. Reusing existing macros makes this look nicer.
>>>>>>>>>
>>>>>>>>> We always want to have the number of pages (> 0) to the next section
>>>>>>>>> boundary, starting from the current pfn.
>>>>>>>>>
>>>>>>>>> Suggested-by: Segher Boessenkool <segher@xxxxxxxxxxxxxxxxxxx>
>>>>>>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>>>>>>>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>>>>>>>>> Cc: Oscar Salvador <osalvador@xxxxxxx>
>>>>>>>>> Cc: Baoquan He <bhe@xxxxxxxxxx>
>>>>>>>>> Cc: Wei Yang <richardw.yang@xxxxxxxxxxxxxxx>
>>>>>>>>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> Reviewed-by: Wei Yang <richardw.yang@xxxxxxxxxxxxxxx>
>>>>>>>>
>>>>>>>> BTW, I got one question about hotplug size requirement.
>>>>>>>>
>>>>>>>> I thought the hotplug range should be section size aligned, while taking a
>>>>>>>> look into current code function check_hotplug_memory_range() guard the range.
>>>>>>
>>>>>> A good question. The current code should be block size aligned. I
>>>>>> remember in some places we assume each block comprise all the sections.
>>>>>> Can't imagine one or some of them are half section filled.
>>>>>
>>>>> I could be wrong, half filled block may not cause problem. 
>>>>>
>>>>
>>>> David must be angry about our flooding the mail list :-)
>>>
>>> Believe he won't, :-) If you like, we can talk off line.
>>>
>>>>
>>>> Check the code again, there are two memory range check:
>>>>
>>>>   * check_hotplug_memory_range(), block/section aligned
>>>>   * check_pfn_span(), subsection aligned
>>>>
>>>> The second check, check_pfn_span() in __add_pages(), enable the capability to
>>>> add a memory range with subsection size.
>>>>
>>>> This means hotplug still keeps section alignment.
>>>
>>> memremap_pages() also call add_pages(), it doesn't have the
>>> check_hotplug_memory_range() invocation. check_pfn_span() is made for
>>> it specifically.
>>>
>>
>> If my understanding is correct, memremap_pages() is used to add some dev
>> memory to system. This is the use case which Dan want to enable for
>> sub-section. Since memremap_pages() is not called in mem-hotplug path, this
>> doesn't affect the hotplug range alignment.
> 
> Yeah, we are on the same page.

We allow sub-section hoy-add only for device memory/hmm. BIOS often
align PMEM devices to sub-sections, and not supporting this makes life
difficult to support these devices. (You cannot simply cut off something
of a disk :) )

System memory can only be hotplugged in memory block granularity, the
same granularity onlining/offlining from user space will happen. Boot
memory, however, can be sub-section aligned, but can never be offlined
(as it contains holes) and therefore never be removed.

>>
>>>>
>>>> BTW, __add_pages() share the same logic as __remove_pages(). Why not change it
>>>> too? Do I miss something or I don't have the latest source code?
>>>
>>> Good question, and I think it need. Just David is refactoring/cleaning
>>> up the remove_pages() code path, this is found out by Segher from patch
>>> reviewing.
>>
>> Ah, we may need a following cleanup :-)
> 
> Agree. See what David will say. Fold it into this patch, or anyone post
> a new one.
> 

Yes, I only cleaned up __remove_pages() for now. I can send a similar
cleanup for __add_pages().

Will resend this patch, also taking care of __add_pages(). Thanks!

-- 
Thanks,

David / dhildenb