Re: [PATCH v1 2/4] mm/memory_hotplug: Handle unaligned start and nr_pages in online_pages_blocks()

[David Hildenbrand]

On 14.08.19 16:08, Michal Hocko wrote:
> On Fri 09-08-19 14:56:59, David Hildenbrand wrote:
>> Take care of nr_pages not being a power of two and start not being
>> properly aligned. Essentially, what walk_system_ram_range() could provide
>> to us. get_order() will round-up in case it's not a power of two.
>>
>> This should only apply to memory blocks that contain strange memory
>> resources (especially with holes), not to ordinary DIMMs.
> 
> I would really like to see an example of such setup before making the
> code hard to read. Because I am not really sure something like that
> exists at all.

I don't have a real-live example at hand (founds this while exploring
the code), however, the linked commit changed it without stating why it
would be safe to do so.

In case you have an idea on how to make this code easier to read, please
let me know.

> 
>> Fixes: a9cd410a3d29 ("mm/page_alloc.c: memory hotplug: free pages as higher order")
>> Cc: Arun KS <arunks@xxxxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Oscar Salvador <osalvador@xxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxx>
>> Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>> ---
>>  mm/memory_hotplug.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 3706a137d880..2abd938c8c45 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -640,6 +640,10 @@ static int online_pages_blocks(unsigned long start, unsigned long nr_pages)
>>  	while (start < end) {
>>  		order = min(MAX_ORDER - 1,
>>  			get_order(PFN_PHYS(end) - PFN_PHYS(start)));
>> +		/* make sure the PFN is aligned and we don't exceed the range */
>> +		while (!IS_ALIGNED(start, 1ul << order) ||
>> +		       (1ul << order) > end - start)
>> +			order--;
>>  		(*online_page_callback)(pfn_to_page(start), order);
>>  
>>  		onlined_pages += (1UL << order);
>> -- 
>> 2.21.0
> 


-- 

Thanks,

David / dhildenb