Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331278 > unrolled thread

[PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-10 16:40 +0100
    [PATCH v6 2/3] cpufreq: intel_pstate: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-10 16:40 +0100
    [PATCH v6 3/3] cpufreq: governor: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-10 16:40 +0100
    Re: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-11 00:20 +0100
      RE: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Doug Smythies" <dsmythies@telus.net> - 2016-02-12 00:00 +0100
        Re: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-12 00:30 +0100
          RE: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Doug Smythies" <dsmythies@telus.net> - 2016-02-12 02:10 +0100
            Re: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-12 02:30 +0100
        RE: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Doug Smythies" <dsmythies@telus.net> - 2016-02-12 08:30 +0100
          Re: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-12 14:40 +0100
            RE: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Doug Smythies" <dsmythies@telus.net> - 2016-02-12 18:40 +0100
              Re: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-13 00:30 +0100
    RE: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks "Doug Smythies" <dsmythies@telus.net> - 2016-02-11 00:20 +0100
      Re: [PATCH v6 0/3] cpufreq: Replace timers with utilization update  callbacks Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2016-02-11 07:10 +0100

#1331278 — [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks

Hi,

I thought it would be useful to send an update of this (adding Ingo, as Peter
has not been responsive lately).  The version goes straight to 6 as patch [3/3]
has already gone through 5 revisions.

The intro below still applies, so let me quote it.

On Friday, January 29, 2016 11:52:15 PM Rafael J. Wysocki wrote:
> Hi,
> 
> The following patch series introduces a mechanism allowing the cpufreq core
> and "setpolicy" drivers to provide utilization update callbacks to be invoked
> by the scheduler on utilization changes.  Those callbacks can be used to run
> the sampling and frequency adjustments code (intel_pstate) or to schedule the
> execution of that code in process context (cpufreq core) instead of per-CPU
> deferrable timers used in cpufreq today (which Thomas complained about during
> the last Kernel Summit).
> 
> [1/3] Introduce a mechanism for calling into cpufreq from the scheduler and
>       registering callbacks to be executed from there.
> 
> [2/3] Modify intel_pstate to use the mechanism introduced by [1/3] instead
>       of per-CPU deferrable timers to do its work.
> 
> This isn't entirely straightforward as the scheduler context running those
> callbacks is really special.  Among other things it can only use raw
> spinlocks and cannot invoke wake_up_process() directly.  Also, calling
> ktime_get() from there may be too expensive on some systems.  All that has to
> be taken into account, but even then the change allows some lines of code to be
> cut from the driver.
> 
> Some performance and energy consumption measurements have been carried out with
> an earlier version of this patch and it looks like the changes lead to a
> slightly better performing system that consumes slightly less energy at the
> same time overall.
> 
> [3/3] Modify the cpufreq core to use the mechanism introduced by [1/3] instead
>       of per-CPU deferrable timers to queue up the execution of governor work.
> 
> Again, this isn't really straightforward for the above reasons, but still the
> code size is reduced a bit by the changes.
> 

As it turns out, patch [3/3] appears to lead to improvements in both overall
system performance and energy consumption at the same time (the are small, but
measurable).  It also unlocks further simplifications and fixes in the cpufreq
core code, so we want it badly. :-)

The most significant change from the previous version of the set is that [1/3]
now also triggers cpufreq updates from the RT and DL sched classes to avoid
stalling it in situations when no CFS activity is taking place on the CPU due
to RT/DL tasks activity (as pointed out by Steve).

As stated in a reply to Juri, the scheduler-provided utilization numbers are
not used by cpufreq at this time, but we will be using them going forward.
 
The patches are on top of 4.5-rc3 and have been tested on x86 machines.

There aleady is a metric ton of stuff to go on top of them, so I'd like to
make progress here if at all possible,

I'll put this set (along with all the stuff depending on it) into the
pm-cpufreq-test branch of the linux-pm tree.

Thanks,
Rafael

[toc] | [next] | [standalone]

#1331279 — [PATCH v6 2/3] cpufreq: intel_pstate: Replace timers with utilization update callbacks

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Instead of using a per-CPU deferrable timer for utilization sampling
and P-states adjustments, register a utilization update callback that
will be invoked from the scheduler on utilization changes.

The sampling rate is still the same as what was used for the deferrable
timers, so the functional impact of this patch should not be significant.

Based on an earlier patch from Srinivas Pandruvada.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---

No changes from the previous version.

---
 drivers/cpufreq/intel_pstate.c |  103 +++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 64 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -71,7 +71,7 @@ struct sample {
 	u64 mperf;
 	u64 tsc;
 	int freq;
-	ktime_t time;
+	u64 time;
 };
 
 struct pstate_data {
@@ -103,13 +103,13 @@ struct _pid {
 struct cpudata {
 	int cpu;
 
-	struct timer_list timer;
+	struct update_util_data update_util;
 
 	struct pstate_data pstate;
 	struct vid_data vid;
 	struct _pid pid;
 
-	ktime_t last_sample_time;
+	u64	last_sample_time;
 	u64	prev_aperf;
 	u64	prev_mperf;
 	u64	prev_tsc;
@@ -120,6 +120,7 @@ struct cpudata {
 static struct cpudata **all_cpu_data;
 struct pstate_adjust_policy {
 	int sample_rate_ms;
+	s64 sample_rate_ns;
 	int deadband;
 	int setpoint;
 	int p_gain_pct;
@@ -712,7 +713,7 @@ static void core_set_pstate(struct cpuda
 	if (limits->no_turbo && !limits->turbo_disabled)
 		val |= (u64)1 << 32;
 
-	wrmsrl_on_cpu(cpudata->cpu, MSR_IA32_PERF_CTL, val);
+	wrmsrl(MSR_IA32_PERF_CTL, val);
 }
 
 static int knl_get_turbo_pstate(void)
@@ -883,7 +884,7 @@ static inline void intel_pstate_calc_bus
 	sample->core_pct_busy = (int32_t)core_pct;
 }
 
-static inline void intel_pstate_sample(struct cpudata *cpu)
+static inline void intel_pstate_sample(struct cpudata *cpu, u64 time)
 {
 	u64 aperf, mperf;
 	unsigned long flags;
@@ -900,7 +901,7 @@ static inline void intel_pstate_sample(s
 	local_irq_restore(flags);
 
 	cpu->last_sample_time = cpu->sample.time;
-	cpu->sample.time = ktime_get();
+	cpu->sample.time = time;
 	cpu->sample.aperf = aperf;
 	cpu->sample.mperf = mperf;
 	cpu->sample.tsc =  tsc;
@@ -915,22 +916,6 @@ static inline void intel_pstate_sample(s
 	cpu->prev_tsc = tsc;
 }
 
-static inline void intel_hwp_set_sample_time(struct cpudata *cpu)
-{
-	int delay;
-
-	delay = msecs_to_jiffies(50);
-	mod_timer_pinned(&cpu->timer, jiffies + delay);
-}
-
-static inline void intel_pstate_set_sample_time(struct cpudata *cpu)
-{
-	int delay;
-
-	delay = msecs_to_jiffies(pid_params.sample_rate_ms);
-	mod_timer_pinned(&cpu->timer, jiffies + delay);
-}
-
 static inline int32_t get_target_pstate_use_cpu_load(struct cpudata *cpu)
 {
 	struct sample *sample = &cpu->sample;
@@ -970,8 +955,7 @@ static inline int32_t get_target_pstate_
 static inline int32_t get_target_pstate_use_performance(struct cpudata *cpu)
 {
 	int32_t core_busy, max_pstate, current_pstate, sample_ratio;
-	s64 duration_us;
-	u32 sample_time;
+	u64 duration_ns;
 
 	/*
 	 * core_busy is the ratio of actual performance to max
@@ -990,18 +974,16 @@ static inline int32_t get_target_pstate_
 	core_busy = mul_fp(core_busy, div_fp(max_pstate, current_pstate));
 
 	/*
-	 * Since we have a deferred timer, it will not fire unless
-	 * we are in C0.  So, determine if the actual elapsed time
-	 * is significantly greater (3x) than our sample interval.  If it
-	 * is, then we were idle for a long enough period of time
-	 * to adjust our busyness.
+	 * Since our utilization update callback will not run unless we are
+	 * in C0, check if the actual elapsed time is significantly greater (3x)
+	 * than our sample interval.  If it is, then we were idle for a long
+	 * enough period of time to adjust our busyness.
 	 */
-	sample_time = pid_params.sample_rate_ms  * USEC_PER_MSEC;
-	duration_us = ktime_us_delta(cpu->sample.time,
-				     cpu->last_sample_time);
-	if (duration_us > sample_time * 3) {
-		sample_ratio = div_fp(int_tofp(sample_time),
-				      int_tofp(duration_us));
+	duration_ns = cpu->sample.time - cpu->last_sample_time;
+	if ((s64)duration_ns > pid_params.sample_rate_ns * 3
+	    && cpu->last_sample_time > 0) {
+		sample_ratio = div_fp(int_tofp(pid_params.sample_rate_ns),
+				      int_tofp(duration_ns));
 		core_busy = mul_fp(core_busy, sample_ratio);
 	}
 
@@ -1031,23 +1013,17 @@ static inline void intel_pstate_adjust_b
 		sample->freq);
 }
 
-static void intel_hwp_timer_func(unsigned long __data)
-{
-	struct cpudata *cpu = (struct cpudata *) __data;
-
-	intel_pstate_sample(cpu);
-	intel_hwp_set_sample_time(cpu);
-}
-
-static void intel_pstate_timer_func(unsigned long __data)
+static void intel_pstate_update_util(struct update_util_data *data, u64 time,
+				     unsigned long util, unsigned long max)
 {
-	struct cpudata *cpu = (struct cpudata *) __data;
-
-	intel_pstate_sample(cpu);
+	struct cpudata *cpu = container_of(data, struct cpudata, update_util);
+	u64 delta_ns = time - cpu->sample.time;
 
-	intel_pstate_adjust_busy_pstate(cpu);
-
-	intel_pstate_set_sample_time(cpu);
+	if ((s64)delta_ns >= pid_params.sample_rate_ns) {
+		intel_pstate_sample(cpu, time);
+		if (!hwp_active)
+			intel_pstate_adjust_busy_pstate(cpu);
+	}
 }
 
 #define ICPU(model, policy) \
@@ -1095,24 +1071,19 @@ static int intel_pstate_init_cpu(unsigne
 
 	cpu->cpu = cpunum;
 
-	if (hwp_active)
+	if (hwp_active) {
 		intel_pstate_hwp_enable(cpu);
+		pid_params.sample_rate_ms = 50;
+		pid_params.sample_rate_ns = 50 * NSEC_PER_MSEC;
+	}
 
 	intel_pstate_get_cpu_pstates(cpu);
 
-	init_timer_deferrable(&cpu->timer);
-	cpu->timer.data = (unsigned long)cpu;
-	cpu->timer.expires = jiffies + HZ/100;
-
-	if (!hwp_active)
-		cpu->timer.function = intel_pstate_timer_func;
-	else
-		cpu->timer.function = intel_hwp_timer_func;
-
 	intel_pstate_busy_pid_reset(cpu);
-	intel_pstate_sample(cpu);
+	intel_pstate_sample(cpu, 0);
 
-	add_timer_on(&cpu->timer, cpunum);
+	cpu->update_util.func = intel_pstate_update_util;
+	cpufreq_set_update_util_data(cpunum, &cpu->update_util);
 
 	pr_debug("intel_pstate: controlling: cpu %d\n", cpunum);
 
@@ -1196,7 +1167,9 @@ static void intel_pstate_stop_cpu(struct
 
 	pr_debug("intel_pstate: CPU %d exiting\n", cpu_num);
 
-	del_timer_sync(&all_cpu_data[cpu_num]->timer);
+	cpufreq_set_update_util_data(cpu_num, NULL);
+	synchronize_rcu();
+
 	if (hwp_active)
 		return;
 
@@ -1260,6 +1233,7 @@ static int intel_pstate_msrs_not_valid(v
 static void copy_pid_params(struct pstate_adjust_policy *policy)
 {
 	pid_params.sample_rate_ms = policy->sample_rate_ms;
+	pid_params.sample_rate_ns = pid_params.sample_rate_ms * NSEC_PER_MSEC;
 	pid_params.p_gain_pct = policy->p_gain_pct;
 	pid_params.i_gain_pct = policy->i_gain_pct;
 	pid_params.d_gain_pct = policy->d_gain_pct;
@@ -1451,7 +1425,8 @@ out:
 	get_online_cpus();
 	for_each_online_cpu(cpu) {
 		if (all_cpu_data[cpu]) {
-			del_timer_sync(&all_cpu_data[cpu]->timer);
+			cpufreq_set_update_util_data(cpu, NULL);
+			synchronize_rcu();
 			kfree(all_cpu_data[cpu]);
 		}
 	}

[toc] | [prev] | [next] | [standalone]

#1331281 — [PATCH v6 3/3] cpufreq: governor: Replace timers with utilization update callbacks

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Instead of using a per-CPU deferrable timer for queuing up governor
work items, register a utilization update callback that will be
invoked from the scheduler on utilization changes.

The sampling rate is still the same as what was used for the
deferrable timers and the added irq_work overhead should be offset by
the eliminated timers overhead, so in theory the functional impact of
this patch should not be significant.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---

Changes from the v5:
- Use #ifdef/#else in gov_queue_irq_work() to avoid build failures.
- Select IRQ_WORK in cpufreq Kconfig to avoid build failures.

No functional changes.

---
 drivers/cpufreq/Kconfig                |    1 
 drivers/cpufreq/cpufreq_conservative.c |    6 -
 drivers/cpufreq/cpufreq_governor.c     |  165 +++++++++++++++------------------
 drivers/cpufreq/cpufreq_governor.h     |   19 ++-
 drivers/cpufreq/cpufreq_ondemand.c     |   43 ++++----
 5 files changed, 114 insertions(+), 120 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -18,6 +18,7 @@
 #define _CPUFREQ_GOVERNOR_H
 
 #include <linux/atomic.h>
+#include <linux/irq_work.h>
 #include <linux/cpufreq.h>
 #include <linux/kernel_stat.h>
 #include <linux/module.h>
@@ -138,11 +139,19 @@ struct cpu_common_dbs_info {
 	 */
 	struct mutex timer_mutex;
 
-	ktime_t time_stamp;
+	u64 last_sample_time;
+	s64 sample_delay_ns;
 	atomic_t skip_work;
+	struct irq_work irq_work;
 	struct work_struct work;
 };
 
+static inline void gov_update_sample_delay(struct cpu_common_dbs_info *shared,
+					   unsigned int delay_us)
+{
+	shared->sample_delay_ns = delay_us * NSEC_PER_USEC;
+}
+
 /* Per cpu structures */
 struct cpu_dbs_info {
 	u64 prev_cpu_idle;
@@ -155,7 +164,7 @@ struct cpu_dbs_info {
 	 * wake-up from idle.
 	 */
 	unsigned int prev_load;
-	struct timer_list timer;
+	struct update_util_data update_util;
 	struct cpu_common_dbs_info *shared;
 };
 
@@ -212,8 +221,7 @@ struct common_dbs_data {
 
 	struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
 	void *(*get_cpu_dbs_info_s)(int cpu);
-	unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy,
-				      bool modify_all);
+	unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
 	void (*gov_check_cpu)(int cpu, unsigned int load);
 	int (*init)(struct dbs_data *dbs_data, bool notify);
 	void (*exit)(struct dbs_data *dbs_data, bool notify);
@@ -270,9 +278,6 @@ static ssize_t show_sampling_rate_min_go
 }
 
 extern struct mutex cpufreq_governor_lock;
-
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
-void gov_cancel_work(struct cpu_common_dbs_info *shared);
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
 int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		struct common_dbs_data *cdata, unsigned int event);
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -128,10 +128,10 @@ void dbs_check_cpu(struct dbs_data *dbs_
 		 * dropped down. So we perform the copy only once, upon the
 		 * first wake-up from idle.)
 		 *
-		 * Detecting this situation is easy: the governor's deferrable
-		 * timer would not have fired during CPU-idle periods. Hence
-		 * an unusually large 'wall_time' (as compared to the sampling
-		 * rate) indicates this scenario.
+		 * Detecting this situation is easy: the governor's utilization
+		 * update handler would not have run during CPU-idle periods.
+		 * Hence, an unusually large 'wall_time' (as compared to the
+		 * sampling rate) indicates this scenario.
 		 *
 		 * prev_load can be zero in two cases and we must recalculate it
 		 * for both cases:
@@ -161,72 +161,48 @@ void dbs_check_cpu(struct dbs_data *dbs_
 }
 EXPORT_SYMBOL_GPL(dbs_check_cpu);
 
-void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
+void gov_set_update_util(struct cpu_common_dbs_info *shared,
+			 unsigned int delay_us)
 {
+	struct cpufreq_policy *policy = shared->policy;
 	struct dbs_data *dbs_data = policy->governor_data;
-	struct cpu_dbs_info *cdbs;
 	int cpu;
 
+	gov_update_sample_delay(shared, delay_us);
+	shared->last_sample_time = 0;
+
 	for_each_cpu(cpu, policy->cpus) {
-		cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
-		cdbs->timer.expires = jiffies + delay;
-		add_timer_on(&cdbs->timer, cpu);
+		struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+
+		cpufreq_set_update_util_data(cpu, &cdbs->update_util);
 	}
 }
-EXPORT_SYMBOL_GPL(gov_add_timers);
+EXPORT_SYMBOL_GPL(gov_set_update_util);
 
-static inline void gov_cancel_timers(struct cpufreq_policy *policy)
+static inline void gov_clear_update_util(struct cpufreq_policy *policy)
 {
-	struct dbs_data *dbs_data = policy->governor_data;
-	struct cpu_dbs_info *cdbs;
 	int i;
 
-	for_each_cpu(i, policy->cpus) {
-		cdbs = dbs_data->cdata->get_cpu_cdbs(i);
-		del_timer_sync(&cdbs->timer);
-	}
+	for_each_cpu(i, policy->cpus)
+		cpufreq_set_update_util_data(i, NULL);
+
+	synchronize_rcu();
 }
 
-void gov_cancel_work(struct cpu_common_dbs_info *shared)
+static void gov_cancel_work(struct cpu_common_dbs_info *shared)
 {
-	/* Tell dbs_timer_handler() to skip queuing up work items. */
+	/* Tell dbs_update_util_handler() to skip queuing up work items. */
 	atomic_inc(&shared->skip_work);
 	/*
-	 * If dbs_timer_handler() is already running, it may not notice the
-	 * incremented skip_work, so wait for it to complete to prevent its work
-	 * item from being queued up after the cancel_work_sync() below.
-	 */
-	gov_cancel_timers(shared->policy);
-	/*
-	 * In case dbs_timer_handler() managed to run and spawn a work item
-	 * before the timers have been canceled, wait for that work item to
-	 * complete and then cancel all of the timers set up by it.  If
-	 * dbs_timer_handler() runs again at that point, it will see the
-	 * positive value of skip_work and won't spawn any more work items.
+	 * If dbs_update_util_handler() is already running, it may not notice
+	 * the incremented skip_work, so wait for it to complete to prevent its
+	 * work item from being queued up after the cancel_work_sync() below.
 	 */
+	gov_clear_update_util(shared->policy);
+	irq_work_sync(&shared->irq_work);
 	cancel_work_sync(&shared->work);
-	gov_cancel_timers(shared->policy);
 	atomic_set(&shared->skip_work, 0);
 }
-EXPORT_SYMBOL_GPL(gov_cancel_work);
-
-/* Will return if we need to evaluate cpu load again or not */
-static bool need_load_eval(struct cpu_common_dbs_info *shared,
-			   unsigned int sampling_rate)
-{
-	if (policy_is_shared(shared->policy)) {
-		ktime_t time_now = ktime_get();
-		s64 delta_us = ktime_us_delta(time_now, shared->time_stamp);
-
-		/* Do nothing if we recently have sampled */
-		if (delta_us < (s64)(sampling_rate / 2))
-			return false;
-		else
-			shared->time_stamp = time_now;
-	}
-
-	return true;
-}
 
 static void dbs_work_handler(struct work_struct *work)
 {
@@ -234,56 +210,70 @@ static void dbs_work_handler(struct work
 					cpu_common_dbs_info, work);
 	struct cpufreq_policy *policy;
 	struct dbs_data *dbs_data;
-	unsigned int sampling_rate, delay;
-	bool eval_load;
+	unsigned int delay;
 
 	policy = shared->policy;
 	dbs_data = policy->governor_data;
 
-	/* Kill all timers */
-	gov_cancel_timers(policy);
-
-	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
-		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
-
-		sampling_rate = cs_tuners->sampling_rate;
-	} else {
-		struct od_dbs_tuners *od_tuners = dbs_data->tuners;
-
-		sampling_rate = od_tuners->sampling_rate;
-	}
-
-	eval_load = need_load_eval(shared, sampling_rate);
-
 	/*
-	 * Make sure cpufreq_governor_limits() isn't evaluating load in
-	 * parallel.
+	 * Make sure cpufreq_governor_limits() isn't evaluating load or the
+	 * ondemand governor isn't updating the sampling rate in parallel.
 	 */
 	mutex_lock(&shared->timer_mutex);
-	delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
+	delay = dbs_data->cdata->gov_dbs_timer(policy);
+	shared->sample_delay_ns = jiffies_to_nsecs(delay);
 	mutex_unlock(&shared->timer_mutex);
 
+	/*
+	 * If the atomic operation below is reordered with respect to the
+	 * sample delay modification, the utilization update handler may end
+	 * up using a stale sample delay value.
+	 */
+	smp_mb__before_atomic();
 	atomic_dec(&shared->skip_work);
+}
 
-	gov_add_timers(policy, delay);
+static void dbs_irq_work(struct irq_work *irq_work)
+{
+	struct cpu_common_dbs_info *shared;
+
+	shared = container_of(irq_work, struct cpu_common_dbs_info, irq_work);
+	schedule_work(&shared->work);
 }
 
-static void dbs_timer_handler(unsigned long data)
+static inline void gov_queue_irq_work(struct cpu_common_dbs_info *shared)
 {
-	struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+#ifdef CONFIG_SMP
+	irq_work_queue_on(&shared->irq_work, smp_processor_id());
+#else
+	irq_work_queue(&shared->irq_work);
+#endif
+}
+
+static void dbs_update_util_handler(struct update_util_data *data, u64 time,
+				    unsigned long util, unsigned long max)
+{
+	struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
 	struct cpu_common_dbs_info *shared = cdbs->shared;
 
 	/*
-	 * Timer handler may not be allowed to queue the work at the moment,
-	 * because:
-	 * - Another timer handler has done that
-	 * - We are stopping the governor
-	 * - Or we are updating the sampling rate of the ondemand governor
+	 * The work may not be allowed to be queued up right now.
+	 * Possible reasons:
+	 * - Work has already been queued up or is in progress.
+	 * - The governor is being stopped.
+	 * - It is too early (too little time from the previous sample).
 	 */
-	if (atomic_inc_return(&shared->skip_work) > 1)
-		atomic_dec(&shared->skip_work);
-	else
-		queue_work(system_wq, &shared->work);
+	if (atomic_inc_return(&shared->skip_work) == 1) {
+		u64 delta_ns;
+
+		delta_ns = time - shared->last_sample_time;
+		if ((s64)delta_ns >= shared->sample_delay_ns) {
+			shared->last_sample_time = time;
+			gov_queue_irq_work(shared);
+			return;
+		}
+	}
+	atomic_dec(&shared->skip_work);
 }
 
 static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -315,6 +305,7 @@ static int alloc_common_dbs_info(struct
 
 	mutex_init(&shared->timer_mutex);
 	atomic_set(&shared->skip_work, 0);
+	init_irq_work(&shared->irq_work, dbs_irq_work);
 	INIT_WORK(&shared->work, dbs_work_handler);
 	return 0;
 }
@@ -467,9 +458,6 @@ static int cpufreq_governor_start(struct
 		io_busy = od_tuners->io_is_busy;
 	}
 
-	shared->policy = policy;
-	shared->time_stamp = ktime_get();
-
 	for_each_cpu(j, policy->cpus) {
 		struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
 		unsigned int prev_load;
@@ -485,10 +473,9 @@ static int cpufreq_governor_start(struct
 		if (ignore_nice)
 			j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
 
-		__setup_timer(&j_cdbs->timer, dbs_timer_handler,
-			      (unsigned long)j_cdbs,
-			      TIMER_DEFERRABLE | TIMER_IRQSAFE);
+		j_cdbs->update_util.func = dbs_update_util_handler;
 	}
+	shared->policy = policy;
 
 	if (cdata->governor == GOV_CONSERVATIVE) {
 		struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -505,7 +492,7 @@ static int cpufreq_governor_start(struct
 		od_ops->powersave_bias_init_cpu(cpu);
 	}
 
-	gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
+	gov_set_update_util(shared, sampling_rate);
 	return 0;
 }
 
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -191,7 +191,7 @@ static void od_check_cpu(int cpu, unsign
 	}
 }
 
-static unsigned int od_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
 {
 	struct dbs_data *dbs_data = policy->governor_data;
 	unsigned int cpu = policy->cpu;
@@ -200,9 +200,6 @@ static unsigned int od_dbs_timer(struct
 	struct od_dbs_tuners *od_tuners = dbs_data->tuners;
 	int delay = 0, sample_type = dbs_info->sample_type;
 
-	if (!modify_all)
-		goto max_delay;
-
 	/* Common NORMAL_SAMPLE setup */
 	dbs_info->sample_type = OD_NORMAL_SAMPLE;
 	if (sample_type == OD_SUB_SAMPLE) {
@@ -218,7 +215,6 @@ static unsigned int od_dbs_timer(struct
 		}
 	}
 
-max_delay:
 	if (!delay)
 		delay = delay_for_sampling_rate(od_tuners->sampling_rate
 				* dbs_info->rate_mult);
@@ -264,7 +260,6 @@ static void update_sampling_rate(struct
 		struct od_cpu_dbs_info_s *dbs_info;
 		struct cpu_dbs_info *cdbs;
 		struct cpu_common_dbs_info *shared;
-		unsigned long next_sampling, appointed_at;
 
 		dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
 		cdbs = &dbs_info->cdbs;
@@ -288,20 +283,28 @@ static void update_sampling_rate(struct
 		 * policy will be governed by dbs_data, otherwise there can be
 		 * multiple policies that are governed by the same dbs_data.
 		 */
-		if (dbs_data != policy->governor_data)
-			continue;
-
-		/*
-		 * Checking this for any CPU should be fine, timers for all of
-		 * them are scheduled together.
-		 */
-		next_sampling = jiffies + usecs_to_jiffies(new_rate);
-		appointed_at = dbs_info->cdbs.timer.expires;
-
-		if (time_before(next_sampling, appointed_at)) {
-			gov_cancel_work(shared);
-			gov_add_timers(policy, usecs_to_jiffies(new_rate));
-
+		if (dbs_data == policy->governor_data) {
+			mutex_lock(&shared->timer_mutex);
+			/*
+			 * On 32-bit architectures this may race with the
+			 * sample_delay_ns read in dbs_update_util_handler(),
+			 * but that really doesn't matter.  If the read returns
+			 * a value that's too big, the sample will be skipped,
+			 * but the next invocation of dbs_update_util_handler()
+			 * (when the update has been completed) will take a
+			 * sample.  If the returned value is too small, the
+			 * sample will be taken immediately, but that isn't a
+			 * problem, as we want the new rate to take effect
+			 * immediately anyway.
+			 *
+			 * If this runs in parallel with dbs_work_handler(), we
+			 * may end up overwriting the sample_delay_ns value that
+			 * it has just written, but the difference should not be
+			 * too big and it will be corrected next time a sample
+			 * is taken, so it shouldn't be significant.
+			 */
+			gov_update_sample_delay(shared, new_rate);
+			mutex_unlock(&shared->timer_mutex);
 		}
 	}
 
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -115,14 +115,12 @@ static void cs_check_cpu(int cpu, unsign
 	}
 }
 
-static unsigned int cs_dbs_timer(struct cpufreq_policy *policy, bool modify_all)
+static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
 {
 	struct dbs_data *dbs_data = policy->governor_data;
 	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
 
-	if (modify_all)
-		dbs_check_cpu(dbs_data, policy->cpu);
-
+	dbs_check_cpu(dbs_data, policy->cpu);
 	return delay_for_sampling_rate(cs_tuners->sampling_rate);
 }
 
Index: linux-pm/drivers/cpufreq/Kconfig
===================================================================
--- linux-pm.orig/drivers/cpufreq/Kconfig
+++ linux-pm/drivers/cpufreq/Kconfig
@@ -3,6 +3,7 @@ menu "CPU Frequency scaling"
 config CPU_FREQ
 	bool "CPU Frequency scaling"
 	select SRCU
+	select IRQ_WORK
 	help
 	  CPU Frequency scaling allows you to change the clock speed of 
 	  CPUs on the fly. This is a nice method to save power, because 

[toc] | [prev] | [next] | [standalone]

#1331576

On Wednesday, February 10, 2016 03:11:43 PM Doug Smythies wrote:
> On 2016.02.10 07:17 Rafael J. Wysocki wrote:
> > On Friday, January 29, 2016 11:52:15 PM Rafael J. Wysocki wrote:
> >>
> >> The following patch series introduces a mechanism allowing the cpufreq core
> >> and "setpolicy" drivers to provide utilization update callbacks to be invoked
> >> by the scheduler on utilization changes.  Those callbacks can be used to run
> >> the sampling and frequency adjustments code (intel_pstate) or to schedule the
> >> execution of that code in process context (cpufreq core) instead of per-CPU
> >> deferrable timers used in cpufreq today (which Thomas complained about during
> >> the last Kernel Summit).
> 
> This patch set solves a long standing issue with the intel_pstate driver.

Good to hear that, thanks!

> The issue began with the introduction of the "duration" method for deciding
> if the CPU had been idle for a long time resulting in forcing the
> target pstate downwards. Often this was the correct action, but sometimes this
> was the wrong thing to do, because the cpu was actually very busy, but just so
> happened to be idle on jiffy boundaries (perhaps similar to what Steve Muckle
> was referring to on another branch of this thread).
> 
> For an idle system, this patch set seems to change the maximum duration from
> 4 seconds to 0.5 seconds for most CPUs. However, when using v1 of patches 1
> and 2 of 3 and v5 of 3 of 3, sometimes the durations (time between passes of
> the intel-pstate driver for a given CPU) of upwards of 120 seconds were observed.
> When patches 1, 2, and 3 of 3 v6 were used, the maximum observed durations of an
> idle system were on the order of 500 milliseconds for most CPUs, but CPU 6
> sometimes went to 3.5 seconds and CPU 7 sometimes went to 4 seconds (small
> sample space, I'll consider to run an overnight test for a much much larger
> sample space). Note 4 seconds, is O.K., and what it was before, I'm just noting
> it is all.
> 
> I have a bunch of graphs, if anyone wants to see the supporting data.

It would be good to see how the data with and without the patchset compare
to each other if you have that.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1332417

On 2016.02.10 15:18 Rafael J. Wysocki wrote:
> On Wednesday, February 10, 2016 03:11:43 PM Doug Smythies wrote:
>> On 2016.02.10 07:17 Rafael J. Wysocki wrote:
>>> On Friday, January 29, 2016 11:52:15 PM Rafael J. Wysocki wrote:
>>>> 
>> This patch set solves a long standing issue with the intel_pstate driver.

> Good to hear that, thanks!

>> The issue began with the introduction of the "duration" method for deciding
>> if the CPU had been idle for a long time resulting in forcing the
>> target pstate downwards. Often this was the correct action, but sometimes this
>> was the wrong thing to do, because the cpu was actually very busy, but just so
>> happened to be idle on jiffy boundaries (perhaps similar to what Steve Muckle
>> was referring to on another branch of this thread).

>> I have a bunch of graphs, if anyone wants to see the supporting data.

> It would be good to see how the data with and without the patchset compare
> to each other if you have that.

Please see:
double u double u double u dot smythies dot com /~doug/linux/intel_pstate/rjw_patch_set/index.html

Specific duration tests graphs are posted, and also a bunch of idle tests graphs are posted.
The references section includes links to all raw and post processed data.

Note that on my 2 hour idle tests, I had a few 300 second durations
on CPU 6 with the v5 patch set.
(likely what Steve Muckle was referring to.)
Such long durations did not occur in v6 or v7 2 hour idle tests.

Very interesting patterns in the 2 hour idle tests durations for
individual CPUs.

On 2016.02.10 22:03 Srinivas Pandruvada wrote:

>> My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)
> Thanks Doug. If you have specific workloads, please compare performance.

My work so far has been testing functionality, with unrealistic workloads specifically
designed to exaggerate issues, in this case the duration problem.

I'll look at some real world workload scenarios.

What I do have from my 2 hour idle tests is the of total number of passes through
the intel_pstate driver:

Control sample: Kernel 4.3-rc3: 37949 passes.
Kernel 4.3-rc3 + rjw 3 patch set v5: 180355 passes
Kernel 4.3-rc3 + rjw 3 patch set v6: 201307 passes
Kernel 4.3-rc3 + rjw 3 patch set v7: 203619 passes

While I should have, I did not run turbostat to get idle energy and/or power.
However, a 1 hour idle test with turbostat gave (Package Joules):
Control sample: Kernel 4.3-rc3: 13788 J or 3.83 Watts
Kernel 4.3-rc3 + rjw 3 patch set v7: 13929 J or 3.87 Watts

... Doug

[toc] | [prev] | [next] | [standalone]

#1332434

Hi Doug,

On Thu, Feb 11, 2016 at 11:50 PM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2016.02.10 15:18 Rafael J. Wysocki wrote:
>> On Wednesday, February 10, 2016 03:11:43 PM Doug Smythies wrote:
>>> On 2016.02.10 07:17 Rafael J. Wysocki wrote:
>>>> On Friday, January 29, 2016 11:52:15 PM Rafael J. Wysocki wrote:
>>>>>
>>> This patch set solves a long standing issue with the intel_pstate driver.
>
>> Good to hear that, thanks!
>
>>> The issue began with the introduction of the "duration" method for deciding
>>> if the CPU had been idle for a long time resulting in forcing the
>>> target pstate downwards. Often this was the correct action, but sometimes this
>>> was the wrong thing to do, because the cpu was actually very busy, but just so
>>> happened to be idle on jiffy boundaries (perhaps similar to what Steve Muckle
>>> was referring to on another branch of this thread).
>
>>> I have a bunch of graphs, if anyone wants to see the supporting data.
>
>> It would be good to see how the data with and without the patchset compare
>> to each other if you have that.
>
> Please see:
> double u double u double u dot smythies dot com /~doug/linux/intel_pstate/rjw_patch_set/index.html

Thanks for the data.

> Specific duration tests graphs are posted, and also a bunch of idle tests graphs are posted.
> The references section includes links to all raw and post processed data.
>
> Note that on my 2 hour idle tests, I had a few 300 second durations
> on CPU 6 with the v5 patch set.
> (likely what Steve Muckle was referring to.)
> Such long durations did not occur in v6 or v7 2 hour idle tests.

OK, that suggests that using rq_lock(rq) in patch [1/3] is a win.

> Very interesting patterns in the 2 hour idle tests durations for
> individual CPUs.
>
> On 2016.02.10 22:03 Srinivas Pandruvada wrote:
>
>>> My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)
>> Thanks Doug. If you have specific workloads, please compare performance.
>
> My work so far has been testing functionality, with unrealistic workloads specifically
> designed to exaggerate issues, in this case the duration problem.
>
> I'll look at some real world workload scenarios.
>
> What I do have from my 2 hour idle tests is the of total number of passes through
> the intel_pstate driver:
>
> Control sample: Kernel 4.3-rc3: 37949 passes.
> Kernel 4.3-rc3 + rjw 3 patch set v5: 180355 passes
> Kernel 4.3-rc3 + rjw 3 patch set v6: 201307 passes
> Kernel 4.3-rc3 + rjw 3 patch set v7: 203619 passes

That reflects how things work with the changes.  The driver is called
more often now and has to decide whether or not to take a sample.

It would be interesting to see how many of those were samples that
were actually taken if you can instrument that.

> While I should have, I did not run turbostat to get idle energy and/or power.
> However, a 1 hour idle test with turbostat gave (Package Joules):
> Control sample: Kernel 4.3-rc3: 13788 J or 3.83 Watts
> Kernel 4.3-rc3 + rjw 3 patch set v7: 13929 J or 3.87 Watts

So it shows a slight increase in energy consumption with your
workloads.  It is not as much as to make me worry in any way, but I'm
wondering if performance is better too as a result (and how much
better if so).

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1332479

On 2016.02.11 15:28 Rafael J. Wysocki wrote:
> On 2106.02.11 14:50 Doug Smythies wrote:

>> What I do have from my 2 hour idle tests is the of total number of passes through
>> the intel_pstate driver:
>>
>> Control sample: Kernel 4.3-rc3: 37949 passes.
>> Kernel 4.3-rc3 + rjw 3 patch set v5: 180355 passes
>> Kernel 4.3-rc3 + rjw 3 patch set v6: 201307 passes
>> Kernel 4.3-rc3 + rjw 3 patch set v7: 203619 passes

> That reflects how things work with the changes.  The driver is called
> more often now and has to decide whether or not to take a sample.

Opps. I didn't understand that point, and so only now looked more
closely at the code.

> It would be interesting to see how many of those were samples that
> were actually taken if you can instrument that.

So, those are samples that were taken. There is no trace information
acquired when the new code decides not to take a sample (or so is my
understanding from a quick look).

I did find a couple of cases where the duration (elapsed time between
samples on a given CPU) was less than the nominal sample time. The search
was not exhaustive. (Likely O.K. within expected jitter, just noting
is all. The post processing tools use the kernel clock to do the
calculation, as the duration calculated by the driver is not in the trace
data.)

2 hour idle test: v5 patch 9.955 mSec sample 10078 CPU 1
2 hour idle test: v7 patch 9.968 mSec sample 49476 CPU 3
Duration load test: v7 patch 9.982 mSec sample 10997 CPU 2

... Doug

[toc] | [prev] | [next] | [standalone]

#1332483

On Fri, Feb 12, 2016 at 2:02 AM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2016.02.11 15:28 Rafael J. Wysocki wrote:
>> On 2106.02.11 14:50 Doug Smythies wrote:
>
>>> What I do have from my 2 hour idle tests is the of total number of passes through
>>> the intel_pstate driver:
>>>
>>> Control sample: Kernel 4.3-rc3: 37949 passes.
>>> Kernel 4.3-rc3 + rjw 3 patch set v5: 180355 passes
>>> Kernel 4.3-rc3 + rjw 3 patch set v6: 201307 passes
>>> Kernel 4.3-rc3 + rjw 3 patch set v7: 203619 passes
>
>> That reflects how things work with the changes.  The driver is called
>> more often now and has to decide whether or not to take a sample.
>
> Opps. I didn't understand that point, and so only now looked more
> closely at the code.
>
>> It would be interesting to see how many of those were samples that
>> were actually taken if you can instrument that.
>
> So, those are samples that were taken. There is no trace information
> acquired when the new code decides not to take a sample (or so is my
> understanding from a quick look).

That's correct.  The trace only covers the samples that were actually taken.

> I did find a couple of cases where the duration (elapsed time between
> samples on a given CPU) was less than the nominal sample time. The search
> was not exhaustive. (Likely O.K. within expected jitter, just noting
> is all. The post processing tools use the kernel clock to do the
> calculation, as the duration calculated by the driver is not in the trace
> data.)
>
> 2 hour idle test: v5 patch 9.955 mSec sample 10078 CPU 1
> 2 hour idle test: v7 patch 9.968 mSec sample 49476 CPU 3
> Duration load test: v7 patch 9.982 mSec sample 10997 CPU 2

OK, so the order of magnitude looks reasonable at least.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1332554

On 2016.02.11 14:50 Doug Smythies wrote:
> On 2016.02.10 22:03 Srinivas Pandruvada wrote:
>> On Wednesday, February 10, 2016 03:11:43 PM Doug Smythies wrote:

>>> My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)
>> Thanks Doug. If you have specific workloads, please compare performance.

> My work so far has been testing functionality, with unrealistic workloads specifically
> designed to exaggerate issues, in this case the duration problem.
>
> I'll look at some real world workload scenarios.

Turbostat used for package power, starts before Phoronix tests starts,
and ends after Phoronix test ends.

Control Sample: Kernel 4.5-rc3:
Phoronix ffmpeg: turbostat 180 Sec. 12.07 Sec. Ave. 27.14 Watts.
Phoronix apache: turbostat 200 Sec. 19797.0 R.P.S. Ave. 34.01 Watts.
Phoronix kernel: turbostat 180 Sec. 139.93 Sec. 49.09 Watts.
Phoronix Postmark (Disk Test): turbostat 200 Sec. 5813 T.P.S. Ave. 21.33 Watts.

Kernel 4.5-rc3 + RJW 3 patch set version 7:
Phoronix ffmpeg: turbostat 180 Sec. 11.67 Sec. Ave. 27.35 Watts.
Phoronix apache: turbostat 200 Sec. 19430.7 R.P.S. Ave. 34.18 Watts.
Phoronix kernel: turbostat 180 Sec. 139.81 Sec. 48.80 Watts.
Phoronix Postmark (Disk Test): turbostat 200 Sec. 5683 T.P.S. Ave. 22.41 Watts.

... Doug

[toc] | [prev] | [next] | [standalone]

#1332735

On Fri, Feb 12, 2016 at 8:25 AM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2016.02.11 14:50 Doug Smythies wrote:
>> On 2016.02.10 22:03 Srinivas Pandruvada wrote:
>>> On Wednesday, February 10, 2016 03:11:43 PM Doug Smythies wrote:
>
>>>> My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)
>>> Thanks Doug. If you have specific workloads, please compare performance.
>
>> My work so far has been testing functionality, with unrealistic workloads specifically
>> designed to exaggerate issues, in this case the duration problem.
>>
>> I'll look at some real world workload scenarios.
>
> Turbostat used for package power, starts before Phoronix tests starts,
> and ends after Phoronix test ends.
>
> Control Sample: Kernel 4.5-rc3:
> Phoronix ffmpeg: turbostat 180 Sec. 12.07 Sec. Ave. 27.14 Watts.
> Phoronix apache: turbostat 200 Sec. 19797.0 R.P.S. Ave. 34.01 Watts.
> Phoronix kernel: turbostat 180 Sec. 139.93 Sec. 49.09 Watts.
> Phoronix Postmark (Disk Test): turbostat 200 Sec. 5813 T.P.S. Ave. 21.33 Watts.
>
> Kernel 4.5-rc3 + RJW 3 patch set version 7:
> Phoronix ffmpeg: turbostat 180 Sec. 11.67 Sec. Ave. 27.35 Watts.
> Phoronix apache: turbostat 200 Sec. 19430.7 R.P.S. Ave. 34.18 Watts.
> Phoronix kernel: turbostat 180 Sec. 139.81 Sec. 48.80 Watts.
> Phoronix Postmark (Disk Test): turbostat 200 Sec. 5683 T.P.S. Ave. 22.41 Watts.

Thanks for the results!

The Postmark result is somewhat below expectations (especially with
respect to the energy consumption), but we should be able to improve
that by using the util numbers intelligently.

Do you have full turbostat reports from those runs by any chance?  I'm
wondering what happens to the idle state residencies, for example.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1332964

On 2016.02.12 05:39 Rafael J. Wysocki wrote:
> On Fri, Feb 12, 2016 at 8:25 AM, Doug Smythies <dsmythies@telus.net> wrote:
>> On 2016.02.11 14:50 Doug Smythies wrote:
>>> On 2016.02.10 22:03 Srinivas Pandruvada wrote:
>>>> On Wednesday, February 10, 2016 03:11:43 PM Doug Smythies wrote:
>>
>>>>> My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)
>>> Thanks Doug. If you have specific workloads, please compare performance.
>>
>>> My work so far has been testing functionality, with unrealistic workloads specifically
>>> designed to exaggerate issues, in this case the duration problem.
>>>
>>> I'll look at some real world workload scenarios.
>>
>> Turbostat used for package power, starts before Phoronix tests starts,
>> and ends after Phoronix test ends.
>>
>> Control Sample: Kernel 4.5-rc3:
>> Phoronix ffmpeg: turbostat 180 Sec. 12.07 Sec. Ave. 27.14 Watts.
>> Phoronix apache: turbostat 200 Sec. 19797.0 R.P.S. Ave. 34.01 Watts.
>> Phoronix kernel: turbostat 180 Sec. 139.93 Sec. 49.09 Watts.
>> Phoronix Postmark (Disk Test): turbostat 200 Sec. 5813 T.P.S. Ave. 21.33 Watts.
>>
>> Kernel 4.5-rc3 + RJW 3 patch set version 7:
>> Phoronix ffmpeg: turbostat 180 Sec. 11.67 Sec. Ave. 27.35 Watts.
>> Phoronix apache: turbostat 200 Sec. 19430.7 R.P.S. Ave. 34.18 Watts.
>> Phoronix kernel: turbostat 180 Sec. 139.81 Sec. 48.80 Watts.
>> Phoronix Postmark (Disk Test): turbostat 200 Sec. 5683 T.P.S. Ave. 22.41 Watts.

> Thanks for the results!
>
> The Postmark result is somewhat below expectations (especially with
> respect to the energy consumption), but we should be able to improve
> that by using the util numbers intelligently.
>
> Do you have full turbostat reports from those runs by any chance?  I'm
> wondering what happens to the idle state residencies, for example.

I did not keep the turbostat output, however it is easy enough to
re-do the tests. I'll send you the stuff off-list, and copy
Srinivas.

By the way, there is an anomaly in my 2 hour idle data (v7), where
CPU 7 should have had sample passes through the intel_pstate driver.
It did not, rather hitting the 4 second time limit instead.
10 occurrences in 7200 seconds. I sent you an off-list html format
e-mail with more details. There may be other anomalies I didn't
find yet.

... Doug

[toc] | [prev] | [next] | [standalone]

#1333205

On Fri, Feb 12, 2016 at 6:33 PM, Doug Smythies <dsmythies@telus.net> wrote:
> On 2016.02.12 05:39 Rafael J. Wysocki wrote:
>> On Fri, Feb 12, 2016 at 8:25 AM, Doug Smythies <dsmythies@telus.net> wrote:
>>> On 2016.02.11 14:50 Doug Smythies wrote:
>>>> On 2016.02.10 22:03 Srinivas Pandruvada wrote:
>>>>> On Wednesday, February 10, 2016 03:11:43 PM Doug Smythies wrote:
>>>
>>>>>> My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)
>>>> Thanks Doug. If you have specific workloads, please compare performance.
>>>
>>>> My work so far has been testing functionality, with unrealistic workloads specifically
>>>> designed to exaggerate issues, in this case the duration problem.
>>>>
>>>> I'll look at some real world workload scenarios.
>>>
>>> Turbostat used for package power, starts before Phoronix tests starts,
>>> and ends after Phoronix test ends.
>>>
>>> Control Sample: Kernel 4.5-rc3:
>>> Phoronix ffmpeg: turbostat 180 Sec. 12.07 Sec. Ave. 27.14 Watts.
>>> Phoronix apache: turbostat 200 Sec. 19797.0 R.P.S. Ave. 34.01 Watts.
>>> Phoronix kernel: turbostat 180 Sec. 139.93 Sec. 49.09 Watts.
>>> Phoronix Postmark (Disk Test): turbostat 200 Sec. 5813 T.P.S. Ave. 21.33 Watts.
>>>
>>> Kernel 4.5-rc3 + RJW 3 patch set version 7:
>>> Phoronix ffmpeg: turbostat 180 Sec. 11.67 Sec. Ave. 27.35 Watts.
>>> Phoronix apache: turbostat 200 Sec. 19430.7 R.P.S. Ave. 34.18 Watts.
>>> Phoronix kernel: turbostat 180 Sec. 139.81 Sec. 48.80 Watts.
>>> Phoronix Postmark (Disk Test): turbostat 200 Sec. 5683 T.P.S. Ave. 22.41 Watts.
>
>> Thanks for the results!
>>
>> The Postmark result is somewhat below expectations (especially with
>> respect to the energy consumption), but we should be able to improve
>> that by using the util numbers intelligently.
>>
>> Do you have full turbostat reports from those runs by any chance?  I'm
>> wondering what happens to the idle state residencies, for example.
>
> I did not keep the turbostat output, however it is easy enough to
> re-do the tests. I'll send you the stuff off-list, and copy
> Srinivas.

Thanks!

> By the way, there is an anomaly in my 2 hour idle data (v7), where
> CPU 7 should have had sample passes through the intel_pstate driver.
> It did not, rather hitting the 4 second time limit instead.

That most likely means that we had not scheduled anything on that CPU
for that time.  Not entirely unlikely if the system was generally
mostly idle.

The CPU activity you observed might be related to interrupts in which
case we wouldn't receive updates from the scheduler.

> 10 occurrences in 7200 seconds. I sent you an off-list html format
> e-mail with more details. There may be other anomalies I didn't
> find yet.

Well, I guess we'll see.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1331578

On 2016.02.10 07:17 Rafael J. Wysocki wrote:
> On Friday, January 29, 2016 11:52:15 PM Rafael J. Wysocki wrote:
>>
>> The following patch series introduces a mechanism allowing the cpufreq core
>> and "setpolicy" drivers to provide utilization update callbacks to be invoked
>> by the scheduler on utilization changes.  Those callbacks can be used to run
>> the sampling and frequency adjustments code (intel_pstate) or to schedule the
>> execution of that code in process context (cpufreq core) instead of per-CPU
>> deferrable timers used in cpufreq today (which Thomas complained about during
>> the last Kernel Summit).

This patch set solves a long standing issue with the intel_pstate driver.
The issue began with the introduction of the "duration" method for deciding
if the CPU had been idle for a long time resulting in forcing the
target pstate downwards. Often this was the correct action, but sometimes this
was the wrong thing to do, because the cpu was actually very busy, but just so
happened to be idle on jiffy boundaries (perhaps similar to what Steve Muckle
was referring to on another branch of this thread).

For an idle system, this patch set seems to change the maximum duration from
4 seconds to 0.5 seconds for most CPUs. However, when using v1 of patches 1
and 2 of 3 and v5 of 3 of 3, sometimes the durations (time between passes of
the intel-pstate driver for a given CPU) of upwards of 120 seconds were observed.
When patches 1, 2, and 3 of 3 v6 were used, the maximum observed durations of an
idle system were on the order of 500 milliseconds for most CPUs, but CPU 6
sometimes went to 3.5 seconds and CPU 7 sometimes went to 4 seconds (small
sample space, I'll consider to run an overnight test for a much much larger
sample space). Note 4 seconds, is O.K., and what it was before, I'm just noting
it is all.

I have a bunch of graphs, if anyone wants to see the supporting data.

My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)

[toc] | [prev] | [next] | [standalone]

#1331704 — Re: [PATCH v6 0/3] cpufreq: Replace timers with utilization update callbacks

On 02/10/2016 03:11 PM, Doug Smythies wrote:
> On 2016.02.10 07:17 Rafael J. Wysocki wrote:
>> On Friday, January 29, 2016 11:52:15 PM Rafael J. Wysocki wrote:
>>> The following patch series introduces a mechanism allowing the cpufreq core
>>> and "setpolicy" drivers to provide utilization update callbacks to be invoked
>>> by the scheduler on utilization changes.  Those callbacks can be used to run
>>> the sampling and frequency adjustments code (intel_pstate) or to schedule the
>>> execution of that code in process context (cpufreq core) instead of per-CPU
>>> deferrable timers used in cpufreq today (which Thomas complained about during
>>> the last Kernel Summit).
> This patch set solves a long standing issue with the intel_pstate driver.
> The issue began with the introduction of the "duration" method for deciding
> if the CPU had been idle for a long time resulting in forcing the
> target pstate downwards. Often this was the correct action, but sometimes this
> was the wrong thing to do, because the cpu was actually very busy, but just so
> happened to be idle on jiffy boundaries (perhaps similar to what Steve Muckle
> was referring to on another branch of this thread).
>
> For an idle system, this patch set seems to change the maximum duration from
> 4 seconds to 0.5 seconds for most CPUs. However, when using v1 of patches 1
> and 2 of 3 and v5 of 3 of 3, sometimes the durations (time between passes of
> the intel-pstate driver for a given CPU) of upwards of 120 seconds were observed.
> When patches 1, 2, and 3 of 3 v6 were used, the maximum observed durations of an
> idle system were on the order of 500 milliseconds for most CPUs, but CPU 6
> sometimes went to 3.5 seconds and CPU 7 sometimes went to 4 seconds (small
> sample space, I'll consider to run an overnight test for a much much larger
> sample space). Note 4 seconds, is O.K., and what it was before, I'm just noting
> it is all.
>
> I have a bunch of graphs, if anyone wants to see the supporting data.
>
> My test computer has an older model i7 (Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)
Thanks Doug. If you have specific workloads, please compare performance.

- Srinivas
>
>
> --
> 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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web