[v1] Fixes and cleanups to compaction

[PATCH 6/9] mm/compaction: rename is_via_compact_memory to compaction_with_allocation_order

Posted by Kemeng Shi 2 years, 6 months ago

We have order = -1 via proactive compaction, the is_via_compact_memory is
not proper name anymore.
As cc->order informs the compaction to satisfy a allocation with that
order, so rename it to compaction_with_allocation_order.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 mm/compaction.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index d8416d3dd445..b5a699ed526b 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2055,12 +2055,11 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 }
 
 /*
- * order == -1 is expected when compacting via
- * /proc/sys/vm/compact_memory
+ * compact to satisfy allocation with target order
  */
-static inline bool is_via_compact_memory(int order)
+static inline bool compaction_with_allocation_order(int order)
 {
-	return order == -1;
+	return order != -1;
 }
 
 /*
@@ -2200,7 +2199,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
 		goto out;
 	}
 
-	if (is_via_compact_memory(cc->order))
+	if (!compaction_with_allocation_order(cc->order))
 		return COMPACT_CONTINUE;
 
 	/*
@@ -2390,7 +2389,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
 
 	cc->migratetype = gfp_migratetype(cc->gfp_mask);
 
-	if (!is_via_compact_memory(cc->order)) {
+	if (compaction_with_allocation_order(cc->order)) {
 		unsigned long watermark;
 
 		/* Allocation can already succeed, nothing to do */
-- 
2.30.0

Re: [PATCH 6/9] mm/compaction: rename is_via_compact_memory to compaction_with_allocation_order

Posted by Baolin Wang 2 years, 5 months ago


On 8/5/2023 7:07 PM, Kemeng Shi wrote:
> We have order = -1 via proactive compaction, the is_via_compact_memory is
> not proper name anymore.
> As cc->order informs the compaction to satisfy a allocation with that
> order, so rename it to compaction_with_allocation_order.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>   mm/compaction.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index d8416d3dd445..b5a699ed526b 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2055,12 +2055,11 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>   }
>   
>   /*
> - * order == -1 is expected when compacting via
> - * /proc/sys/vm/compact_memory
> + * compact to satisfy allocation with target order
>    */
> -static inline bool is_via_compact_memory(int order)
> +static inline bool compaction_with_allocation_order(int order)

I know naming is hard, but this name is not good enough that can show 
the compaction mode. But the original one could.

>   {
> -	return order == -1;
> +	return order != -1;
>   }
>   
>   /*
> @@ -2200,7 +2199,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
>   		goto out;
>   	}
>   
> -	if (is_via_compact_memory(cc->order))
> +	if (!compaction_with_allocation_order(cc->order))
>   		return COMPACT_CONTINUE;
>   
>   	/*
> @@ -2390,7 +2389,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>   
>   	cc->migratetype = gfp_migratetype(cc->gfp_mask);
>   
> -	if (!is_via_compact_memory(cc->order)) {
> +	if (compaction_with_allocation_order(cc->order)) {
>   		unsigned long watermark;
>   
>   		/* Allocation can already succeed, nothing to do */

Re: [PATCH 6/9] mm/compaction: rename is_via_compact_memory to compaction_with_allocation_order

Posted by Kemeng Shi 2 years, 5 months ago


on 8/15/2023 4:58 PM, Baolin Wang wrote:
> 
> 
> On 8/5/2023 7:07 PM, Kemeng Shi wrote:
>> We have order = -1 via proactive compaction, the is_via_compact_memory is
>> not proper name anymore.
>> As cc->order informs the compaction to satisfy a allocation with that
>> order, so rename it to compaction_with_allocation_order.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>   mm/compaction.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index d8416d3dd445..b5a699ed526b 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -2055,12 +2055,11 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>>   }
>>     /*
>> - * order == -1 is expected when compacting via
>> - * /proc/sys/vm/compact_memory
>> + * compact to satisfy allocation with target order
>>    */
>> -static inline bool is_via_compact_memory(int order)
>> +static inline bool compaction_with_allocation_order(int order)
> 
> I know naming is hard, but this name is not good enough that can show the compaction mode. But the original one could.
> 
Yes, I agree with this, but name and comment of is_via_compact_memory may
mislead reader that order == -1 is equivalent to compaction from
/proc/sys/vm/compact_memory.
Actually, we have several approaches to trigger compaction with order == -1:
1. via /proc/sys/vm/compact_memory
2. via /sys/devices/system/node/nodex/compact
3. via proactive compact

Instead of indicate compaction is tirggerred by compact_memocy or anything,
order == -1 implies if compaction is triggerrred to meet allocation with high
order and we will stop compaction if allocation with target order will success.

>>   {
>> -    return order == -1;
>> +    return order != -1;
>>   }
>>     /*
>> @@ -2200,7 +2199,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
>>           goto out;
>>       }
>>   -    if (is_via_compact_memory(cc->order))
>> +    if (!compaction_with_allocation_order(cc->order))
>>           return COMPACT_CONTINUE;
>>         /*
>> @@ -2390,7 +2389,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>>         cc->migratetype = gfp_migratetype(cc->gfp_mask);
>>   -    if (!is_via_compact_memory(cc->order)) {
>> +    if (compaction_with_allocation_order(cc->order)) {
>>           unsigned long watermark;
>>             /* Allocation can already succeed, nothing to do */
>

Re: [PATCH 6/9] mm/compaction: rename is_via_compact_memory to compaction_with_allocation_order

Posted by Baolin Wang 2 years, 5 months ago


On 8/15/2023 8:04 PM, Kemeng Shi wrote:
> 
> 
> on 8/15/2023 4:58 PM, Baolin Wang wrote:
>>
>>
>> On 8/5/2023 7:07 PM, Kemeng Shi wrote:
>>> We have order = -1 via proactive compaction, the is_via_compact_memory is
>>> not proper name anymore.
>>> As cc->order informs the compaction to satisfy a allocation with that
>>> order, so rename it to compaction_with_allocation_order.
>>>
>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>> ---
>>>    mm/compaction.c | 11 +++++------
>>>    1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index d8416d3dd445..b5a699ed526b 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -2055,12 +2055,11 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>>>    }
>>>      /*
>>> - * order == -1 is expected when compacting via
>>> - * /proc/sys/vm/compact_memory
>>> + * compact to satisfy allocation with target order
>>>     */
>>> -static inline bool is_via_compact_memory(int order)
>>> +static inline bool compaction_with_allocation_order(int order)
>>
>> I know naming is hard, but this name is not good enough that can show the compaction mode. But the original one could.
>>
> Yes, I agree with this, but name and comment of is_via_compact_memory may
> mislead reader that order == -1 is equivalent to compaction from
> /proc/sys/vm/compact_memory.
> Actually, we have several approaches to trigger compaction with order == -1:
> 1. via /proc/sys/vm/compact_memory
> 2. via /sys/devices/system/node/nodex/compact
> 3. via proactive compact

They can all be called proactive compaction.

> 
> Instead of indicate compaction is tirggerred by compact_memocy or anything,
> order == -1 implies if compaction is triggerrred to meet allocation with high
> order and we will stop compaction if allocation with target order will success.

IMO, the is_via_compact_memory() function helps people better 
distinguish the compaction logic we have under direct compaction or 
kcompactd compaction, while proactive compaction does not concern itself 
with these details. But compaction_with_allocation_order() will make me 
just wonder why we should compare with -1. So I don't think this patch 
is worth it, but as you said above, we can add more comments to make it 
more clear.

>>>    {
>>> -    return order == -1;
>>> +    return order != -1;
>>>    }
>>>      /*
>>> @@ -2200,7 +2199,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
>>>            goto out;
>>>        }
>>>    -    if (is_via_compact_memory(cc->order))
>>> +    if (!compaction_with_allocation_order(cc->order))
>>>            return COMPACT_CONTINUE;
>>>          /*
>>> @@ -2390,7 +2389,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>>>          cc->migratetype = gfp_migratetype(cc->gfp_mask);
>>>    -    if (!is_via_compact_memory(cc->order)) {
>>> +    if (compaction_with_allocation_order(cc->order)) {
>>>            unsigned long watermark;
>>>              /* Allocation can already succeed, nothing to do */
>>

Re: [PATCH 6/9] mm/compaction: rename is_via_compact_memory to compaction_with_allocation_order

Posted by Kemeng Shi 2 years, 5 months ago


on 8/19/2023 8:14 PM, Baolin Wang wrote:
> 
> 
> On 8/15/2023 8:04 PM, Kemeng Shi wrote:
>>
>>
>> on 8/15/2023 4:58 PM, Baolin Wang wrote:
>>>
>>>
>>> On 8/5/2023 7:07 PM, Kemeng Shi wrote:
>>>> We have order = -1 via proactive compaction, the is_via_compact_memory is
>>>> not proper name anymore.
>>>> As cc->order informs the compaction to satisfy a allocation with that
>>>> order, so rename it to compaction_with_allocation_order.
>>>>
>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>>> ---
>>>>    mm/compaction.c | 11 +++++------
>>>>    1 file changed, 5 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>> index d8416d3dd445..b5a699ed526b 100644
>>>> --- a/mm/compaction.c
>>>> +++ b/mm/compaction.c
>>>> @@ -2055,12 +2055,11 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>>>>    }
>>>>      /*
>>>> - * order == -1 is expected when compacting via
>>>> - * /proc/sys/vm/compact_memory
>>>> + * compact to satisfy allocation with target order
>>>>     */
>>>> -static inline bool is_via_compact_memory(int order)
>>>> +static inline bool compaction_with_allocation_order(int order)
>>>
>>> I know naming is hard, but this name is not good enough that can show the compaction mode. But the original one could.
>>>
>> Yes, I agree with this, but name and comment of is_via_compact_memory may
>> mislead reader that order == -1 is equivalent to compaction from
>> /proc/sys/vm/compact_memory.
>> Actually, we have several approaches to trigger compaction with order == -1:
>> 1. via /proc/sys/vm/compact_memory
>> 2. via /sys/devices/system/node/nodex/compact
>> 3. via proactive compact
> 
> They can all be called proactive compaction.
I have considered rename to is_proactive_compaction. But "proactive compaction"
in comments of compaction.c mostly implies to compaction triggerred from
/proc/sys/vm/compaction_proactiveness. So "proactive compaction" itself looks
ambiguous...
> 
>>
>> Instead of indicate compaction is tirggerred by compact_memocy or anything,
>> order == -1 implies if compaction is triggerrred to meet allocation with high
>> order and we will stop compaction if allocation with target order will success.
> 
> IMO, the is_via_compact_memory() function helps people better distinguish the compaction logic we have under direct compaction or kcompactd compaction, while proactive compaction does not concern itself with these details. But compaction_with_allocation_order() will make me just wonder why we should compare with -1. So I don't think this patch is worth it, but as you said above, we can add more comments to make it more clear.
> 
Sure, no insistant on this.
Is it looks good to you just change comment of is_via_compact_memory to:
We need do compaction proactively with order == -1
order == -1 is expected for proactive compaction via:
1. via /proc/sys/vm/compact_memory
2. via /sys/devices/system/node/nodex/compact
3. /proc/sys/vm/compaction_proactiveness

>>>>    {
>>>> -    return order == -1;
>>>> +    return order != -1;
>>>>    }
>>>>      /*
>>>> @@ -2200,7 +2199,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
>>>>            goto out;
>>>>        }
>>>>    -    if (is_via_compact_memory(cc->order))
>>>> +    if (!compaction_with_allocation_order(cc->order))
>>>>            return COMPACT_CONTINUE;
>>>>          /*
>>>> @@ -2390,7 +2389,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>>>>          cc->migratetype = gfp_migratetype(cc->gfp_mask);
>>>>    -    if (!is_via_compact_memory(cc->order)) {
>>>> +    if (compaction_with_allocation_order(cc->order)) {
>>>>            unsigned long watermark;
>>>>              /* Allocation can already succeed, nothing to do */
>>>
> 
>

Re: [PATCH 6/9] mm/compaction: rename is_via_compact_memory to compaction_with_allocation_order

Posted by Baolin Wang 2 years, 5 months ago


On 8/22/2023 9:51 AM, Kemeng Shi wrote:
> 
> 
> on 8/19/2023 8:14 PM, Baolin Wang wrote:
>>
>>
>> On 8/15/2023 8:04 PM, Kemeng Shi wrote:
>>>
>>>
>>> on 8/15/2023 4:58 PM, Baolin Wang wrote:
>>>>
>>>>
>>>> On 8/5/2023 7:07 PM, Kemeng Shi wrote:
>>>>> We have order = -1 via proactive compaction, the is_via_compact_memory is
>>>>> not proper name anymore.
>>>>> As cc->order informs the compaction to satisfy a allocation with that
>>>>> order, so rename it to compaction_with_allocation_order.
>>>>>
>>>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>>>>> ---
>>>>>     mm/compaction.c | 11 +++++------
>>>>>     1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>>> index d8416d3dd445..b5a699ed526b 100644
>>>>> --- a/mm/compaction.c
>>>>> +++ b/mm/compaction.c
>>>>> @@ -2055,12 +2055,11 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
>>>>>     }
>>>>>       /*
>>>>> - * order == -1 is expected when compacting via
>>>>> - * /proc/sys/vm/compact_memory
>>>>> + * compact to satisfy allocation with target order
>>>>>      */
>>>>> -static inline bool is_via_compact_memory(int order)
>>>>> +static inline bool compaction_with_allocation_order(int order)
>>>>
>>>> I know naming is hard, but this name is not good enough that can show the compaction mode. But the original one could.
>>>>
>>> Yes, I agree with this, but name and comment of is_via_compact_memory may
>>> mislead reader that order == -1 is equivalent to compaction from
>>> /proc/sys/vm/compact_memory.
>>> Actually, we have several approaches to trigger compaction with order == -1:
>>> 1. via /proc/sys/vm/compact_memory
>>> 2. via /sys/devices/system/node/nodex/compact
>>> 3. via proactive compact
>>
>> They can all be called proactive compaction.
> I have considered rename to is_proactive_compaction. But "proactive compaction"
> in comments of compaction.c mostly implies to compaction triggerred from
> /proc/sys/vm/compaction_proactiveness. So "proactive compaction" itself looks
> ambiguous...
>>
>>>
>>> Instead of indicate compaction is tirggerred by compact_memocy or anything,
>>> order == -1 implies if compaction is triggerrred to meet allocation with high
>>> order and we will stop compaction if allocation with target order will success.
>>
>> IMO, the is_via_compact_memory() function helps people better distinguish the compaction logic we have under direct compaction or kcompactd compaction, while proactive compaction does not concern itself with these details. But compaction_with_allocation_order() will make me just wonder why we should compare with -1. So I don't think this patch is worth it, but as you said above, we can add more comments to make it more clear.
>>
> Sure, no insistant on this.
> Is it looks good to you just change comment of is_via_compact_memory to:
> We need do compaction proactively with order == -1
> order == -1 is expected for proactive compaction via:
> 1. via /proc/sys/vm/compact_memory
> 2. via /sys/devices/system/node/nodex/compact
> 3. /proc/sys/vm/compaction_proactiveness

Look good to me. Thanks.

> 
>>>>>     {
>>>>> -    return order == -1;
>>>>> +    return order != -1;
>>>>>     }
>>>>>       /*
>>>>> @@ -2200,7 +2199,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
>>>>>             goto out;
>>>>>         }
>>>>>     -    if (is_via_compact_memory(cc->order))
>>>>> +    if (!compaction_with_allocation_order(cc->order))
>>>>>             return COMPACT_CONTINUE;
>>>>>           /*
>>>>> @@ -2390,7 +2389,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc)
>>>>>           cc->migratetype = gfp_migratetype(cc->gfp_mask);
>>>>>     -    if (!is_via_compact_memory(cc->order)) {
>>>>> +    if (compaction_with_allocation_order(cc->order)) {
>>>>>             unsigned long watermark;
>>>>>               /* Allocation can already succeed, nothing to do */
>>>>
>>
>>