The comment indicates that when searching for a suitable NUMA node, we
should ensure that the selected node benefits both the task and its NUMA
group. However, the current implementation can only guarantee that either
the task or the group benefits, but not necessarily both.

Signed-off-by: Jianyong Wu <wujianyong@hygon.cn>
---
 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b173a059315c..58c899738399 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2568,7 +2568,7 @@ static int task_numa_migrate(struct task_struct *p)
 			/* Only consider nodes where both task and groups benefit */
 			taskimp = task_weight(p, nid, dist) - taskweight;
 			groupimp = group_weight(p, nid, dist) - groupweight;
-			if (taskimp < 0 && groupimp < 0)
+			if (taskimp < 0 || groupimp < 0)
 				continue;
 
 			env.dist = dist;
-- 
2.43.0

On 8/29/2025 4:55 PM, Jianyong Wu wrote:
> The comment indicates that when searching for a suitable NUMA node, we
> should ensure that the selected node benefits both the task and its NUMA
> group. However, the current implementation can only guarantee that either
> the task or the group benefits, but not necessarily both.
> 
> Signed-off-by: Jianyong Wu <wujianyong@hygon.cn>
> ---
>   kernel/sched/fair.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index b173a059315c..58c899738399 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2568,7 +2568,7 @@ static int task_numa_migrate(struct task_struct *p)
>   			/* Only consider nodes where both task and groups benefit */
>   			taskimp = task_weight(p, nid, dist) - taskweight;
>   			groupimp = group_weight(p, nid, dist) - groupweight;
> -			if (taskimp < 0 && groupimp < 0)
> +			if (taskimp < 0 || groupimp < 0) 
Perhaps you misunderstand the comment, && means either the task or the
group has NO benefit from this migration, it wouldn't be done.
But if you replace it with ||, you will ignore the target node that
could benefit either the task or the group.

There is more logic to consider the benefit for both task & group
in the later function part.

One question, why not
if (taskimp <= 0 && groupimp <= 0) ?

Thanks,
Ethan
>   				continue;
>   
>   			env.dist = dist;

Hello Ethan,

Thanks for reply.

There is inconsistency between the comments and the code. See the discussion in an older patch here https://lkml.org/lkml/2015/6/16/540

Following that, the issue is with the comments, not the code.

Bests
Jianyong 

> -----Original Message-----
> From: Ethan Zhao <etzhao1900@gmail.com>
> Sent: Friday, September 5, 2025 9:13 AM
> To: Jianyong Wu <wujianyong@hygon.cn>
> Cc: jianyong.wu@outlook.com; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] sched/fair: fix task_numa_migrate to consider both task
> and group benefits
> 
> 
> 
> On 8/29/2025 4:55 PM, Jianyong Wu wrote:
> > The comment indicates that when searching for a suitable NUMA node, we
> > should ensure that the selected node benefits both the task and its
> > NUMA group. However, the current implementation can only guarantee
> > that either the task or the group benefits, but not necessarily both.
> >
> > Signed-off-by: Jianyong Wu <wujianyong@hygon.cn>
> > ---
> >   kernel/sched/fair.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index
> > b173a059315c..58c899738399 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -2568,7 +2568,7 @@ static int task_numa_migrate(struct task_struct
> *p)
> >   			/* Only consider nodes where both task and groups benefit
> */
> >   			taskimp = task_weight(p, nid, dist) - taskweight;
> >   			groupimp = group_weight(p, nid, dist) - groupweight;
> > -			if (taskimp < 0 && groupimp < 0)
> > +			if (taskimp < 0 || groupimp < 0)
> Perhaps you misunderstand the comment, && means either the task or the
> group has NO benefit from this migration, it wouldn't be done.
> But if you replace it with ||, you will ignore the target node that could benefit
> either the task or the group.
> 
> There is more logic to consider the benefit for both task & group in the later
> function part.
> 
> One question, why not
> if (taskimp <= 0 && groupimp <= 0) ?
> 
> Thanks,
> Ethan
> >   				continue;
> >
> >   			env.dist = dist;
> 

On 9/5/2025 10:14 AM, Jianyong Wu wrote:
> Hello Ethan,
> 
> Thanks for reply.
> 
> There is inconsistency between the comments and the code. See the discussion in an older patch here https://lkml.org/lkml/2015/6/16/540
The literal meaning of the comment appears somewhat misleading,
we always need to understand it combined with code context. or
needs more wording effort.

Thanks,
Ethan>
> Following that, the issue is with the comments, not the code.
> 
> Bests
> Jianyong
> 
>> -----Original Message-----
>> From: Ethan Zhao <etzhao1900@gmail.com>
>> Sent: Friday, September 5, 2025 9:13 AM
>> To: Jianyong Wu <wujianyong@hygon.cn>
>> Cc: jianyong.wu@outlook.com; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] sched/fair: fix task_numa_migrate to consider both task
>> and group benefits
>>
>>
>>
>> On 8/29/2025 4:55 PM, Jianyong Wu wrote:
>>> The comment indicates that when searching for a suitable NUMA node, we
>>> should ensure that the selected node benefits both the task and its
>>> NUMA group. However, the current implementation can only guarantee
>>> that either the task or the group benefits, but not necessarily both.
>>>
>>> Signed-off-by: Jianyong Wu <wujianyong@hygon.cn>
>>> ---
>>>    kernel/sched/fair.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index
>>> b173a059315c..58c899738399 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -2568,7 +2568,7 @@ static int task_numa_migrate(struct task_struct
>> *p)
>>>    			/* Only consider nodes where both task and groups benefit
>> */
>>>    			taskimp = task_weight(p, nid, dist) - taskweight;
>>>    			groupimp = group_weight(p, nid, dist) - groupweight;
>>> -			if (taskimp < 0 && groupimp < 0)
>>> +			if (taskimp < 0 || groupimp < 0)
>> Perhaps you misunderstand the comment, && means either the task or the
>> group has NO benefit from this migration, it wouldn't be done.
>> But if you replace it with ||, you will ignore the target node that could benefit
>> either the task or the group.
>>
>> There is more logic to consider the benefit for both task & group in the later
>> function part.
>>
>> One question, why not
>> if (taskimp <= 0 && groupimp <= 0) ?
>>
>> Thanks,
>> Ethan
>>>    				continue;
>>>
>>>    			env.dist = dist;
>>
>