Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1330546 > unrolled thread

Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page()  when zone is contiguous Vlastimil Babka <vbabka@suse.cz> - 2016-02-09 19:00 +0100
    Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page()  when zone is contiguous Andrew Morton <akpm@linux-foundation.org> - 2016-02-09 22:00 +0100
      Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page()  when zone is contiguous Vlastimil Babka <vbabka@suse.cz> - 2016-02-10 14:50 +0100
        Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page()  when zone is contiguous Andrew Morton <akpm@linux-foundation.org> - 2016-02-10 20:00 +0100
          Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page()  when zone is contiguous Joonsoo Kim <js1304@gmail.com> - 2016-02-11 03:00 +0100

#1330546 — Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous

On 02/05/2016 05:11 PM, Joonsoo Kim wrote:
> Yeah, it seems wrong to me. :)
> Here goes fix.

Doesn't apply for me, even after fixing the most obvious line wraps.
Seems like the version in mmotm is still your original patch and
Andrew's hotfix?

[toc] | [next] | [standalone]

#1330652

On Tue, 9 Feb 2016 18:58:32 +0100 Vlastimil Babka <vbabka@suse.cz> wrote:

> On 02/05/2016 05:11 PM, Joonsoo Kim wrote:
> > Yeah, it seems wrong to me. :)
> > Here goes fix.
> 
> Doesn't apply for me, even after fixing the most obvious line wraps.
> Seems like the version in mmotm is still your original patch and
> Andrew's hotfix?

Yes, that patch was hopelessly mailer-mangled.  I painstakingly fixed
it up and generated the incremental:


From: Joonsoo Kim <js1304@gmail.com>
Subject: mm-compaction-speed-up-pageblock_pfn_to_page-when-zone-is-contiguous-v3

v3
o remove pfn_valid_within() check for all pages in the pageblock
because pageblock_pfn_to_page() is only called with pageblock aligned pfn.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Reported-by: Aaron Lu <aaron.lu@intel.com>
Tested-by: Aaron Lu <aaron.lu@intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Rik van Riel <riel@redhat.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/page_alloc.c |  139 ++++++++++++++++++++++------------------------
 1 file changed, 69 insertions(+), 70 deletions(-)

diff -puN mm/page_alloc.c~mm-compaction-speed-up-pageblock_pfn_to_page-when-zone-is-contiguous-v3 mm/page_alloc.c
--- a/mm/page_alloc.c~mm-compaction-speed-up-pageblock_pfn_to_page-when-zone-is-contiguous-v3
+++ a/mm/page_alloc.c
@@ -1128,6 +1128,75 @@ void __init __free_pages_bootmem(struct
 	return __free_pages_boot_core(page, pfn, order);
 }
 
+/*
+ * Check that the whole (or subset of) a pageblock given by the interval of
+ * [start_pfn, end_pfn) is valid and within the same zone, before scanning it
+ * with the migration of free compaction scanner. The scanners then need to
+ * use only pfn_valid_within() check for arches that allow holes within
+ * pageblocks.
+ *
+ * Return struct page pointer of start_pfn, or NULL if checks were not passed.
+ *
+ * It's possible on some configurations to have a setup like node0 node1 node0
+ * i.e. it's possible that all pages within a zones range of pages do not
+ * belong to a single zone. We assume that a border between node0 and node1
+ * can occur within a single pageblock, but not a node0 node1 node0
+ * interleaving within a single pageblock. It is therefore sufficient to check
+ * the first and last page of a pageblock and avoid checking each individual
+ * page in a pageblock.
+ */
+struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
+				     unsigned long end_pfn, struct zone *zone)
+{
+	struct page *start_page;
+	struct page *end_page;
+
+	/* end_pfn is one past the range we are checking */
+	end_pfn--;
+
+	if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
+		return NULL;
+
+	start_page = pfn_to_page(start_pfn);
+
+	if (page_zone(start_page) != zone)
+		return NULL;
+
+	end_page = pfn_to_page(end_pfn);
+
+	/* This gives a shorter code than deriving page_zone(end_page) */
+	if (page_zone_id(start_page) != page_zone_id(end_page))
+		return NULL;
+
+	return start_page;
+}
+
+void set_zone_contiguous(struct zone *zone)
+{
+	unsigned long block_start_pfn = zone->zone_start_pfn;
+	unsigned long block_end_pfn;
+
+	block_end_pfn = ALIGN(block_start_pfn + 1, pageblock_nr_pages);
+	for (; block_start_pfn < zone_end_pfn(zone);
+			block_start_pfn = block_end_pfn,
+			 block_end_pfn += pageblock_nr_pages) {
+
+		block_end_pfn = min(block_end_pfn, zone_end_pfn(zone));
+
+		if (!__pageblock_pfn_to_page(block_start_pfn,
+					     block_end_pfn, zone))
+			return;
+	}
+
+	/* We confirm that there is no hole */
+	zone->contiguous = true;
+}
+
+void clear_zone_contiguous(struct zone *zone)
+{
+	zone->contiguous = false;
+}
+
 #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
 static void __init deferred_free_range(struct page *page,
 					unsigned long pfn, int nr_pages)
@@ -1304,76 +1373,6 @@ void __init page_alloc_init_late(void)
 		set_zone_contiguous(zone);
 }
 
-/*
- * Check that the whole (or subset of) a pageblock given by the interval of
- * [start_pfn, end_pfn) is valid and within the same zone, before scanning it
- * with the migration of free compaction scanner. The scanners then need to
- * use only pfn_valid_within() check for arches that allow holes within
- * pageblocks.
- *
- * Return struct page pointer of start_pfn, or NULL if checks were not passed.
- *
- * It's possible on some configurations to have a setup like node0 node1 node0
- * i.e. it's possible that all pages within a zones range of pages do not
- * belong to a single zone. We assume that a border between node0 and node1
- * can occur within a single pageblock, but not a node0 node1 node0
- * interleaving within a single pageblock. It is therefore sufficient to check
- * the first and last page of a pageblock and avoid checking each individual
- * page in a pageblock.
- */
-struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
-				unsigned long end_pfn, struct zone *zone)
-{
-	struct page *start_page;
-	struct page *end_page;
-
-	/* end_pfn is one past the range we are checking */
-	end_pfn--;
-
-	if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
-		return NULL;
-
-	start_page = pfn_to_page(start_pfn);
-
-	if (page_zone(start_page) != zone)
-		return NULL;
-
-	end_page = pfn_to_page(end_pfn);
-
-	/* This gives a shorter code than deriving page_zone(end_page) */
-	if (page_zone_id(start_page) != page_zone_id(end_page))
-		return NULL;
-
-	return start_page;
-}
-
-void set_zone_contiguous(struct zone *zone)
-{
-	unsigned long block_start_pfn = zone->zone_start_pfn;
-	unsigned long block_end_pfn;
-	unsigned long pfn;
-
-	block_end_pfn = ALIGN(block_start_pfn + 1, pageblock_nr_pages);
-	for (; block_start_pfn < zone_end_pfn(zone);
-		block_start_pfn = block_end_pfn,
-		block_end_pfn += pageblock_nr_pages) {
-
-		block_end_pfn = min(block_end_pfn, zone_end_pfn(zone));
-
-		if (!__pageblock_pfn_to_page(block_start_pfn,
-					block_end_pfn, zone))
-			return;
-	}
-
-	/* We confirm that there is no hole */
-	zone->contiguous = true;
-}
-
-void clear_zone_contiguous(struct zone *zone)
-{
-	zone->contiguous = false;
-}
-
 #ifdef CONFIG_CMA
 /* Free whole pageblock and set its migration type to MIGRATE_CMA. */
 void __init init_cma_reserved_pageblock(struct page *page)
_

[toc] | [prev] | [next] | [standalone]

#1331178

On 02/09/2016 09:53 PM, Andrew Morton wrote:
> On Tue, 9 Feb 2016 18:58:32 +0100 Vlastimil Babka <vbabka@suse.cz> wrote:
> 
>> On 02/05/2016 05:11 PM, Joonsoo Kim wrote:
>>> Yeah, it seems wrong to me. :)
>>> Here goes fix.
>>
>> Doesn't apply for me, even after fixing the most obvious line wraps.
>> Seems like the version in mmotm is still your original patch and
>> Andrew's hotfix?
> 
> Yes, that patch was hopelessly mailer-mangled.  I painstakingly fixed
> it up and generated the incremental:

Thanks a lot. My review of the final patch also involved pain (due to
the cold, not the patch!).

You can take my Acked-by, but I also find the definitions of
set_zone_contiguous/clear_zone_contiguous() "in the header of the
consumer" (hotplug) somewhat unusual. It works, but e.g. mm/internal.h
would be more expected.

Then there's this:

> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -509,6 +509,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
>  	int start_sec, end_sec;
>  	struct vmem_altmap *altmap;
>  
> +	clear_zone_contiguous(zone);
> +
>  	/* during initialize mem_map, align hot-added range to section */
>  	start_sec = pfn_to_section_nr(phys_start_pfn);
>  	end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1);
> @@ -540,6 +542,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
>  	}
>  	vmemmap_populate_print_last();
>  
> +	set_zone_contiguous(zone);
> +
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(__add_pages);

Between the clear and set, __add_pages() might return with -EINVAL,
leaving the flag cleared potentially forever. Not critical, probably
rare, but it should be possible to avoid this by moving the clear below
the altmap check?

Thanks,
Vlastimil

[toc] | [prev] | [next] | [standalone]

#1331440

On Wed, 10 Feb 2016 14:42:57 +0100 Vlastimil Babka <vbabka@suse.cz> wrote:

> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -509,6 +509,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
> >  	int start_sec, end_sec;
> >  	struct vmem_altmap *altmap;
> >  
> > +	clear_zone_contiguous(zone);
> > +
> >  	/* during initialize mem_map, align hot-added range to section */
> >  	start_sec = pfn_to_section_nr(phys_start_pfn);
> >  	end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1);
> > @@ -540,6 +542,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
> >  	}
> >  	vmemmap_populate_print_last();
> >  
> > +	set_zone_contiguous(zone);
> > +
> >  	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(__add_pages);
> 
> Between the clear and set, __add_pages() might return with -EINVAL,
> leaving the flag cleared potentially forever. Not critical, probably
> rare, but it should be possible to avoid this by moving the clear below
> the altmap check?

um, yes.  return-in-the-middle-of-a-function strikes again.

--- a/mm/memory_hotplug.c~mm-compaction-speed-up-pageblock_pfn_to_page-when-zone-is-contiguous-fix
+++ a/mm/memory_hotplug.c
@@ -526,7 +526,8 @@ int __ref __add_pages(int nid, struct zo
 		if (altmap->base_pfn != phys_start_pfn
 				|| vmem_altmap_offset(altmap) > nr_pages) {
 			pr_warn_once("memory add fail, invalid altmap\n");
-			return -EINVAL;
+			err = -EINVAL;
+			goto out;
 		}
 		altmap->alloc = 0;
 	}
@@ -544,9 +545,8 @@ int __ref __add_pages(int nid, struct zo
 		err = 0;
 	}
 	vmemmap_populate_print_last();
-
+out:
 	set_zone_contiguous(zone);
-
 	return err;
 }
 EXPORT_SYMBOL_GPL(__add_pages);
_

[toc] | [prev] | [next] | [standalone]

#1331651

2016-02-11 3:58 GMT+09:00 Andrew Morton <akpm@linux-foundation.org>:
> On Wed, 10 Feb 2016 14:42:57 +0100 Vlastimil Babka <vbabka@suse.cz> wrote:
>
>> > --- a/mm/memory_hotplug.c
>> > +++ b/mm/memory_hotplug.c
>> > @@ -509,6 +509,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
>> >     int start_sec, end_sec;
>> >     struct vmem_altmap *altmap;
>> >
>> > +   clear_zone_contiguous(zone);
>> > +
>> >     /* during initialize mem_map, align hot-added range to section */
>> >     start_sec = pfn_to_section_nr(phys_start_pfn);
>> >     end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1);
>> > @@ -540,6 +542,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
>> >     }
>> >     vmemmap_populate_print_last();
>> >
>> > +   set_zone_contiguous(zone);
>> > +
>> >     return err;
>> >  }
>> >  EXPORT_SYMBOL_GPL(__add_pages);
>>
>> Between the clear and set, __add_pages() might return with -EINVAL,
>> leaving the flag cleared potentially forever. Not critical, probably
>> rare, but it should be possible to avoid this by moving the clear below
>> the altmap check?
>
> um, yes.  return-in-the-middle-of-a-function strikes again.
>
> --- a/mm/memory_hotplug.c~mm-compaction-speed-up-pageblock_pfn_to_page-when-zone-is-contiguous-fix
> +++ a/mm/memory_hotplug.c
> @@ -526,7 +526,8 @@ int __ref __add_pages(int nid, struct zo
>                 if (altmap->base_pfn != phys_start_pfn
>                                 || vmem_altmap_offset(altmap) > nr_pages) {
>                         pr_warn_once("memory add fail, invalid altmap\n");
> -                       return -EINVAL;
> +                       err = -EINVAL;
> +                       goto out;
>                 }
>                 altmap->alloc = 0;
>         }
> @@ -544,9 +545,8 @@ int __ref __add_pages(int nid, struct zo
>                 err = 0;
>         }
>         vmemmap_populate_print_last();
> -
> +out:
>         set_zone_contiguous(zone);
> -
>         return err;
>  }
>  EXPORT_SYMBOL_GPL(__add_pages);

Sorry for late response. I was on biggest holiday in Korea until now.
It seems that there is no issue left.
Andrew, Vlastimil, thanks for fixes and review.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web