Messages in this thread | | | Date | Wed, 29 Jun 2022 15:05:10 +0800 | Subject | Re: [PATCH v4 5/7] sched/fair: skip SIS domain search if fully busy | From | Abel Wu <> |
| |
On 6/29/22 9:11 AM, Josh Don Wrote: > On Mon, Jun 27, 2022 at 11:53 PM Abel Wu <wuyun.abel@bytedance.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
| |