Received-SPF: pass (google.com: domain of linux-kernel+bounces-31739-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <a561029e-993d-726d-18ce-0bc014e6533c@quicinc.com>
Date: Sat, 20 Jan 2024 13:22:03 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Ashay Jaiswal <quic_ashayj@quicinc.com>
Subject: Re: [PATCH v2 8/8] sched/pelt: Introduce PELT multiplier
To: Qais Yousef <qyousef@layalina.io>, Ingo Molnar <mingo@kernel.org>,
        "Peter
 Zijlstra" <peterz@infradead.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Vincent Guittot
	<vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>
CC: <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>,
        <quic_anshar@quicinc.com>, <quic_atulpant@quicinc.com>,
        <quic_shashim@quicinc.com>, <quic_rgottimu@quicinc.com>,
        <quic_adharmap@quicinc.com>, <quic_kshivnan@quicinc.com>,
        <quic_pkondeti@quicinc.com>
References: <20231208002342.367117-1-qyousef@layalina.io>
 <20231208002342.367117-9-qyousef@layalina.io>
Content-Language: en-US
In-Reply-To: <20231208002342.367117-9-qyousef@layalina.io>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

Hello Qais Yousef,

We ran few benchmarks with PELT multiplier patch on a Snapdragon 8Gen2
based internal Android device and we are observing significant
improvements with PELT8 configuration compared to PELT32.

Following are some of the benchmark results with PELT32 and PELT8
configuration:

+-----------------+---------------+----------------+----------------+
| Test case                       |     PELT32     |     PELT8      |
+-----------------+---------------+----------------+----------------+
|                 |    Overall    |     711543     |     971275     |
|                 +---------------+----------------+----------------+
|                 |    CPU        |     193704     |     224378     |
|                 +---------------+----------------+----------------+
|ANTUTU V9.3.9    |    GPU        |     284650     |     424774     |
|                 +---------------+----------------+----------------+
|                 |    MEM        |     125207     |     160548     |
|                 +---------------+----------------+----------------+
|                 |    UX         |     107982     |     161575     |
+-----------------+---------------+----------------+----------------+
|                 |   Single core |     1170       |     1268       |
|GeekBench V5.4.4 +---------------+----------------+----------------+
|                 |   Multi core  |     2530       |     3797       |
+-----------------+---------------+----------------+----------------+
|                 |    Twitter    |     >50 Janks  |     0          |
|     SCROLL      +---------------+----------------+----------------+
|                 |    Contacts   |     >30 Janks  |     0          |
+-----------------+---------------+----------------+----------------+

Please let us know if you need any support with running any further
workloads for PELT32/PELT8 experiments, we can help with running the
experiments.

Thank you,
Ashay Jaiswal

On 12/8/2023 5:53 AM, Qais Yousef wrote:
> 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 the
> 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 via
> 
> 	kernel.sched_pelt_multiplier=[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 cost
> of more power. Under powered systems can particularly benefit from
> smaller values. Powerful systems can still benefit from smaller values
> 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 user
> and sysadmin a deterministic way to control the triangular power, perf
> 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 PELT
> 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(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b4a1c8ea9e12..9c8626b4ddff 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -745,7 +745,7 @@ static void update_rq_clock_task(struct rq *rq, s64 delta)
>  	if ((irq_delta + steal) && sched_feat(NONTASK_CAPACITY))
>  		update_irq_load_avg(rq, irq_delta + steal);
>  #endif
> -	update_rq_clock_pelt(rq, delta);
> +	update_rq_clock_task_mult(rq, delta);
>  }
>  
>  void update_rq_clock(struct rq *rq)
> diff --git a/kernel/sched/pelt.c b/kernel/sched/pelt.c
> index 00a1b9c1bf16..0a10e56f76c7 100644
> --- a/kernel/sched/pelt.c
> +++ b/kernel/sched/pelt.c
> @@ -468,6 +468,54 @@ int update_irq_load_avg(struct rq *rq, u64 running)
>  }
>  #endif /* CONFIG_HAVE_SCHED_AVG_IRQ */
>  
> +__read_mostly unsigned int sched_pelt_lshift;
> +static unsigned int sched_pelt_multiplier = 1;
> +
> +static int set_sched_pelt_multiplier(const char *val, const struct kernel_param *kp)
> +{
> +	int ret;
> +
> +	ret = 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 = -EINVAL;
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	sched_pelt_multiplier = 1;
> +	return ret;
> +}
> +
> +static const struct kernel_param_ops sched_pelt_multiplier_ops = {
> +	.set = set_sched_pelt_multiplier,
> +	.get = 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 continued to run
>   * for @delta us.
> @@ -482,7 +530,7 @@ unsigned long approximate_util_avg(unsigned long util, u64 delta)
>  	if (unlikely(!delta))
>  		return util;
>  
> -	accumulate_sum(delta, &sa, 1, 0, 1);
> +	accumulate_sum(delta << sched_pelt_lshift, &sa, 1, 0, 1);
>  	___update_load_avg(&sa, 0);
>  
>  	return sa.util_avg;
> @@ -494,7 +542,7 @@ unsigned long approximate_util_avg(unsigned long util, u64 delta)
>  u64 approximate_runtime(unsigned long util)
>  {
>  	struct sched_avg sa = {};
> -	u64 delta = 1024; // period = 1024 = ~1ms
> +	u64 delta = 1024 << sched_pelt_lshift; // period = 1024 = ~1ms
>  	u64 runtime = 0;
>  
>  	if (unlikely(!util))
> diff --git a/kernel/sched/pelt.h b/kernel/sched/pelt.h
> index 3a0e0dc28721..9b35b5072bae 100644
> --- a/kernel/sched/pelt.h
> +++ b/kernel/sched/pelt.h
> @@ -61,6 +61,14 @@ static inline void cfs_se_util_change(struct sched_avg *avg)
>  	WRITE_ONCE(avg->util_est.enqueued, enqueued);
>  }
>  
> +static inline u64 rq_clock_task_mult(struct rq *rq)
> +{
> +	lockdep_assert_rq_held(rq);
> +	assert_clock_updated(rq);
> +
> +	return rq->clock_task_mult;
> +}
> +
>  static inline u64 rq_clock_pelt(struct rq *rq)
>  {
>  	lockdep_assert_rq_held(rq);
> @@ -72,7 +80,7 @@ static inline u64 rq_clock_pelt(struct rq *rq)
>  /* The rq is idle, we can sync to clock_task */
>  static inline void _update_idle_rq_clock_pelt(struct rq *rq)
>  {
> -	rq->clock_pelt  = rq_clock_task(rq);
> +	rq->clock_pelt = rq_clock_task_mult(rq);
>  
>  	u64_u32_store(rq->clock_idle, rq_clock(rq));
>  	/* Paired with smp_rmb in migrate_se_pelt_lag() */
> @@ -121,6 +129,27 @@ static inline void update_rq_clock_pelt(struct rq *rq, s64 delta)
>  	rq->clock_pelt += delta;
>  }
>  
> +extern unsigned int sched_pelt_lshift;
> +
> +/*
> + * absolute time   |1      |2      |3      |4      |5      |6      |
> + * @ mult = 1      --------****************--------****************-
> + * @ mult = 2      --------********----------------********---------
> + * @ mult = 4      --------****--------------------****-------------
> + * clock task mult
> + * @ mult = 2      |   |   |2  |3  |   |   |   |   |5  |6  |   |   |
> + * @ mult = 4      | | | | |2|3| | | | | | | | | | |5|6| | | | | | |
> + *
> + */
> +static inline void update_rq_clock_task_mult(struct rq *rq, s64 delta)
> +{
> +	delta <<= READ_ONCE(sched_pelt_lshift);
> +
> +	rq->clock_task_mult += delta;
> +
> +	update_rq_clock_pelt(rq, delta);
> +}
> +
>  /*
>   * When rq becomes idle, we have to check if it has lost idle time
>   * because it was fully busy. A rq is fully used when the /Sum util_sum
> @@ -147,7 +176,7 @@ static inline void update_idle_rq_clock_pelt(struct rq *rq)
>  	 * rq's clock_task.
>  	 */
>  	if (util_sum >= divider)
> -		rq->lost_idle_time += rq_clock_task(rq) - rq->clock_pelt;
> +		rq->lost_idle_time += rq_clock_task_mult(rq) - rq->clock_pelt;
>  
>  	_update_idle_rq_clock_pelt(rq);
>  }
> @@ -218,13 +247,18 @@ update_irq_load_avg(struct rq *rq, u64 running)
>  	return 0;
>  }
>  
> -static inline u64 rq_clock_pelt(struct rq *rq)
> +static inline u64 rq_clock_task_mult(struct rq *rq)
>  {
>  	return rq_clock_task(rq);
>  }
>  
> +static inline u64 rq_clock_pelt(struct rq *rq)
> +{
> +	return rq_clock_task_mult(rq);
> +}
> +
>  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;
>  	u64			clock_pelt;
>  	unsigned long		lost_idle_time;
>  	u64			clock_pelt_idle;