Re: [PATCH] cpufreq: governor: Don't use 'timer' keyword

[Rafael J. Wysocki]

On Tuesday, November 08, 2016 11:06:33 AM Viresh Kumar wrote:
> The earlier implementation of governors used background timers and so
> functions, mutex, etc had 'timer' keyword in their names.
> 
> But that's not true anymore. Replace 'timer' with 'update', as those
> functions, variables are based around updates to frequency.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  drivers/cpufreq/cpufreq_conservative.c |  4 ++--
>  drivers/cpufreq/cpufreq_governor.c     | 18 +++++++++---------
>  drivers/cpufreq/cpufreq_governor.h     |  4 ++--
>  drivers/cpufreq/cpufreq_ondemand.c     | 15 +++++++--------
>  4 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 13475890d792..fa5ece3915a1 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -58,7 +58,7 @@ static inline unsigned int get_freq_target(struct cs_dbs_tuners *cs_tuners,
>   * Any frequency increase takes it to the maximum frequency. Frequency reduction
>   * happens at minimum steps of 5% (default) of maximum frequency
>   */
> -static unsigned int cs_dbs_timer(struct cpufreq_policy *policy)
> +static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  {
>  	struct policy_dbs_info *policy_dbs = policy->governor_data;
>  	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
> @@ -305,7 +305,7 @@ static void cs_start(struct cpufreq_policy *policy)
>  static struct dbs_governor cs_governor = {
>  	.gov = CPUFREQ_DBS_GOVERNOR_INITIALIZER("conservative"),
>  	.kobj_type = { .default_attrs = cs_attributes },
> -	.gov_dbs_timer = cs_dbs_timer,
> +	.gov_dbs_update = cs_dbs_update,
>  	.alloc = cs_alloc,
>  	.free = cs_free,
>  	.init = cs_init,
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 642dd0f183a8..372947416b75 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -61,7 +61,7 @@ ssize_t store_sampling_rate(struct gov_attr_set *attr_set, const char *buf,
>  	 * entries can't be freed concurrently.
>  	 */
>  	list_for_each_entry(policy_dbs, &attr_set->policy_list, list) {
> -		mutex_lock(&policy_dbs->timer_mutex);
> +		mutex_lock(&policy_dbs->update_mutex);
>  		/*
>  		 * On 32-bit architectures this may race with the
>  		 * sample_delay_ns read in dbs_update_util_handler(), but that
> @@ -76,7 +76,7 @@ ssize_t store_sampling_rate(struct gov_attr_set *attr_set, const char *buf,
>  		 * taken, so it shouldn't be significant.
>  		 */
>  		gov_update_sample_delay(policy_dbs, 0);
> -		mutex_unlock(&policy_dbs->timer_mutex);
> +		mutex_unlock(&policy_dbs->update_mutex);
>  	}
>  
>  	return count;
> @@ -236,9 +236,9 @@ static void dbs_work_handler(struct work_struct *work)
>  	 * Make sure cpufreq_governor_limits() isn't evaluating load or the
>  	 * ondemand governor isn't updating the sampling rate in parallel.
>  	 */
> -	mutex_lock(&policy_dbs->timer_mutex);
> -	gov_update_sample_delay(policy_dbs, gov->gov_dbs_timer(policy));
> -	mutex_unlock(&policy_dbs->timer_mutex);
> +	mutex_lock(&policy_dbs->update_mutex);
> +	gov_update_sample_delay(policy_dbs, gov->gov_dbs_update(policy));
> +	mutex_unlock(&policy_dbs->update_mutex);
>  
>  	/* Allow the utilization update handler to queue up more work. */
>  	atomic_set(&policy_dbs->work_count, 0);
> @@ -348,7 +348,7 @@ static struct policy_dbs_info *alloc_policy_dbs_info(struct cpufreq_policy *poli
>  		return NULL;
>  
>  	policy_dbs->policy = policy;
> -	mutex_init(&policy_dbs->timer_mutex);
> +	mutex_init(&policy_dbs->update_mutex);
>  	atomic_set(&policy_dbs->work_count, 0);
>  	init_irq_work(&policy_dbs->irq_work, dbs_irq_work);
>  	INIT_WORK(&policy_dbs->work, dbs_work_handler);
> @@ -367,7 +367,7 @@ static void free_policy_dbs_info(struct policy_dbs_info *policy_dbs,
>  {
>  	int j;
>  
> -	mutex_destroy(&policy_dbs->timer_mutex);
> +	mutex_destroy(&policy_dbs->update_mutex);
>  
>  	for_each_cpu(j, policy_dbs->policy->related_cpus) {
>  		struct cpu_dbs_info *j_cdbs = &per_cpu(cpu_dbs, j);
> @@ -547,10 +547,10 @@ void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
>  {
>  	struct policy_dbs_info *policy_dbs = policy->governor_data;
>  
> -	mutex_lock(&policy_dbs->timer_mutex);
> +	mutex_lock(&policy_dbs->update_mutex);
>  	cpufreq_policy_apply_limits(policy);
>  	gov_update_sample_delay(policy_dbs, 0);
>  
> -	mutex_unlock(&policy_dbs->timer_mutex);
> +	mutex_unlock(&policy_dbs->update_mutex);
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_limits);
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index ef1037e9c92b..9660cc6b4b39 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -85,7 +85,7 @@ struct policy_dbs_info {
>  	 * Per policy mutex that serializes load evaluation from limit-change
>  	 * and work-handler.
>  	 */
> -	struct mutex timer_mutex;
> +	struct mutex update_mutex;
>  
>  	u64 last_sample_time;
>  	s64 sample_delay_ns;
> @@ -135,7 +135,7 @@ struct dbs_governor {
>  	 */
>  	struct dbs_data *gdbs_data;
>  
> -	unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
> +	unsigned int (*gov_dbs_update)(struct cpufreq_policy *policy);
>  	struct policy_dbs_info *(*alloc)(void);
>  	void (*free)(struct policy_dbs_info *policy_dbs);
>  	int (*init)(struct dbs_data *dbs_data);
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 3a1f49f5f4c6..1e2bd9851fba 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -169,7 +169,7 @@ static void od_update(struct cpufreq_policy *policy)
>  	}
>  }
>  
> -static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
> +static unsigned int od_dbs_update(struct cpufreq_policy *policy)
>  {
>  	struct policy_dbs_info *policy_dbs = policy->governor_data;
>  	struct dbs_data *dbs_data = policy_dbs->dbs_data;
> @@ -191,7 +191,7 @@ static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
>  	od_update(policy);
>  
>  	if (dbs_info->freq_lo) {
> -		/* Setup timer for SUB_SAMPLE */
> +		/* Setup SUB_SAMPLE */
>  		dbs_info->sample_type = OD_SUB_SAMPLE;
>  		return dbs_info->freq_hi_delay_us;
>  	}
> @@ -255,11 +255,11 @@ static ssize_t store_sampling_down_factor(struct gov_attr_set *attr_set,
>  	list_for_each_entry(policy_dbs, &attr_set->policy_list, list) {
>  		/*
>  		 * Doing this without locking might lead to using different
> -		 * rate_mult values in od_update() and od_dbs_timer().
> +		 * rate_mult values in od_update() and od_dbs_update().
>  		 */
> -		mutex_lock(&policy_dbs->timer_mutex);
> +		mutex_lock(&policy_dbs->update_mutex);
>  		policy_dbs->rate_mult = 1;
> -		mutex_unlock(&policy_dbs->timer_mutex);
> +		mutex_unlock(&policy_dbs->update_mutex);
>  	}
>  
>  	return count;
> @@ -374,8 +374,7 @@ static int od_init(struct dbs_data *dbs_data)
>  		dbs_data->up_threshold = MICRO_FREQUENCY_UP_THRESHOLD;
>  		/*
>  		 * In nohz/micro accounting case we set the minimum frequency
> -		 * not depending on HZ, but fixed (very low). The deferred
> -		 * timer might skip some samples if idle/sleeping as needed.
> +		 * not depending on HZ, but fixed (very low).
>  		*/
>  		dbs_data->min_sampling_rate = MICRO_FREQUENCY_MIN_SAMPLE_RATE;
>  	} else {
> @@ -415,7 +414,7 @@ static struct od_ops od_ops = {
>  static struct dbs_governor od_dbs_gov = {
>  	.gov = CPUFREQ_DBS_GOVERNOR_INITIALIZER("ondemand"),
>  	.kobj_type = { .default_attrs = od_attributes },
> -	.gov_dbs_timer = od_dbs_timer,
> +	.gov_dbs_update = od_dbs_update,
>  	.alloc = od_alloc,
>  	.free = od_free,
>  	.init = od_init,
> 

Applied.

Thanks,
Rafael