Re: [PATCH 06/12] cpufreq: governor: Keep single copy of information common to policy->cpus

[Viresh Kumar <viresh.kumar@linaro.org>]

On 15-06-15, 11:45, Preeti U Murthy wrote:
> On 06/11/2015 04:21 PM, Viresh Kumar wrote:
> > @@ -320,6 +363,7 @@ static void cpufreq_governor_exit(struct cpufreq_policy *policy,
> >  		}
> > 
> >  		cdata->exit(dbs_data, policy->governor->initialized == 1);
> > +		free_ccdbs(policy, cdata);
> 
> This is a per-policy data structure, so why free it only when all the
> users of the governor are gone ? We should be freeing it when a policy
> is asked to exit, which is independent of references to this governor by
> other policy cpus. This would mean freeing it outside this if condition.

Right.

> > @@ -348,11 +393,13 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
> >  		io_busy = od_tuners->io_is_busy;
> >  	}
> > 
> > +	ccdbs->time_stamp = ktime_get();
> > +	mutex_init(&ccdbs->timer_mutex);
> > +
> >  	for_each_cpu(j, policy->cpus) {
> >  		struct cpu_dbs_info *j_cdbs = cdata->get_cpu_cdbs(j);
> >  		unsigned int prev_load;
> > 
> > -		j_cdbs->policy = policy;
> 
> This is not convincing. INIT and EXIT should be typically used to
> initiate and free 'governor' specific data structures. START and STOP
> should be used for 'policy wide/cpu wide' initialization and making
> NULL. Atleast thats how the current code appears to be designed.
> 
> Now, ccdbs is a policy wide data structure. We can perhaps allocate and
> free ccdbs during INIT and EXIT, but initiating the values and making
> them NULL must be done in START and STOP respectively. You initiate the
> time_stamp and timer_mutex in START, why not initialize policy also
> here? This will help maintain consistency in code too.

I don't think it was done to use it early and so moving it to
START/STOP should be fine.
 
> > @@ -406,10 +452,7 @@ static void cpufreq_governor_stop(struct cpufreq_policy *policy,
> >  		cs_dbs_info->enable = 0;
> >  	}
> > 
> > -	gov_cancel_work(dbs_data, policy);
> > -
> > -	mutex_destroy(&cdbs->timer_mutex);
> > -	cdbs->policy = NULL;
> 
> Same here. For the same reason as above, the value for policy must be
> nullified in STOP. Besides, policy is initiated a value explicitly in
> INIT, but invalidated in EXIT by freeing ccdbs in this patch. There is
> lack of consistency.
> 
> There is another consequence. Freeing a data structure does not
> necessarily mean setting it to NULL. It can be some random address. This
> will break places which check for NULL policy.

If we are freeing ccdbs, then using ccdbs->policy isn't valid anymore.
And so while freeing ccdbs, we don't really need to set its fields to
NULL.

> > +	mutex_destroy(&ccdbs->timer_mutex);
> 
> Another point which you may have taken care of in the subsequent
> patches. I will mention here nevertheless.
> 
> The timer_mutex is destroyed, but the cdbs->policy is not NULL until we
> call EXIT. So when cpufreq_governor_limits() is called, it checks for
> the existence of ccdbs, which succeeds. But when it tries to take the
> timer_mutex it dereferences NULL.

Hmm, so I should keep checking for cdbs->ccdbs->policy instead and
make it NULL in STOP..

Nice work Preeti. Thanks.

-- 
viresh