Received-SPF: pass (google.com: domain of linux-kernel+bounces-51606-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20231208002342.367117-1-qyousef@layalina.io> <20231208002342.367117-9-qyousef@layalina.io>
 <Zbk0DhibX0oDLk1s@vingu-book> <20240201222428.xd2sylnz66wrczal@airbuntu>
In-Reply-To: <20240201222428.xd2sylnz66wrczal@airbuntu>
From: Vincent Guittot <vincent.guittot@linaro.org>
Date: Sun, 4 Feb 2024 12:32:36 +0100
Message-ID: <CAKfTPtBeiqgndxzVu=hu3awbejpsKehww5GvNcB3p57A9mmg=g@mail.gmail.com>
Subject: Re: [PATCH v2 8/8] sched/pelt: Introduce PELT multiplier
To: Qais Yousef <qyousef@layalina.io>
Cc: Ingo Molnar <mingo@kernel.org>, Peter Zijlstra <peterz@infradead.org>, 
	"Rafael J. Wysocki" <rafael@kernel.org>, Viresh Kumar <viresh.kumar@linaro.org>, 
	Dietmar Eggemann <dietmar.eggemann@arm.com>, linux-kernel@vger.kernel.org, 
	linux-pm@vger.kernel.org, Lukasz Luba <lukasz.luba@arm.com>, Wei Wang <wvw@google.com>, 
	Rick Yiu <rickyiu@google.com>, Chung-Kai Mei <chungkai@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, 1 Feb 2024 at 23:24, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 01/30/24 18:38, Vincent Guittot wrote:
> > Le vendredi 08 d=C3=A9c. 2023 =C3=A0 00:23:42 (+0000), Qais Yousef a =
=C3=A9crit :
> > > From: Vincent Donnefort <vincent.donnefort@arm.com>
> > >
> > > The new sched_pelt_multiplier boot param allows a user to set a clock
> > > multiplier to x2 or x4 (x1 being the default). This clock multiplier
> > > artificially speeds up PELT ramp up/down similarly to use a faster
> > > half-life than the default 32ms.
> > >
> > >   - x1: 32ms half-life
> > >   - x2: 16ms half-life
> > >   - x4: 8ms  half-life
> > >
> > > Internally, a new clock is created: rq->clock_task_mult. It sits in t=
he
> > > clock hierarchy between rq->clock_task and rq->clock_pelt.
> > >
> > > The param is set as read only and can only be changed at boot time vi=
a
> > >
> > >     kernel.sched_pelt_multiplier=3D[1, 2, 4]
> > >
> > > PELT has a big impact on the overall system response and reactiveness=
 to
> > > change. Smaller PELT HF means it'll require less time to reach the
> > > maximum performance point of the system when the system become fully
> > > busy; and equally shorter time to go back to lowest performance point
> > > when the system goes back to idle.
> > >
> > > This faster reaction impacts both dvfs response and migration time
> > > between clusters in HMP system.
> > >
> > > Smaller PELT values are expected to give better performance at the co=
st
> > > of more power. Under powered systems can particularly benefit from
> > > smaller values. Powerful systems can still benefit from smaller value=
s
> > > if they want to be tuned towards perf more and power is not the major
> > > concern for them.
> > >
> > > This combined with respone_time_ms from schedutil should give the use=
r
> > > and sysadmin a deterministic way to control the triangular power, per=
f
> > > and thermals for their system. The default response_time_ms will half
> > > as PELT HF halves.
> > >
> > > Update approximate_{util_avg, runtime}() to take into account the PEL=
T
> > > HALFLIFE multiplier.
> > >
> > > Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> > > Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > > [Converted from sysctl to boot param and updated commit message]
> > > Signed-off-by: Qais Yousef (Google) <qyousef@layalina.io>
> > > ---
> > >  kernel/sched/core.c  |  2 +-
> > >  kernel/sched/pelt.c  | 52 ++++++++++++++++++++++++++++++++++++++++++=
--
> > >  kernel/sched/pelt.h  | 42 +++++++++++++++++++++++++++++++----
> > >  kernel/sched/sched.h |  1 +
> > >  4 files changed, 90 insertions(+), 7 deletions(-)
> > >
> >
> > ...
> >
> > > +__read_mostly unsigned int sched_pelt_lshift;
> > > +static unsigned int sched_pelt_multiplier =3D 1;
> > > +
> > > +static int set_sched_pelt_multiplier(const char *val, const struct k=
ernel_param *kp)
> > > +{
> > > +   int ret;
> > > +
> > > +   ret =3D param_set_int(val, kp);
> > > +   if (ret)
> > > +           goto error;
> > > +
> > > +   switch (sched_pelt_multiplier)  {
> > > +   case 1:
> > > +           fallthrough;
> > > +   case 2:
> > > +           fallthrough;
> > > +   case 4:
> > > +           WRITE_ONCE(sched_pelt_lshift,
> > > +                      sched_pelt_multiplier >> 1);
> > > +           break;
> > > +   default:
> > > +           ret =3D -EINVAL;
> > > +           goto error;
> > > +   }
> > > +
> > > +   return 0;
> > > +
> > > +error:
> > > +   sched_pelt_multiplier =3D 1;
> > > +   return ret;
> > > +}
> > > +
> > > +static const struct kernel_param_ops sched_pelt_multiplier_ops =3D {
> > > +   .set =3D set_sched_pelt_multiplier,
> > > +   .get =3D param_get_int,
> > > +};
> > > +
> > > +#ifdef MODULE_PARAM_PREFIX
> > > +#undef MODULE_PARAM_PREFIX
> > > +#endif
> > > +/* XXX: should we use sched as prefix? */
> > > +#define MODULE_PARAM_PREFIX "kernel."
> > > +module_param_cb(sched_pelt_multiplier, &sched_pelt_multiplier_ops, &=
sched_pelt_multiplier, 0444);
> > > +MODULE_PARM_DESC(sched_pelt_multiplier, "PELT HALFLIFE helps control=
 the responsiveness of the system.");
> > > +MODULE_PARM_DESC(sched_pelt_multiplier, "Accepted value: 1 32ms PELT=
 HALIFE - roughly 200ms to go from 0 to max performance point (default).");
> > > +MODULE_PARM_DESC(sched_pelt_multiplier, "                2 16ms PELT=
 HALIFE - roughly 100ms to go from 0 to max performance point.");
> > > +MODULE_PARM_DESC(sched_pelt_multiplier, "                4  8ms PELT=
 HALIFE - roughly  50ms to go from 0 to max performance point.");
> > > +
> > >  /*
> > >   * Approximate the new util_avg value assuming an entity has continu=
ed to run
> > >   * for @delta us.
> >
> > ...
> >
> > > +
> > >  static inline void
> > > -update_rq_clock_pelt(struct rq *rq, s64 delta) { }
> > > +update_rq_clock_task_mult(struct rq *rq, s64 delta) { }
> > >
> > >  static inline void
> > >  update_idle_rq_clock_pelt(struct rq *rq) { }
> > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > index bbece0eb053a..a7c89c623250 100644
> > > --- a/kernel/sched/sched.h
> > > +++ b/kernel/sched/sched.h
> > > @@ -1029,6 +1029,7 @@ struct rq {
> > >     u64                     clock;
> > >     /* Ensure that all clocks are in the same cache line */
> > >     u64                     clock_task ____cacheline_aligned;
> > > +   u64                     clock_task_mult;
> >
> > I'm not sure that we want yet another clock and this doesn't apply for =
irq_avg.
> >
> > What about the below is simpler and I think cover all cases ?
>
> Looks better, yes. I'll change to this and if no issues come up I'll add =
your
> signed-off-by if that's okay with you for the next version.

Yes, that's okay

>
>
> Thanks!
>
> --
> Qais Yousef
>
> >
> > diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c
> > index f951c44f1d52..5cdd147b7abe 100644
> > --- a/kernel/sched/pelt.c
> > +++ b/kernel/sched/pelt.c
> > @@ -180,6 +180,7 @@ static __always_inline int
> >  ___update_load_sum(u64 now, struct sched_avg *sa,
> >                 unsigned long load, unsigned long runnable, int running=
)
> >  {
> > +     int time_shift;
> >       u64 delta;
> >
> >       delta =3D now - sa->last_update_time;
> > @@ -195,12 +196,17 @@ ___update_load_sum(u64 now, struct sched_avg *sa,
> >       /*
> >        * Use 1024ns as the unit of measurement since it's a reasonable
> >        * approximation of 1us and fast to compute.
> > +      * On top of this, we can change the half-time period from the de=
fault
> > +      * 32ms to a shorter value. This is equivalent to left shifting t=
he
> > +      * time.
> > +      * Merge both right and left shifts in one single right shift
> >        */
> > -     delta >>=3D 10;
> > +     time_shift =3D 10 - sched_pelt_lshift;
> > +     delta >>=3D time_shift;
> >       if (!delta)
> >               return 0;
> >
> > -     sa->last_update_time +=3D delta << 10;
> > +     sa->last_update_time +=3D delta << time_shift;
> >
> >       /*
> >        * running is a subset of runnable (weight) so running can't be s=
et if
> >
> >
> >
> > >     u64                     clock_pelt;
> > >     unsigned long           lost_idle_time;
> > >     u64                     clock_pelt_idle;
> > > --
> > > 2.34.1
> > >