All of lore.kernel.org
 help / color / mirror / Atom feed
From: Xuewen Yan <xuewen.yan94@gmail.com>
To: Qais Yousef <qais.yousef@arm.com>
Cc: Xuewen Yan <xuewen.yan@unisoc.com>,
	dietmar.eggemann@arm.com, lukasz.luba@arm.com, rafael@kernel.org,
	viresh.kumar@linaro.org, mingo@redhat.com, peterz@infradead.org,
	vincent.guittot@linaro.org, rostedt@goodmis.org,
	linux-kernel@vger.kernel.org, di.shen@unisoc.com
Subject: Re: [PATCH] sched: Take thermal pressure into account when determine rt fits capacity
Date: Mon, 9 May 2022 10:29:50 +0800	[thread overview]
Message-ID: <CAB8ipk--Y8HxetcmUhBmtWq6Mmd726QmDbcbibGLERJw_PUqkQ@mail.gmail.com> (raw)
In-Reply-To: <20220503144352.lxduzhl6jq6xdhw2@airbuntu>

Hi Qais

Sorry for the late reply.

On Tue, May 3, 2022 at 10:43 PM Qais Yousef <qais.yousef@arm.com> wrote:
>
> Hi Xuewen
>
> On 05/01/22 11:20, Xuewen Yan wrote:
> > Hi Qais
> > Thanks for the patient explanation.:)
> > And I have some other concerns.
> >
> > On Wed, Apr 27, 2022 at 6:58 PM Qais Yousef <qais.yousef@arm.com> wrote:
> > >
> > > On 04/27/22 09:38, Xuewen Yan wrote:
> > > > > > > The best (simplest) way forward IMHO is to introduce a new function
> > > > > > >
> > > > > > >         bool cpu_in_capacity_inversion(int cpu);
> >
> > Maybe the implementation of this function, I have not thought of a
> > good solution.
> > (1)how to define the inversion, if the cpu has two
> > cluster(little/big),it is easy, but still need mark which is the big
> > cpu...
>
> I'd define it as:
>
>         capacity_orig_of(cpu) - thermal_pressure(cpu) < capacity_orig_of(next_level_cpu)
ok.
>
> > (2)because the mainline kernel should be common, if the cpu has three
> > or more clusters, maybe the mid cpus also would be inversion;
>
> Yes. I pray this is highly unlikely though! We should cater for it still.
>
> > (3)what time update the cpu inversion state, if we judge the cpu
> > inversion whenever the thermal pressure changed, could we receive the
> > complexity? because may we should traverse all possible cpu.
>
> In my head, it would make sense to detect the inversion in
> update_cpu_capacity() OR in topology_update_thermal_pressure(). So at whatever
> rate this happens at.
>
> Does this answer your question?
Yes, get.
>
> Basically I believe something like this should be enough (completely untested)
>
> --->8---
>
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index a68482d66535..44c7c2598d87 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8399,16 +8399,37 @@ static unsigned long scale_rt_capacity(int cpu)
>
>  static void update_cpu_capacity(struct sched_domain *sd, int cpu)
>  {
> +       unsigned long capacity_orig = arch_scale_cpu_capacity(cpu);
>         unsigned long capacity = scale_rt_capacity(cpu);
>         struct sched_group *sdg = sd->groups;
> +       struct rq *rq = cpu_rq(cpu);
>
> -       cpu_rq(cpu)->cpu_capacity_orig = arch_scale_cpu_capacity(cpu);
> +       rq->cpu_capacity_orig = capacity_orig;
>
>         if (!capacity)
>                 capacity = 1;
>
> -       cpu_rq(cpu)->cpu_capacity = capacity;
> -       trace_sched_cpu_capacity_tp(cpu_rq(cpu));
> +       rq->cpu_capacity = capacity;
> +       trace_sched_cpu_capacity_tp(rq);
> +
> +       if (static_branch_unlikely(&sched_asym_cpucapacity)) {
> +               unsigned long inv_cap = capacity_orig - thermal_load_avg(rq);

Indeed, I prefer arch_thermal_pressure here, because the
thermal_load_avg would change over time,
but the inv_cap's update period may could not keep up with his changes.

> +
> +               rq->cpu_capacity_inverted = 0;
> +
> +               for_each_possible_cpu(cpu) {
> +                       unsigned long cap = arch_scale_cpu_capacity(cpu);
> +
> +                       if (capacity_orig <= cap)
> +                               continue;
> +
> +                       if (cap > inv_cap) {
> +                               rq->cpu_capacity_inverted = inv_cap;
> +                               break;
> +                       }
> +               }
> +
> +       }
>
>         sdg->sgc->capacity = capacity;
>         sdg->sgc->min_capacity = capacity;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 8dccb34eb190..bfe84c870bf9 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -992,6 +992,7 @@ struct rq {
>
>         unsigned long           cpu_capacity;
>         unsigned long           cpu_capacity_orig;
> +       unsigned long           cpu_capacity_inverted;
>
>         struct callback_head    *balance_callback;
>
> @@ -2807,6 +2808,11 @@ static inline unsigned long capacity_orig_of(int cpu)
>         return cpu_rq(cpu)->cpu_capacity_orig;
>  }
>
> +static inline unsigned long cpu_in_capacity_inversion(int cpu)
> +{
> +       return cpu_rq(cpu)->cpu_capacity_inverted;
> +}
> +
>  /**
>   * enum cpu_util_type - CPU utilization type
>   * @FREQUENCY_UTIL:    Utilization used to select frequency
>
>
> --->8---

The patch is amazing for me, and the complexity is not too high. Would
you please push the patch?
I think the idea is yours, I don't want to use it as my patch v2.

Thanks!

---
xuewen

  reply	other threads:[~2022-05-09  2:31 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-07  5:19 [PATCH] sched: Take thermal pressure into account when determine rt fits capacity Xuewen Yan
2022-04-11  8:21 ` Dietmar Eggemann
2022-04-11  8:52   ` Xuewen Yan
2022-04-11 14:07     ` Dietmar Eggemann
2022-04-13 13:25       ` Lukasz Luba
2022-04-16  2:47         ` Xuewen Yan
2022-04-19  7:14           ` Vincent Guittot
2022-04-19 12:01             ` Lukasz Luba
2022-04-19 12:51               ` Vincent Guittot
2022-04-19 14:13                 ` Lukasz Luba
2022-04-21  8:29                   ` Vincent Guittot
2022-04-21 10:57                     ` Lukasz Luba
2022-04-26  7:39                       ` Vincent Guittot
2022-04-29  9:27                         ` Lukasz Luba
2022-04-20 13:51 ` Qais Yousef
2022-04-21  8:07   ` Xuewen Yan
2022-04-21 16:15     ` Qais Yousef
2022-04-25  1:31       ` Xuewen Yan
2022-04-25 16:12         ` Qais Yousef
2022-04-26  2:07           ` Xuewen Yan
2022-04-26  8:09             ` Vincent Guittot
2022-04-26  9:30               ` Qais Yousef
2022-04-26 10:06                 ` Vincent Guittot
2022-04-26 13:06                   ` Qais Yousef
2022-04-26  9:21             ` Qais Yousef
2022-04-27  1:38               ` Xuewen Yan
2022-04-27 10:58                 ` Qais Yousef
2022-05-01  3:20                   ` Xuewen Yan
2022-05-03 14:43                     ` Qais Yousef
2022-05-09  2:29                       ` Xuewen Yan [this message]
2022-05-10 14:56                         ` Qais Yousef
2022-05-10 17:44                           ` Lukasz Luba
2022-05-10 18:44                             ` Qais Yousef
2022-05-10 22:03                               ` Lukasz Luba
2022-05-14 15:01                                 ` Xuewen Yan
2022-05-14 23:55                                   ` Qais Yousef
2022-05-15  0:53                                     ` [PATCH] sched/rt: Support multi-criterion fitness search for kernel test robot
2022-05-15  1:43                                     ` kernel test robot
2022-05-19 14:16                                     ` [sched] 0eee64011b: canonical_address#:#[##] kernel test robot
2022-05-19 14:16                                       ` kernel test robot
2022-06-15 10:13                                     ` [PATCH] sched: Take thermal pressure into account when determine rt fits capacity Qais Yousef
2022-06-15 11:17                                       ` Xuewen Yan
2022-06-15 13:54                                         ` Qais Yousef

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAB8ipk--Y8HxetcmUhBmtWq6Mmd726QmDbcbibGLERJw_PUqkQ@mail.gmail.com \
    --to=xuewen.yan94@gmail.com \
    --cc=di.shen@unisoc.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lukasz.luba@arm.com \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=qais.yousef@arm.com \
    --cc=rafael@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=xuewen.yan@unisoc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.