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 46D1B24A7C6 for ; Thu, 16 Jan 2025 17:35:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737048954; cv=none; b=srlOyB56WMMmh+9VnaqBD8tb0LwLJMhS2Zf2HBpX4TXzAWZ42dsBzn5RikTdhvS9mLylw8XYeIS+3KFwxs5z3oy3BP6EORMuR2vFIeuOcRqD2FCAhLL/UTNcA6O7TPe7Tqd0LTNQCr8k9EdF3ECpDSTzd+57xZG1jzdR9asHnX4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737048954; c=relaxed/simple; bh=5Uph751VoBjOWELz541y2ZAbn8hGS0RouR3kZ+Zan1s=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=hNmSzIhBJKXw6bGNfYsfLi3MOgUNiVpjS1PK3DIlFMZyGl9msLY0TIektJV4u29TxIDU6UCanZxLNbEJ3DyzNP4whOwY3M7M6FpnGMF/TJytGgLqXHWx6GjPfmU16akGcyvfzCmyAsT4E+XHZ3ncuRBTslgkM/RkALIqgV8HAH8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 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 B715E106F; Thu, 16 Jan 2025 09:36:19 -0800 (PST) Received: from [10.154.93.158] (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1FBC63F73F; Thu, 16 Jan 2025 09:35:44 -0800 (PST) Message-ID: <91e59f29-355b-47f9-838e-4e97011cf122@arm.com> Date: Thu, 16 Jan 2025 18:35:42 +0100 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 6/7 v2] sched/fair: Add misfit case to push task callback for EAS To: Vincent Guittot , mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com, dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de, vschneid@redhat.com, lukasz.luba@arm.com, rafael.j.wysocki@intel.com, linux-kernel@vger.kernel.org Cc: qyousef@layalina.io, hongyan.xia2@arm.com, christian.loehle@arm.com, qperret@google.com References: <20241217160720.2397239-1-vincent.guittot@linaro.org> <20241217160720.2397239-7-vincent.guittot@linaro.org> Content-Language: en-US From: Pierre Gondois In-Reply-To: <20241217160720.2397239-7-vincent.guittot@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 12/17/24 17:07, Vincent Guittot wrote: > Some task misfit cases can be handled directly by the push callback > instead of triggering an idle load balance to pull the task on a better > CPU. Aren't misfit tasks migrated using active_load_balance_cpu_stop() rather than the push mechanism ? Also, I don't see cases where a misfit task would not be migrated by either the push mechanism or the misfit handling present in this patch. Is it possible to detail a case where the misfit load balancer would still be needed ? > > Signed-off-by: Vincent Guittot > > # Conflicts: > # kernel/sched/fair.c > --- > kernel/sched/fair.c | 53 +++++++++++++++++++++++++++++---------------- > 1 file changed, 34 insertions(+), 19 deletions(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 2affc063da55..9bddb094ee21 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -8541,6 +8541,8 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) > target_stat.runnable = cpu_runnable(cpu_rq(cpu)); > target_stat.capa = capacity_of(cpu); > target_stat.nr_running = cpu_rq(cpu)->cfs.h_nr_runnable; > + if ((p->on_rq) && (!p->se.sched_delayed) && (cpu == prev_cpu)) > + target_stat.nr_running--; > > /* If the target needs a lower OPP, then look up for > * the corresponding OPP and its associated cost. > @@ -8623,48 +8625,58 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu) > > static inline bool task_misfit_cpu(struct task_struct *p, int cpu) > { > - unsigned long max_capa = get_actual_cpu_capacity(cpu); > - unsigned long util = task_util_est(p); > + unsigned long max_capa, util; > + > + if (p->nr_cpus_allowed == 1) > + return false; > > - max_capa = min(max_capa, uclamp_eff_value(p, UCLAMP_MAX)); > - util = max(util, task_runnable(p)); > + max_capa = min(get_actual_cpu_capacity(cpu), > + uclamp_eff_value(p, UCLAMP_MAX)); > + util = max(task_util_est(p), task_runnable(p)); > > /* > * Return true only if the task might not sleep/wakeup because of a low > * compute capacity. Tasks, which wake up regularly, will be handled by > * feec(). > */ > - return (util > max_capa); > + if (util > max_capa) > + return true; > + > + /* Return true if the task doesn't fit anymore to run on the cpu */ > + if ((arch_scale_cpu_capacity(cpu) < p->max_allowed_capacity) && !task_fits_cpu(p, cpu)) > + return true; This logic seems to already be present in update_misfit_status(). Maybe it would be good to factorize it to have a common criteria for misfit tasks. > + > + return false; > } > > static int active_load_balance_cpu_stop(void *data); > > -static inline void migrate_misfit_task(struct task_struct *p, struct rq *rq) > +static inline bool migrate_misfit_task(struct task_struct *p, struct rq *rq) > { > int new_cpu, cpu = cpu_of(rq); > > if (!sched_energy_enabled() || is_rd_overutilized(rq->rd)) > - return; > + return false; > > if (WARN_ON(!p)) > - return; > + return false; > > - if (WARN_ON(p != rq->curr)) > - return; > + if (WARN_ON(!task_current(rq, p))) > + return false; > > if (is_migration_disabled(p)) > - return; > + return false; > > - if ((rq->nr_running > 1) || (p->nr_cpus_allowed == 1)) > - return; > + if (rq->nr_running > 1) > + return false; NIT: Maybe the condition (p->nr_cpus_allowed == 1) could have already been part of task_misfit_cpu() in the previous patch. > > if (!task_misfit_cpu(p, cpu)) > - return; > + return false; > > new_cpu = find_energy_efficient_cpu(p, cpu); > > if (new_cpu == cpu) > - return; > + return false; > > /* > * ->active_balance synchronizes accesses to > @@ -8675,13 +8687,15 @@ static inline void migrate_misfit_task(struct task_struct *p, struct rq *rq) > rq->active_balance = 1; > rq->push_cpu = new_cpu; > } else > - return; > + return false; > > raw_spin_rq_unlock(rq); > stop_one_cpu_nowait(cpu, > active_load_balance_cpu_stop, rq, > &rq->active_balance_work); > raw_spin_rq_lock(rq); > + > + return true; > } > > static inline int has_pushable_tasks(struct rq *rq) > @@ -13299,9 +13313,10 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued) > if (static_branch_unlikely(&sched_numa_balancing)) > task_tick_numa(rq, curr); > > - migrate_misfit_task(curr, rq); > - update_misfit_status(curr, rq); > - check_update_overutilized_status(task_rq(curr)); > + if (!migrate_misfit_task(curr, rq)) { > + update_misfit_status(curr, rq); If the system is not-OU, the only case I see where migrate_misfit_task() would not detect a misfit task and update_misfit_status() would is if there is another task on the rq. I.e. through: migrate_misfit_task() \-if (rq->nr_running > 1) return false; However in this case, the push callback should migrate the misfit task. So is it still necessary to look for misfit task through sched_balance_find_src_group() ? > + check_update_overutilized_status(task_rq(curr)); > + } > > task_tick_core(rq, curr); > }