From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-175.mta0.migadu.com (out-175.mta0.migadu.com [91.218.175.175]) (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 BA81381741 for ; Fri, 27 Dec 2024 04:40:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.175 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735274417; cv=none; b=joMYe/WJjeuP+B3siPlcC00fLK5hrE3e4zmOZfjIZAc5UQ61jQKAtIRzP6ytVV77C0JBWAZEu00SWTT62p8ABvlnRVTzGhcQ8cwd6kAMvTgjj0xrA8zf5jrlzxpugakP1dYZWCutoyJmT3SQ0cXmfVdsVdY1RNNESEJAyLqJVaI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735274417; c=relaxed/simple; bh=f3Shbqc103e6Iu/Bzv6XShBOvL/LfF/sjvUFmeq1/qs=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=QnG16HupXQaoGwFU37cfHBZBaxuXHTpNQqHYSGG/OobH6NJau5VIsSRg5FbtI2KnDZ6PBtQVVjb7bCf8P7ftnyJCCQWb6pE/qmhRLWl75drQj9g3Ir51BShfaNUsK/HQoK/7Ej8ACVH3VZaHLVhv63ZT+75F5ajjviS6p8WjM+0= 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=labOtX5Z; arc=none smtp.client-ip=91.218.175.175 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="labOtX5Z" Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1735274410; 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=aSvuRfs2WJoT/7XLbqDEbCIPTXZIJUWbe5XjE3rGocY=; b=labOtX5ZuONhXlpf4ZF9FktjpUOm245paIb2fCprqRO3aFeEP9QtV77gYi6SZfnSpG78dk VWtsSpMH6OR70QZPEYJ02Ev3IF+z195Z8xD6XNRX9QQsn+rveqeah7kpjVgVh/ocNIEFMU bXNUDYuBVOEqb3wlqewj5Ggm5exrMYo= Date: Fri, 27 Dec 2024 12:40:00 +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 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> <409b4a72-483e-467b-8d00-9a8dae48bdc9@linux.dev> <4e6e7308-1d39-427d-af47-2957025f501b@amd.com> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou In-Reply-To: <4e6e7308-1d39-427d-af47-2957025f501b@amd.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT On 2024/12/27 12:10, K Prateek Nayak wrote: > Hello there, > [...] >> >> 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. Great, thanks for your time! Signed-off-by: Chengming Zhou > >> >> 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. Yes. > > 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. I thought it's good to preserve the current flow that: if (restore) return; if (migrate) ... else if (wakeup) ... As for early return when `task_on_cpu()`, it looks right to me. Anyway, it's not a migrate or wakeup from PSI POV. Thanks! > >>                  /* Wakeup of new or sleeping task */ >>                  if (p->in_iowait) >>                          clear |= TSK_IOWAIT; >