From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 8D40A7319C for ; Thu, 21 Dec 2023 12:30:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 723952F4; Thu, 21 Dec 2023 04:31:30 -0800 (PST) Received: from [10.1.25.50] (e132833.arm.com [10.1.25.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C14A73F5A1; Thu, 21 Dec 2023 04:30:42 -0800 (PST) Message-ID: Date: Thu, 21 Dec 2023 12:30:41 +0000 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 v7 10/23] sched: Split out __sched() deactivate task logic into a helper Content-Language: en-US To: John Stultz , LKML Cc: Joel Fernandes , Qais Yousef , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Valentin Schneider , Steven Rostedt , Ben Segall , Zimuzo Ezeozue , Youssef Esmat , Mel Gorman , Daniel Bristot de Oliveira , Will Deacon , Waiman Long , Boqun Feng , "Paul E. McKenney" , Xuewen Yan , K Prateek Nayak , Thomas Gleixner , kernel-team@android.com References: <20231220001856.3710363-1-jstultz@google.com> <20231220001856.3710363-11-jstultz@google.com> From: Metin Kaya In-Reply-To: <20231220001856.3710363-11-jstultz@google.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 20/12/2023 12:18 am, John Stultz wrote: > As we're going to re-use the deactivation logic, > split it into a helper. > > Cc: Joel Fernandes > Cc: Qais Yousef > Cc: Ingo Molnar > Cc: Peter Zijlstra > Cc: Juri Lelli > Cc: Vincent Guittot > Cc: Dietmar Eggemann > Cc: Valentin Schneider > Cc: Steven Rostedt > Cc: Ben Segall > Cc: Zimuzo Ezeozue > Cc: Youssef Esmat > Cc: Mel Gorman > Cc: Daniel Bristot de Oliveira > Cc: Will Deacon > Cc: Waiman Long > Cc: Boqun Feng > Cc: "Paul E. McKenney" > Cc: Metin Kaya > Cc: Xuewen Yan > Cc: K Prateek Nayak > Cc: Thomas Gleixner > Cc: kernel-team@android.com > Signed-off-by: John Stultz > --- > v6: > * Define function as static to avoid "no previous prototype" > warnings as Reported-by: kernel test robot > v7: > * Rename state task_state to be more clear, as suggested by > Metin Kaya > --- > kernel/sched/core.c | 66 +++++++++++++++++++++++++-------------------- > 1 file changed, 37 insertions(+), 29 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 0ce34f5c0e0c..34acd80b6bd0 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -6571,6 +6571,42 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf) > # define SM_MASK_PREEMPT SM_PREEMPT > #endif > * Better to have a comment (e.g., in which conditions try_to_deactivate_task() returns true or false) here. * try_to_deactivate_task() is temporarily used by 2 commits in the patch set (i.e., it's only called by __schedule() just like in this patch at the end of the series). However, it's nice to make that big __scheduler() function a bit modular as we discussed off-list. So, should we move this function out of the Proxy Execution patch set to get it merged independently? > +static bool try_to_deactivate_task(struct rq *rq, struct task_struct *p, > + unsigned long task_state) > +{ > + if (signal_pending_state(task_state, p)) { > + WRITE_ONCE(p->__state, TASK_RUNNING); > + } else { > + p->sched_contributes_to_load = > + (task_state & TASK_UNINTERRUPTIBLE) && > + !(task_state & TASK_NOLOAD) && > + !(task_state & TASK_FROZEN); > + > + if (p->sched_contributes_to_load) > + rq->nr_uninterruptible++; > + > + /* > + * __schedule() ttwu() > + * prev_state = prev->state; if (p->on_rq && ...) > + * if (prev_state) goto out; > + * p->on_rq = 0; smp_acquire__after_ctrl_dep(); > + * p->state = TASK_WAKING > + * > + * Where __schedule() and ttwu() have matching control dependencies. > + * > + * After this, schedule() must not care about p->state any more. > + */ > + deactivate_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_NOCLOCK); > + > + if (p->in_iowait) { > + atomic_inc(&rq->nr_iowait); > + delayacct_blkio_start(); > + } > + return true; > + } > + return false; > +} > + > /* > * __schedule() is the main scheduler function. > * > @@ -6662,35 +6698,7 @@ static void __sched notrace __schedule(unsigned int sched_mode) > */ > prev_state = READ_ONCE(prev->__state); > if (!(sched_mode & SM_MASK_PREEMPT) && prev_state) { > - if (signal_pending_state(prev_state, prev)) { > - WRITE_ONCE(prev->__state, TASK_RUNNING); > - } else { > - prev->sched_contributes_to_load = > - (prev_state & TASK_UNINTERRUPTIBLE) && > - !(prev_state & TASK_NOLOAD) && > - !(prev_state & TASK_FROZEN); > - > - if (prev->sched_contributes_to_load) > - rq->nr_uninterruptible++; > - > - /* > - * __schedule() ttwu() > - * prev_state = prev->state; if (p->on_rq && ...) > - * if (prev_state) goto out; > - * p->on_rq = 0; smp_acquire__after_ctrl_dep(); > - * p->state = TASK_WAKING > - * > - * Where __schedule() and ttwu() have matching control dependencies. > - * > - * After this, schedule() must not care about p->state any more. > - */ > - deactivate_task(rq, prev, DEQUEUE_SLEEP | DEQUEUE_NOCLOCK); > - > - if (prev->in_iowait) { > - atomic_inc(&rq->nr_iowait); > - delayacct_blkio_start(); > - } > - } > + try_to_deactivate_task(rq, prev, prev_state); > switch_count = &prev->nvcsw; > } >