On Wed, Jan 30, 2013 at 09:21:41PM +0530, Viresh Kumar wrote:
On 30 January 2013 18:30, Fabio Baltieri fabio.baltieri@linaro.org wrote:
Modify ondemand timer to not resample CPU utilization if recently sampled from another SW coordinated core.
Signed-off-by: Fabio Baltieri fabio.baltieri@linaro.org
I might be wrong but i have some real concerns over this patch.
This is what i believe is your idea:
- because a cpu can sleep, create timer per cpu
- then check load again only when no other cpu in policy->cpus has checked load within sampling time interval.
Correct?
Yes.
drivers/cpufreq/cpufreq_governor.c | 3 ++ drivers/cpufreq/cpufreq_governor.h | 1 + drivers/cpufreq/cpufreq_ondemand.c | 58 +++++++++++++++++++++++++++++++------- 3 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 8393d6e..46f96a4 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -289,6 +289,9 @@ second_time: } mutex_unlock(&dbs_data->mutex);
/* Initiate timer time stamp */cpu_cdbs->time_stamp = ktime_get();We have updated time_stamp only for policy->cpu's cdbs.
Yes, see below.
for_each_cpu(j, policy->cpus) dbs_timer_init(dbs_data, j, *sampling_rate); break;diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c index 93bb56d..13ceb3c 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -216,23 +216,23 @@ static void od_check_cpu(int cpu, unsigned int load_freq) } }
-static void od_dbs_timer(struct work_struct *work) +static void od_timer_update(struct od_cpu_dbs_info_s *dbs_info, bool sample,
struct delayed_work *dw){
struct od_cpu_dbs_info_s *dbs_info =container_of(work, struct od_cpu_dbs_info_s, cdbs.work.work); unsigned int cpu = dbs_info->cdbs.cpu; int delay, sample_type = dbs_info->sample_type;mutex_lock(&dbs_info->cdbs.timer_mutex);/* Common NORMAL_SAMPLE setup */ dbs_info->sample_type = OD_NORMAL_SAMPLE; if (sample_type == OD_SUB_SAMPLE) { delay = dbs_info->freq_lo_jiffies;__cpufreq_driver_target(dbs_info->cdbs.cur_policy,dbs_info->freq_lo, CPUFREQ_RELATION_H);
if (sample)__cpufreq_driver_target(dbs_info->cdbs.cur_policy,dbs_info->freq_lo,CPUFREQ_RELATION_H); } else {
dbs_check_cpu(&od_dbs_data, cpu);
if (sample)dbs_check_cpu(&od_dbs_data, cpu); if (dbs_info->freq_lo) { /* Setup timer for SUB_SAMPLE */ dbs_info->sample_type = OD_SUB_SAMPLE;@@ -243,11 +243,49 @@ static void od_dbs_timer(struct work_struct *work) } }
schedule_delayed_work_on(smp_processor_id(), &dbs_info->cdbs.work,delay);
schedule_delayed_work_on(smp_processor_id(), dw, delay);+}
All good until now.
+static void od_timer_coordinated(struct od_cpu_dbs_info_s *dbs_info_local,
struct delayed_work *dw)+{
struct od_cpu_dbs_info_s *dbs_info;ktime_t time_now;s64 delta_us;bool sample = true;--start--
/* use leader CPU's dbs_info */dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info_local->cdbs.cpu);mutex_lock(&dbs_info->cdbs.timer_mutex);time_now = ktime_get();delta_us = ktime_us_delta(time_now, dbs_info->cdbs.time_stamp);/* Do nothing if we recently have sampled */if (delta_us < (s64)(od_tuners.sampling_rate / 2))sample = false;elsedbs_info->cdbs.time_stamp = time_now;--end--
Instead of two routines (this and the one below), we can have one and can put the above code in the if (coordinated cpus case). That will save some redundant code.
Ok but then you have two dbs_infos mixing up in the same code and it start to become harder to read (first version was with a single function I think).
Another issue that i see is: Current routine will be called from timer handler of every cpu and so above calculations will happen for every cpu. Here, you are taking the diff of last timestamp of cpu "x" with cpu "x" current timestamp, but what you should have really done is the diff b/w current timestamp with the timestamp of last change on any cpu in policy->cpus.
Isn't that how it works now? The current cpu ktime is not checked against its own, but against the "leader" cpu (dbs_info_local->cdbs.cpu), that's why it's initialized only for the first.
Maybe I should have used dbs_info_leader/dbs_info instead of dbs_info_local/dbs_info.
Over that, timestamp for all cpu's isn't initialized early in the code at GOV_START.
Am i correct?
As above.
Fabio
od_timer_update(dbs_info, sample, dw); mutex_unlock(&dbs_info->cdbs.timer_mutex);}
+static void od_dbs_timer(struct work_struct *work) +{
struct delayed_work *dw = to_delayed_work(work);struct od_cpu_dbs_info_s *dbs_info =container_of(work, struct od_cpu_dbs_info_s, cdbs.work.work);if (dbs_sw_coordinated_cpus(&dbs_info->cdbs)) {od_timer_coordinated(dbs_info, dw);} else {mutex_lock(&dbs_info->cdbs.timer_mutex);od_timer_update(dbs_info, true, dw);mutex_unlock(&dbs_info->cdbs.timer_mutex);}+}