From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-DM6-obe.outbound.protection.outlook.com (mail-dm6nam10on2089.outbound.protection.outlook.com [40.107.93.89]) (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 EDB18487A5 for ; Fri, 27 Dec 2024 04:11:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=40.107.93.89 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735272672; cv=fail; b=Pnq+/CY3yUq9yyi5kmu1x54e2TwrBpWIWGQOKlNDL8ncLLlcpE+4DRy90sTn9lhVwRIUA4s0OoTcCG35Qxvgavev2qxq/c5fFeAM9jN0NbL1GDeTaRu3lXHpBwcDgMxmuCk8YIrXFDXftKQ0hENzrKfs7MfUFUjM44T8b78wTwo= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735272672; c=relaxed/simple; bh=PBey8c3Pz4sTD1GAmaV+PeyP3gJcHFRVphs73JlotHc=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=sCY95rRaAXcfHD9OZQ2lwu6a1+MITLbZujnDGzfLUmgr/wBYwQDpp41Q9WVfhGNYjU7ocht3VBDxHL7RDdtKmHpu95fk6leLRkGhpSAYJvfHQYlp/JTn/AbUhHWBMlHG8mzM3BhFWifCCMkWg+vuZf8iXDIiOeFFoQM0jczAvno= 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=U7W5yVLL; arc=fail smtp.client-ip=40.107.93.89 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="U7W5yVLL" ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=Yk6s94wcsCOngrBXu8tgNr7jX3m11+j6KyEWxD4z+T5/SZYpAsQyTEWrgoSwhLy4G4FEMk/7U0va8A/k8GoxdNlr371pzBCdTDnC02J8BkpjLovCw3x8haKul6CS8z1IUXzQMA5LhxZxmJuMug0tPp2lCZPHEiNcjGgX+yieG4WdhmHEOcTB3sR5e3SVBejtA728lB31181H7GHjsgLIQnCaw0b6TYFBTJ7xv4lsYpPuBRxPuuSw7BvWXr2hyQxd1QiN8Zzv1c/mXl2H+nuxRey69ipeiNCV2U/O6Bq+ibeNs5xLPzHxpiXZoJvctIka3uEiz5BazjNIh/ezGinQ8w== 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=01O0KooDbnEjw9V7G87n4eupyP2xO3Rcai3hjfxyi0A=; b=L/8T0fLm3gseapEKaEcpQEH8oGv+2DuXzf/8gnAQEeMUciwwmhKoH1HySoQYJnW+J+bj0S2kSlb0VGS/ErHXs5VivqPYQF8lPUK3UkRm2oSLreoG20AsBp+4RSvpow0C4903WqwA7Ym3z7n9SuQE24JQW66i8b6oRNr/E9ztOXbjZ9LntWvlDYJIsM6k32732mVCxBGlzY+x/IUTmCBe8c1W+Uw7ADg8LzNbQM/6Cg1pLrY7z1/N/tCb46qCXHUDgqxPik476+D26W05rqtc14ud4C+pOOwqWvYXx7T/lbRgHGqoaibQSDnlU5qUXVfnqt0J5jGzbQcGFa9u5iKMqQ== 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=01O0KooDbnEjw9V7G87n4eupyP2xO3Rcai3hjfxyi0A=; b=U7W5yVLLeTsDaWG9KoQOH6JOoZ9hGDAEYSuPpD7T67RbEYzzlh/4e3yZnmfxAlpR+pnZ5W94OQfZc9/H1xXHzpLAqwsG6w2KeYd743DLJTdGsfR//VKfMyZWQpFFSytETh8d4DWooRd5fvkexpje5vYfA0cyvAScD42LUpOnmVI= Received: from MW4PR03CA0300.namprd03.prod.outlook.com (2603:10b6:303:b5::35) by DS0PR12MB8528.namprd12.prod.outlook.com (2603:10b6:8:160::6) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8293.14; Fri, 27 Dec 2024 04:10:59 +0000 Received: from CY4PEPF0000EE36.namprd05.prod.outlook.com (2603:10b6:303:b5:cafe::f) by MW4PR03CA0300.outlook.office365.com (2603:10b6:303:b5::35) with Microsoft SMTP Server (version=TLS1_3, cipher=TLS_AES_256_GCM_SHA384) id 15.20.8293.14 via Frontend Transport; Fri, 27 Dec 2024 04:10:58 +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 CY4PEPF0000EE36.mail.protection.outlook.com (10.167.242.42) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.8293.12 via Frontend Transport; Fri, 27 Dec 2024 04:10:57 +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 22:10:50 -0600 Message-ID: <4e6e7308-1d39-427d-af47-2957025f501b@amd.com> Date: Fri, 27 Dec 2024 09:40:47 +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> <6bb3fd31-6b26-4bbf-8833-e4842b1dc463@amd.com> <103e4236-c01e-4286-9152-007d9a249a65@linux.dev> <409b4a72-483e-467b-8d00-9a8dae48bdc9@linux.dev> Content-Language: en-US From: K Prateek Nayak In-Reply-To: <409b4a72-483e-467b-8d00-9a8dae48bdc9@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: CY4PEPF0000EE36:EE_|DS0PR12MB8528:EE_ X-MS-Office365-Filtering-Correlation-Id: e1df94c2-02b1-441d-c562-08dd262c7716 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0;ARA:13230040|376014|7416014|36860700013|82310400026|1800799024; X-Microsoft-Antispam-Message-Info: =?utf-8?B?YmtYNk1SK2tJbjNsZStWN3VYK3k4WWwzNnorQjhrWUZKNmxLT3I2c0hUMldU?= =?utf-8?B?TlBtcE4zZ3FMdmp5UDFVeWd3TllaZzQ3MFBxOWlkQzQ5ZnBjb3FHWFBsdHMx?= =?utf-8?B?WWRLcmttT0tqVy9zQ0krYmNTcTh4MnZTd1ZhQjdwa1AvSWZ4NEJmWnlhVlNs?= =?utf-8?B?ZGNaZFEzV1FhSlhUUDVzL2ZZMHBFMTNJLzVmeXQybUYyWDJTZ1J2VHRUeGJn?= =?utf-8?B?QVVHcVlZb0ZkckI4dUZiN05xem1VeXBDRFp5dG84ZFFLSnlSRDJIcDJOQ3Ur?= =?utf-8?B?eFlTS3A2M2FxbVlhby9rMEFnekpzZWQwVG43TFljdDNGeVFVTGlncVV2bzhs?= =?utf-8?B?TXhSQVJYWFczalRYTm1HMGRkZDRsSHRQOHhaRE5NNmlUYkE4MTZHamEvcWVS?= =?utf-8?B?WVhaRnF6UXVHcmZOaStnd0doUWRmV3lGaTl4dkRVNEIxSjhxQUZreU9kbjF5?= =?utf-8?B?anZuYVF6aFpmZmV1WFYzbjNTeHl1eWFtL1Z4aFErdUpHMzlLU0lYTjNHVSto?= =?utf-8?B?SHg4V0pHTGp5cVJ0S1NOOWNIb2Z6TG1sRkhiUmZadnBQV05TczllZ0FHY2Rj?= =?utf-8?B?bWsyOGdSd1FaazAvNGVBMEV3eG1ORTI1Vjk1TXFZSXlTK0grWFltQitzQm1O?= =?utf-8?B?YmZic2xZeWNGWkVsY2VzVExIN2ttck9OSHdWRlcrNWg2SXBGVllXQ0tpdE1W?= =?utf-8?B?RmZZYmNBbzdDVFh1RDZjZkU1SDdtY085K3htaUcveGJyNHgxT1lkN0h1KzhK?= =?utf-8?B?WnVwYTVCeXZYUlNMYStkbE9mQ1JzZlYvYjNQSEQ5bW9QdTR0TkU0ZitZZFNv?= =?utf-8?B?dWhXNXg5ZlZDalFacG03QnJ1SXdINGUyN1hSbTVuYndkUWtPMHlHOWROSHQ2?= =?utf-8?B?bTQzWWZXU2VzN1lsRWJsc3EvV3VmcGRSYTVJZXBqR2NwTlQzbldBbmtVK1NT?= =?utf-8?B?cXlBWkY4bHNUMlNGdWtqNFFLWmxLZG56dVFiZmFVMnlRRUorVXVXR3QwYmRw?= =?utf-8?B?NHlxRDM4NmhLcGlmZVFSSjN5amFsdnBjUnVwTHBUbmRwcnZQOG45M0NQU3Zr?= =?utf-8?B?d1pBUC9qQXFud2ZzTE9CajlKUzFXOGliY0l0Ui9CZ2lOYVNsV3dramY3eG1t?= =?utf-8?B?dGVLM0l0NnhGVDJXNDFqVUdtYWlhMG1PRHZJaWFncURHNWlLcG5yRyt0MXRk?= =?utf-8?B?QjU5elVvRUM2UzFTd2gwTXl2YXZ3WStqTmFFMnczN0JRZUtxS3hmWmRJRnBV?= =?utf-8?B?ZDd0MnBaNDA5YzJPUTIvMHdCY3l6eXBEZ3lMaDFnWVdqdVRodGtSMTlKREZh?= =?utf-8?B?N01FaysyNGVaNUdxS21oRjRUWFN4N2xiMzlHVUc4SGhHSzIwT3Bnc3F5SjFQ?= =?utf-8?B?RHZBOXpJWnVROERUOWM5eFhZL0Zld3Z2VzNhcmJHeldXVmZudUE1ekJzOVJ1?= =?utf-8?B?YUVaVEdibnRMQWJKUnBJVkhHYWFaY3MzUVNSU3Nhbk5tcDlOeUJzelVwSHN1?= =?utf-8?B?bXpWU3M2VGxvUTRLZnNiOUlCTks5OXZ1Y2tFT1duT2hZMVdQVFJDQlRNOC9R?= =?utf-8?B?MDRocTFZOUREZ244cVh1eUdNSEJuaVFVa2doMm9BVnJhUXV5M1liMk9NS09n?= =?utf-8?B?ajlWN3djaFJFSThmNTZUTUJsV1h3SGZ4Rjg0Y253ZTA2TnQ1ZXR6bWtscml5?= =?utf-8?B?YlZYMDVyclM0bG9mL3lFZzNLZHpHSzYzWTRBOHNoMm01WGIzSjJiMHBNR1lx?= =?utf-8?B?cHRqbE5NUGpLSGVvOFcyVWxNWmVlaW8rNXErQS84YWtwTUhrWXBWUFBudUN4?= =?utf-8?B?d1Q1V1V4cDArSWd0T1Z5emxOc3p3WGNlMVZiaG1pUTNpTHBGeHpWNGtqejdI?= =?utf-8?B?V0oyZUE3Z0ZWcFN4OEpPQ0xJK3JnQWtOQXc1ZXZEaUVJNVZQelN2bEEzMDd2?= =?utf-8?Q?z4g/0Fc0zTxXYume90LDQB78zxt21hAP?= 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)(376014)(7416014)(36860700013)(82310400026)(1800799024);DIR:OUT;SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Dec 2024 04:10:57.3157 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: e1df94c2-02b1-441d-c562-08dd262c7716 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: CY4PEPF0000EE36.namprd05.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB8528 Hello there, On 12/26/2024 9:12 PM, Chengming Zhou wrote: > [..snip..] >>>>> >>>>> 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); >> >> Right, p->on_rq is cleared if not delayed dequeue. >> >>> >>> wouldn't this allow the task to be migrated? P.S. I have not encountered a >> >> But p->on_cpu hasn't been cleared until finish_task(prev). I forgot those are two different flags! Thank you for clarifying :) >> >> We can see in `can_migrate_task()`, we can't migrate `task_on_cpu()` >> task. Anyway, its code still running on the cpu, we can't migrate it >> to another cpu and run its code concurrently. >> >>> 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. >> >> Yes, it's blocked by try_to_block_task(), just ignored (deferred) by >> PSI. During the time before `psi_sched_switch()`, it maybe enqueued >> by ttwu(), which should be ignored by PSI too. >> >> At last `psi_sched_switch()` called with "sleep == false", just don't >> notice this transient dequeue & enqueue operations. And the flags are >> still consistent. >> >>> >>>> >>>> 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. >> >> Ok, this "block" value is stale when `psi_sched_switch()` called. >> >> So we would see these inconsistent psi flags changes. >> >>> >>> 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! > > Just made a quick fix and tested passed using your script. Thank you! The diff seems to be malformed as a result of whitespaces but I was able to test if by recreating the diff. Feel free to add: Reported-by: K Prateek Nayak Closes: https://lore.kernel.org/lkml/20241226053441.1110-1-kprateek.nayak@amd.com/ Tested-by: K Prateek Nayak If you can give your sign off, I could add a commit message and send it on your behalf too. > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 3e5a6bf587f9..065ac76c47f9 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -6641,7 +6641,6 @@ static void __sched notrace __schedule(int sched_mode) >          * as a preemption by schedule_debug() and RCU. >          */ >         bool preempt = sched_mode > SM_NONE; > -       bool block = false; >         unsigned long *switch_count; >         unsigned long prev_state; >         struct rq_flags rf; > @@ -6702,7 +6701,7 @@ static void __sched notrace __schedule(int sched_mode) >                         goto picked; >                 } >         } else if (!preempt && prev_state) { > -               block = try_to_block_task(rq, prev, prev_state); > +               try_to_block_task(rq, prev, prev_state); >                 switch_count = &prev->nvcsw; >         } > > @@ -6748,7 +6747,8 @@ static void __sched notrace __schedule(int sched_mode) > >                 migrate_disable_switch(rq, prev); >                 psi_account_irqtime(rq, prev, next); > -               psi_sched_switch(prev, next, block); > +               psi_sched_switch(prev, next, !task_on_rq_queued(prev) || > +                                               prev->se.sched_delayed); > >                 trace_sched_switch(preempt, prev, next, prev_state); > > diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h > index 8ee0add5a48a..65efe45fcc77 100644 > --- a/kernel/sched/stats.h > +++ b/kernel/sched/stats.h > @@ -150,7 +150,7 @@ static inline void psi_enqueue(struct task_struct *p, int flags) >                 set = TSK_RUNNING; >                 if (p->in_memstall) >                         set |= TSK_MEMSTALL | TSK_MEMSTALL_RUNNING; > -       } else { > +       } else if (!task_on_cpu(task_rq(p), p)) { One small nit. here If the task is on CPU at this point, both set and clear are 0 but psi_task_change() is still called and I don't see it bailing out if it doesn't have to adjust any flags. Can we instead just do an early return if task_on_cpu(task_rq(p), p) returns true? I've tested that version too and I haven't seen any splats. >                 /* Wakeup of new or sleeping task */ >                 if (p->in_iowait) >                         clear |= TSK_IOWAIT; -- Thanks and Regards, Prateek