Hi
I found some bugs when integrating the 3.18 EAS backport [1] into
the LSK 3.18 based kernel I look after for ARM's Juno and Versatile
Express boards. These patches are my fixes for those bugs, I don't know
whether they are useful or relevent to other versions of EAS. If they
are OK, I guess I should at least add them to [1] ?
----------------------------------------------------------------
Jon Medhurst (3):
arm: Fix build error "conflicting types for 'scale_cpu_capacity'"
arm: Fix #if/#ifdef mixup in topology.c
sched/tune: Avoid null pointer dereference in schedtune_add_cluster_nrg
arch/arm/include/asm/topology.h | 1 +
arch/arm/kernel/topology.c | 2 +-
kernel/sched/tune.c | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)
[1] http://git.linaro.org/arm/eas/kernel.git/shortlog/refs/heads/linux-3.18-eas…
On 2016年05月26日 17:44, eas-dev-request(a)lists.linaro.org wrote:
> Send eas-dev mailing list submissions to
> eas-dev(a)lists.linaro.org
>
> To subscribe or unsubscribe via the World Wide Web, visit
> https://lists.linaro.org/mailman/listinfo/eas-dev
> or, via email, send a message with subject or body 'help' to
> eas-dev-request(a)lists.linaro.org
>
> You can reach the person managing the list at
> eas-dev-owner(a)lists.linaro.org
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of eas-dev digest..."
>
>
> Today's Topics:
>
> 1. Re: [PATCH] sched/tune: fix payoff calculation for
> performance constraint region (Patrick Bellasi)
> 2. Re: [PATCH] sched/tune: fix payoff calculation for
> performance constraint region (Leo Yan)
> 3. Re: [PATCH] sched/tune: fix payoff calculation for
> performance constraint region (Patrick Bellasi)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Wed, 25 May 2016 18:53:28 +0100
> From: Patrick Bellasi <patrick.bellasi(a)arm.com>
> To: Leo Yan <leo.yan(a)linaro.org>
> Cc: eas-dev(a)lists.linaro.org
> Subject: Re: [Eas-dev] [PATCH] sched/tune: fix payoff calculation for
> performance constraint region
> Message-ID: <20160525175328.GA15730@e105326-lin>
> Content-Type: text/plain; charset="utf-8"
>
> Hi Leo,
> thanks a lot for reviewing SchedTune and pointing out this issue.
>
> Actually going through the code I've noticed another big issue
> related to the definition of the acceptable regions, following
> commented inline. Basically, with the current implementation
> we was getting a correct "by chance" C region, while the acceptance
> for the B region was completely wrong.
>
> In attachment a new version of the patch, please have a look and let
> me know if you have any doubt and/or suggestions.
> If the patch is ok for you, lemme also know if it's ok for you to add
> your sign-off.
>
> Cheers Patrick
>
> On 23-May 21:47, Leo Yan wrote:
>> On Fri, May 20, 2016 at 12:24:49AM +0800, Leo Yan wrote:
>>> When calculate payoff criteria for performance constraint region,
>>> the inequality formula is wrong:
>>>
>>> cap_delta / nrg_delta > cap_gain / nrg_gain
>>>
>>> Here nrg_delta < 0, so when multiply it both side then should then
>>> multiplying nrg_delta inverts the inequality:
>>>
>>> nrg_delta * cap_gain > cap_delta * nrg_gain
>>>
>>> So finally we can get unified formula for both performance constraint
>>> region and performance boost region. So this patch unified these the
>>> calculation after fixed inequality formula.
>>>
>>> Signed-off-by: Leo Yan <leo.yan(a)linaro.org>
>>> ---
>>> kernel/sched/tune.c | 54 ++++++++++++++++++++++++++++-------------------------
>>> 1 file changed, 29 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/kernel/sched/tune.c b/kernel/sched/tune.c
>>> index 9d8eeb4..1da85a8 100644
>>> --- a/kernel/sched/tune.c
>>> +++ b/kernel/sched/tune.c
>>> @@ -58,36 +58,40 @@ __schedtune_accept_deltas(int nrg_delta, int cap_delta,
>>> int perf_boost_idx, int perf_constrain_idx)
>>> {
>>> int payoff = -INT_MAX;
>>> + int idx = -1;
>>>
>>> /* Performance Boost (B) region */
>>> - if (nrg_delta > 0 && cap_delta > 0) {
>>> - /*
>>> - * Evaluate "Performance Boost" vs "Energy Increase"
>>> - * payoff criteria:
>>> - * cap_delta / nrg_delta < cap_gain / nrg_gain
>>> - * which is:
>>> - * nrg_delta * cap_gain > cap_delta * nrg_gain
>>> - */
>>> - payoff = nrg_delta * threshold_gains[perf_boost_idx].cap_gain;
>>> - payoff -= cap_delta * threshold_gains[perf_boost_idx].nrg_gain;
>>> - return payoff;
>>> - }
>>> -
>>> + if (nrg_delta > 0 && cap_delta > 0)
>
> Looking better at the called, I think it's worth con accept also
> points in the P axis, thus:
>
> if (nrg_delta >= 0 && cap_delta > 0)
>
>
>>> + idx = perf_boost_idx;
>>> /* Performance Constraint (C) region */
>>> - if (nrg_delta < 0 && cap_delta < 0) {
>>> - /*
>>> - * Evaluate "Performance Boost" vs "Energy Increase"
>>> - * payoff criteria:
>>> - * cap_delta / nrg_delta > cap_gain / nrg_gain
>>> - * which is:
>>> - * cap_delta * nrg_gain > nrg_delta * cap_gain
>>> - */
>>> - payoff = cap_delta * threshold_gains[perf_constrain_idx].nrg_gain;
>>> - payoff -= nrg_delta * threshold_gains[perf_constrain_idx].cap_gain;
>>> - return payoff;
>>> - }
>>> + else if (nrg_delta < 0 && cap_delta < 0)
>
> For the same considerations we should better accept points in the E
> axis, thus:
>
> else if (nrg_delta < 0 && cap_delta <= 0)
>
>>> + idx = perf_constrain_idx;
>>>
>>> /* Default: reject schedule candidate */
>>> + if (idx == -1)
>>> + return payoff;
>>> +
>>> + /*
>>> + * Evaluate "Performance Boost" vs "Energy Increase"
>>> + *
>>> + * - Performance Boost (B) region
>>> + *
>>> + * Condition: nrg_delta > 0 && cap_delta > 0
>>> + * Payoff criteria:
>>> + * cap_delta / nrg_delta < cap_gain / nrg_gain =
>
Looking better to put the condition "nrg_delta == 0" or "cap_delta == 0"
in function "accept_deltas", to avoid fetch rcu lock and more functions
called, thus:
/* Optimal (O) region */
if ((nrg_delta < 0 && cap_delta >= 0) || (nrg_delta <=0 && cap_delta >
0)) {
trace_sched_tune_filter(nrg_delta, cap_delta, 0, 0, 1, 0);
return INT_MAX;
}
> Here the inequality has a wrong direction.
> The schedule candidate acceptable in the B region are those for which:
>
> cap_gain / nrg_gain < cap_delta / nrg_delta
>
> which represents points in the "upper cut".
>
> Thus:
>>> + * nrg_delta * cap_gain > cap_delta * nrg_gain
>
> has to be:
> cap_gain * nrg_delta < cap_delta * nrg_gain
>
> Which results into a "positive accept" payoff defined as:
>
> payoff = (cap_delta * nrg_gain) - (cap_gain * nrg_delta)
>
>>> + * (note: nrg_delta > 0, nrg_gain > 0)
>>> + *
>>> + * - Performance Constraint (C) region
>>> + *
>>> + * Condition: nrg_delta < 0 && cap_delta < 0
>>> + * payoff criteria:
>>> + * cap_delta / nrg_delta > cap_gain / nrg_gain =
>>> + * nrg_delta * cap_gain > cap_delta * nrg_gain
>
>
> In the C region we have both a wrong definition and the sign error you
> reported, which turned out to provide a "by change" correct
> implementation, which is:
>
> cap_gain / nrg_gain > cap_delta / nrg_delta =
> cap_gain * nrg_delta < cap_delta * nrg_gain
>
> Which results into a "positive accept" payoff defined as:
>
> payoff = (cap_delta * nrg_gain) - (cap_gain * nrg_delta)
>
> The same as for the B region...
>
>>> + * (note: nrg_delta < 0, nrg_gain > 0)
>>> + */
>>> + payoff = nrg_delta * threshold_gains[perf_boost_idx].cap_gain;
>>> + payoff -= cap_delta * threshold_gains[perf_boost_idx].nrg_gain;
>>
>> Sorry, here should be:
>> + payoff = nrg_delta * threshold_gains[idx].cap_gain;
>> + payoff -= cap_delta * threshold_gains[idx].nrg_gain;
>
> ... which means that this two operations have to be inverted:
>
> payoff = cap_delta * threshold_gains[gain_idx].nrg_gain;
> payoff -= nrg_delta * threshold_gains[gain_idx].cap_gain;
>
>>
>>> return payoff;
>>> }
>>>
>>> --
>>> 1.9.1
>>>
>>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Dear Dev,
Your parcel has arrived at May 28. Courier was unable to deliver the parcel to you.
Shipment Label is attached to email.
Yours trully,
Warren Carney,
Sr. Support Manager.
Dear Dev,
We could not deliver your item.
You can review complete details of your order in the find attached.
Yours faithfully,
Dwight Mccall,
Sr. Delivery Manager.
Hi Deitmar,
[ + eas-dev ]
On Sun, May 22, 2016 at 06:06:47PM +0100, Dietmar Eggemann wrote:
> On 05/21/2016 06:25 AM, Leo Yan wrote:
> >On Fri, May 20, 2016 at 08:08:16PM +0100, Dietmar Eggemann wrote:
> >>Hi Leo,
> >>
> >>try the attached testcase in LISA which runs the wl's in '/tg_1/tg_11'
> >>You can create different tg level-hierarchies in rfc_tg.config if you wish.
> >>
> >>08:05:17 DEBUG : sudo -- sh -c '/root/devlib-target/bin/shutils cgroups_run_into /tg_1/tg_11 '\''/root/devlib-target/bin/rt-app /root/devlib-target/run_dir/06_pct_00.json'\'''
> >
> >Very appreciate for the case.
>
> I thought about this again and maybe you want to test task migration of
> a task running in a task group? This would make much more sense than
> only running task in a task group in case you want to test the pelt signals.
After enable EAS, I can see the task running in task group is migrated
between different CPUs when task is waken up.
> I added some functionality to rt-app which lets you restrict the cpu
> affinity of a task per phase of its run so you can create a task inside
> a task group which alternates between two cpus while running. This
> migration is done by the running task (so it's
> sched_setaffinity()->__set_cpus_allowed_ptr()->stop_one_cpu(...,
> migration_cpu_stop, ...)->__migrate_task()->move_queued_task()
>
> So if you interested in this just ask me on eas-dev so I can share the
> rt-app functionality and a how-to build rt-app on the list for a broader
> audience.
Yes, this is another path we should test for task migration. So could
you share this on mailing list? We also can consider to integrate this
into rt-app's repo.
Thanks,
Leo Yan
In current code the CPU's idle state cpufreq_pstates::idle is initialized
to '-1'; and until parse first "cpu_idle" event for the CPU then set CPU's
idle state to '0' or '1' corresponding to active or idle. This will cause
error for P-state's statistics: from the beginning to first "cpu_idle"
event, during this period the CPU's idle state is '-1' so function
cpu_change_pstate() will always think it's first update and finally abandon
previous time.
This will introduce very big error if the CPU is always running and never
run into idle state. So this patch is to fix this issue by initialize CPU's
idle state before parse P-state and C-state's time. Initialize CPU's idle
state according to first cpu_idle log:
- If the CPU first cpu_idle state is '-1', that means from the beginning
the CPU is stayed on idle state;
- If the CPU first cpu_idle state is other value, means the CPU is active.
Signed-off-by: Leo Yan <leo.yan(a)linaro.org>
---
tracefile_idlestat.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/tracefile_idlestat.c b/tracefile_idlestat.c
index 3430693..fb0b3d8 100644
--- a/tracefile_idlestat.c
+++ b/tracefile_idlestat.c
@@ -152,6 +152,55 @@ int load_text_data_line(char *buffer, struct cpuidle_datas *datas, char *format,
return get_wakeup_irq(datas, buffer);
}
+/**
+ * init_cpu_idle_state - Init CPU's idle state according to first cpu_idle log.
+ * For a specific cpu_idle event, its state is '-1' then that means from the
+ * beginning the CPU is stayed on idle state; Otherwise means the CPU is active.
+ * So initilize per-CPU idle flag to get more accurate time.
+ *
+ * @datas: structure for P-state and C-state's statistics
+ * @f: the file handle of the idlestat trace file
+ */
+void init_cpu_idle_state(struct cpuidle_datas *datas, FILE *f)
+{
+ struct cpufreq_pstates *ps;
+ unsigned int state, freq, cpu;
+ double time;
+ char buffer[BUFSIZE];
+
+ do {
+ if (strstr(buffer, "cpu_idle")) {
+ if (sscanf(buffer, TRACE_FORMAT, &time, &state, &cpu)
+ != 3) {
+ fprintf(stderr, "warning: Unrecognized cpuidle "
+ "record. The result of analysis might "
+ "be wrong.\n");
+ return -1;
+ }
+ }
+
+ ps = &(datas->pstates[cpu]);
+
+ /* CPU's state has been initialized, skip it */
+ if (ps->idle != -1)
+ continue;
+
+ /*
+ * The CPU's first cpu_idle is '-1', means CPU is staying in
+ * idle state and exit from idle until first cpu_idle event.
+ * Otherwise, means the CPU is active at beginning.
+ */
+ if (state == -1)
+ ps->idle = 1;
+ else
+ ps->idle = 0;
+
+ } while (fgets(buffer, BUFSIZE, f));
+
+ /* After traverse file, reset offset */
+ fseek(f, 0, SEEK_SET);
+}
+
void load_text_data_lines(FILE *f, char *buffer, struct cpuidle_datas *datas)
{
double begin = 0, end = 0;
@@ -159,6 +208,8 @@ void load_text_data_lines(FILE *f, char *buffer, struct cpuidle_datas *datas)
setup_topo_states(datas);
+ init_cpu_idle_state(datas, f);
+
do {
if (load_text_data_line(buffer, datas, TRACE_FORMAT,
&begin, &end, &start) != -1) {
--
1.9.1
Hi Patrick,
[ + eas-dev ]
With non-root user in Android, I cannot add PID to SchedTune's cgroup;
At beginning I thought it's related with cgroup's file node
attribution, so tried to use "root" user to change permission with
"a+rwx", even so still cannot set cgroup's node by non-root user.
hikey:/ $ su
hikey:/ # chmod a+rwx /sys/fs/cgroup/stune/performance/cgroup.procs
hikey:/ # exit
hikey:/ $ echo 1937 > /sys/fs/cgroup/stune/performance/cgroup.procs
hikey:/ $ cat /sys/fs/cgroup/stune/performance/cgroup.procs
Do you have suggestion for what's the formal method for adding PID to
SchedTune's cgroup with non-root user?
Thanks,
Leo Yan