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

From: Yadwinder Singh Brar
Date: Fri Nov 07 2014 - 06:33:22 EST

Next message: Andy Yan: "[PATCH V4 3/6] dw-hdmi: make checkpatch happy"
Previous message: Andy Yan: "[PATCH V4 2/6] dw-hdmi: move imx-hdmi to bridge/dw-hdmi"
In reply to: Eduardo Valentin: "Re: [PATCH] thermal: cpu_cooling: Update always cpufreq policy with thermal constraints"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello Eduardo Valentin,

On Thursday, November 06, 2014 1:19 PM, Eduardo Valentin wrote:
> 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?
>

Sure, will post updated patch.

> > > >
> > > > 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?
>

Yes, Userspace is not able to set scaling_max_freq more than the
cooling device constraint(cpufreq_val).

But I have a question here, Is it OK to silently set scaling_max_freq
less than the requested value from userspace ?

> > >
> > > 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?
>

Exactly !! though in existing kernel we don't have multiple devices yet.

> 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.
>

yes ..

> 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?
>

Sure.

Best Regards,
Yadwinder

>
> 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
> > > >
> >

--
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/

Next message: Andy Yan: "[PATCH V4 3/6] dw-hdmi: make checkpatch happy"
Previous message: Andy Yan: "[PATCH V4 2/6] dw-hdmi: move imx-hdmi to bridge/dw-hdmi"
In reply to: Eduardo Valentin: "Re: [PATCH] thermal: cpu_cooling: Update always cpufreq policy with thermal constraints"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]