linux-kernel - Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65186cee-358a-5c23-49e0-5507730941ad@redhat.com>
Date:   Fri, 28 Feb 2020 12:14:47 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Baoquan He <bhe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Oscar Salvador <osalvador@...e.de>,
        Michal Hocko <mhocko@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Wei Yang <richardw.yang@...ux.intel.com>
Subject: Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()

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@...nel.crashing.org>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Oscar Salvador <osalvador@...e.de>
>> Cc: Michal Hocko <mhocko@...nel.org>
>> Cc: Baoquan He <bhe@...hat.com>
>> Cc: Dan Williams <dan.j.williams@...el.com>
>> Cc: Wei Yang <richardw.yang@...ux.intel.com>
>> Signed-off-by: David Hildenbrand <david@...hat.com>
>> ---
>>  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