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

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

[David Rientjes]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1981 bytes --]

Fix build:

mm/compaction.c: In function a??isolate_freepages_blocka??:
mm/compaction.c:469:4: error: implicit declaration of function a??pageblock_skip_persistenta?? [-Werror=implicit-function-declaration]
    if (pageblock_skip_persistent(page, order)) {
    ^
mm/compaction.c:470:5: error: implicit declaration of function a??set_pageblock_skipa?? [-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)
 {

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

[Michal Hocko]

On Sun 20-08-17 17:36:41, David Rientjes wrote:
> Fix build:
> 
> mm/compaction.c: In function a??isolate_freepages_blocka??:
> mm/compaction.c:469:4: error: implicit declaration of function a??pageblock_skip_persistenta?? [-Werror=implicit-function-declaration]
>     if (pageblock_skip_persistent(page, order)) {
>     ^
> mm/compaction.c:470:5: error: implicit declaration of function a??set_pageblock_skipa?? [-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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Vlastimil Babka]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Vlastimil Babka]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Vlastimil Babka]

On 08/23/2017 10:41 AM, Vlastimil Babka wrote:
> 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()?
 
Ok, here's a patch implementing my suggestions.

----8<----

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

[David Rientjes]

On Wed, 23 Aug 2017, Vlastimil Babka wrote:

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

The numbers were from https://marc.info/?l=linux-mm&m=150231232707999 
where very large amounts (>90% of system RAM) were hugetlb pages.  We can 
supplement this changelog with the following if it helps:

"""
Currently, kcompactd ignores pageblock skip information that can become 
useful if it is known that memory should not be considered by both the 
migration and freeing scanners.  Abundant hugetlb memory is a good example 
of memory that is needlessly (and expensively) scanned since the hugepage 
order normally matches the pageblock order.

For example, on a sysctl with very large amounts of memory reserved by the 
hugetlb subsystem:

compact_migrate_scanned 2931254031294 
compact_free_scanned    102707804816705 
compact_isolated        1309145254 

Kcompactd ends up successfully isolating ~0.0012% of memory that is 
scans (the above does not involve direct compaction).

A follow-up change will set the pageblock skip for this memory since it is 
never useful for either scanner.
"""

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

I'm not sure that is helpful in a context where no high-order memory can 
call direct compaction that kcompactd needlessly scanning the same memory 
over and over is beneficial.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Wed, 23 Aug 2017, Vlastimil Babka wrote:

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

Yes, any page where compound_order(page) == pageblock_order would probably 
benefit from the same treatment.  I haven't encountered such an issue, 
however, so I thought it was best to restrict it only to hugetlb: hugetlb 
memory usually sits in the hugetlb free pool and seldom gets freed under 
normal conditions even when unmapped whereas thp is much more likely to be 
unmapped and split.  I wasn't sure that it was worth the pageblock skip.

> > +}
> > +
> >  /*
> >   * 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()?
> 

I wanted to set the persistent pageblock skip regardless of 
cc->ignore_skip_hint without a local change to update_pageblock_skip().

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Fri, 1 Sep 2017, Vlastimil Babka wrote:

> The pageblock_skip_persistent() function checks for HugeTLB pages of pageblock
> order. When clearing pageblock skip bits for compaction, the bits are not
> cleared for such pageblocks, because they cannot contain base pages suitable
> for migration, nor free pages to use as migration targets.
> 
> This optimization can be simply extended to all compound pages of order equal
> or larger than pageblock order, because migrating such pages (if they support
> it) cannot help sub-pageblock fragmentation. This includes THP's and also
> gigantic HugeTLB pages, which the current implementation doesn't persistently
> skip due to a strict pageblock_order equality check and not recognizing tail
> pages.
> 
> Additionally, this patch removes the pageblock_skip_persistent() calls from
> migration and free scanner, since the generic compound page treatment together
> with update_pageblock_skip() call will also lead to pageblocks starting with a
> large enough compound page being immediately marked for skipping, which then
> becomes persistent.
> 

As mentioned in my other two emails, I'm not sure that persistently 
skipping thp memory is necessary and I disagree that we should not be 
persistently skipping pageblocks when cc->ignore_skip_hint is true.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Vlastimil Babka]

On 09/11/2017 03:07 AM, David Rientjes wrote:
> On Wed, 23 Aug 2017, Vlastimil Babka wrote:
> 
>> 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.
>>
> 
> The numbers were from https://marc.info/?l=linux-mm&m=150231232707999 
> where very large amounts (>90% of system RAM) were hugetlb pages.  We can 
> supplement this changelog with the following if it helps:
> 
> """
> Currently, kcompactd ignores pageblock skip information that can become 
> useful if it is known that memory should not be considered by both the 
> migration and freeing scanners.  Abundant hugetlb memory is a good example 
> of memory that is needlessly (and expensively) scanned since the hugepage 
> order normally matches the pageblock order.
> 
> For example, on a sysctl with very large amounts of memory reserved by the 
> hugetlb subsystem:
> 
> compact_migrate_scanned 2931254031294 
> compact_free_scanned    102707804816705 
> compact_isolated        1309145254 
> 
> Kcompactd ends up successfully isolating ~0.0012% of memory that is 
> scans (the above does not involve direct compaction).

Yeah it would be nice to have the numbers also after the patch and to
see also effect on the kcompactd success rate.

> A follow-up change will set the pageblock skip for this memory since it is 
> never useful for either scanner.
> """
> 
>> 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().
>>
> 
> I'm not sure that is helpful in a context where no high-order memory can 
> call direct compaction that kcompactd needlessly scanning the same memory 
> over and over is beneficial.

The point is that if it becomes beneficial again, we won't know as there
will be still be skip bits.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Vlastimil Babka]

On 09/11/2017 03:12 AM, David Rientjes wrote:
> On Wed, 23 Aug 2017, Vlastimil Babka wrote:
> 
>>> 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?
>>
> 
> Yes, any page where compound_order(page) == pageblock_order would probably 
> benefit from the same treatment.  I haven't encountered such an issue, 
> however, so I thought it was best to restrict it only to hugetlb: hugetlb 
> memory usually sits in the hugetlb free pool and seldom gets freed under 
> normal conditions even when unmapped whereas thp is much more likely to be 
> unmapped and split.  I wasn't sure that it was worth the pageblock skip.

Well, my thinking is that once we start checking page properties when
resetting the skip bits, we might as well try to get the most of it, as
there's no additional cost.

>>> +}
>>> +
>>>  /*
>>>   * 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()?
>>
> 
> I wanted to set the persistent pageblock skip regardless of 
> cc->ignore_skip_hint without a local change to update_pageblock_skip().

After the first patch, there are no ignore_skip_hint users where it
would make that much difference overriding the flag for some pageblocks
(which this effectively does) at the cost of more complicated code.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Mon, 11 Sep 2017, Vlastimil Babka wrote:

> > Yes, any page where compound_order(page) == pageblock_order would probably 
> > benefit from the same treatment.  I haven't encountered such an issue, 
> > however, so I thought it was best to restrict it only to hugetlb: hugetlb 
> > memory usually sits in the hugetlb free pool and seldom gets freed under 
> > normal conditions even when unmapped whereas thp is much more likely to be 
> > unmapped and split.  I wasn't sure that it was worth the pageblock skip.
> 
> Well, my thinking is that once we start checking page properties when
> resetting the skip bits, we might as well try to get the most of it, as
> there's no additional cost.
> 

There's no additional cost, but I have doubts of how persistent the 
conditions you're checking really are.  I know that hugetlb memory 
normally sits in a hugetlb free pool when unmapped by a user process, very 
different from thp memory that can always be unmapped and split.  I would 
consider PageHuge() to be inferred as a more persistent condition than thp 
memory.

> >>> @@ -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()?
> >>
> > 
> > I wanted to set the persistent pageblock skip regardless of 
> > cc->ignore_skip_hint without a local change to update_pageblock_skip().
> 
> After the first patch, there are no ignore_skip_hint users where it
> would make that much difference overriding the flag for some pageblocks
> (which this effectively does) at the cost of more complicated code.
> 

No objection to a patch that sets the skip only as part of 
update_pageblock_skip(), but that is not combined with changing the
pageblock_skip_persistent() logic, which is a separate issue.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Mon, 11 Sep 2017, Vlastimil Babka wrote:

> > A follow-up change will set the pageblock skip for this memory since it is 
> > never useful for either scanner.
> > """
> > 
> >> 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().
> >>
> > 
> > I'm not sure that is helpful in a context where no high-order memory can 
> > call direct compaction that kcompactd needlessly scanning the same memory 
> > over and over is beneficial.
> 
> The point is that if it becomes beneficial again, we won't know as there
> will be still be skip bits.
> 

Why is kcompactd_do_work() not sometimes doing 
__reset_isolation_suitable() in the first place, if only to reset the 
per-zone migration and freeing scanner cached pfns?  It seems fragile to 
rely on other threads doing direct compaction to reset the per-zone state 
of compaction.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>