Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220619120451.95251-1-wuyun.abel@bytedance.com>
 <20220619120451.95251-6-wuyun.abel@bytedance.com> <CABk29NtsTgJT1c-Vg6B5dgYbi+nHEDk2vsBRzRowCXiOPT2qnA@mail.gmail.com>
 <c353f360-e630-ea29-2045-1a9f005d033d@bytedance.com>
In-Reply-To: <c353f360-e630-ea29-2045-1a9f005d033d@bytedance.com>
From:   Josh Don <joshdon@google.com>
Date:   Tue, 28 Jun 2022 18:11:04 -0700
Message-ID: <CABk29NuSONYdmLqUDbJKQfwF3tf1Uv3Yy+WbHkh_gY5FXti1cA@mail.gmail.com>
Subject: Re: [PATCH v4 5/7] sched/fair: skip SIS domain search if fully busy
To:     Abel Wu <wuyun.abel@bytedance.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Mel Gorman <mgorman@suse.de>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Chen Yu <yu.c.chen@intel.com>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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()?