Received-SPF: pass (google.com: domain of linux-kernel+bounces-8810-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <2769a37a-fc76-45b7-9ae7-d328778a25fc@oracle.com>
Date: Fri, 22 Dec 2023 04:04:23 +1100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] sched: Fix tg->load when offlining a CPU
Content-Language: en-US
To: Vincent Guittot <vincent.guittot@linaro.org>, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, vschneid@redhat.com, aaron.lu@intel.com,
        linux-kernel@vger.kernel.org
References: <20231221164014.447122-1-vincent.guittot@linaro.org>
From: Imran Khan <imran.f.khan@oracle.com>
In-Reply-To: <20231221164014.447122-1-vincent.guittot@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?T3NFZFNkS3dkbE9nazI3SStvOEI3NjJoQ0U1bFRKejdFSk9RemlXVFFmQ1Vq?=
 =?utf-8?B?dnNVbWJHVnlkTUQ5VnQwVy9weW4vbXpiMjJweXQ2RFo1T0VuZGd3QnI2dFFo?=
 =?utf-8?B?L0lTKzNiaEhVY3BUZUlWZUppSU9reWNqRTlocDY1T2YvbDBGbTBBQVo4N3E1?=
 =?utf-8?B?dDJZSWp0K0ZuZmJhMVVIdkRERkZ4NnJXV3M3M2FtczJ6UVFOeXpUMjBBVndw?=
 =?utf-8?B?czE2MXNWcW1yYUZBVFh0VkV1Ty83cjdHeEVzdE91QVFBc3plQnU3NjNubVJY?=
 =?utf-8?B?WnJNdDNIMjlzR2Fsd2hrRHZjK2hBZ3NIUGZ6MHAxRVd5ckF4TzVNZ25YZFAv?=
 =?utf-8?B?VGhxT25VSU52WExKT3JzR2xXRGEvVWdhb1Z2NjVPREJGVm9wa3ZWci9HeEtL?=
 =?utf-8?B?aldjNmdEaHcyVG5pZkZIWmJGaFJBYi82cHczUXliZ2g3Z2FwcTd5L091K2pK?=
 =?utf-8?B?ejF1cERaUHdPRjRqb1FvTWxLcVljQ1NlVXkvSnFZN0xYQ0ZERDFFcU5vZGZo?=
 =?utf-8?B?eHlkbXBQeTVSdi94WUJIZnhCcm5LN0RHV1BHNUdXWktHdWMwWk9TcnB1Ny9N?=
 =?utf-8?B?bXFlcldkUHA0blFlN0taR21QYTFPSHhLcjZ1eUk3VDZhWFFyRTNhejVreGsr?=
 =?utf-8?B?V1NCZHlMamxRTWhyOVZSOEd5T1J0WHZHUlU1YzI4bVJhMHBiZHIvbEwrS1o0?=
 =?utf-8?B?T3lJSnVEV2h6c1RyNEhPSUUraDB4MkFPZERmRGZ4SFY5V2RlNmx6WXZQRU1a?=
 =?utf-8?B?eVo1b2VWSzlRT0FKbjBrRDdwTU9ONWF1UU5NMzArSmxTdkk2YzdoSDNIdVg5?=
 =?utf-8?B?QzhIRjhmcmVGOVZubWdBSHhFRzJhenBUcytBOUhaTlJ4TDhWaTM2dGFyVGlI?=
 =?utf-8?B?c1oya2RpQU1MZUtYYkNmVGtqdkhHR2oyR1FVd2tjMi9SQkkxaENjZnlhbVp2?=
 =?utf-8?B?ZXl3TEZNOHg1QXZsVkF6aWMyckwxT1RoTHI3QUtGTFJsVW45bnVBRmEydTZv?=
 =?utf-8?B?UEsvaHBpUzlPOXJKM25WcjdnOXZXUGRzOXJKMUhEcVlkb29xdjZpZzBBRGVo?=
 =?utf-8?B?bUNqd3FRb2h0U1AzL0lIaitxeHEzemRPU3hFbk8xeDJIcTExZjJSelRyOHVo?=
 =?utf-8?B?NnhDb0JhTVZMNVJ2cDR6UUxVY3JrNFFOSCtDbjFYR0IwcGU5UzJEV3FRQVBL?=
 =?utf-8?B?dWNsL25ZT0VSSW9kbFJjZFk2Wm9IckNXNmI3SFNtREZsdW1GOG5JL0xXTzZt?=
 =?utf-8?B?dG0yRVplbXh0ZWRlY2lvdTFtcFZNZ0c2VDVqNE1OZWJDWURQR0E2S0daWXpi?=
 =?utf-8?B?endicW5vWnNyWUhKNHIxMnJFWXdMZmJHUHhRSjF0UGZ1VE9KMnlnVEUrOGxq?=
 =?utf-8?B?VDdFRlg2QjlIV3FncmZvTCtlN3p5Ui9hR1hSa2F0VTBsRVBRSjdvUWNpN0Z1?=
 =?utf-8?B?YklBMkw1RlJabldHbG1VWm8rSnY0dUE2OFdQdHRIS2Q2S0ovSVVsekg3RDVK?=
 =?utf-8?B?THI5WTlaeG9rVUdUUUtDdUJ1ZXMrakYzcmlXVXQ3NWZrSUppenRMZ1QwNEJG?=
 =?utf-8?B?cTVBNW1ya3A3QkFPY2g4TTRuRUo1czJhd1VRUThNMys2YWVPOEFnNGRsYzdH?=
 =?utf-8?B?aDRIOExabXFnMUVZT1JCOEt3cVhScGt0UlpiUDNjbU9HTjJwUjgzdDBBcnNT?=
 =?utf-8?B?SzF1czc5Y0VPRWJPaWx1VW1odlRsY3F4aWhjM2VJNzl0UWRjalRpMlRWUk5l?=
 =?utf-8?B?blplVDdkVW94d20zRk1neUQ5SGZFTlhvbys2WU5IbGxXcStKTGJUY28zQ01D?=
 =?utf-8?B?RHFsbnFpMmJxWWxkd3d6MFRFdk1ndUdTNEtuK1lYZ1Fpa25oV1JvdUdvcUhE?=
 =?utf-8?B?dzJCTmFGVytEZUp2dzg1aVJaM28rb3lENEpZY3kwM0Y1TGZyZ0R2d2RsOTUw?=
 =?utf-8?B?d05mL0N5NStia2dURU5mTXFCaElxM1RMM0NNS25RYTN4eHhWK3gwUndZb29k?=
 =?utf-8?B?Kzl4ZmRjOXhyeTlRUXp5Z1dwbGRPYWZ1ZXlVVEMrY0RJMWhabWhNRHh5VlA0?=
 =?utf-8?B?VTQyMWhkNzlGYlA2ZVFFUkVUd3AzTitaUzhYbjAvT1dESktCRldHTWdDSzhk?=
 =?utf-8?B?ZGloOUNBQkhsM2lhK0E1bG9YdThCQkNKdnJRa1l5dXRlaHBJNTlVQm81OXNG?=
 =?utf-8?B?WFE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	ScWq/imTrgnlrYqdKQdGFnCJh6NFfSoKPaEg8qyFS1sOsrXB64uUns0XJuZTFhhtD7oWw0ivOkLmwjNuSwHnuAagWEQqZBbjx3Tn2rr2bIykNR99nDPoPK+xR8It8IZp5sJFy2r0EGcp+Gjv5MVgLMx1EX0mbs/qt90GDcTnRMgNjnrwRZEAY6jQvf9MMRP+5zGIvFncG3BqV63ZxH2zoKtISUZ7ZG1/rGZwVkUhfITHrVkFXoJMADi5CBkc9Z++BqUWl2zWchgi5BTya+Y/lPVdL96lbsLRvTlC4QodgYwShxqsMDgZmlgCfVUv5tqCN9iYTSl7nSSypPxdT6IO66tESaDPzdN7yfFVujvnfLLriuhslATHwCGX5fX026C6UFDYeJZBsWAkbrheUDacjuYlzrgvgKD+ZSkk6zwCJC9rBT0MlXTIzOdyImfhGbjm7Dgcc50PT8N3exZO9g1NpUwJvnYRLqC+nht3F/sDkqjzC8oec0nLJGzQ0/FghGXBek59wfjpkveEQlpFCdc7f5KtsE5KrR+noiHE7qugsaGbEjNRwudBPko9DMUKnLWi0xkTpM08yrkKDjdvkU96kW4kyOYD8ijVnZd1hu6vshc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e167a597-a8f8-46be-169a-08dc0246e563
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4468.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Dec 2023 17:04:30.2227
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cGV7JymVBHbUA72v5XhXv+2rU4orcp1jTg5WunODhZgePkioLEPSJChs0aOisTB2hrFWKU22PAWYm9v0jcHvuw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5287
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-21_09,2023-12-21_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0 spamscore=0
 mlxlogscore=999 adultscore=0 suspectscore=0 mlxscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312210130
X-Proofpoint-ORIG-GUID: vdTY_ejyld1e9lbcEK60O9s2o3zPNTkP
X-Proofpoint-GUID: vdTY_ejyld1e9lbcEK60O9s2o3zPNTkP

Thanks again for the fix Vincent.

On 22/12/2023 3:40 am, Vincent Guittot wrote:
> When a CPU taken offline, the contribution of its cfs_rqs to task_groups'
> load may remain and impact the calculation of the share of the online
> CPUs.
> Clear the contribution of an offlining CPU to task groups' load and skip
> its contribution while it is inactive.
> 
> Reported-by: Imran Khan <imran.f.khan@oracle.com>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

Reviewed-and-tested-by: Imran Khan <imran.f.khan@oracle.com>

> ---
>  kernel/sched/fair.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 07200b4751b3..07edb7dce671 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4094,6 +4094,10 @@ static inline void update_tg_load_avg(struct cfs_rq *cfs_rq)
>  	if (cfs_rq->tg == &root_task_group)
>  		return;
>  
> +	/* rq has been offline and doesn't contribute anymore to the share */
> +	if (!cpu_active(cpu_of(rq_of(cfs_rq))))
> +		return;
> +
>  	/*
>  	 * For migration heavy workloads, access to tg->load_avg can be
>  	 * unbound. Limit the update rate to at most once per ms.
> @@ -4110,6 +4114,49 @@ static inline void update_tg_load_avg(struct cfs_rq *cfs_rq)
>  	}
>  }
>  
> +static inline void clear_tg_load_avg(struct cfs_rq *cfs_rq)
> +{
> +	long delta;
> +	u64 now;
> +
> +	/*
> +	 * No need to update load_avg for root_task_group as it is not used.
> +	 */
> +	if (cfs_rq->tg == &root_task_group)
> +		return;
> +
> +	now = sched_clock_cpu(cpu_of(rq_of(cfs_rq)));
> +	delta = 0 - cfs_rq->tg_load_avg_contrib;
> +	atomic_long_add(delta, &cfs_rq->tg->load_avg);
> +	cfs_rq->tg_load_avg_contrib = 0;
> +	cfs_rq->last_update_tg_load_avg = now;
> +}
> +
> +/* cpu offline callback */
> +static void __maybe_unused clear_tg_offline_cfs_rqs(struct rq *rq)
> +{
> +	struct task_group *tg;
> +
> +	lockdep_assert_rq_held(rq);
> +
> +	/*
> +	 * The rq clock has already been updated in the
> +	 * set_rq_offline(), so we should skip updating
> +	 * the rq clock again in unthrottle_cfs_rq().
> +	 */
> +	rq_clock_start_loop_update(rq);
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(tg, &task_groups, list) {
> +		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
> +
> +		clear_tg_load_avg(cfs_rq);
> +	}
> +	rcu_read_unlock();
> +
> +	rq_clock_stop_loop_update(rq);
> +}
> +
>  /*
>   * Called within set_task_rq() right before setting a task's CPU. The
>   * caller only guarantees p->pi_lock is held; no other assumptions,
> @@ -12423,6 +12470,9 @@ static void rq_offline_fair(struct rq *rq)
>  
>  	/* Ensure any throttled groups are reachable by pick_next_task */
>  	unthrottle_offline_cfs_rqs(rq);
> +
> +	/* Ensure that we remove rq contribution to group share */
> +	clear_tg_offline_cfs_rqs(rq);
>  }
>  
>  #endif /* CONFIG_SMP */