[PATCH v2 0/2] mm/memory_hotplug: __add_pages() and __remove_pages() cleanups

[PATCH v2 0/2] mm/memory_hotplug: __add_pages() and __remove_pages() cleanups

[David Hildenbrand]

This is the follow up of:
	"[PATCH v1] mm/memory_hotplug: Easier calculation to get pages to
	 next section boundary" [1]

Cleanups as a follow up on commit 52fb87c81f11 ("mm/memory_hotplug:
cleanup __remove_pages()").

v1 -> v2:
- "mm/memory_hotplug: simplify calculation of number of pages in
   __remove_pages()"
-- Rephrased subject/description, but left patch as is
- "mm/memory_hotplug: cleanup __add_pages()"
-- Make the logic match the logic in __remove_pages()

[1] https://lkml.kernel.org/r/20200205135251.37488-1-david@redhat.com

David Hildenbrand (2):
  mm/memory_hotplug: simplify calculation of number of pages in
    __remove_pages()
  mm/memory_hotplug: cleanup __add_pages()

 mm/memory_hotplug.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

-- 
2.24.1

[PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[David Hildenbrand]

In commit 52fb87c81f11 ("mm/memory_hotplug: cleanup __remove_pages()"),
we cleaned up __remove_pages(), and introduced a shorter variant to
calculate the number of pages to the next section boundary.

Turns out we can make this calculation easier to read. We always want to
have the number of pages (> 0) to the next section boundary, starting from
the current pfn.

We'll clean up __remove_pages() in a follow-up patch and directly make
use of this computation.

Suggested-by: 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 4a9b3f6c6b37..8fe7e32dad48 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -534,7 +534,8 @@ void __remove_pages(unsigned long pfn, unsigned long nr_pages,
 	for (; pfn < end_pfn; pfn += cur_nr_pages) {
 		cond_resched();
 		/* Select all remaining pages up to the next section boundary */
-		cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
+		cur_nr_pages = min(end_pfn - pfn,
+				   SECTION_ALIGN_UP(pfn + 1) - pfn);
 		__remove_section(pfn, cur_nr_pages, map_offset, altmap);
 		map_offset = 0;
 	}
-- 
2.24.1

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[Baoquan He]

On 02/28/20 at 10:58am, David Hildenbrand wrote:
> In commit 52fb87c81f11 ("mm/memory_hotplug: cleanup __remove_pages()"),
> we cleaned up __remove_pages(), and introduced a shorter variant to
> calculate the number of pages to the next section boundary.
> 
> Turns out we can make this calculation easier to read. We always want to
> have the number of pages (> 0) to the next section boundary, starting from
> the current pfn.
> 
> We'll clean up __remove_pages() in a follow-up patch and directly make
> use of this computation.
> 
> Suggested-by: 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4a9b3f6c6b37..8fe7e32dad48 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -534,7 +534,8 @@ void __remove_pages(unsigned long pfn, unsigned long nr_pages,
>  	for (; pfn < end_pfn; pfn += cur_nr_pages) {
>  		cond_resched();
>  		/* Select all remaining pages up to the next section boundary */
> -		cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
> +		cur_nr_pages = min(end_pfn - pfn,
> +				   SECTION_ALIGN_UP(pfn + 1) - pfn);

Reviewed-by: Baoquan He <bhe@redhat.com>

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[Wei Yang]

On Fri, Feb 28, 2020 at 10:58:18AM +0100, David Hildenbrand wrote:
>In commit 52fb87c81f11 ("mm/memory_hotplug: cleanup __remove_pages()"),
>we cleaned up __remove_pages(), and introduced a shorter variant to
>calculate the number of pages to the next section boundary.
>
>Turns out we can make this calculation easier to read. We always want to
>have the number of pages (> 0) to the next section boundary, starting from
>the current pfn.
>
>We'll clean up __remove_pages() in a follow-up patch and directly make
>use of this computation.
>
>Suggested-by: 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>

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>

>---
> mm/memory_hotplug.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 4a9b3f6c6b37..8fe7e32dad48 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -534,7 +534,8 @@ void __remove_pages(unsigned long pfn, unsigned long nr_pages,
> 	for (; pfn < end_pfn; pfn += cur_nr_pages) {
> 		cond_resched();
> 		/* Select all remaining pages up to the next section boundary */
>-		cur_nr_pages = min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
>+		cur_nr_pages = min(end_pfn - pfn,
>+				   SECTION_ALIGN_UP(pfn + 1) - pfn);
> 		__remove_section(pfn, cur_nr_pages, map_offset, altmap);
> 		map_offset = 0;
> 	}
>-- 
>2.24.1

-- 
Wei Yang
Help you, Help me

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[David Hildenbrand]

On 28.02.20 10:58, David Hildenbrand wrote:
> In commit 52fb87c81f11 ("mm/memory_hotplug: cleanup __remove_pages()"),
> we cleaned up __remove_pages(), and introduced a shorter variant to
> calculate the number of pages to the next section boundary.
> 
> Turns out we can make this calculation easier to read. We always want to
> have the number of pages (> 0) to the next section boundary, starting from
> the current pfn.

@Andrew

This patch seems to have another of these weird MIME crap in it. (my
other patches in -next seem to be fine)

See

https://lore.kernel.org/linux-mm/20200228095819.10750-2-david@redhat.com/raw

"
... fro=
m"

should simply be "... from"

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[Linus Torvalds]

On Sun, Mar 29, 2020 at 12:19 PM David Hildenbrand <david@redhat.com> wrote:
>
> This patch seems to have another of these weird MIME crap in it. (my
> other patches in -next seem to be fine)
>
> See
>
> https://lore.kernel.org/linux-mm/20200228095819.10750-2-david@redhat.com/raw

That email actually looks fine.

Yes, it has that

   fro=
   m

pattern, but it also has

   Content-Transfer-Encoding: quoted-printable

so the recipient should be doing the right thing with that pattern.

The patch itself also has MIME encoding in it:

   - cur_nr_pages =3D min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
   + cur_nr_pages =3D min(end_pfn - pfn,

so the patch wouldn't even apply unless the recipient did the proper
MIME decode of the message.

That's also why the non-raw message looks fine:

  https://lore.kernel.org/linux-mm/20200228095819.10750-2-david@redhat.com/

because the raw message data has the proper encoding information.

In contrast, look at the email that Andrew sent me and that I complained about:

   https://lore.kernel.org/linux-mm/20200329021719.MBKzW0xSl%25akpm@linux-foundation.org/

and notice how that *non-raw* email has that

   Withou=
   t

pattern in it. And when you look at the raw one:

  https://lore.kernel.org/linux-mm/20200329021719.MBKzW0xSl%25akpm@linux-foundation.org/raw

it has no content transfer encoding line in the headers.

                 Linus

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[David Hildenbrand]

> Am 29.03.2020 um 22:10 schrieb Linus Torvalds <torvalds@linux-foundation.org>:
> 
> On Sun, Mar 29, 2020 at 12:19 PM David Hildenbrand <david@redhat.com> wrote:
>> 
>> This patch seems to have another of these weird MIME crap in it. (my
>> other patches in -next seem to be fine)
>> 
>> See
>> 
>> https://lore.kernel.org/linux-mm/20200228095819.10750-2-david@redhat.com/raw
> 
> That email actually looks fine.
> 
> Yes, it has that
> 
>   fro=
>   m
> 
> pattern, but it also has
> 
>   Content-Transfer-Encoding: quoted-printable
> 
> so the recipient should be doing the right thing with that pattern.
> 
> The patch itself also has MIME encoding in it:
> 
>   - cur_nr_pages =3D min(end_pfn - pfn, -(pfn | PAGE_SECTION_MASK));
>   + cur_nr_pages =3D min(end_pfn - pfn,
> 
> so the patch wouldn't even apply unless the recipient did the proper
> MIME decode of the message.
> 
> That's also why the non-raw message looks fine:
> 
>  https://lore.kernel.org/linux-mm/20200228095819.10750-2-david@redhat.com/
> 
> because the raw message data has the proper encoding information.
> 
> In contrast, look at the email that Andrew sent me and that I complained about:
> 
>   https://lore.kernel.org/linux-mm/20200329021719.MBKzW0xSl%25akpm@linux-foundation.org/
> 
> and notice how that *non-raw* email has that
> 
>   Withou=
>   t
> 
> pattern in it. And when you look at the raw one:
> 
>  https://lore.kernel.org/linux-mm/20200329021719.MBKzW0xSl%25akpm@linux-foundation.org/raw
> 
> it has no content transfer encoding line in the headers.

Interesting, at least the patch in -next is messed up. I remember Andrew adapted some scripts, maybe this is a leftover.

Cheers!

> 
>                 Linus
> 

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[Linus Torvalds]

On Sun, Mar 29, 2020 at 1:17 PM David Hildenbrand <david@redhat.com> wrote:
>
> Interesting, at least the patch in -next is messed up. I remember Andrew adapted some scripts, maybe this is a leftover.

It does look like s broken MIME decoding script somewhere.

But it's odd, because as mentioned, Andrew definitely handles MIME in
other places correctly - including your messages when it comes to the
patch data itself. It's only the message above the patch that hasn't
been properly decoded.

Curious.

              Linus

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[Linus Torvalds]

On Sun, Mar 29, 2020 at 1:09 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> In contrast, look at the email that Andrew sent me and that I complained about:
>
>    https://lore.kernel.org/linux-mm/20200329021719.MBKzW0xSl%25akpm@linux-foundation.org/

Hmm. I'm trying to figure out how and where Andrew got the original from you.

There's

  https://lore.kernel.org/linux-mm/20200124155336.17126-1-david@redhat.com/raw

but again, that one actually looks fine. It has that

   Content-Transfer-Encoding: quoted-printable

header line, but it doesn't even have the "=\n" pattern in the text at
all. It does have MIME encoding in the patch, but that's all fine.

Then there's a new version:

   https://lore.kernel.org/linux-mm/20200128093542.6908-1-david@redhat.com/raw

and that one *does* have the "Withou=\nt" pattern in it. But it still
has the proper

   Content-Transfer-Encoding: quoted-printable

in it, so the recipient should decode it just fine (and again, you can
see that in the non-raw email - it looks just fine).

So your emails on lore look fine. I'm not seeing how that got corrupted.

               Linus

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[David Hildenbrand]

> Am 29.03.2020 um 22:19 schrieb Linus Torvalds <torvalds@linux-foundation.org>:
> 
> On Sun, Mar 29, 2020 at 1:09 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> 
>> In contrast, look at the email that Andrew sent me and that I complained about:
>> 
>>   https://lore.kernel.org/linux-mm/20200329021719.MBKzW0xSl%25akpm@linux-foundation.org/
> 
> Hmm. I'm trying to figure out how and where Andrew got the original from you.
> 
> There's
> 
>  https://lore.kernel.org/linux-mm/20200124155336.17126-1-david@redhat.com/raw
> 
> but again, that one actually looks fine. It has that
> 
>   Content-Transfer-Encoding: quoted-printable
> 
> header line, but it doesn't even have the "=\n" pattern in the text at
> all. It does have MIME encoding in the patch, but that's all fine.
> 
> Then there's a new version:
> 
>   https://lore.kernel.org/linux-mm/20200128093542.6908-1-david@redhat.com/raw
> 
> and that one *does* have the "Withou=\nt" pattern in it. But it still
> has the proper
> 
>   Content-Transfer-Encoding: quoted-printable
> 
> in it, so the recipient should decode it just fine (and again, you can
> see that in the non-raw email - it looks just fine).
> 
> So your emails on lore look fine. I'm not seeing how that got corrupted.

*maybe* Andrew updated only the patch description, copying the raw content. Eventually he converts MIME only when importing, so the description got corrupted.

... or the mail he received via cc got messed up by my mailing infrastructure.

Cheers

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[Andrew Morton]

On Sun, 29 Mar 2020 22:41:18 +0200 David Hildenbrand <david@redhat.com> wrote:

> >   https://lore.kernel.org/linux-mm/20200128093542.6908-1-david@redhat.com/raw
> > 
> > and that one *does* have the "Withou=\nt" pattern in it. But it still
> > has the proper
> > 
> >   Content-Transfer-Encoding: quoted-printable
> > 
> > in it, so the recipient should decode it just fine (and again, you can
> > see that in the non-raw email - it looks just fine).
> > 
> > So your emails on lore look fine. I'm not seeing how that got corrupted.
> 
> *maybe* Andrew updated only the patch description, copying the raw content. Eventually he converts MIME only when importing, so the description got corrupted.
> 
> ... or the mail he received via cc got messed up by my mailing infrastructure.

Something like that.  It's sylpheed being weird.  In some situations
(save as...) it will perform the mime conversion but in others (%f in
an action menu) it does not.  So for David's patches I do save-as for
the patch and manually edit the mime out of changelog.  It's basically
"only David" so I haven't got around to doing anything smarter.

My nightly check-all-the-patches-for-various-cruft script emails me
about =3D but I didn't' have a test for "o=$m".  I just added one.

Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()

[Linus Torvalds]

On Mon, Mar 30, 2020 at 3:14 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> My nightly check-all-the-patches-for-various-cruft script emails me
> about =3D but I didn't' have a test for "o=$m".  I just added one.

=3D may be the common one, but =20 and =09 are others that end up
showing up when whitespace gets quoted for various reasons.

Another one is =46 for 'F'. Why? Because some mailers think that
"From" at the beginning of a line is the mbox beginning marker, and
they'll escape any line that begins with "From" to use "=46rom"
instead.

Those mailers are wrong (at a _minimum_ it's "From " with a space, and
you generally should be even stricter than that), but it happens.

And obviously, if there is real 8-bit stuff, you'll get all the real
odd hex noise.

               Linus

[PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()

[David Hildenbrand]

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);
 		if (err)
 			break;
-		pfn += pfns;
-		nr_pages -= pfns;
 		cond_resched();
 	}
 	vmemmap_populate_print_last();
-- 
2.24.1

Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()

[Baoquan He]

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
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.

Reviewed-by: Baoquan He <bhe@redhat.com>

>  		if (err)
>  			break;
> -		pfn += pfns;
> -		nr_pages -= pfns;
>  		cond_resched();
>  	}
>  	vmemmap_populate_print_last();
> -- 
> 2.24.1
> 

Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()

[David Hildenbrand]

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

Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()

[Baoquan He]

On 02/28/20 at 12:14pm, David Hildenbrand wrote:
> 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

Hmm, sometime we do use iterator to loop over only, it's just not so good.
People usually have their own preferred coding style, and try to optimize to
remove the itch in heart, totally understand. I have no strong objection
to this, as long as it gets support from reviewers, it's not a bad
thing.

> 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

Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()

[Wei Yang]

On Fri, Feb 28, 2020 at 10:58:19AM +0100, 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>

Reviewed-by: Wei Yang <richard.weiyang@gmail.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);
> 		if (err)
> 			break;
>-		pfn += pfns;
>-		nr_pages -= pfns;
> 		cond_resched();
> 	}
> 	vmemmap_populate_print_last();
>-- 
>2.24.1

-- 
Wei Yang
Help you, Help me