| Message ID | 1359550803-18577-3-git-send-email-fabio.baltieri@linaro.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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? > --- > 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. > 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; > + else > + dbs_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. 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. Over that, timestamp for all cpu's isn't initialized early in the code at GOV_START. Am i correct? > + 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); > + } > +} > + -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > > + else > > + dbs_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); > > + } > > +} > > +
On 30 January 2013 22:16, Fabio Baltieri <fabio.baltieri@linaro.org> wrote: > 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. This routine is called as wq handler. Which will recover dbs_info from work using container_of. Which would give dbs_info_local for the cpu j. Then we will execute below code. + /* use leader CPU's dbs_info */ + dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info_local->cdbs.cpu); dbs_info_local->cdbs.cpu was uninitialized for all cpus except policy->cpu. And so, might be initialized with 0 as its a global variable... But if you offline cpu 0 and online it back, then policy->cpu would be 1 and this logic, which worked by mistake will fail. + mutex_lock(&dbs_info->cdbs.timer_mutex); + + time_now = ktime_get(); + delta_us = ktime_us_delta(time_now, dbs_info->cdbs.time_stamp); and so as this. Correct? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Viresh, On Wed, Jan 30, 2013 at 10:41:08PM +0530, Viresh Kumar wrote: > On 30 January 2013 22:16, Fabio Baltieri <fabio.baltieri@linaro.org> wrote: > > 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. > > This routine is called as wq handler. Which will recover dbs_info from work > using container_of. Which would give dbs_info_local for the cpu j. > > Then we will execute below code. > > + /* use leader CPU's dbs_info */ > + dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info_local->cdbs.cpu); > > dbs_info_local->cdbs.cpu was uninitialized for all cpus except policy->cpu. Ok, now I see the problem: cdbs->cpu is initialized only on the leader cpu and this is working by coincidence on my system, while cdbs->time_stamp is initialized only on the leader cpu, and that should be correct even when cpu hotplugging as that's reinitialized every time. That's a fix so I'll send a patch just to set ->cpu into the for_each_cpu cycle. Thanks, Fabio > And so, might be initialized with 0 as its a global variable... But if you > offline cpu 0 and online it back, then policy->cpu would be 1 and this logic, > which worked by mistake will fail. > > + mutex_lock(&dbs_info->cdbs.timer_mutex); > + > + time_now = ktime_get(); > + delta_us = ktime_us_delta(time_now, dbs_info->cdbs.time_stamp); > > and so as this. > > Correct? > -- > To unsubscribe from this list: send the line "unsubscribe cpufreq" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31 January 2013 14:09, Fabio Baltieri <fabio.baltieri@linaro.org> wrote: > Ok, now I see the problem: cdbs->cpu is initialized only on the leader > cpu and this is working by coincidence on my system, while > cdbs->time_stamp is initialized only on the leader cpu, and that should > be correct even when cpu hotplugging as that's reinitialized every time. > > That's a fix so I'll send a patch just to set ->cpu into the > for_each_cpu cycle. That's not enough. You need to set ->cpu to j and not policy->cpu as we discussed it earlier (params to timer init and exit.). And so, we need to get policy->cpu somehow and get its timestamp. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 31, 2013 at 02:12:29PM +0530, Viresh Kumar wrote: > On 31 January 2013 14:09, Fabio Baltieri <fabio.baltieri@linaro.org> wrote: > > Ok, now I see the problem: cdbs->cpu is initialized only on the leader > > cpu and this is working by coincidence on my system, while > > cdbs->time_stamp is initialized only on the leader cpu, and that should > > be correct even when cpu hotplugging as that's reinitialized every time. > > > > That's a fix so I'll send a patch just to set ->cpu into the > > for_each_cpu cycle. > > That's not enough. You need to set ->cpu to j and not policy->cpu as we > discussed it earlier (params to timer init and exit.). > > And so, we need to get policy->cpu somehow and get its timestamp. Current code uses ->cpu to track policy->cpu and get the timestamp, I can modify it to point to the current cpu and use it in timer_init *and* add a new field just to track leader_cpu but I don't see the benefits. Am I missing something? Fabio
On 31 January 2013 14:36, Fabio Baltieri <fabio.baltieri@linaro.org> wrote: > Current code uses ->cpu to track policy->cpu and get the timestamp, I > can modify it to point to the current cpu and use it in timer_init *and* > add a new field just to track leader_cpu but I don't see the benefits. > Am I missing something? Current code doesn't use cdbs->cpu for any cpu leaving the leader. So, we can use that field to keep local cpus. And for any cpu we can get the updated leader cpu with following: j_cdbs->cur_policy->cpu :) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(); + for_each_cpu(j, policy->cpus) dbs_timer_init(dbs_data, j, *sampling_rate); break; diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h index 5bf6fb8..aaf073d 100644 --- a/drivers/cpufreq/cpufreq_governor.h +++ b/drivers/cpufreq/cpufreq_governor.h @@ -82,6 +82,7 @@ struct cpu_dbs_common_info { * the governor or limits. */ struct mutex timer_mutex; + ktime_t time_stamp; }; struct od_cpu_dbs_info_s { 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); +} + +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; + + /* 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; + else + dbs_info->cdbs.time_stamp = time_now; + + 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); + } +} + /************************** sysfs interface ************************/ static ssize_t show_sampling_rate_min(struct kobject *kobj,
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> --- 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(-)