RE: [PATCH] sched/fair: fix task_numa_migrate to consider both task and group benefits

[Jianyong Wu <wujianyong@hygon.cn>]

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