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

[Viresh Kumar]

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 */
> 

-- 
viresh