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

Groups > linux.kernel > #1712520 > unrolled thread

[patch 1/2] mm, compaction: kcompactd should not ignore pageblock skip

Back to article view | Back to linux.kernel

Contents

  [patch 1/2] mm, compaction: kcompactd should not ignore pageblock  skip David Rientjes <rientjes@google.com> - 2017-08-16 01:40 +0200
    [patch 2/2] mm, compaction: persistently skip hugetlbfs pageblocks David Rientjes <rientjes@google.com> - 2017-08-16 01:40 +0200
      Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs  pageblocks Michal Hocko <mhocko@kernel.org> - 2017-08-18 10:50 +0200
        [patch -mm] mm, compaction: persistently skip hugetlbfs pageblocks  fix David Rientjes <rientjes@google.com> - 2017-08-21 02:40 +0200
          Re: [patch -mm] mm, compaction: persistently skip hugetlbfs  pageblocks fix Michal Hocko <mhocko@kernel.org> - 2017-08-21 08:40 +0200
      Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs  pageblocks Vlastimil Babka <vbabka@suse.cz> - 2017-08-23 10:50 +0200
    Re: [patch 1/2] mm, compaction: kcompactd should not ignore pageblock  skip Vlastimil Babka <vbabka@suse.cz> - 2017-08-23 10:30 +0200

#1712520 — [patch 1/2] mm, compaction: kcompactd should not ignore pageblock skip

Kcompactd is needlessly ignoring pageblock skip information.  It is doing
MIGRATE_SYNC_LIGHT compaction, which is no more powerful than
MIGRATE_SYNC compaction.

If compaction recently failed to isolate memory from a set of pageblocks,
there is nothing to indicate that kcompactd will be able to do so, or
that it is beneficial from attempting to isolate memory.

Use the pageblock skip hint to avoid rescanning pageblocks needlessly
until that information is reset.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/compaction.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1927,9 +1927,8 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 		.total_free_scanned = 0,
 		.classzone_idx = pgdat->kcompactd_classzone_idx,
 		.mode = MIGRATE_SYNC_LIGHT,
-		.ignore_skip_hint = true,
+		.ignore_skip_hint = false,
 		.gfp_mask = GFP_KERNEL,
-
 	};
 	trace_mm_compaction_kcompactd_wake(pgdat->node_id, cc.order,
 							cc.classzone_idx);

[toc] | [next] | [standalone]

#1712521 — [patch 2/2] mm, compaction: persistently skip hugetlbfs pageblocks

It is pointless to migrate hugetlb memory as part of memory compaction if
the hugetlb size is equal to the pageblock order.  No defragmentation is
occurring in this condition.

It is also pointless to for the freeing scanner to scan a pageblock where
a hugetlb page is pinned.  Unconditionally skip these pageblocks, and do
so peristently so that they are not rescanned until it is observed that
these hugepages are no longer pinned.

It would also be possible to do this by involving the hugetlb subsystem
in marking pageblocks to no longer be skipped when they hugetlb pages are
freed.  This is a simple solution that doesn't involve any additional
subsystems in pageblock skip manipulation.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/compaction.c | 48 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -217,6 +217,20 @@ static void reset_cached_positions(struct zone *zone)
 				pageblock_start_pfn(zone_end_pfn(zone) - 1);
 }
 
+/*
+ * Hugetlbfs pages should consistenly be skipped until updated by the hugetlb
+ * subsystem.  It is always pointless to compact pages of pageblock_order and
+ * the free scanner can reconsider when no longer huge.
+ */
+static bool pageblock_skip_persistent(struct page *page, unsigned int order)
+{
+	if (!PageHuge(page))
+		return false;
+	if (order != pageblock_order)
+		return false;
+	return true;
+}
+
 /*
  * This function is called to clear all cached information on pageblocks that
  * should be skipped for page isolation when the migrate and free page scanner
@@ -241,6 +255,8 @@ static void __reset_isolation_suitable(struct zone *zone)
 			continue;
 		if (zone != page_zone(page))
 			continue;
+		if (pageblock_skip_persistent(page, compound_order(page)))
+			continue;
 
 		clear_pageblock_skip(page);
 	}
@@ -448,13 +464,15 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 		 * and the only danger is skipping too much.
 		 */
 		if (PageCompound(page)) {
-			unsigned int comp_order = compound_order(page);
-
-			if (likely(comp_order < MAX_ORDER)) {
-				blockpfn += (1UL << comp_order) - 1;
-				cursor += (1UL << comp_order) - 1;
+			const unsigned int order = compound_order(page);
+
+			if (pageblock_skip_persistent(page, order)) {
+				set_pageblock_skip(page);
+				blockpfn = end_pfn;
+			} else if (likely(order < MAX_ORDER)) {
+				blockpfn += (1UL << order) - 1;
+				cursor += (1UL << order) - 1;
 			}
-
 			goto isolate_fail;
 		}
 
@@ -771,11 +789,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 		 * danger is skipping too much.
 		 */
 		if (PageCompound(page)) {
-			unsigned int comp_order = compound_order(page);
-
-			if (likely(comp_order < MAX_ORDER))
-				low_pfn += (1UL << comp_order) - 1;
+			const unsigned int order = compound_order(page);
 
+			if (pageblock_skip_persistent(page, order)) {
+				set_pageblock_skip(page);
+				low_pfn = end_pfn;
+			} else if (likely(order < MAX_ORDER))
+				low_pfn += (1UL << order) - 1;
 			goto isolate_fail;
 		}
 
@@ -837,7 +857,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 			 * is safe to read and it's 0 for tail pages.
 			 */
 			if (unlikely(PageCompound(page))) {
-				low_pfn += (1UL << compound_order(page)) - 1;
+				const unsigned int order = compound_order(page);
+
+				if (pageblock_skip_persistent(page, order)) {
+					set_pageblock_skip(page);
+					low_pfn = end_pfn;
+				} else
+					low_pfn += (1UL << order) - 1;
 				goto isolate_fail;
 			}
 		}

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

#1714712 — Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs pageblocks

I am getting 
mm/compaction.c: In function 'isolate_freepages_block':
mm/compaction.c:469:4: error: implicit declaration of function 'pageblock_skip_persistent' [-Werror=implicit-function-declaration]
    if (pageblock_skip_persistent(page, order)) {
    ^
mm/compaction.c:470:5: error: implicit declaration of function 'set_pageblock_skip' [-Werror=implicit-function-declaration]
     set_pageblock_skip(page);

when compaction is disabled because isolate_freepages_block is defined
also when CONFIG_COMPACTION=n. I haven't checked how to fix this
properly yet.
-- 
Michal Hocko
SUSE Labs

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

#1716049 — [patch -mm] mm, compaction: persistently skip hugetlbfs pageblocks fix

[Multipart message — attachments visible in raw view] — view raw

Fix build:

mm/compaction.c: In function ‘isolate_freepages_block’:
mm/compaction.c:469:4: error: implicit declaration of function ‘pageblock_skip_persistent’ [-Werror=implicit-function-declaration]
    if (pageblock_skip_persistent(page, order)) {
    ^
mm/compaction.c:470:5: error: implicit declaration of function ‘set_pageblock_skip’ [-Werror=implicit-function-declaration]
     set_pageblock_skip(page);
     ^

CMA doesn't guarantee pageblock skip will get reset when migration and 
freeing scanners meet, and pageblock skip is a CONFIG_COMPACTION only 
feature, so disable it when CONFIG_COMPACTION=n.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/pageblock-flags.h | 11 +++++++++++
 mm/compaction.c                 |  8 +++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
--- a/include/linux/pageblock-flags.h
+++ b/include/linux/pageblock-flags.h
@@ -96,6 +96,17 @@ void set_pfnblock_flags_mask(struct page *page,
 #define set_pageblock_skip(page) \
 			set_pageblock_flags_group(page, 1, PB_migrate_skip,  \
 							PB_migrate_skip)
+#else
+static inline bool get_pageblock_skip(struct page *page)
+{
+	return false;
+}
+static inline void clear_pageblock_skip(struct page *page)
+{
+}
+static inline void set_pageblock_skip(struct page *page)
+{
+}
 #endif /* CONFIG_COMPACTION */
 
 #endif	/* PAGEBLOCK_FLAGS_H */
diff --git a/mm/compaction.c b/mm/compaction.c
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -322,7 +322,13 @@ static inline bool isolation_suitable(struct compact_control *cc,
 	return true;
 }
 
-static void update_pageblock_skip(struct compact_control *cc,
+static inline bool pageblock_skip_persistent(struct page *page,
+					     unsigned int order)
+{
+	return false;
+}
+
+static inline void update_pageblock_skip(struct compact_control *cc,
 			struct page *page, unsigned long nr_isolated,
 			bool migrate_scanner)
 {

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

#1716113 — Re: [patch -mm] mm, compaction: persistently skip hugetlbfs pageblocks fix

On Sun 20-08-17 17:36:41, David Rientjes wrote:
> Fix build:
> 
> mm/compaction.c: In function ‘isolate_freepages_block’:
> mm/compaction.c:469:4: error: implicit declaration of function ‘pageblock_skip_persistent’ [-Werror=implicit-function-declaration]
>     if (pageblock_skip_persistent(page, order)) {
>     ^
> mm/compaction.c:470:5: error: implicit declaration of function ‘set_pageblock_skip’ [-Werror=implicit-function-declaration]
>      set_pageblock_skip(page);
>      ^
> 
> CMA doesn't guarantee pageblock skip will get reset when migration and 
> freeing scanners meet, and pageblock skip is a CONFIG_COMPACTION only 
> feature, so disable it when CONFIG_COMPACTION=n.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

Yes, this passes the compilation test for me.

> ---
>  include/linux/pageblock-flags.h | 11 +++++++++++
>  mm/compaction.c                 |  8 +++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
> --- a/include/linux/pageblock-flags.h
> +++ b/include/linux/pageblock-flags.h
> @@ -96,6 +96,17 @@ void set_pfnblock_flags_mask(struct page *page,
>  #define set_pageblock_skip(page) \
>  			set_pageblock_flags_group(page, 1, PB_migrate_skip,  \
>  							PB_migrate_skip)
> +#else
> +static inline bool get_pageblock_skip(struct page *page)
> +{
> +	return false;
> +}
> +static inline void clear_pageblock_skip(struct page *page)
> +{
> +}
> +static inline void set_pageblock_skip(struct page *page)
> +{
> +}
>  #endif /* CONFIG_COMPACTION */
>  
>  #endif	/* PAGEBLOCK_FLAGS_H */
> diff --git a/mm/compaction.c b/mm/compaction.c
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -322,7 +322,13 @@ static inline bool isolation_suitable(struct compact_control *cc,
>  	return true;
>  }
>  
> -static void update_pageblock_skip(struct compact_control *cc,
> +static inline bool pageblock_skip_persistent(struct page *page,
> +					     unsigned int order)
> +{
> +	return false;
> +}
> +
> +static inline void update_pageblock_skip(struct compact_control *cc,
>  			struct page *page, unsigned long nr_isolated,
>  			bool migrate_scanner)
>  {


-- 
Michal Hocko
SUSE Labs

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

#1718164 — Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs pageblocks

On 08/16/2017 01:39 AM, David Rientjes wrote:
> It is pointless to migrate hugetlb memory as part of memory compaction if
> the hugetlb size is equal to the pageblock order.  No defragmentation is
> occurring in this condition.
> 
> It is also pointless to for the freeing scanner to scan a pageblock where
> a hugetlb page is pinned.  Unconditionally skip these pageblocks, and do
> so peristently so that they are not rescanned until it is observed that
> these hugepages are no longer pinned.
> 
> It would also be possible to do this by involving the hugetlb subsystem
> in marking pageblocks to no longer be skipped when they hugetlb pages are
> freed.  This is a simple solution that doesn't involve any additional
> subsystems in pageblock skip manipulation.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/compaction.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -217,6 +217,20 @@ static void reset_cached_positions(struct zone *zone)
>  				pageblock_start_pfn(zone_end_pfn(zone) - 1);
>  }
>  
> +/*
> + * Hugetlbfs pages should consistenly be skipped until updated by the hugetlb
> + * subsystem.  It is always pointless to compact pages of pageblock_order and
> + * the free scanner can reconsider when no longer huge.
> + */
> +static bool pageblock_skip_persistent(struct page *page, unsigned int order)
> +{
> +	if (!PageHuge(page))
> +		return false;
> +	if (order != pageblock_order)
> +		return false;
> +	return true;

Why just HugeTLBfs? There's also no point in migrating/finding free
pages in THPs. Actually, any compound page of pageblock order?

> +}
> +
>  /*
>   * This function is called to clear all cached information on pageblocks that
>   * should be skipped for page isolation when the migrate and free page scanner
> @@ -241,6 +255,8 @@ static void __reset_isolation_suitable(struct zone *zone)
>  			continue;
>  		if (zone != page_zone(page))
>  			continue;
> +		if (pageblock_skip_persistent(page, compound_order(page)))
> +			continue;

I like the idea of how persistency is achieved by rechecking in the reset.

>  
>  		clear_pageblock_skip(page);
>  	}
> @@ -448,13 +464,15 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>  		 * and the only danger is skipping too much.
>  		 */
>  		if (PageCompound(page)) {
> -			unsigned int comp_order = compound_order(page);
> -
> -			if (likely(comp_order < MAX_ORDER)) {
> -				blockpfn += (1UL << comp_order) - 1;
> -				cursor += (1UL << comp_order) - 1;
> +			const unsigned int order = compound_order(page);
> +
> +			if (pageblock_skip_persistent(page, order)) {
> +				set_pageblock_skip(page);
> +				blockpfn = end_pfn;
> +			} else if (likely(order < MAX_ORDER)) {
> +				blockpfn += (1UL << order) - 1;
> +				cursor += (1UL << order) - 1;
>  			}

Is this new code (and below) really necessary? The existing code should
already lead to skip bit being set via update_pageblock_skip()?

Thanks,
Vlastimil

> -
>  			goto isolate_fail;
>  		}
>  
> @@ -771,11 +789,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		 * danger is skipping too much.
>  		 */
>  		if (PageCompound(page)) {
> -			unsigned int comp_order = compound_order(page);
> -
> -			if (likely(comp_order < MAX_ORDER))
> -				low_pfn += (1UL << comp_order) - 1;
> +			const unsigned int order = compound_order(page);
>  
> +			if (pageblock_skip_persistent(page, order)) {
> +				set_pageblock_skip(page);
> +				low_pfn = end_pfn;
> +			} else if (likely(order < MAX_ORDER))
> +				low_pfn += (1UL << order) - 1;
>  			goto isolate_fail;
>  		}
>  
> @@ -837,7 +857,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  			 * is safe to read and it's 0 for tail pages.
>  			 */
>  			if (unlikely(PageCompound(page))) {
> -				low_pfn += (1UL << compound_order(page)) - 1;
> +				const unsigned int order = compound_order(page);
> +
> +				if (pageblock_skip_persistent(page, order)) {
> +					set_pageblock_skip(page);
> +					low_pfn = end_pfn;
> +				} else
> +					low_pfn += (1UL << order) - 1;
>  				goto isolate_fail;
>  			}
>  		}
> 

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

#1718137

On 08/16/2017 01:39 AM, David Rientjes wrote:
> Kcompactd is needlessly ignoring pageblock skip information.  It is doing
> MIGRATE_SYNC_LIGHT compaction, which is no more powerful than
> MIGRATE_SYNC compaction.
> 
> If compaction recently failed to isolate memory from a set of pageblocks,
> there is nothing to indicate that kcompactd will be able to do so, or
> that it is beneficial from attempting to isolate memory.
> 
> Use the pageblock skip hint to avoid rescanning pageblocks needlessly
> until that information is reset.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

It would be much better if patches like this were accompanied by some
numbers.

Also there's now a danger that in cases where there's no direct
compaction happening (just kcompactd), nothing will ever call
__reset_isolation_suitable().

> ---
>  mm/compaction.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1927,9 +1927,8 @@ static void kcompactd_do_work(pg_data_t *pgdat)
>  		.total_free_scanned = 0,
>  		.classzone_idx = pgdat->kcompactd_classzone_idx,
>  		.mode = MIGRATE_SYNC_LIGHT,
> -		.ignore_skip_hint = true,
> +		.ignore_skip_hint = false,
>  		.gfp_mask = GFP_KERNEL,
> -
>  	};
>  	trace_mm_compaction_kcompactd_wake(pgdat->node_id, cc.order,
>  							cc.classzone_idx);
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web