linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: David Hildenbrand <david@redhat.com>
To: Baoquan He <bhe@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Segher Boessenkool <segher@kernel.crashing.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Oscar Salvador <osalvador@suse.de>,
	Michal Hocko <mhocko@kernel.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Wei Yang <richardw.yang@linux.intel.com>
Subject: Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()
Date: Fri, 28 Feb 2020 12:14:47 +0100	[thread overview]
Message-ID: <65186cee-358a-5c23-49e0-5507730941ad@redhat.com> (raw)
In-Reply-To: <20200228103442.GL24216@MiWiFi-R3L-srv>

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



  reply	other threads:[~2020-02-28 11:15 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-28  9:58 [PATCH v2 0/2] mm/memory_hotplug: __add_pages() and __remove_pages() cleanups David Hildenbrand
2020-02-28  9:58 ` [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages() David Hildenbrand
2020-02-28 10:22   ` Baoquan He
2020-02-28 13:25   ` Wei Yang
2020-03-29 19:19   ` David Hildenbrand
2020-03-29 20:09     ` Linus Torvalds
2020-03-29 20:17       ` David Hildenbrand
2020-03-29 20:26         ` Linus Torvalds
2020-03-29 20:18       ` Linus Torvalds
2020-03-29 20:41         ` David Hildenbrand
2020-03-30 22:14           ` Andrew Morton
2020-03-30 22:27             ` Linus Torvalds
2020-02-28  9:58 ` [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages() David Hildenbrand
2020-02-28 10:34   ` Baoquan He
2020-02-28 11:14     ` David Hildenbrand [this message]
2020-03-01  5:39       ` Baoquan He
2020-02-28 13:26   ` Wei Yang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=65186cee-358a-5c23-49e0-5507730941ad@redhat.com \
    --to=david@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=bhe@redhat.com \
    --cc=dan.j.williams@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=osalvador@suse.de \
    --cc=richardw.yang@linux.intel.com \
    --cc=segher@kernel.crashing.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).