From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752517AbdBCEJZ (ORCPT ); Thu, 2 Feb 2017 23:09:25 -0500 Received: from mail-pf0-f178.google.com ([209.85.192.178]:33165 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752009AbdBCEJX (ORCPT ); Thu, 2 Feb 2017 23:09:23 -0500 Date: Fri, 3 Feb 2017 09:39:18 +0530 From: Viresh Kumar To: Marcelo Tosatti Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Paolo Bonzini , Radim Krcmar , "Rafael J. Wysocki" Subject: Re: [patch 1/3] cpufreq: implement min/max/up/down functions Message-ID: <20170203040918.GJ7458@vireshk-i7> References: <20170202174755.946578704@redhat.com> <20170202174921.814002151@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170202174921.814002151@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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