Re: [RFC PATCH 11/13] sched: migration changes for core scheduling
On 2020/6/14 2:59, Vineeth Remanan Pillai wrote: > On Fri, Jun 12, 2020 at 10:25 PM Joel Fernandes > wrote: >> >> Ok, so I take it that you will make it so in v6 then, unless of course >> someone else objects. >> > Yes, just wanted to hear from Aubrey, Tim and others as well to see > if we have not missed anything obvious. Will have this in v6 if > there are no objections. > > Thanks for bringing this up! > > ~Vineeth > Yes, this makes sense to me, no need to find idle core in select_idle_cpu(). Thanks to catch this! Thanks, -Aubrey
Re: [RFC PATCH 11/13] sched: migration changes for core scheduling
On Fri, Jun 12, 2020 at 10:25 PM Joel Fernandes wrote: > > Ok, so I take it that you will make it so in v6 then, unless of course > someone else objects. > Yes, just wanted to hear from Aubrey, Tim and others as well to see if we have not missed anything obvious. Will have this in v6 if there are no objections. Thanks for bringing this up! ~Vineeth
Re: [RFC PATCH 11/13] sched: migration changes for core scheduling
On Fri, Jun 12, 2020 at 05:32:01PM -0400, Vineeth Remanan Pillai wrote: > > AFAIR, that's what v4 did: > > > > if (available_idle_cpu(cpu)) > > #ifdef CONFIG_SCHED_CORE > > if (sched_core_enabled(cpu_rq(cpu)) && > > (p->core_cookie == > > cpu_rq(cpu)->core->core_cookie)) > > break; > > #else > > break; > > #endif > > > This patch was initially not in v4 and this is a merging of 4 patches > suggested post-v4. During the initial round, code was like above. But since > there looked like a code duplication in the different migration paths, > it was consolidated into sched_core_cookie_match() and it caused this > extra logic to this specific code path. As you mentioned, I also feel > we do not need to check for core idleness in this path. Ok, so I take it that you will make it so in v6 then, unless of course someone else objects. thanks! - Joel
Re: [RFC PATCH 11/13] sched: migration changes for core scheduling
On Fri, Jun 12, 2020 at 9:21 AM Joel Fernandes wrote: > > > +#ifdef CONFIG_SCHED_CORE > > + if (available_idle_cpu(cpu) && > > + sched_core_cookie_match(cpu_rq(cpu), p)) > > + break; > > +#else > > select_idle_cpu() is called only if no idle core could be found in the LLC by > select_idle_core(). > > So, would it be better here to just do the cookie equality check directly > instead of calling the sched_core_cookie_match() helper? More so, because > select_idle_sibling() is a fastpath. > Agree, this makes sense to me. > AFAIR, that's what v4 did: > > if (available_idle_cpu(cpu)) > #ifdef CONFIG_SCHED_CORE > if (sched_core_enabled(cpu_rq(cpu)) && > (p->core_cookie == > cpu_rq(cpu)->core->core_cookie)) > break; > #else > break; > #endif > This patch was initially not in v4 and this is a merging of 4 patches suggested post-v4. During the initial round, code was like above. But since there looked like a code duplication in the different migration paths, it was consolidated into sched_core_cookie_match() and it caused this extra logic to this specific code path. As you mentioned, I also feel we do not need to check for core idleness in this path. Thanks, Vineeth
Re: [RFC PATCH 11/13] sched: migration changes for core scheduling
On Wed, Mar 04, 2020 at 05:00:01PM +, vpillai wrote:
> From: Aubrey Li
>
> - Don't migrate if there is a cookie mismatch
> Load balance tries to move task from busiest CPU to the
> destination CPU. When core scheduling is enabled, if the
> task's cookie does not match with the destination CPU's
> core cookie, this task will be skipped by this CPU. This
> mitigates the forced idle time on the destination CPU.
>
> - Select cookie matched idle CPU
> In the fast path of task wakeup, select the first cookie matched
> idle CPU instead of the first idle CPU.
>
> - Find cookie matched idlest CPU
> In the slow path of task wakeup, find the idlest CPU whose core
> cookie matches with task's cookie
>
> - Don't migrate task if cookie not match
> For the NUMA load balance, don't migrate task to the CPU whose
> core cookie does not match with task's cookie
>
> Signed-off-by: Aubrey Li
> Signed-off-by: Tim Chen
> Signed-off-by: Vineeth Remanan Pillai
> ---
> kernel/sched/fair.c | 55 +---
> kernel/sched/sched.h | 29 +++
> 2 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 1c9a80d8dbb8..f42ceecb749f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1789,6 +1789,15 @@ static void task_numa_find_cpu(struct task_numa_env
> *env,
> if (!cpumask_test_cpu(cpu, env->p->cpus_ptr))
> continue;
>
> +#ifdef CONFIG_SCHED_CORE
> + /*
> + * Skip this cpu if source task's cookie does not match
> + * with CPU's core cookie.
> + */
> + if (!sched_core_cookie_match(cpu_rq(cpu), env->p))
> + continue;
> +#endif
> +
> env->dst_cpu = cpu;
> task_numa_compare(env, taskimp, groupimp, maymove);
> }
> @@ -5660,8 +5669,13 @@ find_idlest_group_cpu(struct sched_group *group,
> struct task_struct *p, int this
>
> /* Traverse only the allowed CPUs */
> for_each_cpu_and(i, sched_group_span(group), p->cpus_ptr) {
> + struct rq *rq = cpu_rq(i);
> +
> +#ifdef CONFIG_SCHED_CORE
> + if (!sched_core_cookie_match(rq, p))
> + continue;
> +#endif
> if (available_idle_cpu(i)) {
> - struct rq *rq = cpu_rq(i);
> struct cpuidle_state *idle = idle_get_state(rq);
> if (idle && idle->exit_latency < min_exit_latency) {
> /*
> @@ -5927,8 +5941,14 @@ static int select_idle_cpu(struct task_struct *p,
> struct sched_domain *sd, int t
> return si_cpu;
> if (!cpumask_test_cpu(cpu, p->cpus_ptr))
> continue;
> +#ifdef CONFIG_SCHED_CORE
> + if (available_idle_cpu(cpu) &&
> + sched_core_cookie_match(cpu_rq(cpu), p))
> + break;
> +#else
select_idle_cpu() is called only if no idle core could be found in the LLC by
select_idle_core().
So, would it be better here to just do the cookie equality check directly
instead of calling the sched_core_cookie_match() helper? More so, because
select_idle_sibling() is a fastpath.
AFAIR, that's what v4 did:
if (available_idle_cpu(cpu))
#ifdef CONFIG_SCHED_CORE
if (sched_core_enabled(cpu_rq(cpu)) &&
(p->core_cookie == cpu_rq(cpu)->core->core_cookie))
break;
#else
break;
#endif
Thoughts? thanks,
- Joel
