On 28.02.20 11:34, Baoquan He wrote:
> On 02/28/20 at 10:58am, David Hildenbrand wrote:
>> Let's drop the basically unused section stuff and simplify. The logic
>> now matches the logic in __remove_pages().
>>
>> Cc: Segher Boessenkool <segher@xxxxxxxxxxxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Oscar Salvador <osalvador@xxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>> Cc: Baoquan He <bhe@xxxxxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Cc: Wei Yang <richardw.yang@xxxxxxxxxxxxxxx>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>> ---
>> mm/memory_hotplug.c | 18 +++++++-----------
>> 1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 8fe7e32dad48..1a00b5a37ef6 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn,
>> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> struct mhp_restrictions *restrictions)
>> {
>> + const unsigned long end_pfn = pfn + nr_pages;
>> + unsigned long cur_nr_pages;
>> int err;
>> - unsigned long nr, start_sec, end_sec;
>> struct vmem_altmap *altmap = restrictions->altmap;
>>
>> err = check_hotplug_memory_addressable(pfn, nr_pages);
>> @@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> if (err)
>> return err;
>>
>> - start_sec = pfn_to_section_nr(pfn);
>> - end_sec = pfn_to_section_nr(pfn + nr_pages - 1);
>> - for (nr = start_sec; nr <= end_sec; nr++) {
>> - unsigned long pfns;
>> -
>> - pfns = min(nr_pages, PAGES_PER_SECTION
>> - - (pfn & ~PAGE_SECTION_MASK));
>> - err = sparse_add_section(nid, pfn, pfns, altmap);
>> + for (; pfn < end_pfn; pfn += cur_nr_pages) {
>> + /* Select all remaining pages up to the next section boundary */
>> + cur_nr_pages = min(end_pfn - pfn,
>> + SECTION_ALIGN_UP(pfn + 1) - pfn);
>> + err = sparse_add_section(nid, pfn, cur_nr_pages, altmap);
>
> Honestly, I am not a big fan of this kind of code refactoring. The old
> code may span seveal more lines or define several several more veriables,
> but logic is clear, and no visible defect. It's hard to say how much we
I'm sorry, but iterating over variables and not using a single one in
the body is definitely not clean, at least IMHO. Leftover from
sub-section hotadd support.
> can benefit from this kind of code simplifying, and reviewing it will take
> people more time. While for the code style consistency with
> __remove_page(), I would like to see it's merged. My personal opinion.
Thanks!
--
Thanks,
David / dhildenb
