From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-180.mta0.migadu.com (out-180.mta0.migadu.com [91.218.175.180]) (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 5B48F10E0 for ; Thu, 26 Dec 2024 15:42:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.180 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735227770; cv=none; b=JF2ZaPClDPTpUXG2SeJham3fZB6Ce9PCfHQsq6S/xP6bejNscm/yyrFmFAEmswzg6TNXUOADOkd3WZ8oMbcOcWw8o+dS/D+2wiksAuUeGXiKhSpLqD8F9WG+61jxZ360oWinzSIbi1nTehv9ApTOA6Qa9cIACF5ZsbtBugZhDWQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735227770; c=relaxed/simple; bh=M0hx9vtOB4LLiwx0khRX0JjCvIHzzP2xyJNhz4FffWA=; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References: In-Reply-To:Content-Type; b=oihfDFv/zeKmcYSm7wLry6MSTIkNr/SYSCfRkXN1tDebJXkxPMIL8YbbALeTZgor9mgYO6ZJIqTB+F6aUhjHcOXv5klU/lNe2My/m/iCwxNiWa69obDnzs7SUVrwmcvdCi2yRv9O/PN4KRJsmzZUx2Rmpa4SffV2DfTk66s4FCs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=PqqaY3Dz; arc=none smtp.client-ip=91.218.175.180 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="PqqaY3Dz" Message-ID: <409b4a72-483e-467b-8d00-9a8dae48bdc9@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1735227764; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vhWrI0B8NbHUjWym7YmzCoD4E+JpuHgAWqNtww16368=; b=PqqaY3DzEv9AEduRGYjkNJ39MdIskUhFJtvvwujdSJImUYA2MlDWsQluycDRj9GcSOcSJt 5tLMFiNhIqpR6IxYHwlNBIyT6G6IPFuiYzuJsbvNboKfY4tWivcIlnk4k2lZrg8dfJHrY6 ZMqIwv60+55AsH90c/0vQqkanJgfEA0= Date: Thu, 26 Dec 2024 23:42:32 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH] psi: Fix race when task wakes up before psi_sched_switch() adjusts flags X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou To: K Prateek Nayak , Johannes Weiner , Suren Baghdasaryan , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , linux-kernel@vger.kernel.org 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> In-Reply-To: <103e4236-c01e-4286-9152-007d9a249a65@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT On 2024/12/26 19:35, Chengming Zhou wrote: > On 2024/12/26 19:04, K Prateek Nayak wrote: >> 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) > > Ah, right :) > >> >>> >>>>      [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); > > 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). > > 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. 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)) { /* Wakeup of new or sleeping task */ if (p->in_iowait) clear |= TSK_IOWAIT;