Re: [PATCH] thermal: cpu_cooling: Update always cpufreq policy with thermal constraints

[Eduardo Valentin]

Hello Yadwinder,

On Thu, Nov 06, 2014 at 04:26:27PM +0530, Yadwinder Singh Brar wrote:
> Hello Eduardo Valentin,
> 
> On Thursday, November 06, 2014 2:17 AM, Eduardo Valentin wrote:
> > Hello Yadwinder,
> > 
> > On Wed, Nov 05, 2014 at 05:46:25PM +0530, Yadwinder Singh Brar wrote:
> > > Existing code updates cupfreq policy only while executing
> > > cpufreq_apply_cooling() function (i.e. when notify_device !=
> > NOTIFY_INVALID).
> > 
> > Correct. The case you mention is when we receive a notification from
> > cpufreq. But also, updates the cpufreq policy when the cooling device
> > changes state, a call from thermal framework.
> 
> I mentioned thermal framework case only, existing code updates
> cupfreq policy only when (notify_device != NOTIFY_INVALID),
> which happens only when notification comes from cpufreq_update_policy
> while changing cooling device's state(i.e. cpufreq_apply_cooling()).
> In case of other notifications notify_device is always NOTIFY_INVALID.

I see your point.

> 
> > 
> > > It doesn't apply constraints when cpufreq policy update happens from
> > > any other place but it should update the cpufreq policy with thermal
> > > constraints every time when there is a cpufreq policy update, to keep
> > > state of cpufreq_cooling_device and max_feq of cpufreq policy in
> > sync.
> > 
> > I am not sure I follow you here. Can you please elaborate on 'any other
> > places'? Could you please mention (also in the commit log) what are the
> > case the current code does not cover? For instance, the
> > cpufreq_apply_cooling gets called on notification coming from thermal
> > subsystem, and for changes in cpufreq subsystem,
> > cpufreq_thermal_notifier is called.
> > 
> 
> "Other places" mean possible places from where cpufreq_update_policy()
> can be called at runtime, an instance in present kernel is cpufreq_resume().
> But since cpufreq_update_policy() is an exposed API, I think
> for robustness, generic cpu cooling should be able to take care of any
> possible case(in future as well).
> 

OK. I understand now. Can you please improve your commit message by
adding the descriptions you mentioned here?

> > >
> > > This patch modifies code to maintain a global cpufreq_dev_list and
> > get
> > > corresponding cpufreq_cooling_device for policy's cpu from
> > > cpufreq_dev_list when there is any policy update.
> > 
> > OK. Please give real examples of when the current code fails to catch
> > the event.
> > 
> 
> While resuming cpufreq updates cpufreq_policy for boot cpu and it
> restores default policy->usr_policy irrespective of cooling device's
> cpufreq_state since notification gets missed because (notify_device ==
> NOTIFY_INVALID).
> Another thing, Rather I would say an issue, I observed is that
> Userspace is able to change max_freq irrespective of cooling
> device's state, as notification gets missed.
>

Include also the examples above you gave.

Have you verified if with this patch the issue with userland goes away?
   
> > 
> > BR,
> > 
> > Eduardo Valentin
> > 
> > >
> > > Signed-off-by: Yadwinder Singh Brar <yadi.brar@xxxxxxxxxxx>
> > > ---
> > >  drivers/thermal/cpu_cooling.c |   37 ++++++++++++++++++++-----------
> > ------
> > >  1 files changed, 20 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/thermal/cpu_cooling.c
> > > b/drivers/thermal/cpu_cooling.c index 1ab0018..5546278 100644
> > > --- a/drivers/thermal/cpu_cooling.c
> > > +++ b/drivers/thermal/cpu_cooling.c
> > > @@ -50,15 +50,14 @@ struct cpufreq_cooling_device {
> > >  	unsigned int cpufreq_state;
> > >  	unsigned int cpufreq_val;
> > >  	struct cpumask allowed_cpus;
> > > +	struct list_head node;
> > >  };
> > >  static DEFINE_IDR(cpufreq_idr);
> > >  static DEFINE_MUTEX(cooling_cpufreq_lock);
> > >
> > >  static unsigned int cpufreq_dev_count;
> > >
> > > -/* notify_table passes value to the CPUFREQ_ADJUST callback
> > function.
> > > */ -#define NOTIFY_INVALID NULL -static struct cpufreq_cooling_device
> > > *notify_device;
> > > +static LIST_HEAD(cpufreq_dev_list);
> > >
> > >  /**
> > >   * get_idr - function to get a unique id.
> > > @@ -287,15 +286,12 @@ static int cpufreq_apply_cooling(struct
> > > cpufreq_cooling_device *cpufreq_device,
> > >
> > >  	cpufreq_device->cpufreq_state = cooling_state;
> > >  	cpufreq_device->cpufreq_val = clip_freq;
> > > -	notify_device = cpufreq_device;
> > >
> > >  	for_each_cpu(cpuid, mask) {
> > >  		if (is_cpufreq_valid(cpuid))
> > >  			cpufreq_update_policy(cpuid);
> > >  	}
> > >
> > > -	notify_device = NOTIFY_INVALID;
> > > -
> > >  	return 0;
> > >  }
> > >
> > > @@ -316,21 +312,27 @@ static int cpufreq_thermal_notifier(struct
> > > notifier_block *nb,  {
> > >  	struct cpufreq_policy *policy = data;
> > >  	unsigned long max_freq = 0;
> > > +	struct cpufreq_cooling_device *cpufreq_dev;
> > >
> > > -	if (event != CPUFREQ_ADJUST || notify_device == NOTIFY_INVALID)
> > > +	if (event != CPUFREQ_ADJUST)
> > >  		return 0;
> > >
> > > -	if (cpumask_test_cpu(policy->cpu, &notify_device->allowed_cpus))
> > > -		max_freq = notify_device->cpufreq_val;
> > > -	else
> > > -		return 0;
> > > +	mutex_lock(&cooling_cpufreq_lock);
> > > +	list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, node) {
> > > +		if (!cpumask_test_cpu(policy->cpu,
> > > +					&cpufreq_dev->allowed_cpus))
> > > +			continue;
> > >
> > > -	/* Never exceed user_policy.max */
> > > -	if (max_freq > policy->user_policy.max)
> > > -		max_freq = policy->user_policy.max;
> > > +		max_freq = cpufreq_dev->cpufreq_val;
> > >
> 
> I think I missed to post updated patch,
> Actually it should be :
> 
> +	if (!cpufreq_dev->cpufreq_val)
> +		cpufreq_dev->cpufreq_val = get_cpu_frequency(
> +
> cpumask_any(&cpufreq_dev->allowed_cpus),
> +							cpufreq_dev->state);
> +	max_freq = cpufreq_dev->cpufreq_val;
> 
> I will send another version of patch.

ok.

> 
> > > -	if (policy->max != max_freq)
> > > -		cpufreq_verify_within_limits(policy, 0, max_freq);
> > > +		/* Never exceed user_policy.max */
> > > +		if (max_freq > policy->user_policy.max)
> > > +			max_freq = policy->user_policy.max;
> > > +
> > > +		if (policy->max != max_freq)
> > > +			cpufreq_verify_within_limits(policy, 0, max_freq);
> > > +	}
> > > +	mutex_unlock(&cooling_cpufreq_lock);
> > 
> > So, the problem is when we have several cpu cooling devices and you
> > want to search for the real max among the existing cpu cooling devices?
> >

By max, I meant the maximun constraint (lowest frequency).

> 
> Sorry I didn't get your question completely I think.
> No, above code doesn't find max among existing cooling devices.
> It simply finds cooling device corresponding to policy's cpu
> and applies updates policy accordingly.

yeah, that's what it does, but for all matching devices, correct?

well, in fact your code is going through all cpu cooling devices that
match the cpu ids and applying their max allowed freq, when they differ
from policy->max. cpufreq_verify_within_limits will update the policy if
your request is lower than policy max. Then you will, in the end, apply
the lowest among the existing matching cpu cooling devices.

Which, turns out to be the correct thing to do, since we are not doing
it per request in single cooling devices.

Can you please also add a comment about this strategy?


BR, 

Eduardo Valentin

> 
> Regards,
> Yadwinder
>  
> > >  	return 0;
> > >  }
> > > @@ -486,7 +488,7 @@ __cpufreq_cooling_register(struct device_node
> > *np,
> > >  		cpufreq_register_notifier(&thermal_cpufreq_notifier_block,
> > >  					  CPUFREQ_POLICY_NOTIFIER);
> > >  	cpufreq_dev_count++;
> > > -
> > > +	list_add(&cpufreq_dev->node, &cpufreq_dev_list);
> > >  	mutex_unlock(&cooling_cpufreq_lock);
> > >
> > >  	return cool_dev;
> > > @@ -549,6 +551,7 @@ void cpufreq_cooling_unregister(struct
> > > thermal_cooling_device *cdev)
> > >
> > >  	cpufreq_dev = cdev->devdata;
> > >  	mutex_lock(&cooling_cpufreq_lock);
> > > +	list_del(&cpufreq_dev->node);
> > >  	cpufreq_dev_count--;
> > >
> > >  	/* Unregister the notifier for the last cpufreq cooling device */
> > > --
> > > 1.7.0.4
> > >
>

Attachment: signature.asc
Description: Digital signature