Received-SPF: pass (google.com: domain of linux-kernel+bounces-95193-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240220225622.2626569-1-qyousef@layalina.io> <20240220225622.2626569-3-qyousef@layalina.io>
 <d6699c3a-3df6-46a3-98db-e07c8722f106@arm.com> <20240303174416.7m3gv5wywcmedov4@airbuntu>
 <20240306214704.uditboboedut2lm2@airbuntu>
In-Reply-To: <20240306214704.uditboboedut2lm2@airbuntu>
From: Vincent Guittot <vincent.guittot@linaro.org>
Date: Thu, 7 Mar 2024 10:14:19 +0100
Message-ID: <CAKfTPtBLUkZ0hEd8K=e9wjg+zn9N5jgia-7wwLa3jaeYK+qkCw@mail.gmail.com>
Subject: Re: [PATCH v6 2/4] sched/fair: Check a task has a fitting cpu when
 updating misfit
To: Qais Yousef <qyousef@layalina.io>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>, Ingo Molnar <mingo@kernel.org>, 
	Peter Zijlstra <peterz@infradead.org>, linux-kernel@vger.kernel.org, 
	Pierre Gondois <Pierre.Gondois@arm.com>
Content-Type: text/plain; charset="UTF-8"

On Wed, 6 Mar 2024 at 22:47, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 03/03/24 17:44, Qais Yousef wrote:
>
> >       diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >       index 174687252e1a..b0e60a565945 100644
> >       --- a/kernel/sched/fair.c
> >       +++ b/kernel/sched/fair.c
> >       @@ -8260,6 +8260,8 @@ static void set_task_max_allowed_capacity(struct task_struct *p)
> >                       cpumask_t *cpumask;
> >
> >                       cpumask = cpu_capacity_span(entry);
> >       +               if (!cpumask_intersects(cpu_active_mask, cpumask))
> >       +                       continue;
> >                       if (!cpumask_intersects(p->cpus_ptr, cpumask))
> >                               continue;
> >
> >       @@ -8269,6 +8271,53 @@ static void set_task_max_allowed_capacity(struct task_struct *p)
> >               rcu_read_unlock();
> >        }
> >
> >       +static void __update_tasks_max_allowed_capacity(unsigned long capacity)
> >       +{
> >       +       struct task_struct *g, *p;
> >       +
> >       +       for_each_process_thread(g, p) {
> >       +               if (fair_policy(p->policy) && p->max_allowed_capacity == capacity)
>
> This condition actually not good enough. We need to differentiate between going
> online/offline. I didn't want to call set_task_max_allowed_capacity()
> unconditionally and make hotplug even slower.

But should we even try to fix this ? hotplugging a cpu is a special
case and with patch 4 you will not increase lb_interval anymore

>
> I'm doing more testing and will post v8 once done. I need to cater for a new
> user when dynamic EM changes capacities too.. Small things can snow ball easily
> hehe.
>
> >       +                       set_task_max_allowed_capacity(p);
> >       +       }
> >       +}
> >       +
> >       +/*
> >       + * Handle a cpu going online/offline changing the available capacity levels.
> >       + */
> >       +static void update_tasks_max_allowed_capacity(int cpu, bool online)
> >       +{
> >       +       struct asym_cap_data *entry;
> >       +       bool do_update = false;
> >       +
> >       +       if (!sched_asym_cpucap_active())
> >       +               return;
> >       +
> >       +       if (cpuhp_tasks_frozen)
> >       +               return;
> >       +
> >       +       rcu_read_lock();
> >       +       /* Did a capacity level appear/disappear? */
> >       +       list_for_each_entry_rcu(entry, &asym_cap_list, link) {
> >       +               unsigned int nr_active;
> >       +               cpumask_t *cpumask;
> >       +
> >       +               cpumask = cpu_capacity_span(entry);
> >       +
> >       +               if (!cpumask_test_cpu(cpu, cpumask))
> >       +                       continue;
> >       +
> >       +               nr_active = cpumask_weight_and(cpu_active_mask, cpumask);
> >       +               if (online)
> >       +                       do_update = nr_active == 1;
> >       +               else
> >       +                       do_update = !nr_active;
> >       +               break;
> >       +       }
> >       +       if (do_update)
> >       +               __update_tasks_max_allowed_capacity(entry->capacity);
> >       +       rcu_read_unlock();
> >       +}
> >       +
> >        static void set_cpus_allowed_fair(struct task_struct *p, struct affinity_context *ctx)
> >        {
> >               set_cpus_allowed_common(p, ctx);
> >       @@ -12500,6 +12549,8 @@ static void rq_online_fair(struct rq *rq)
> >               update_sysctl();
> >
> >               update_runtime_enabled(rq);
> >       +
> >       +       update_tasks_max_allowed_capacity(cpu_of(rq), true);
> >        }
> >
> >        static void rq_offline_fair(struct rq *rq)
> >       @@ -12511,6 +12562,8 @@ static void rq_offline_fair(struct rq *rq)
> >
> >               /* Ensure that we remove rq contribution to group share: */
> >               clear_tg_offline_cfs_rqs(rq);
> >       +
> >       +       update_tasks_max_allowed_capacity(cpu_of(rq), false);
> >        }
> >
> >        #endif /* CONFIG_SMP */
> > --
> > 2.34.1