Message-ID: <d9be50f1-087e-d854-ed54-88731f4d5335@bytedance.com>
Date:   Wed, 29 Jun 2022 15:05:10 +0800
From:   Abel Wu <wuyun.abel@...edance.com>
To:     Josh Don <joshdon@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Mel Gorman <mgorman@...e.de>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Chen Yu <yu.c.chen@...el.com>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        K Prateek Nayak <kprateek.nayak@....com>,
        "Gautham R . Shenoy" <gautham.shenoy@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 5/7] sched/fair: skip SIS domain search if fully busy


On 6/29/22 9:11 AM, Josh Don Wrote:
> On Mon, Jun 27, 2022 at 11:53 PM Abel Wu <wuyun.abel@...edance.com> wrote:
>>
>>>>
>>>> -static inline bool test_idle_cores(int cpu)
>>>> +static inline enum sd_state sd_get_state(int cpu)
>>>>    {
>>>>           struct sched_domain_shared *sds;
>>>>
>>>>           sds = rcu_dereference(per_cpu(sd_llc_shared, cpu));
>>>>           if (sds)
>>>> -               return READ_ONCE(sds->has_idle_cores);
>>>> +               return READ_ONCE(sds->state);
>>>>
>>>> -       return false;
>>>> +       return sd_has_icpus;
>>>> +}
>>>
>>> Why is default not sd_is_busy?
>>
>> The state of sd_is_busy will prevent us from searching the LLC. By
>> design, both sd_has_icores and sd_is_busy indicate deterministic
>> status: has idle cores / no idle cpu exists. While sd_has_icpus is
>> not deterministic, it means there could be unoccupied cpus.
>>
>> The naming seems misleading, it would be nice to have other options.
> 
> sd_has_icores isn't deterministic; when the last fully idle core gets
> an occupied sibling, it will take until the next select_idle_cpu() to
> mark the state as sd_has_icpus instead.

Yes, it's not deterministic in nature, but we treat it as deterministic.
As long as sd_has_icores, a full scan will be fired no matter there are
any idle cores or not.

> 
> A comment here and also at the enum definitions would be helpful I think.

Agreed. I will add some comments here. State descriptions are already
above their definitions, please let me know if any modification needed.

> 
>>>
>>>> +
>>>> +static inline void set_idle_cores(int cpu, int idle)
>>>
>>> nit: Slightly confusing to call the param 'idle', since in the case it
>>> is false we still mark icpus. Consider possibly 'core_idle'.
>>
>> What about changing the param 'cpu' to 'core'?
> 
> I think keeping it as "cpu" is fine, since as "core" that would imply
> some per-core state (when we're still setting this per-cpu).

The function has already been there for a long time, and I haven't
changed its semantics, so maybe it isn't that confusing.. Does the
following naming make things clearer?

static inline void set_idle_cores(int cpu, int has_icores);
static inline void set_idle_cpus(int cpu, int has_icpus);

> 
>>>>           for_each_cpu_and(i, sched_group_span(group), env->cpus) {
>>>>                   struct rq *rq = cpu_rq(i);
>>>> @@ -8692,6 +8740,9 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>>>>                   nr_running = rq->nr_running;
>>>>                   sgs->sum_nr_running += nr_running;
>>>>
>>>> +               if (update_core)
>>>> +                       sd_classify(sds, rq);
>>>> +
>>>>                   if (nr_running > 1)
>>>>                           *sg_status |= SG_OVERLOAD;
>>>>
>>>> @@ -9220,6 +9271,12 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p, int this_cpu)
>>>>           return idlest;
>>>>    }
>>>>
>>>> +static void sd_update_state(struct lb_env *env, struct sd_lb_stats *sds)
>>>> +{
>>>> +       if (sds->sd_state == sd_has_icpus && !test_idle_cpus(env->dst_cpu))
>>>> +               set_idle_cpus(env->dst_cpu, true);
>>>> +}
>>>
>>> We're only setting state to has_icpus here in sd_update_state. That
>>> doesn't feel good enough, since we're only updating state for
>>> env->dst_cpu; all the other per-cpu state will remain stale (ie.
>>> falsely sd_is_busy).
>>
>> It's LLC-wide shared :)
> 
> Oh wow, yea that's the thing I missed... Thanks.
> 
>>> I think you also want a case in __update_idle_core() to call
>>> set_idle_cores(core, 0) in the case where we have a non-idle sibling,
>>> since we want to at least mark has_icpus even if the entire core isn't
>>> idle.
> 
> More specifically, in the __update_idle_core() function, if the
> sibling is still busy and the sd_state is sd_is_busy, we should
> instead mark it as sd_has_icpus, since the current cpu is guaranteed
> to be going idle.

The sd_is_busy state will be cleared during newidle balance. And the
state should not set back to is_busy between the gap before the cpu
actually goes idle.

> 
> Additionally, to be consistent with what we're calling "idle"
> elsewhere, I think you mean to have __update_idle_core() check either
> available_idle_cpu() or sched_idle_cpu()?

I think the condition should be aligned with SIS that an unoccupied
cpu satisfies "idle_cpu(cpu) || sched_idle_cpu(cpu)". The function
available_idle_cpu() is not used in load balancing due to its not
being used immediately.

Thanks & BR,
Abel

Powered by blists - more mailing lists