[PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[Baolin Wang]

It can not improve the fragmentation if we isolate the target free pages
exceeding cc->order, especially when the cc->order is less than pageblock_order.
For example, suppose the pageblock_order is MAX_ORDER (size is 4M) and cc->order
is 2M THP size, we should not isolate other 2M free pages to be the migration
target, which can not improve the fragmentation.

Moreover this is also applicable for large folio compaction.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 27ada42924d5..066b72b3471a 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1346,12 +1346,14 @@ static bool suitable_migration_target(struct compact_control *cc,
 {
 	/* If the page is a large free page, then disallow migration */
 	if (PageBuddy(page)) {
+		int order = cc->order > 0 ? cc->order : pageblock_order;
+
 		/*
 		 * We are checking page_order without zone->lock taken. But
 		 * the only small danger is that we skip a potentially suitable
 		 * pageblock, so it's not worth to check order for valid range.
 		 */
-		if (buddy_order_unsafe(page) >= pageblock_order)
+		if (buddy_order_unsafe(page) >= order)
 			return false;
 	}
 
-- 
2.39.3

[PATCH 2/2] mm: compaction: update the cc->nr_migratepages when allocating or freeing the freepages

[Baolin Wang]

Currently we will use 'cc->nr_freepages >= cc->nr_migratepages' comparison
to ensure that enough freepages are isolated in isolate_freepages(), however
it just decreases the cc->nr_freepages without updating cc->nr_migratepages
in compaction_alloc(), which will waste more CPU cycles and cause too many
freepages to be isolated.

So we should also update the cc->nr_migratepages when allocating or freeing
the freepages to avoid isolating excess freepages. And I can see fewer free
pages are scanned and isolated when running thpcompact on my Arm64 server:
                                       k6.7         k6.7_patched
Ops Compaction pages isolated      120692036.00   118160797.00
Ops Compaction migrate scanned     131210329.00   154093268.00
Ops Compaction free scanned       1090587971.00  1080632536.00
Ops Compact scan efficiency               12.03          14.26

Moreover, I did not see an obvious latency improvements, this is likely because
isolating freepages is not the bottleneck in the thpcompact test case.
                              k6.7                  k6.7_patched
Amean     fault-both-1      1089.76 (   0.00%)     1080.16 *   0.88%*
Amean     fault-both-3      1616.48 (   0.00%)     1636.65 *  -1.25%*
Amean     fault-both-5      2266.66 (   0.00%)     2219.20 *   2.09%*
Amean     fault-both-7      2909.84 (   0.00%)     2801.90 *   3.71%*
Amean     fault-both-12     4861.26 (   0.00%)     4733.25 *   2.63%*
Amean     fault-both-18     7351.11 (   0.00%)     6950.51 *   5.45%*
Amean     fault-both-24     9059.30 (   0.00%)     9159.99 *  -1.11%*
Amean     fault-both-30    10685.68 (   0.00%)    11399.02 *  -6.68%*

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/compaction.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/compaction.c b/mm/compaction.c
index 066b72b3471a..6c84e3a5b32b 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1779,6 +1779,7 @@ static struct folio *compaction_alloc(struct folio *src, unsigned long data)
 	dst = list_entry(cc->freepages.next, struct folio, lru);
 	list_del(&dst->lru);
 	cc->nr_freepages--;
+	cc->nr_migratepages--;
 
 	return dst;
 }
@@ -1794,6 +1795,7 @@ static void compaction_free(struct folio *dst, unsigned long data)
 
 	list_add(&dst->lru, &cc->freepages);
 	cc->nr_freepages++;
+	cc->nr_migratepages++;
 }
 
 /* possible outcome of isolate_migratepages */
-- 
2.39.3

Re: [PATCH 2/2] mm: compaction: update the cc->nr_migratepages when allocating or freeing the freepages

[Mel Gorman]

On Mon, Jan 22, 2024 at 09:01:54PM +0800, Baolin Wang wrote:
> Currently we will use 'cc->nr_freepages >= cc->nr_migratepages' comparison
> to ensure that enough freepages are isolated in isolate_freepages(), however
> it just decreases the cc->nr_freepages without updating cc->nr_migratepages
> in compaction_alloc(), which will waste more CPU cycles and cause too many
> freepages to be isolated.
> 
> So we should also update the cc->nr_migratepages when allocating or freeing
> the freepages to avoid isolating excess freepages. And I can see fewer free
> pages are scanned and isolated when running thpcompact on my Arm64 server:
>                                        k6.7         k6.7_patched
> Ops Compaction pages isolated      120692036.00   118160797.00
> Ops Compaction migrate scanned     131210329.00   154093268.00
> Ops Compaction free scanned       1090587971.00  1080632536.00
> Ops Compact scan efficiency               12.03          14.26
> 
> Moreover, I did not see an obvious latency improvements, this is likely because
> isolating freepages is not the bottleneck in the thpcompact test case.
>                               k6.7                  k6.7_patched
> Amean     fault-both-1      1089.76 (   0.00%)     1080.16 *   0.88%*
> Amean     fault-both-3      1616.48 (   0.00%)     1636.65 *  -1.25%*
> Amean     fault-both-5      2266.66 (   0.00%)     2219.20 *   2.09%*
> Amean     fault-both-7      2909.84 (   0.00%)     2801.90 *   3.71%*
> Amean     fault-both-12     4861.26 (   0.00%)     4733.25 *   2.63%*
> Amean     fault-both-18     7351.11 (   0.00%)     6950.51 *   5.45%*
> Amean     fault-both-24     9059.30 (   0.00%)     9159.99 *  -1.11%*
> Amean     fault-both-30    10685.68 (   0.00%)    11399.02 *  -6.68%*
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 2/2] mm: compaction: update the cc->nr_migratepages when allocating or freeing the freepages

[Vlastimil Babka]

On 1/22/24 14:01, Baolin Wang wrote:
> Currently we will use 'cc->nr_freepages >= cc->nr_migratepages' comparison
> to ensure that enough freepages are isolated in isolate_freepages(), however
> it just decreases the cc->nr_freepages without updating cc->nr_migratepages
> in compaction_alloc(), which will waste more CPU cycles and cause too many
> freepages to be isolated.

Hm yeah I guess this can happen with fast_isolate_freepages() if it returns
with something but not all the freepages that are expected to be needed, and
then we get to isolate_freepages() again.

> So we should also update the cc->nr_migratepages when allocating or freeing
> the freepages to avoid isolating excess freepages. And I can see fewer free
> pages are scanned and isolated when running thpcompact on my Arm64 server:
>                                        k6.7         k6.7_patched
> Ops Compaction pages isolated      120692036.00   118160797.00
> Ops Compaction migrate scanned     131210329.00   154093268.00
> Ops Compaction free scanned       1090587971.00  1080632536.00
> Ops Compact scan efficiency               12.03          14.26
> 
> Moreover, I did not see an obvious latency improvements, this is likely because
> isolating freepages is not the bottleneck in the thpcompact test case.
>                               k6.7                  k6.7_patched
> Amean     fault-both-1      1089.76 (   0.00%)     1080.16 *   0.88%*
> Amean     fault-both-3      1616.48 (   0.00%)     1636.65 *  -1.25%*
> Amean     fault-both-5      2266.66 (   0.00%)     2219.20 *   2.09%*
> Amean     fault-both-7      2909.84 (   0.00%)     2801.90 *   3.71%*
> Amean     fault-both-12     4861.26 (   0.00%)     4733.25 *   2.63%*
> Amean     fault-both-18     7351.11 (   0.00%)     6950.51 *   5.45%*
> Amean     fault-both-24     9059.30 (   0.00%)     9159.99 *  -1.11%*
> Amean     fault-both-30    10685.68 (   0.00%)    11399.02 *  -6.68%*
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/compaction.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 066b72b3471a..6c84e3a5b32b 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1779,6 +1779,7 @@ static struct folio *compaction_alloc(struct folio *src, unsigned long data)
>  	dst = list_entry(cc->freepages.next, struct folio, lru);
>  	list_del(&dst->lru);
>  	cc->nr_freepages--;
> +	cc->nr_migratepages--;

This is breaking the tracepoint TRACE_EVENT(mm_compaction_migratepages)
which does

__entry->nr_failed = cc->nr_migratepages - nr_succeeded;

and is called after migrate_pages() finishes, so now this will underflow.

Probably need to get a snapshot of cc->nr_migratepages before calling
migrate_pages() and then feed that to the tracepoint instead of cc.

>  
>  	return dst;
>  }
> @@ -1794,6 +1795,7 @@ static void compaction_free(struct folio *dst, unsigned long data)
>  
>  	list_add(&dst->lru, &cc->freepages);
>  	cc->nr_freepages++;
> +	cc->nr_migratepages++;
>  }
>  
>  /* possible outcome of isolate_migratepages */

Re: [PATCH 2/2] mm: compaction: update the cc->nr_migratepages when allocating or freeing the freepages

[Baolin Wang]

On 2024/2/12 18:32, Vlastimil Babka wrote:
> On 1/22/24 14:01, Baolin Wang wrote:
>> Currently we will use 'cc->nr_freepages >= cc->nr_migratepages' comparison
>> to ensure that enough freepages are isolated in isolate_freepages(), however
>> it just decreases the cc->nr_freepages without updating cc->nr_migratepages
>> in compaction_alloc(), which will waste more CPU cycles and cause too many
>> freepages to be isolated.
> 
> Hm yeah I guess this can happen with fast_isolate_freepages() if it returns
> with something but not all the freepages that are expected to be needed, and
> then we get to isolate_freepages() again.

Yes.

> 
>> So we should also update the cc->nr_migratepages when allocating or freeing
>> the freepages to avoid isolating excess freepages. And I can see fewer free
>> pages are scanned and isolated when running thpcompact on my Arm64 server:
>>                                         k6.7         k6.7_patched
>> Ops Compaction pages isolated      120692036.00   118160797.00
>> Ops Compaction migrate scanned     131210329.00   154093268.00
>> Ops Compaction free scanned       1090587971.00  1080632536.00
>> Ops Compact scan efficiency               12.03          14.26
>>
>> Moreover, I did not see an obvious latency improvements, this is likely because
>> isolating freepages is not the bottleneck in the thpcompact test case.
>>                                k6.7                  k6.7_patched
>> Amean     fault-both-1      1089.76 (   0.00%)     1080.16 *   0.88%*
>> Amean     fault-both-3      1616.48 (   0.00%)     1636.65 *  -1.25%*
>> Amean     fault-both-5      2266.66 (   0.00%)     2219.20 *   2.09%*
>> Amean     fault-both-7      2909.84 (   0.00%)     2801.90 *   3.71%*
>> Amean     fault-both-12     4861.26 (   0.00%)     4733.25 *   2.63%*
>> Amean     fault-both-18     7351.11 (   0.00%)     6950.51 *   5.45%*
>> Amean     fault-both-24     9059.30 (   0.00%)     9159.99 *  -1.11%*
>> Amean     fault-both-30    10685.68 (   0.00%)    11399.02 *  -6.68%*
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/compaction.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 066b72b3471a..6c84e3a5b32b 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1779,6 +1779,7 @@ static struct folio *compaction_alloc(struct folio *src, unsigned long data)
>>   	dst = list_entry(cc->freepages.next, struct folio, lru);
>>   	list_del(&dst->lru);
>>   	cc->nr_freepages--;
>> +	cc->nr_migratepages--;
> 
> This is breaking the tracepoint TRACE_EVENT(mm_compaction_migratepages)
> which does
> 
> __entry->nr_failed = cc->nr_migratepages - nr_succeeded;
> 
> and is called after migrate_pages() finishes, so now this will underflow.
> 
> Probably need to get a snapshot of cc->nr_migratepages before calling
> migrate_pages() and then feed that to the tracepoint instead of cc.

Ah, good catch. Will fix in next version. Thanks.

Re: [PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[Mel Gorman]

On Mon, Jan 22, 2024 at 09:01:53PM +0800, Baolin Wang wrote:
> It can not improve the fragmentation if we isolate the target free pages
> exceeding cc->order, especially when the cc->order is less than pageblock_order.
> For example, suppose the pageblock_order is MAX_ORDER (size is 4M) and cc->order
> is 2M THP size, we should not isolate other 2M free pages to be the migration
> target, which can not improve the fragmentation.
> 
> Moreover this is also applicable for large folio compaction.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[Vlastimil Babka]

On 1/22/24 14:01, Baolin Wang wrote:
> It can not improve the fragmentation if we isolate the target free pages
> exceeding cc->order, especially when the cc->order is less than pageblock_order.
> For example, suppose the pageblock_order is MAX_ORDER (size is 4M) and cc->order
> is 2M THP size, we should not isolate other 2M free pages to be the migration
> target, which can not improve the fragmentation.
> 
> Moreover this is also applicable for large folio compaction.

So why not Cc: Zi Yan? (done)

> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

I doubt this will make much difference, because if such a larger order free
page exists, we shouldn't have a reason to be compacting for a lower order
in the first place?

> ---
>  mm/compaction.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 27ada42924d5..066b72b3471a 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1346,12 +1346,14 @@ static bool suitable_migration_target(struct compact_control *cc,
>  {
>  	/* If the page is a large free page, then disallow migration */
>  	if (PageBuddy(page)) {
> +		int order = cc->order > 0 ? cc->order : pageblock_order;
> +
>  		/*
>  		 * We are checking page_order without zone->lock taken. But
>  		 * the only small danger is that we skip a potentially suitable
>  		 * pageblock, so it's not worth to check order for valid range.
>  		 */
> -		if (buddy_order_unsafe(page) >= pageblock_order)
> +		if (buddy_order_unsafe(page) >= order)
>  			return false;
>  	}
>  

Re: [PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

On 12 Feb 2024, at 4:13, Vlastimil Babka wrote:

> On 1/22/24 14:01, Baolin Wang wrote:
>> It can not improve the fragmentation if we isolate the target free pages
>> exceeding cc->order, especially when the cc->order is less than pageblock_order.
>> For example, suppose the pageblock_order is MAX_ORDER (size is 4M) and cc->order
>> is 2M THP size, we should not isolate other 2M free pages to be the migration
>> target, which can not improve the fragmentation.
>>
>> Moreover this is also applicable for large folio compaction.
>
> So why not Cc: Zi Yan? (done)
>

Thanks.

Hi Baolin,

How often do you see this happening?

>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>
> I doubt this will make much difference, because if such a larger order free
> page exists, we shouldn't have a reason to be compacting for a lower order
> in the first place?

Unless kswapd gets us such a free block in the background right after
get_page_from_freelist() and before compaction finishes in the allocation
slow path.

If this happens often and cc->order is not -1, it might be better to stop
compaction and get_page_from_freelist() to save cycles on unnecessary pfn
scanning. For completeness, when cc->order == -1, the logic does not change.


>
>> ---
>>  mm/compaction.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 27ada42924d5..066b72b3471a 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1346,12 +1346,14 @@ static bool suitable_migration_target(struct compact_control *cc,
>>  {
>>  	/* If the page is a large free page, then disallow migration */
>>  	if (PageBuddy(page)) {
>> +		int order = cc->order > 0 ? cc->order : pageblock_order;
>> +
>>  		/*
>>  		 * We are checking page_order without zone->lock taken. But
>>  		 * the only small danger is that we skip a potentially suitable
>>  		 * pageblock, so it's not worth to check order for valid range.
>>  		 */
>> -		if (buddy_order_unsafe(page) >= pageblock_order)
>> +		if (buddy_order_unsafe(page) >= order)
>>  			return false;
>>  	}
>>


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[Baolin Wang]

On 2024/2/12 23:00, Zi Yan wrote:
> On 12 Feb 2024, at 4:13, Vlastimil Babka wrote:
> 
>> On 1/22/24 14:01, Baolin Wang wrote:
>>> It can not improve the fragmentation if we isolate the target free pages
>>> exceeding cc->order, especially when the cc->order is less than pageblock_order.
>>> For example, suppose the pageblock_order is MAX_ORDER (size is 4M) and cc->order
>>> is 2M THP size, we should not isolate other 2M free pages to be the migration
>>> target, which can not improve the fragmentation.
>>>
>>> Moreover this is also applicable for large folio compaction.
>>
>> So why not Cc: Zi Yan? (done)
>>
> 
> Thanks.
> 
> Hi Baolin,
> 
> How often do you see this happening?

This is theoretically analyzed from the code inspection.

>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>
>> I doubt this will make much difference, because if such a larger order free
>> page exists, we shouldn't have a reason to be compacting for a lower order
>> in the first place?
> 
> Unless kswapd gets us such a free block in the background right after
> get_page_from_freelist() and before compaction finishes in the allocation
> slow path.
> 
> If this happens often and cc->order is not -1, it might be better to stop
> compaction and get_page_from_freelist() to save cycles on unnecessary pfn
> scanning. For completeness, when cc->order == -1, the logic does not change.

Yes, this is one possible case. There are also some other concurrent 
scenarios, such as when compaction is running (after 
compaction_suitable()), at the same time, other applications release a 
large folio to the free list. In this case, the free large folio 
scanning should also be avoided.

>>> ---
>>>   mm/compaction.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 27ada42924d5..066b72b3471a 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -1346,12 +1346,14 @@ static bool suitable_migration_target(struct compact_control *cc,
>>>   {
>>>   	/* If the page is a large free page, then disallow migration */
>>>   	if (PageBuddy(page)) {
>>> +		int order = cc->order > 0 ? cc->order : pageblock_order;
>>> +
>>>   		/*
>>>   		 * We are checking page_order without zone->lock taken. But
>>>   		 * the only small danger is that we skip a potentially suitable
>>>   		 * pageblock, so it's not worth to check order for valid range.
>>>   		 */
>>> -		if (buddy_order_unsafe(page) >= pageblock_order)
>>> +		if (buddy_order_unsafe(page) >= order)
>>>   			return false;
>>>   	}
>>>
> 
> 
> --
> Best Regards,
> Yan, Zi

Re: [PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[Andrew Morton]

On Mon, 19 Feb 2024 10:55:59 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> 
> 
> On 2024/2/12 23:00, Zi Yan wrote:
> > On 12 Feb 2024, at 4:13, Vlastimil Babka wrote:
> > 
> >> On 1/22/24 14:01, Baolin Wang wrote:
> >>> It can not improve the fragmentation if we isolate the target free pages
> >>> exceeding cc->order, especially when the cc->order is less than pageblock_order.
> >>> For example, suppose the pageblock_order is MAX_ORDER (size is 4M) and cc->order
> >>> is 2M THP size, we should not isolate other 2M free pages to be the migration
> >>> target, which can not improve the fragmentation.
> >>>
> >>> Moreover this is also applicable for large folio compaction.
> >>
> >> So why not Cc: Zi Yan? (done)
> >>
> > 
> > Thanks.
> > 
> > Hi Baolin,
> > 
> > How often do you see this happening?
> 
> This is theoretically analyzed from the code inspection.
> 
> >>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> >>
> >> I doubt this will make much difference, because if such a larger order free
> >> page exists, we shouldn't have a reason to be compacting for a lower order
> >> in the first place?
> > 
> > Unless kswapd gets us such a free block in the background right after
> > get_page_from_freelist() and before compaction finishes in the allocation
> > slow path.
> > 
> > If this happens often and cc->order is not -1, it might be better to stop
> > compaction and get_page_from_freelist() to save cycles on unnecessary pfn
> > scanning. For completeness, when cc->order == -1, the logic does not change.
> 
> Yes, this is one possible case. There are also some other concurrent 
> scenarios, such as when compaction is running (after 
> compaction_suitable()), at the same time, other applications release a 
> large folio to the free list. In this case, the free large folio 
> scanning should also be avoided.

This went quiet.

We have an ack from Mel.  Are people OK with sending this change
upstream?

Re: [PATCH 1/2] mm: compaction: limit the suitable target page order to be less than cc->order

[Vlastimil Babka]

On 2/21/24 23:15, Andrew Morton wrote:
> On Mon, 19 Feb 2024 10:55:59 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>> 
>> >>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> >>
>> >> I doubt this will make much difference, because if such a larger order free
>> >> page exists, we shouldn't have a reason to be compacting for a lower order
>> >> in the first place?
>> > 
>> > Unless kswapd gets us such a free block in the background right after
>> > get_page_from_freelist() and before compaction finishes in the allocation
>> > slow path.
>> > 
>> > If this happens often and cc->order is not -1, it might be better to stop
>> > compaction and get_page_from_freelist() to save cycles on unnecessary pfn
>> > scanning. For completeness, when cc->order == -1, the logic does not change.
>> 
>> Yes, this is one possible case. There are also some other concurrent 
>> scenarios, such as when compaction is running (after 
>> compaction_suitable()), at the same time, other applications release a 
>> large folio to the free list. In this case, the free large folio 
>> scanning should also be avoided.
> 
> This went quiet.
> 
> We have an ack from Mel.  Are people OK with sending this change
> upstream?

It's not wrong, so I'm OK.