diff mbox

[1/3] cpufreq: implement min/max/up/down functions

Message ID 20170202174921.814002151@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marcelo Tosatti Feb. 2, 2017, 5:47 p.m. UTC
Implement functions in cpufreq userspace code to:
	* Change current frequency to {max,min,up,down} frequencies.

up/down being relative to current one.

These will be used to implement KVM hypercalls for the guest
to issue frequency changes.

Current situation with DPDK and frequency changes is as follows:
An algorithm in the guest decides when to increase/decrease
frequency based on the queue length of the device. 

On the host, a power manager daemon is used to listen for 
frequency change requests (on another core) and issue these 
requests.

However frequency changes are performance sensitive events because:
On a change from low load condition to max load condition,
the frequency should be raised as soon as possible.
Sending a virtio-serial notification to another pCPU, 
waiting for that pCPU to initiate an IPI to the requestor pCPU
to change frequency, is slower and more cache costly than 
a direct hypercall to host to switch the frequency.

Moreover, if the pCPU where the power manager daemon is running 
is not busy spinning on requests from the isolated DPDK vcpus,
there is also the cost of HLT wakeup for that pCPU.

Instructions to setup:
Disable the intel_pstate driver (intel_pstate=disable host kernel
command line option), and set cpufreq userspace governor for
the isolated pCPU.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
 drivers/cpufreq/cpufreq_userspace.c |  172 ++++++++++++++++++++++++++++++++++++
 include/linux/cpufreq.h             |    7 +
 2 files changed, 179 insertions(+)

Comments

Viresh Kumar Feb. 3, 2017, 4:09 a.m. UTC | #1
On 02-02-17, 15:47, Marcelo Tosatti wrote:
> +++ kvm-pvfreq/drivers/cpufreq/cpufreq_userspace.c	2017-02-02 15:32:53.456262640 -0200
> @@ -118,6 +118,178 @@
>  	mutex_unlock(&userspace_mutex);
>  }
>  
> +static int cpufreq_is_userspace_governor(int cpu)
> +{
> +	int ret;
> +
> +	mutex_lock(&userspace_mutex);
> +	ret = per_cpu(cpu_is_managed, cpu);

The userspace governor is buggy in the sense that cpu_is_managed is only updated
for the policy->cpu and not any other CPU in that policy. But then it was never
used with anything other than policy->cpu, so it was fine.

But now that you are allowing any CPU number here, you need to do one of these:
- Either set cpu_is_managed for all the CPUs from a policy
- Or get the policy first and pass policy->cpu here.

> +	mutex_unlock(&userspace_mutex);
> +
> +	return ret;
> +}

All 4 routines defined below have too much in common and it would be very easy
to write a common routine cpufreq_userspace_freq_change(), which can be called
in all the four cases. You can pass a function pointer to that, which can give
min, max, up, or down frequencies. That will make it more robust and less error
prone.

> +int cpufreq_userspace_freq_up(int cpu)
> +{
> +	unsigned int curfreq, nextminfreq;
> +	unsigned int ret = 0;
> +	struct cpufreq_frequency_table *pos, *table;
> +	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +
> +	if (!policy)
> +		return -EINVAL;
> +
> +	if (!cpufreq_is_userspace_governor(cpu)) {
> +		cpufreq_cpu_put(policy);
> +		return -EINVAL;
> +	}

Because the userspace_mutex is dropped after that routine returned, there is no
guarantee that 'cpu' is still managed by this governor. And so you need to make
sure that you drops the locks only at the end.

> +
> +	cpufreq_cpu_put(policy);

This must be called only after you are done using the policy, to make sure that
the policy doesn't get freed while you are using it.

> +	mutex_lock(&userspace_mutex);
> +	table = policy->freq_table;
> +	if (!table) {
> +		mutex_unlock(&userspace_mutex);
> +		return -ENODEV;
> +	}
> +	nextminfreq = cpufreq_quick_get_max(cpu);

Just use policy->max here, why waste time ?

> +	curfreq = policy->cur;
> +
> +	cpufreq_for_each_valid_entry(pos, table) {
> +		if (pos->frequency > curfreq &&
> +		    pos->frequency < nextminfreq)
> +			nextminfreq = pos->frequency;
> +	}

The above part can be a routine of its own, whose pointer will be passed to
cpufreq_userspace_freq_change().

> +
> +	if (nextminfreq != curfreq) {

You are missing similar checks in the last two routines, any special reason for
that ?

> +		unsigned int *setspeed = policy->governor_data;
> +
> +		*setspeed = nextminfreq;
> +		ret = __cpufreq_driver_target(policy, nextminfreq,
> +					      CPUFREQ_RELATION_L);
> +	} else
> +		ret = 1;

Why ret 1? What are the callers expected to do on seeing this value? Maybe
return 0 as the desired freq is set by the governor ?

And always use {} for even single line code if the 'if' block has them.

> +	mutex_unlock(&userspace_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(cpufreq_userspace_freq_up);
> +++ kvm-pvfreq/include/linux/cpufreq.h	2017-01-31 14:20:00.508613672 -0200
> @@ -890,4 +890,11 @@
>  int cpufreq_generic_init(struct cpufreq_policy *policy,
>  		struct cpufreq_frequency_table *table,
>  		unsigned int transition_latency);
> +#ifdef CONFIG_CPU_FREQ
> +int cpufreq_userspace_freq_down(int cpu);
> +int cpufreq_userspace_freq_up(int cpu);
> +int cpufreq_userspace_freq_max(int cpu);
> +int cpufreq_userspace_freq_min(int cpu);
> +#else

Don't want to put dummy routines here? Then why the blank #else part ?

> +#endif
>  #endif /* _LINUX_CPUFREQ_H */
>
diff mbox

Patch

Index: kvm-pvfreq/drivers/cpufreq/cpufreq_userspace.c
===================================================================
--- kvm-pvfreq.orig/drivers/cpufreq/cpufreq_userspace.c	2017-01-31 10:41:54.102575877 -0200
+++ kvm-pvfreq/drivers/cpufreq/cpufreq_userspace.c	2017-02-02 15:32:53.456262640 -0200
@@ -118,6 +118,178 @@ 
 	mutex_unlock(&userspace_mutex);
 }
 
+static int cpufreq_is_userspace_governor(int cpu)
+{
+	int ret;
+
+	mutex_lock(&userspace_mutex);
+	ret = per_cpu(cpu_is_managed, cpu);
+	mutex_unlock(&userspace_mutex);
+
+	return ret;
+}
+
+int cpufreq_userspace_freq_up(int cpu)
+{
+	unsigned int curfreq, nextminfreq;
+	unsigned int ret = 0;
+	struct cpufreq_frequency_table *pos, *table;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+
+	if (!policy)
+		return -EINVAL;
+
+	if (!cpufreq_is_userspace_governor(cpu)) {
+		cpufreq_cpu_put(policy);
+		return -EINVAL;
+	}
+
+	cpufreq_cpu_put(policy);
+
+	mutex_lock(&userspace_mutex);
+	table = policy->freq_table;
+	if (!table) {
+		mutex_unlock(&userspace_mutex);
+		return -ENODEV;
+	}
+	nextminfreq = cpufreq_quick_get_max(cpu);
+	curfreq = policy->cur;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		if (pos->frequency > curfreq &&
+		    pos->frequency < nextminfreq)
+			nextminfreq = pos->frequency;
+	}
+
+	if (nextminfreq != curfreq) {
+		unsigned int *setspeed = policy->governor_data;
+
+		*setspeed = nextminfreq;
+		ret = __cpufreq_driver_target(policy, nextminfreq,
+					      CPUFREQ_RELATION_L);
+	} else
+		ret = 1;
+	mutex_unlock(&userspace_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cpufreq_userspace_freq_up);
+
+int cpufreq_userspace_freq_down(int cpu)
+{
+	unsigned int curfreq, prevmaxfreq;
+	unsigned int ret = 0;
+	struct cpufreq_frequency_table *pos, *table;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+
+	if (!policy)
+		return -EINVAL;
+
+	if (!cpufreq_is_userspace_governor(cpu)) {
+		cpufreq_cpu_put(policy);
+		return -EINVAL;
+	}
+
+	cpufreq_cpu_put(policy);
+
+	mutex_lock(&userspace_mutex);
+	table = policy->freq_table;
+	if (!table) {
+		mutex_unlock(&userspace_mutex);
+		return -ENODEV;
+	}
+	prevmaxfreq = policy->min;
+	curfreq = policy->cur;
+
+	cpufreq_for_each_valid_entry(pos, table) {
+		if (pos->frequency < curfreq &&
+		    pos->frequency > prevmaxfreq)
+			prevmaxfreq = pos->frequency;
+	}
+
+	if (prevmaxfreq != curfreq) {
+		unsigned int *setspeed = policy->governor_data;
+
+		*setspeed = prevmaxfreq;
+		ret = __cpufreq_driver_target(policy, prevmaxfreq,
+					      CPUFREQ_RELATION_L);
+	} else
+		ret = 1;
+	mutex_unlock(&userspace_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cpufreq_userspace_freq_down);
+
+int cpufreq_userspace_freq_max(int cpu)
+{
+	unsigned int maxfreq;
+	unsigned int ret = 0;
+	struct cpufreq_frequency_table *table;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	unsigned int *setspeed = policy->governor_data;
+
+
+	if (!policy)
+		return -EINVAL;
+
+	if (!cpufreq_is_userspace_governor(cpu)) {
+		cpufreq_cpu_put(policy);
+		return -EINVAL;
+	}
+
+	cpufreq_cpu_put(policy);
+
+	mutex_lock(&userspace_mutex);
+	table = policy->freq_table;
+	if (!table) {
+		mutex_unlock(&userspace_mutex);
+		return -ENODEV;
+	}
+	maxfreq = cpufreq_quick_get_max(cpu);
+
+	*setspeed = maxfreq;
+	ret = __cpufreq_driver_target(policy, maxfreq, CPUFREQ_RELATION_L);
+	mutex_unlock(&userspace_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cpufreq_userspace_freq_max);
+
+int cpufreq_userspace_freq_min(int cpu)
+{
+	unsigned int minfreq;
+	unsigned int ret = 0;
+	struct cpufreq_frequency_table *table;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	unsigned int *setspeed = policy->governor_data;
+
+	if (!policy)
+		return -EINVAL;
+
+	if (!cpufreq_is_userspace_governor(cpu)) {
+		cpufreq_cpu_put(policy);
+		return -EINVAL;
+	}
+	minfreq = policy->min;
+
+	cpufreq_cpu_put(policy);
+
+	mutex_lock(&userspace_mutex);
+	table = policy->freq_table;
+	if (!table) {
+		mutex_unlock(&userspace_mutex);
+		return -ENODEV;
+	}
+
+	*setspeed = minfreq;
+	ret = __cpufreq_driver_target(policy, minfreq, CPUFREQ_RELATION_L);
+	mutex_unlock(&userspace_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cpufreq_userspace_freq_min);
+
 static struct cpufreq_governor cpufreq_gov_userspace = {
 	.name		= "userspace",
 	.init		= cpufreq_userspace_policy_init,
Index: kvm-pvfreq/include/linux/cpufreq.h
===================================================================
--- kvm-pvfreq.orig/include/linux/cpufreq.h	2017-01-31 10:41:54.102575877 -0200
+++ kvm-pvfreq/include/linux/cpufreq.h	2017-01-31 14:20:00.508613672 -0200
@@ -890,4 +890,11 @@ 
 int cpufreq_generic_init(struct cpufreq_policy *policy,
 		struct cpufreq_frequency_table *table,
 		unsigned int transition_latency);
+#ifdef CONFIG_CPU_FREQ
+int cpufreq_userspace_freq_down(int cpu);
+int cpufreq_userspace_freq_up(int cpu);
+int cpufreq_userspace_freq_max(int cpu);
+int cpufreq_userspace_freq_min(int cpu);
+#else
+#endif
 #endif /* _LINUX_CPUFREQ_H */