[PATCH 0/2] mm: oom: terminate the oom_evaluate_task() loop early

[PATCH 0/2] mm: oom: terminate the oom_evaluate_task() loop early

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

This patch series improves the performance of select_bad_process() by
terminating the oom_evaluate_task() loop early (if points == LONG_MAX).

ZhangPeng (2):
  mm: oom: remove unnecessary goto in oom_evaluate_task()
  mm: oom: terminate the oom_evaluate_task() loop early

 mm/oom_kill.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/2] mm: oom: remove unnecessary goto in oom_evaluate_task()

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

Remove redundant goto statement in oom_evaluate_task() to simplify the
code a bit. No functional modification involved.

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
---
 mm/oom_kill.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 44bde56ecd02..10f7826c4035 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -335,14 +335,12 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 */
 	if (oom_task_origin(task)) {
 		points = LONG_MAX;
-		goto select;
+	} else {
+		points = oom_badness(task, oc->totalpages);
+		if (points == LONG_MIN || points < oc->chosen_points)
+			goto next;
 	}
 
-	points = oom_badness(task, oc->totalpages);
-	if (points == LONG_MIN || points < oc->chosen_points)
-		goto next;
-
-select:
 	if (oc->chosen)
 		put_task_struct(oc->chosen);
 	get_task_struct(task);
-- 
2.25.1

Re: [PATCH 1/2] mm: oom: remove unnecessary goto in oom_evaluate_task()

[Michal Hocko]

On Mon 14-08-23 14:34:27, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> Remove redundant goto statement in oom_evaluate_task() to simplify the
> code a bit. No functional modification involved.

Quite honestly, I do not see much point in changing the code this way.
We still have other goto labels and also there are other changes
happening where this label could be still benefitial [1]

[1] http://lkml.kernel.org/r/20230810081319.65668-2-zhouchuyi@bytedance.com

> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> ---
>  mm/oom_kill.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 44bde56ecd02..10f7826c4035 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -335,14 +335,12 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
>  	 */
>  	if (oom_task_origin(task)) {
>  		points = LONG_MAX;
> -		goto select;
> +	} else {
> +		points = oom_badness(task, oc->totalpages);
> +		if (points == LONG_MIN || points < oc->chosen_points)
> +			goto next;
>  	}
>  
> -	points = oom_badness(task, oc->totalpages);
> -	if (points == LONG_MIN || points < oc->chosen_points)
> -		goto next;
> -
> -select:
>  	if (oc->chosen)
>  		put_task_struct(oc->chosen);
>  	get_task_struct(task);
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: oom: remove unnecessary goto in oom_evaluate_task()

[zhangpeng (AS)]

On 2023/8/16 14:46, Michal Hocko wrote:

> On Mon 14-08-23 14:34:27, Peng Zhang wrote:
>> From: ZhangPeng <zhangpeng362@huawei.com>
>>
>> Remove redundant goto statement in oom_evaluate_task() to simplify the
>> code a bit. No functional modification involved.
> Quite honestly, I do not see much point in changing the code this way.
> We still have other goto labels and also there are other changes
> happening where this label could be still benefitial [1]
>
> [1] http://lkml.kernel.org/r/20230810081319.65668-2-zhouchuyi@bytedance.com

Yes. Sorry I didn't notice this before.

>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>> ---
>>   mm/oom_kill.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>> index 44bde56ecd02..10f7826c4035 100644
>> --- a/mm/oom_kill.c
>> +++ b/mm/oom_kill.c
>> @@ -335,14 +335,12 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
>>   	 */
>>   	if (oom_task_origin(task)) {
>>   		points = LONG_MAX;
>> -		goto select;
>> +	} else {
>> +		points = oom_badness(task, oc->totalpages);
>> +		if (points == LONG_MIN || points < oc->chosen_points)
>> +			goto next;
>>   	}
>>   
>> -	points = oom_badness(task, oc->totalpages);
>> -	if (points == LONG_MIN || points < oc->chosen_points)
>> -		goto next;
>> -
>> -select:
>>   	if (oc->chosen)
>>   		put_task_struct(oc->chosen);
>>   	get_task_struct(task);
>> -- 
>> 2.25.1

-- 
Best Regards,
Peng

[PATCH 2/2] mm: oom: terminate the oom_evaluate_task() loop early

[Peng Zhang]

From: ZhangPeng <zhangpeng362@huawei.com>

If task is allocating a lot of memory and has been marked to be killed
first, then it gets the highest score (LONG_MAX). Therefore, there is no
need to continue to calculate the points of other tasks. Just terminate
the oom_evaluate_task() loop early, when the task with the highest score
is found. By doing this, we can get some performance gains in
select_bad_process().
To implement it, the return value of oom_evaluate_task() is modified.
When the task with the highest score is found (points == LONG_MAX),
oom_evaluate_task() will return 1 and the loop will terminate early.

Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
---
 mm/oom_kill.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 10f7826c4035..02d11ae043aa 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -346,6 +346,8 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	get_task_struct(task);
 	oc->chosen = task;
 	oc->chosen_points = points;
+	if (points == LONG_MAX)
+		return 1;
 next:
 	return 0;
 abort:
-- 
2.25.1

Re: [PATCH 2/2] mm: oom: terminate the oom_evaluate_task() loop early

[Michal Hocko]

On Mon 14-08-23 14:34:28, Peng Zhang wrote:
> From: ZhangPeng <zhangpeng362@huawei.com>
> 
> If task is allocating a lot of memory and has been marked to be killed
> first, then it gets the highest score (LONG_MAX). Therefore, there is no
> need to continue to calculate the points of other tasks. Just terminate
> the oom_evaluate_task() loop early, when the task with the highest score
> is found. By doing this, we can get some performance gains in
> select_bad_process().

The point of evaluating all tasks is that there might be pre-existing
oom victim still being torn down. If you cut this short you might kill
more tasks than necessary.

So I do not think we want this patch.

> To implement it, the return value of oom_evaluate_task() is modified.
> When the task with the highest score is found (points == LONG_MAX),
> oom_evaluate_task() will return 1 and the loop will terminate early.
> 
> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
> ---
>  mm/oom_kill.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 10f7826c4035..02d11ae043aa 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -346,6 +346,8 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
>  	get_task_struct(task);
>  	oc->chosen = task;
>  	oc->chosen_points = points;
> +	if (points == LONG_MAX)
> +		return 1;
>  next:
>  	return 0;
>  abort:
> -- 
> 2.25.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] mm: oom: terminate the oom_evaluate_task() loop early

[zhangpeng (AS)]

On 2023/8/16 14:48, Michal Hocko wrote:

> On Mon 14-08-23 14:34:28, Peng Zhang wrote:
>> From: ZhangPeng <zhangpeng362@huawei.com>
>>
>> If task is allocating a lot of memory and has been marked to be killed
>> first, then it gets the highest score (LONG_MAX). Therefore, there is no
>> need to continue to calculate the points of other tasks. Just terminate
>> the oom_evaluate_task() loop early, when the task with the highest score
>> is found. By doing this, we can get some performance gains in
>> select_bad_process().
> The point of evaluating all tasks is that there might be pre-existing
> oom victim still being torn down. If you cut this short you might kill
> more tasks than necessary.

Indeed. Thanks for your feedback.

> So I do not think we want this patch.
>> To implement it, the return value of oom_evaluate_task() is modified.
>> When the task with the highest score is found (points == LONG_MAX),
>> oom_evaluate_task() will return 1 and the loop will terminate early.
>>
>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>> ---
>>   mm/oom_kill.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>> index 10f7826c4035..02d11ae043aa 100644
>> --- a/mm/oom_kill.c
>> +++ b/mm/oom_kill.c
>> @@ -346,6 +346,8 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
>>   	get_task_struct(task);
>>   	oc->chosen = task;
>>   	oc->chosen_points = points;
>> +	if (points == LONG_MAX)
>> +		return 1;
>>   next:
>>   	return 0;
>>   abort:
>> -- 
>> 2.25.1

-- 
Best Regards,
Peng