From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (mail-dm6nam12on2084.outbound.protection.outlook.com [40.107.243.84]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 54ED614D708 for ; Thu, 26 Dec 2024 11:05:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=40.107.243.84 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735211109; cv=fail; b=aHjYDca9Ntpx1NCNQto2Ac7Eh89/2+izyDokKHftxxAOYL0RdVM7Qe9j4W8XRU1LOGLjLJ34FvCbiKk6plsURb3hTjqJPvHENKGMerMpGY4bIZI+h+YGnlwXkg3/g4ZL8cHMk8nrr9i1kGF0ISaKmc7NWhLenzOetqCF0oau9dQ= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735211109; c=relaxed/simple; bh=LTriW7qbtJjJq808U9z/zKJKaYiEeGCbcU+frRltUa4=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=olMV+FCZ5X38eCLOVEa7Df5KFnAHYwLlF971MjGh5iQ1V79utR4+/+sHdf5newG4M8Ux5zF4MGxtJlDSo4/knDggX/VumMB3XnaVOpwSCOXvuiM2K1O+0PawkYZrmTcCfEuBpfhGPkgC97oeRW4x+E+iXzauHwZb2j8Vb03nkbs= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=amd.com; spf=fail smtp.mailfrom=amd.com; dkim=pass (1024-bit key) header.d=amd.com header.i=@amd.com header.b=ekNNGDRc; arc=fail smtp.client-ip=40.107.243.84 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=amd.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=amd.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=amd.com header.i=@amd.com header.b="ekNNGDRc" ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=lMxXtWdX4kowRAUarrNuUqZQIJiyddboVyLGuB4HnGw0e4RlbCjvmKuLZnEiLpeV7q2Ls6ltWVOvP41C2SrVTkNJ/4XB9Uw6GmUVL63hhaCXejOjZw+FDzza0Ff9YGtpa3+GnxcoNAB25/LqV1Zm6IlsWHJ5IAOimJ+gl8Vt8zLDcI5Ws3AESlewOg2zVEBOAd022DkMh9IhIvtiS0wmB/nk0VZnNbyGt9HYysDd7B6nZQ5fPzVFseDIo6MdiwjC7QCKR1xzts/hqynCix0AvA6DqG7LZrbesgAO7ji22BkqOTTBZgfbDth4YUhu3nvXLDsLiiHJCMhVXpvDWUpzgQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=B3JAhoZLmXNrIA1xDk2tdz/cPoGRxFv4SAaIqHlv37E=; b=d3vZTQvbIK8SUT5DLvBFX2lEDCYskYAR5/9+R5jtUXghtTasQhfn6wOpcXuxwB0Kek8VBdpiTYIB36xdQnfICNhFkXRtf0Y5XcKeS2KZmlH2RvNphnAixyK7bVmOeKdGsH7c7ON68SZv4xGBEEHJbxQHN4FGG24XSwqYYWGgdgSmqsKSQOZ4qxv7FXOCG3dHYC7kg4mG2ygAE+hEJxtIPfAy1SgeJu5Y0KKGATILRRzcOzCjQN6/Xzy/irHW5fADsvEQ24MJgpyNu+7x9e6/cOQ+jj0CARcmXlUYOvlieB+tezrOmoX3mrLyZFR53WEgMMtQ4OmsbU5f4V715gk9tA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=linux.dev smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=B3JAhoZLmXNrIA1xDk2tdz/cPoGRxFv4SAaIqHlv37E=; b=ekNNGDRchKNyRspu27STzCIV78F+AuxILyTjfUbsgbylx+KdPSvidAwNPeaT8Sd8OBDrXSXHfErjCEO+No3+S+/f+NraUfbSPmAKMrBqeG5T5TTo5JN9c/AjZJ8zDtqV90SkkplB51XqOWJBWp+5I/Fu9bv6YnN1pN5i1YsB+MM= Received: from SJ0PR05CA0079.namprd05.prod.outlook.com (2603:10b6:a03:332::24) by MN2PR12MB4270.namprd12.prod.outlook.com (2603:10b6:208:1d9::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8293.15; Thu, 26 Dec 2024 11:05:00 +0000 Received: from SJ5PEPF000001D1.namprd05.prod.outlook.com (2603:10b6:a03:332:cafe::5e) by SJ0PR05CA0079.outlook.office365.com (2603:10b6:a03:332::24) with Microsoft SMTP Server (version=TLS1_3, cipher=TLS_AES_256_GCM_SHA384) id 15.20.8293.10 via Frontend Transport; Thu, 26 Dec 2024 11:04:59 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 165.204.84.17) smtp.mailfrom=amd.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=amd.com; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C Received: from SATLEXMB04.amd.com (165.204.84.17) by SJ5PEPF000001D1.mail.protection.outlook.com (10.167.242.53) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.8293.12 via Frontend Transport; Thu, 26 Dec 2024 11:04:58 +0000 Received: from [10.252.205.52] (10.180.168.240) by SATLEXMB04.amd.com (10.181.40.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.39; Thu, 26 Dec 2024 05:04:52 -0600 Message-ID: <6bb3fd31-6b26-4bbf-8833-e4842b1dc463@amd.com> Date: Thu, 26 Dec 2024 16:34:44 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] psi: Fix race when task wakes up before psi_sched_switch() adjusts flags To: Chengming Zhou , Johannes Weiner , Suren Baghdasaryan , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , CC: Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Valentin Schneider , Chengming Zhou , Muchun Song , "Gautham R. Shenoy" , Chuyi Zhou References: <20241226053441.1110-1-kprateek.nayak@amd.com> <20df37b9-c653-49d6-83e7-da4f21d5b848@linux.dev> Content-Language: en-US From: K Prateek Nayak In-Reply-To: <20df37b9-c653-49d6-83e7-da4f21d5b848@linux.dev> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-ClientProxiedBy: SATLEXMB03.amd.com (10.181.40.144) To SATLEXMB04.amd.com (10.181.40.145) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SJ5PEPF000001D1:EE_|MN2PR12MB4270:EE_ X-MS-Office365-Filtering-Correlation-Id: ead51f9a-e0b7-449b-fc53-08dd259d236d X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0;ARA:13230040|82310400026|376014|7416014|1800799024|36860700013; X-Microsoft-Antispam-Message-Info: =?utf-8?B?YXFjRjNsYm1jTUJKSDJQN2M4cUgvQ1E2SkM2blpHZWRNbzQwMVNnYTVNV3Jr?= =?utf-8?B?TGpUY3VnaWdYVFA5OFdpTXEyZTNBN3J5ZkFseml0NlpFYnliSC9DRXN1aUNU?= =?utf-8?B?NVo3NlJjdExaTlFNTGdOdE1IbVpvV2dXdXZsazczY2pTaUxaKzZITjlacnIr?= =?utf-8?B?UWd4SUl0SjVtSURvRTlCQ2d6ZW51R0owNnhqU3FZQTJJYndCclZFRHNVUGpX?= =?utf-8?B?aUpxR24wZUg2eFVFZEJCbUY5a0svU2dycFlFMnJFWndGMngzS1M0dTlLbmtq?= =?utf-8?B?WmlVcndaZFB3TVNwTXN4cHVudGh1eWFmOSs0S21Wd0x3ZHl3bkkrdFpMcUd5?= =?utf-8?B?RTZsYU8rTUg2YVNCaG0rR05YUnhnMjNDTGhud2NlcVhMU1B4QlBXNXQvMkdU?= =?utf-8?B?YXZkTHNSVXFicnJRd0c3V0hJOUdxY2pHc1pkbklVYmNJbnZadWNBVzdhei90?= =?utf-8?B?bmt5Yk9sdUJlZlBvd0NGUmxsdS85SkNpbGFZRGhZdEtiWTZaaTRYTEpCM2Mz?= =?utf-8?B?QkxmZWlNV3RXWE1GV2R4dDdJTXFkOTd2c015S2xOZWV6cElLNnBVc1pnaW5y?= =?utf-8?B?TXk3OU5HSWs4N2t3NE91ODB2VDdSL0dtMVpwRTY0d1ozaEZ6RnFRV1B3cWVp?= =?utf-8?B?TVY3TmIrVnQzdHVWc2g5TkExTk8xRk9pb2JHcEpIdXFvbm1pSCtxKzZUbTlx?= =?utf-8?B?RDgvRVFOUkZiWnRqUVVQZVNJdjZDNnozQkRXNTZOdVBtL0lDMk9XUGluMlFk?= =?utf-8?B?Q0lnK2NTWGN3bkZOb0RCblBEZWRkbmdaNXp6d2FsVDJUVm5ub2ZVUGRlS2JU?= =?utf-8?B?L29JaXNmUGFIeC9vRzNKTktaS2EwWUZJNmYzZ1VXSllLSCt0bGRhS2cxcVE1?= =?utf-8?B?SWFSQnhWdmJOdm5IOVh3dnFMVHUxdEVWSkVzZlJ2WG9Ia2Q0QmtESWdSYU9U?= =?utf-8?B?YnRpaklDZFhtOWM3dGQ0T2EzWEFZNHAwU2FWR0gxTldIbWFuSzhYNmxrSFVk?= =?utf-8?B?dThxVk1uZitqbnV2ZWxwUmVNNmh6d2FrWitCc3FySW1iazNLWjJBSFZYak9o?= =?utf-8?B?VmNCbGZjUXRyd3FlQUE5VmN3TEhTRTZMblgrM2dxZnc2M2xnczl4bXEySUFx?= =?utf-8?B?QUVPWG5TbjZzUlJ1WUdVbGlOT29VTFd5aEhhMXdWakh1THpyMDVmZmRHVW5h?= =?utf-8?B?dVZ4K0Q2RkRzQ3hHbjlPQW1DZGl4WTlKS2FoYTRIR2hVQThsZzhPa25wdmds?= =?utf-8?B?SWxTOFNJT0c0MkNZM3I1TjVrUFNTSTMvdXlTaWg4aFlBZjRyZ3NMZzBCSVdG?= =?utf-8?B?S0UxRWxVekRuVElZNjhISnZ4TWN3QzVlUTRsNmNabzFwUEEwWnJhdmdsVXNl?= =?utf-8?B?MmkxVHhOVG1iZUIrNEZMd2pqQ2RRRUR3bXd5NzloZjh6K0dUZzZzeERzcWhx?= =?utf-8?B?Y2Nza1RTcUlPL2w1dEk2OG5iU214eVZXUkdSd21NUWhyTXhvbkRaVGF4YmMx?= =?utf-8?B?ZDgrMSt3Yjc0cFFvbmIxNExDSVlNVGtIRXpFaFY3eHZxRWFxamZvMStjVFpy?= =?utf-8?B?Nm9UUmRuM3dBWU16Ty8vdThQVVpDeXNKL3NSRHV3YzRISEpQMy9QajZsdkxr?= =?utf-8?B?NDgyelllV0prU0EyQ2REZXFMOENzNUcwblpTTlJkZ1B5NlZ5RXZxVkx0Z2RX?= =?utf-8?B?UnBiOEF4WDUreXpHTzFZekUvaVFzbEZiNVIwL0NzV2lzVkgydWdLMDVwck1L?= =?utf-8?B?b1ptY1dFZXp0NWl6SHhNRHRnSjdGUVdvMWt4MFRpTGN6bXg5NGdTTHNOSzhV?= =?utf-8?B?aGFZVlFqTkVvcHB5U0lIVG5GSmVQUWNyRXB4VTBCdjc0RXIwdURLcEtiY1lk?= =?utf-8?B?L2ZvREprV2dTRXdoQnBsRDZDN29WVE1kRXNQV2oyUE9mUE50U0ZpRG0vRlk2?= =?utf-8?B?enpGckxweGt3QnBPYW9LVE1zNG5KRDVtMEZ5ckEwZ0lDSWFvcElDK3ZHN3VO?= =?utf-8?Q?KBT8C2wQp0rhbWiW4b0RyK5pQy8dQY=3D?= X-Forefront-Antispam-Report: CIP:165.204.84.17;CTRY:US;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:SATLEXMB04.amd.com;PTR:InfoDomainNonexistent;CAT:NONE;SFS:(13230040)(82310400026)(376014)(7416014)(1800799024)(36860700013);DIR:OUT;SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Dec 2024 11:04:58.9419 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: ead51f9a-e0b7-449b-fc53-08dd259d236d X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com] X-MS-Exchange-CrossTenant-AuthSource: SJ5PEPF000001D1.namprd05.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4270 Hello there, Thank you for taking a look at the patch! On 12/26/2024 4:13 PM, Chengming Zhou wrote: > Hi, > > On 2024/12/26 13:34, K Prateek Nayak wrote: >> When running hackbench in a cgroup with bandwidth throttling enabled, >> following PSI splat was observed: >> >>      psi: inconsistent task state! task=1831:hackbench cpu=8 psi_flags=14 clear=0 set=4 >> >> When investigating the series of events leading up to the splat, >> following sequence was observed: >>      [008] d..2.: sched_switch: ... ==> next_comm=hackbench next_pid=1831 next_prio=120 >>          ... >>      [008] dN.2.: dequeue_entity(task delayed): task=hackbench pid=1831 cfs_rq->throttled=0 >>      [008] dN.2.: pick_task_fair: check_cfs_rq_runtime() throttled cfs_rq on CPU8 >>      # CPU8 goes into newidle balance and releases the rq lock >>          ... >>      # CPU15 on same LLC Domain is trying to wakeup hackbench(pid=1831) >>      [015] d..4.: psi_flags_change: psi: task state: task=1831:hackbench cpu=8 psi_flags=14 clear=0 set=4 final=14 # Splat (cfs_rq->throttled=1) > > I have a question here, why TSK_ONCPU is not set in psi_flags if > the task hasn't arrived psi_sched_switch()? It is set. "psi_flags" is in fact a hex value so the psi_flags is "0x14" which is (TSK_ONCPU | TSK_RUNNING) > >>      [015] d..4.: sched_wakeup: comm=hackbench pid=1831 prio=120 target_cpu=008 # Task has woken on a throttled hierarchy >>      [008] d..2.: sched_switch: prev_comm=hackbench prev_pid=1831 prev_prio=120 prev_state=S ==> ... >> >> psi_dequeue() relies on psi_sched_switch() to set the correct PSI flags >> for the blocked entity, however, the following race is possible with >> psi_enqueue() / psi_ttwu_dequeue() in the path from psi_dequeue() to >> psi_sched_switch() > > Yeah, this race is introduced by delayed dequeue changes. > > In the past, a sleep task can't be migrated or enqueued before it's done in __schedule(). (finish_task(prev) clear prev->on_cpu.) I see __block_task() doing: smp_store_release(&p->on_rq, 0); wouldn't this allow the task to be migrated? P.S. I have not encountered a case where psi_ttwu_dequeue() has occurred before a psi_sched_switch() but looking at the code, I thought it might be possible (I might very well be wrong) > > Now, ttwu_runnable() can call enqueue_task() on the delayed dequeue task > to bring it schedulable. > > But migration is still impossible, since it's still running on this cpu, > so no psi_ttwu_dequeue(), only psi_enqueue() can happen, right? > > (Actually, there we can enqueue_task() for any sleep task, including > those are not delayed dequeue, if select_task_rq() returns same cpu > as task_cpu(p) to optimize wakeup latency, maybe need to submit a patch > later.) > >> >>      __schedule() >>     rq_lock(rq) >>         try_to_block_task(p) >>         psi_dequeue() >>         [ psi_task_switch() is responsible >>           for adjusting the PSI flags ] >>         put_prev_entity(&p->se)            try_to_wake_up(p) >>         # no runnable task on rq->cfs            ... >>         sched_balance_newidle() >>         raw_spin_rq_unlock(rq)                __task_rq_lock(p) >>         ...                        psi_enqueue()/psi_ttwu_dequeue() [Woops!] >>                                 __task_rq_unlock(p) >>         raw_spin_rq_lock(rq) >>         ... >>         [ p was re-enqueued or has migrated away ] > > Here ttwu_runnable() call enqueue_task() for delayed dequeue task. > > migration can't happen since p->on_cpu is still true. > >>         ... >>         psi_task_switch() [Too late!] >>     raw_spin_rq_unlock(rq) >> >> The wakeup context will see the flags for a running task when the flags >> should have reflected the task being blocked. Similarly, a migration >> context in the wakeup path can clear the flags that psi_sched_switch() >> assumes will be set (TSK_ONCPU / TSK_RUNNING) > > In this ttwu_runnable() -> enqueue_task() case, I think psi_enqueue() > should do nothing at all. > > Why? Because psi_dequeue() is deferred to psi_sched_switch(), so from > PSI POV, this task hasn't gone sleep at all, so psi_enqueue() should NOT > change any state too. (It's not a wakeup or migration from PSI POV.) There I imagined that newidle_balance() can still pull a task that can be selected before prev and with the current implementation where calling try_to_block_task() would still mark it as blocked and the flags would again be inconsistent. > > And the current code of "psi_sched_switch(prev, next, block);" looks > buggy to me too! The "block" value is from try_to_block_task(), then > pick_next_task() may drop and gain rq lock, so we can't use the stale > value for psi_sched_switch(). > > Before we used "task_on_rq_queued(prev)", now we have to also consider > delayed dequeue case, so it should be: > > "!task_on_rq_queued(prev) || prev->se.sched_delayed" Peter had suggested the current approach as opposed to that on: https://lore.kernel.org/lkml/20241004123506.GR18071@noisy.programming.kicks-ass.net/ we can perhaps revisit that in light of this. Again, lot of the observations in the cover letter are from auditing the code itself and I might have missed something; any and all comments are greatly appreciated. -- Thanks and Regards, Prateek > > Thanks! > >> >> Since the TSK_ONCPU flag has to be modified with the rq lock of >> task_cpu() held, use a combination of task_cpu() and TSK_ONCPU checks to >> prevent the race. Specifically: >> >> o psi_enqueue() will clear the TSK_ONCPU flag when it encounters one. >>    psi_enqueue() will only be called with TSK_ONCPU set when the task is >>    being requeued on the same CPU. If the task was migrated, >>    psi_ttwu_dequeue() would have already cleared the PSI flags. >> >>    psi_enqueue() cannot guarantee that this same task will be picked >>    again when the scheduling CPU returns from newidle balance which is >>    why it clears the TSK_ONCPU to mimic a net result of sleep + wakeup >>    without migration. >> >> o When psi_sched_switch() observes that prev's task_cpu() has changes or >>    the TSK_ONCPU flag is not set, a wakeup has raced with the >>    psi_sched_switch() trying to adjust the dequeue flag. If the next is >>    same as the prev, psi_sched_switch() has to now set the TSK_ONCPU flag >>    again. Otherwise, psi_enqueue() or psi_ttwu_dequeue() would have >>    already adjusted the PSI flags and no further changes are required >>    to prev's PSI flags. >> >> With the introduction of DELAY_DEQUEUE, the requeue path is considerably >> shortened and with the addition of bandwidth throttling in the >> __schedule() path, the race window is large enough to observed this >> issue. >> >> Fixes: 4117cebf1a9f ("psi: Optimize task switch inside shared cgroups") >> Signed-off-by: K Prateek Nayak >> --- >> This patch is based on tip:sched/core at commit af98d8a36a96 >> ("sched/fair: Fix CPU bandwidth limit bypass during CPU hotplug") >> >> Reproducer for the PSI splat: >> >>    mkdir /sys/fs/cgroup/test >>    echo $$ > /sys/fs/cgroup/test/cgroup.procs >>    # Ridiculous limit on SMP to throttle multiple rqs at once >>    echo "50000 100000" > /sys/fs/cgroup/test/cpu.max >>    perf bench sched messaging -t -p -l 100000 -g 16 >> >> This worked reliably on my 3rd Generation EPYC System (2 x 64C/128T) but >> also on a 32 vCPU VM. >> --- >> [..snip..]