linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] mm/memory_hotplug: __add_pages() and __remove_pages() cleanups
@ 2020-02-28  9:58 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  9:58 ` [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages() David Hildenbrand
  0 siblings, 2 replies; 17+ messages in thread
From: David Hildenbrand @ 2020-02-28  9:58 UTC (permalink / raw)
  To: linux-kernel; +Cc: linux-mm, 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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  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 ` David Hildenbrand
  2020-02-28 10:22   ` Baoquan He
                     ` (2 more replies)
  2020-02-28  9:58 ` [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages() David Hildenbrand
  1 sibling, 3 replies; 17+ messages in thread
From: David Hildenbrand @ 2020-02-28  9:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, David Hildenbrand, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Baoquan He, Dan Williams, Wei Yang

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



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()
  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  9:58 ` David Hildenbrand
  2020-02-28 10:34   ` Baoquan He
  2020-02-28 13:26   ` Wei Yang
  1 sibling, 2 replies; 17+ messages in thread
From: David Hildenbrand @ 2020-02-28  9:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, David Hildenbrand, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Baoquan He, Dan Williams, Wei Yang

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



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  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
  2 siblings, 0 replies; 17+ messages in thread
From: Baoquan He @ 2020-02-28 10:22 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Dan Williams, Wei Yang

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>



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()
  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
  2020-02-28 13:26   ` Wei Yang
  1 sibling, 1 reply; 17+ messages in thread
From: Baoquan He @ 2020-02-28 10:34 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Dan Williams, Wei Yang

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
> 



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()
  2020-02-28 10:34   ` Baoquan He
@ 2020-02-28 11:14     ` David Hildenbrand
  2020-03-01  5:39       ` Baoquan He
  0 siblings, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2020-02-28 11:14 UTC (permalink / raw)
  To: Baoquan He
  Cc: linux-kernel, linux-mm, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Dan Williams, Wei Yang

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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  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
  2 siblings, 0 replies; 17+ messages in thread
From: Wei Yang @ 2020-02-28 13:25 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Baoquan He, Dan Williams

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()
  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 13:26   ` Wei Yang
  1 sibling, 0 replies; 17+ messages in thread
From: Wei Yang @ 2020-02-28 13:26 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Baoquan He, Dan Williams, 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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()
  2020-02-28 11:14     ` David Hildenbrand
@ 2020-03-01  5:39       ` Baoquan He
  0 siblings, 0 replies; 17+ messages in thread
From: Baoquan He @ 2020-03-01  5:39 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-kernel, linux-mm, Segher Boessenkool, Andrew Morton,
	Oscar Salvador, Michal Hocko, Dan Williams, Wei Yang

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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  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
  2 siblings, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2020-03-29 19:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, Segher Boessenkool, Andrew Morton, Oscar Salvador,
	Michal Hocko, Baoquan He, Dan Williams, Wei Yang, Linus Torvalds

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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  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:18       ` Linus Torvalds
  0 siblings, 2 replies; 17+ messages in thread
From: Linus Torvalds @ 2020-03-29 20:09 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Linux Kernel Mailing List, Linux-MM, Segher Boessenkool,
	Andrew Morton, Oscar Salvador, Michal Hocko, Baoquan He,
	Dan Williams, Wei Yang

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  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
  1 sibling, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2020-03-29 20:17 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Hildenbrand, Linux Kernel Mailing List, Linux-MM,
	Segher Boessenkool, Andrew Morton, Oscar Salvador, Michal Hocko,
	Baoquan He, Dan Williams, Wei Yang



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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  2020-03-29 20:09     ` Linus Torvalds
  2020-03-29 20:17       ` David Hildenbrand
@ 2020-03-29 20:18       ` Linus Torvalds
  2020-03-29 20:41         ` David Hildenbrand
  1 sibling, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2020-03-29 20:18 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Linux Kernel Mailing List, Linux-MM, Segher Boessenkool,
	Andrew Morton, Oscar Salvador, Michal Hocko, Baoquan He,
	Dan Williams, Wei Yang

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  2020-03-29 20:17       ` David Hildenbrand
@ 2020-03-29 20:26         ` Linus Torvalds
  0 siblings, 0 replies; 17+ messages in thread
From: Linus Torvalds @ 2020-03-29 20:26 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Linux Kernel Mailing List, Linux-MM, Segher Boessenkool,
	Andrew Morton, Oscar Salvador, Michal Hocko, Baoquan He,
	Dan Williams, Wei Yang

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  2020-03-29 20:18       ` Linus Torvalds
@ 2020-03-29 20:41         ` David Hildenbrand
  2020-03-30 22:14           ` Andrew Morton
  0 siblings, 1 reply; 17+ messages in thread
From: David Hildenbrand @ 2020-03-29 20:41 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Hildenbrand, Linux Kernel Mailing List, Linux-MM,
	Segher Boessenkool, Andrew Morton, Oscar Salvador, Michal Hocko,
	Baoquan He, Dan Williams, Wei Yang



> 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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  2020-03-29 20:41         ` David Hildenbrand
@ 2020-03-30 22:14           ` Andrew Morton
  2020-03-30 22:27             ` Linus Torvalds
  0 siblings, 1 reply; 17+ messages in thread
From: Andrew Morton @ 2020-03-30 22:14 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Linus Torvalds, Linux Kernel Mailing List, Linux-MM,
	Segher Boessenkool, Oscar Salvador, Michal Hocko, Baoquan He,
	Dan Williams, Wei Yang

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.






^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/2] mm/memory_hotplug: simplify calculation of number of pages in __remove_pages()
  2020-03-30 22:14           ` Andrew Morton
@ 2020-03-30 22:27             ` Linus Torvalds
  0 siblings, 0 replies; 17+ messages in thread
From: Linus Torvalds @ 2020-03-30 22:27 UTC (permalink / raw)
  To: Andrew Morton
  Cc: David Hildenbrand, Linux Kernel Mailing List, Linux-MM,
	Segher Boessenkool, Oscar Salvador, Michal Hocko, Baoquan He,
	Dan Williams, Wei Yang

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-03-30 22:28 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2020-03-01  5:39       ` Baoquan He
2020-02-28 13:26   ` Wei Yang

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