Re: [PATCH 4/4] cpufreq: governor: Implement per policy instancesof governors

From: Francesco Lavra
Date: Sun Feb 10 2013 - 16:13:36 EST


Hi,

On 02/04/2013 12:38 PM, Viresh Kumar wrote:
> Currently, there can't be multiple instances of single governor_type. If we have
> a multi-package system, where we have multiple instances of struct policy (per
> package), we can't have multiple instances of same governor. i.e. We can't have
> multiple instances of ondemand governor for multiple packages.
>
> Governors directory in sysfs is created at /sys/devices/system/cpu/cpufreq/
> governor-name/. Which again reflects that there can be only one instance of a
> governor_type in the system.
>
> This is a bottleneck for multicluster system, where we want different packages
> to use same governor type, but with different tunables.
>
> This patch uses the infrastructure provided by earlier patch and implements
> init/exit routines for ondemand and conservative governors.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> drivers/cpufreq/cpufreq.c | 4 -
> drivers/cpufreq/cpufreq_conservative.c | 142 +++++++++++++----------
> drivers/cpufreq/cpufreq_governor.c | 138 +++++++++++++---------
> drivers/cpufreq/cpufreq_governor.h | 42 ++++---
> drivers/cpufreq/cpufreq_ondemand.c | 205 +++++++++++++++++++--------------
> include/linux/cpufreq.h | 19 +--
> 6 files changed, 314 insertions(+), 236 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1ae78d4..7aacfbf 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1551,9 +1551,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> policy->cpu, event);
> ret = policy->governor->governor(policy, event);
>
> - if (!policy->governor->initialized && (event == CPUFREQ_GOV_START))
> - policy->governor->initialized = 1;
> -
> /* we keep one module reference alive for
> each CPU governed by this CPU */
> if ((event != CPUFREQ_GOV_START) || ret)
> @@ -1577,7 +1574,6 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)
>
> mutex_lock(&cpufreq_governor_mutex);
>
> - governor->initialized = 0;
> err = -EBUSY;
> if (__find_governor(governor->name) == NULL) {
> err = 0;
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
[...]
> +static int cs_init(struct dbs_data *dbs_data)
> +{
> + struct cs_dbs_tuners *tuners;
> +
> + tuners = kzalloc(sizeof(struct cs_dbs_tuners), GFP_KERNEL);
> + if (!tuners) {
> + pr_err("%s: kzalloc failed\n", __func__);
> + return -ENOMEM;
> + }
> +
> + tuners->up_threshold = DEF_FREQUENCY_UP_THRESHOLD;
> + tuners->down_threshold = DEF_FREQUENCY_DOWN_THRESHOLD;
> + tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
> + tuners->ignore_nice = 0;
> + tuners->freq_step = 5;
> +
> + dbs_data->tuners = tuners;

dbs_data->tuners is never freed, which means there is a memory leak
across CPUFREQ_GOV_POLICY_INIT and CPUFREQ_GOV_POLICY_EXIT events.

The same goes for the ondemand governor.

Regards,
Francesco
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/