Messages in this thread | | | From | Josh Don <> | Date | Tue, 28 Jun 2022 18:11:04 -0700 | Subject | Re: [PATCH v4 5/7] sched/fair: skip SIS domain search if fully busy |
| |
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.
A comment here and also at the enum definitions would be helpful I think.
> > > >> + > >> +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).
> >> 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.
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()?
| |