Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1571014705-19646-1-git-send-email-thara.gopinath@linaro.org> <1571014705-19646-3-git-send-email-thara.gopinath@linaro.org>
In-Reply-To: <1571014705-19646-3-git-send-email-thara.gopinath@linaro.org>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Mon, 14 Oct 2019 17:50:06 +0200
Message-ID: <CAKfTPtD13=7VNvZBt9nMwMTg=_2xfJsEAApfFKagwKikh9g6-Q@mail.gmail.com>
Subject: Re: [Patch v3 2/7] sched: Add infrastructure to store and update
 instantaneous thermal pressure
To:     Thara Gopinath <thara.gopinath@linaro.org>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ionela Voinescu <ionela.voinescu@arm.com>,
        Zhang Rui <rui.zhang@intel.com>,
        Eduardo Valentin <edubezval@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Amit Kachhap <amit.kachhap@gmail.com>,
        Javi Merino <javi.merino@kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Thara,

On Mon, 14 Oct 2019 at 02:58, Thara Gopinath <thara.gopinath@linaro.org> wrote:
>
> Add thermal.c and thermal.h files that provides interface
> APIs to initialize, update/average, track, accumulate and decay
> thermal pressure per cpu basis. A per cpu structure max_capacity_info is
> introduced to keep track of instantaneous per cpu thermal pressure.
> Thermal pressure is the delta between max_capacity and cap_capacity.
> API update_periodic_maxcap is called for periodic accumulate and decay
> of the thermal pressure. It is to to be called from a periodic tick
> function. This API calculates the delta between max_capacity and
> cap_capacity and passes on the delta to update_thermal_avg to do the
> necessary accumulate, decay and average. API update_maxcap_capacity is for
> the system to update the thermal pressure by updating cap_capacity.
> Considering, update_periodic_maxcap reads cap_capacity and
> update_maxcap_capacity writes into cap_capacity, one can argue for
> some sort of locking mechanism to avoid a stale value.
> But considering update_periodic_maxcap can be called from a system
> critical path like scheduler tick function, a locking mechanism is not
> ideal. This means that it is possible the value used to
> calculate average thermal pressure for a cpu can be stale for upto 1
> tick period.
>
> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org>
> ---
>  include/linux/sched.h  | 14 +++++++++++
>  kernel/sched/Makefile  |  2 +-
>  kernel/sched/thermal.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  kernel/sched/thermal.h | 13 ++++++++++
>  4 files changed, 94 insertions(+), 1 deletion(-)
>  create mode 100644 kernel/sched/thermal.c
>  create mode 100644 kernel/sched/thermal.h
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 2c2e56b..875ce2b 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1983,6 +1983,20 @@ static inline void rseq_syscall(struct pt_regs *regs)
>
>  #endif
>
> +#ifdef CONFIG_SMP
> +void update_maxcap_capacity(int cpu, u64 capacity);
> +
> +void populate_max_capacity_info(void);
> +#else
> +static inline void update_maxcap_capacity(int cpu, u64 capacity)
> +{
> +}
> +
> +static inline void populate_max_capacity_info(void)
> +{
> +}
> +#endif
> +
>  const struct sched_avg *sched_trace_cfs_rq_avg(struct cfs_rq *cfs_rq);
>  char *sched_trace_cfs_rq_path(struct cfs_rq *cfs_rq, char *str, int len);
>  int sched_trace_cfs_rq_cpu(struct cfs_rq *cfs_rq);
> diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
> index 21fb5a5..4d3b820 100644
> --- a/kernel/sched/Makefile
> +++ b/kernel/sched/Makefile
> @@ -20,7 +20,7 @@ obj-y += core.o loadavg.o clock.o cputime.o
>  obj-y += idle.o fair.o rt.o deadline.o
>  obj-y += wait.o wait_bit.o swait.o completion.o
>
> -obj-$(CONFIG_SMP) += cpupri.o cpudeadline.o topology.o stop_task.o pelt.o
> +obj-$(CONFIG_SMP) += cpupri.o cpudeadline.o topology.o stop_task.o pelt.o thermal.o
>  obj-$(CONFIG_SCHED_AUTOGROUP) += autogroup.o
>  obj-$(CONFIG_SCHEDSTATS) += stats.o
>  obj-$(CONFIG_SCHED_DEBUG) += debug.o
> diff --git a/kernel/sched/thermal.c b/kernel/sched/thermal.c
> new file mode 100644
> index 0000000..5f0b2d4
> --- /dev/null
> +++ b/kernel/sched/thermal.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sceduler Thermal Interactions
> + *
> + *  Copyright (C) 2018 Linaro, Inc., Thara Gopinath <thara.gopinath@linaro.org>
> + */
> +
> +#include <linux/sched.h>
> +#include "sched.h"
> +#include "pelt.h"
> +#include "thermal.h"
> +
> +struct max_capacity_info {
> +       unsigned long max_capacity;
> +       unsigned long cap_capacity;
> +};
> +
> +static DEFINE_PER_CPU(struct max_capacity_info, max_cap);
> +
> +void update_maxcap_capacity(int cpu, u64 capacity)
> +{
> +       struct max_capacity_info *__max_cap;
> +       unsigned long __capacity;
> +
> +       __max_cap = (&per_cpu(max_cap, cpu));
> +       if (!__max_cap) {
> +               pr_err("no max_capacity_info structure for cpu %d\n", cpu);
> +               return;
> +       }
> +
> +       /* Normalize the capacity */
> +       __capacity = (capacity * arch_scale_cpu_capacity(cpu)) >>
> +                                                       SCHED_CAPACITY_SHIFT;
> +       pr_debug("updating cpu%d capped capacity from %lu to %lu\n", cpu, __max_cap->cap_capacity, __capacity);
> +
> +       __max_cap->cap_capacity = __capacity;
> +}
> +
> +void populate_max_capacity_info(void)
> +{
> +       struct max_capacity_info *__max_cap;
> +       u64 capacity;
> +       int cpu;
> +
> +       for_each_possible_cpu(cpu) {
> +               __max_cap = (&per_cpu(max_cap, cpu));
> +               if (!__max_cap)
> +                       continue;
> +               capacity = arch_scale_cpu_capacity(cpu);
> +               __max_cap->max_capacity = capacity;
> +               __max_cap->cap_capacity = capacity;
> +               pr_debug("cpu %d max capacity set to %ld\n", cpu, __max_cap->max_capacity);
> +       }
> +}

everything above seems to be there for the cpu cooling device and
should be included in it instead. The scheduler only need the capacity
capping
The cpu cooling device should just set the delta capacity in the
per-cpu variable (see my comment below)

> +
> +void update_periodic_maxcap(struct rq *rq)
> +{
> +       struct max_capacity_info *__max_cap = (&per_cpu(max_cap, cpu_of(rq)));
> +       unsigned long delta;
> +
> +       if (!__max_cap)
> +               return;
> +
> +       delta = __max_cap->max_capacity - __max_cap->cap_capacity;

Why don't you just save the delta in the per_cpu variable instead of
the struct max_capacity_info ? You have to compute the delta every
tick whereas we can expect it to not change that much compared to the
number of tick

> +       update_thermal_avg(rq_clock_task(rq), rq, delta);
> +}
> diff --git a/kernel/sched/thermal.h b/kernel/sched/thermal.h
> new file mode 100644
> index 0000000..5657cb4
> --- /dev/null
> +++ b/kernel/sched/thermal.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Scheduler thermal interaction internal methods.
> + */
> +
> +#ifdef CONFIG_SMP
> +void update_periodic_maxcap(struct rq *rq);
> +
> +#else
> +static inline void update_periodic_maxcap(struct rq *rq)
> +{
> +}
> +#endif
> --
> 2.1.4
>