Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4ff99651-fee5-f62c-0a19-9bd599de54c5@amd.com>
Date:   Wed, 21 Jun 2023 10:14:10 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 2/2] sched/fair: Cleanup in migrate_degrades_locality() to
 improve readability
Content-Language: en-US
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     mingo@redhat.com, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, vschneid@redhat.com, iamjoonsoo.kim@lge.com,
        linux-kernel@vger.kernel.org, gautham.shenoy@amd.com,
        kprateek.nayak@amd.com, wyes.karny@amd.com
References: <20230614102224.12555-1-swapnil.sapkal@amd.com>
 <20230614102224.12555-3-swapnil.sapkal@amd.com>
 <20230619094529.GL4253@hirez.programming.kicks-ass.net>
From:   Swapnil Sapkal <Swapnil.Sapkal@amd.com>
In-Reply-To: <20230619094529.GL4253@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RzBLNE9aUXdQV3UvMDlDQ0Nuc0Y2eE9YMWNyOE9oek9ONFhVdHJPZmdkd2dk?=
 =?utf-8?B?MGdSdndDVHpYMXJndDhDT1VyS0t1UXNDMFhvWjZZaVRCWGhobXF3aUEzWi8w?=
 =?utf-8?B?MVpTVE0vb1dOS2MyZnhna2tTQW8rbTY5RlBySFN1WThySGxYVTNTNWtlalBo?=
 =?utf-8?B?WnBxNzZIdEkwekgxK3BTUy9CRkJVTXc1VU9nUnVFMDBldmJlTFY0U1M5L3JT?=
 =?utf-8?B?MDVQR0JOZFNPcngyOEJjSkJhWTJEWnRZUnIxZlc0WmUrRnArLzFLdWs5OHh2?=
 =?utf-8?B?M0lvS2REWFZORmZRQ3ZLYStmS05GRmo1OW53d2U1Uk9DOFMzQUo1QVNYYVFP?=
 =?utf-8?B?MUZaZlBFR2lKbXp3T1FrMXIvT212RDJWQVo3Mng0bzFQM3dRMUl1TTRnUlgr?=
 =?utf-8?B?WUg3amJ3MGJpbm92VFUvVXRlWTJFTStxNFpyeWtwYTZ0RzFyaGRCNk9iaC83?=
 =?utf-8?B?S1JwRWtoN0Q4R3hTSEN1QWRLN256SU1qV0ExUUVUSytmNXMrWTE4djVjbWNH?=
 =?utf-8?B?cExuWGxXcTE5a1hnaUZQd3h2RGMxS2cyQlY0bVgwS3A4eTVlazlkYVFWNzM2?=
 =?utf-8?B?VVlwaUE3OUNxVmRmR0tpK1JSUkZxNU1VaVVMUWZOUjRsaFROazErZkh4UkNr?=
 =?utf-8?B?QzA5bU56MWpZcWUwTXNVb3NqbzZoY0g5S0xlZmJaTWlvazdaOVM5L0kyeUVt?=
 =?utf-8?B?c0poM0t2R0MzQWhSN0duRktxd2lETmwvME5Qd0NyOTNZeFRXSVV0N1VRU0tB?=
 =?utf-8?B?RXpkRTZJZzBnajJxekhxSFFmQlhNUjFTWjJXRzVsTU9OeHkrTUwyREd5SlNQ?=
 =?utf-8?B?QlJuWEk4NmRhU3VGUjdieUQyMG0xbnlHbzdFR05hSUtrQnozTTZRRXdvdzhs?=
 =?utf-8?B?ZHJiNE5xaVJCb3FyVlpMenZ2R0Nkdk1DeElwWm9mZzZTTk1zQVhJMHRKUG5C?=
 =?utf-8?B?eUxCVnRwT2s3V0k5dGJVb0pGTW94N0NnWXRBZ1ZIRU9ERlgxYWEwOWEwM2Vp?=
 =?utf-8?B?UnorSmdwQ2JjMlNjMXEzUGZhSjdHTU5NeU5YOXF0K0hEa1loeG5UTllvNlRw?=
 =?utf-8?B?T1hScGJRVnRWOVRWcWY5amF5SmlnbGRWNkhjZFpwNHFjWllSUUhTNjRDdWtI?=
 =?utf-8?B?Q2JGeVN2Z3IrdS9pYTc4SVkzTFdNM2pKZkc2WlhuK2s0T3pmVHEwam5XTlp3?=
 =?utf-8?B?V0ppcTBCeVhtYi9mc1pMZk0zQ1NwaWNSZnVJdC81dmZ5aGlEQjRJMHdFMml1?=
 =?utf-8?B?dXJUUVFhR2dqNzFPbzNMQ0IyK2hVWVFHZDlPOHB4UmFOSFZZY3BuVklVT2Zr?=
 =?utf-8?B?OVhtYlZVNnJSZlpQRC80VzZaQ2ZVVVdEei9mKzJ1ZXpOVFZBUGxtdGl3b3dC?=
 =?utf-8?B?M2c4RWVCOGRJUG4vUk9vUFlvU1JHZTRzcTV6WG1hSUNMNW9FUzdLUWtGR2po?=
 =?utf-8?B?c2tVMUJaRUFXb01KS200RTJxUXJFTWxYYkF6SHl2MzFIZXBKWFBWbnJVQU5X?=
 =?utf-8?B?UWtuOCtML2hDeDFoQXFiS3VHTXJuS0p0MnRxKzAzZVZCY0FuaVFsRDNlQWt3?=
 =?utf-8?B?REIrRDd0bGV5R3BuN0hvemxQa09oNG0xdGkzc3lIZktWWUxDWUhVeisxR2xV?=
 =?utf-8?B?enNpUjk0UjRDYU0vYzZKTklQN1AzZlJ6Q0hXNlZWRUFhVU9tYXRIRXZrRXBQ?=
 =?utf-8?B?WldsU3hOWFFzTkwxU2IvNTJqZUF1Y3J3S0pLNjZ1ZFl2VEpuc3Z3SkpwNTg5?=
 =?utf-8?B?SnVWUHJDcmdDTjRLdjN3VnBpRG1CUHNVVE9UWUhaK21LdVlJNThHZHdQb2Zs?=
 =?utf-8?B?SGlFcGZrQnFJeWR3YVlVQmhQWURiV0p2eEV2MytFV3c1S1gvMlJrRy9UMExq?=
 =?utf-8?B?VG5UWnpBeHFXY0ZpTlJZRU9CK0pLUmdpMkJpbFhLMlhiUDR1ZWJJWDdDaGFC?=
 =?utf-8?B?eG1KZzNPRUd5UDdXYVhwRnZsMFY4bHBIMWhhUEE0RlhxaWhwT2VHYWt2SVNL?=
 =?utf-8?B?S0tXOWhDQlVSQUEzNk1GZUVWNkhmZGt4cVdvN3kyM09JSWMvdnFYNm5DenRy?=
 =?utf-8?B?ZGF6NThHUldMZStJQ3M1SDQvcU9HMS9QQ21KQzNRMDUwemdEUFYvNmR1QjV6?=
 =?utf-8?Q?qAuVEhFHJKsLv5BEabIK8jNJj?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a2a6488f-2193-4679-8665-08db721230ae
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB6165.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jun 2023 04:44:25.8628
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: K27wDK3PXEhpajTUL4ec9axsKvnvdfPMCpjYWTmWE61jm1AabUSuifZykhxvr+o7BYrwg3E1ZesOnYFp+p5kNQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB5714
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello Peter,

On 6/19/2023 3:15 PM, Peter Zijlstra wrote:
> On Wed, Jun 14, 2023 at 10:22:24AM +0000, Swapnil Sapkal wrote:
>> The migrate_degrades_locality() returns tristate value whether
>> the migration will improve locality, degrades locality or no
>> impact. Handle this return values with enum to improve the
>> readability.
> 
> I can see how you ended up there, that tristate is weird, but perhaps
> don't make it more complicated than it should be?
> 
> ---
>   kernel/sched/fair.c | 39 ++++++++++++++++++++-------------------
>   1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 373ff5f55884..a8449f594348 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8446,42 +8446,42 @@ static int task_hot(struct task_struct *p, struct lb_env *env)
>   #ifdef CONFIG_NUMA_BALANCING
>   /*
>    * Returns 1, if task migration degrades locality
> - * Returns 0, if task migration improves locality i.e migration preferred.
> - * Returns -1, if task migration is not affected by locality.
> + * Returns 0, if task migration is not affected by locality.
> + * Returns -1, if task migration improves locality i.e migration preferred.
>    */
Because of the following hunk:

> @@ -8492,14 +8492,14 @@ static int migrate_degrades_locality(struct task_struct *p, struct lb_env *env)
>   		dst_weight = task_weight(p, dst_nid, dist);
>   	}
>   
> -	return dst_weight < src_weight;
> +	return src_weight - dst_weight;
>   }
>   

I suppose we should also change the comment to:
   /*
    * Returns a positive value, if task migration degrades locality
    * Returns 0, if task migration is not affected by locality.
    * Returns a negative value, if task migration improves locality i.e migration preferred.
    */

Do I need to resend v2 with your changes for this patchset?

> -static int migrate_degrades_locality(struct task_struct *p, struct lb_env *env)
> +static long migrate_degrades_locality(struct task_struct *p, struct lb_env *env)
>   {
>   	struct numa_group *numa_group = rcu_dereference(p->numa_group);
>   	unsigned long src_weight, dst_weight;
>   	int src_nid, dst_nid, dist;
>   
>   	if (!static_branch_likely(&sched_numa_balancing))
> -		return -1;
> +		return 0;
>   
>   	if (!p->numa_faults || !(env->sd->flags & SD_NUMA))
> -		return -1;
> +		return 0;
>   
>   	src_nid = cpu_to_node(env->src_cpu);
>   	dst_nid = cpu_to_node(env->dst_cpu);
>   
>   	if (src_nid == dst_nid)
> -		return -1;
> +		return 0;
>   
>   	/* Migrating away from the preferred node is always bad. */
>   	if (src_nid == p->numa_preferred_nid) {
>   		if (env->src_rq->nr_running > env->src_rq->nr_preferred_running)
>   			return 1;
>   		else
> -			return -1;
> +			return 0;
>   	}
>   
>   	/* Encourage migration to the preferred node. */
>   	if (dst_nid == p->numa_preferred_nid)
> -		return 0;
> +		return -1;
>   
>   	/* Leaving a core idle is often worse than degrading locality. */
>   	if (env->idle == CPU_IDLE)
> -		return -1;
> +		return 0;
>   
>   	dist = node_distance(src_nid, dst_nid);
>   	if (numa_group) {
> @@ -8492,14 +8492,14 @@ static int migrate_degrades_locality(struct task_struct *p, struct lb_env *env)
>   		dst_weight = task_weight(p, dst_nid, dist);
>   	}
>   
> -	return dst_weight < src_weight;
> +	return src_weight - dst_weight;
>   }
--
Thanks and regards,
Swapnil