Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1195795 > unrolled thread

Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable Viresh Kumar <viresh.kumar@linaro.org> - 2015-07-30 10:10 +0200
    Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable Viresh Kumar <viresh.kumar@linaro.org> - 2015-07-31 05:20 +0200
      Re: [PATCH] thermal/cpu_cooling: remove local cooling state  variable Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> - 2015-07-31 17:40 +0200
        Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable Viresh Kumar <viresh.kumar@linaro.org> - 2015-08-01 13:40 +0200
          Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable Viresh Kumar <viresh.kumar@linaro.org> - 2015-08-03 05:20 +0200
            Re: [PATCH] thermal/cpu_cooling: remove local cooling state  variable Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> - 2015-08-03 21:40 +0200

#1195795 — Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable

Cc'ing Rafael as well..

On 29-07-15, 17:46, Punit Agrawal wrote:
> [ adding Viresh ]

Thanks. That earned me few more patches ;)

> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> 
> > Hi Agarwal,
> >
> > On Fri, 24 Jul 2015 16:26:12 +0100
> > Punit Agrawal <punit.agrawal@arm.com> wrote:
> >
> >> Radivoje Jovanovic <radivoje.jovanovic@linux.intel.com> writes:
> >> 
> >> > From: Radivoje Jovanovic <radivoje.jovanovic@intel.com>
> >> >
> >> > there is no need to keep local state variable. if another driver
> >> > changes the policy under our feet the cpu_cooling driver will
> >> > have the wrong state. Get current state from the policy directly
> >> > instead
> >> >
> >> 
> >> Although the patch below looks good, it does add additional
> >> processing. I was wondering in what situation do you observe the
> >> problem $SUBJECT solves?
> >> 
> >> Presumably, the policy caps are tighter than those imposed by the cpu
> >> cooling device (cpufreq_thermal_notifier should take care of this).
> >
> > we are using this solution on the platfrom which has user space
> > component control cpufreq throttling. However, user space 
> > component has its limitations so we are using cpu_cooling as a 
> > critical backup. Due to this cpu_cooling does not have correct state
> > as a current state so when the change is needed cpu_cooling does
> > not make the change since it believes it is in the "correct" state.
> > I agree that there is slight increase in processing, but in the case 
> > when user space is changing the policy the notifier will not have
> > access to the current state of the cpu_cooling to change it
> > appropriately.
> >
> 
> Makes sense. Thanks for the explanation.

Sorry, but with what I understood it doesn't make sense. And I can be wrong
here, so please don't laugh at me :)

So, we have two external suppliers to policy->max here:
- user space: which decides the maximum frequency the policy can ever achieve.
- thermal: which decides the maximum safe frequency the policy should ever be
  set to.

We need to set policy->max based on what user requested, but keeping in mind the
thermal limitations.

So if the clipped-freq from thermal is higher than what user has requested, we
don't need to do anything.

But if the clipped-freq is lower than what user has requested, then we need to
correct that to keep the system in safe range.

That's what the code is doing as well.

Now coming to the change you made. What you are saying is, we should report
current state based on the value of policy->max. But why?

policy->max can be lesser than clipped-freq (set by thermal), and the current
state of thermal clipped-freq isn't what policy->max gives.

Now, I didn't understood when you said "cpu_cooling doesn't change the state
since it believes it is in correct state". Can you please explain that with some
example?

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

[toc] | [next] | [standalone]

#1196561

Thanks.

I will try to add more layman terms here to map cooling state with
frequencies. So, the cooling state 0 maps to the highest frequency the
cpufreq table supports, and the highest cooling state n maps to the
lowest frequency. Right ?

On 30-07-15, 13:21, Radivoje Jovanovic wrote:
> In this case both userspace thermal solution and cpu_cooling are
> changing policy->max and the userspace solution will let governor or HW
> (depends on architecture) decide the clipped-freq. Now let us say that
> cpu_cooling has 4 available states 0-3

Lets say: 0 == 1.2 GHz
          1 == 1.1 GHz
          2 == 1 GHz
          3 == 800 MHz

> and let us say that cpu_cooling
> has set the state 1 as the last state.

i.e. cpu_cooling says "don't go over 1.1 GHz"..

> Now userspace component comes in
> and changes the state of the system that matches cpu_cooling state 0.

So, policy->max reaches 1.2 GHz and that is not in sync with
cpu_cooling. Right ?

> cpu_cooling is unaware of this change and does not change the local
> cur_state.

That's where I think you one of us might be incorrect. At this point
when policy->max is changed to 1.2 GHz, a notifier will get issued to
cpu_cooling, which will bring policy->max again to 1.1 GHz and so
things will be back in control.

> Now the temperature changes and cpu_cooling should change
> the system state to 1 (userspace component malfunctioned and is not
> picking up this change) but since the cur_state is already at 1
> cpu_cooling will not do anything since it believes it is in the correct
> state. Hope this explains it better

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

[toc] | [prev] | [next] | [standalone]

#1197130 — Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable

On Fri, 31 Jul 2015 08:48:41 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> Thanks.
> 
> I will try to add more layman terms here to map cooling state with
> frequencies. So, the cooling state 0 maps to the highest frequency the
> cpufreq table supports, and the highest cooling state n maps to the
> lowest frequency. Right ?
> 
> On 30-07-15, 13:21, Radivoje Jovanovic wrote:
> > In this case both userspace thermal solution and cpu_cooling are
> > changing policy->max and the userspace solution will let governor
> > or HW (depends on architecture) decide the clipped-freq. Now let us
> > say that cpu_cooling has 4 available states 0-3
> 
> Lets say: 0 == 1.2 GHz
>           1 == 1.1 GHz
>           2 == 1 GHz
>           3 == 800 MHz
> 
> > and let us say that cpu_cooling
> > has set the state 1 as the last state.
> 
> i.e. cpu_cooling says "don't go over 1.1 GHz"..
> 
> > Now userspace component comes in
> > and changes the state of the system that matches cpu_cooling state
> > 0.
> 
> So, policy->max reaches 1.2 GHz and that is not in sync with
> cpu_cooling. Right ?
> 
> > cpu_cooling is unaware of this change and does not change the local
> > cur_state.
> 
> That's where I think you one of us might be incorrect. At this point
> when policy->max is changed to 1.2 GHz, a notifier will get issued to
> cpu_cooling, which will bring policy->max again to 1.1 GHz and so
> things will be back in control.
I just looked over the notifier in the current upstream (my patch was
made on our production kernel which is 3.14 and has old notifier
implementation with notifier_device in place) and I see your point.
I agree with you that this patch is trivial for the current
implementation since the notifier, as it is currently, will enforce
cpu_cooling policy change at every CPUFREQ_ADJUST which would cause
problems in our current implementation. In our implementation there is
a cpufreq driver that will also change policies during CPUFREQ_ADJUST,
once the request comes from the underlying FW so there would be a fight
who gets there first since cpu_cooling will change the policy in
CPUFREQ_ADJUST notifier_chain and the driver would do the same thing.
It seems to me that better implementation of the cpu_cooling notifer
would be to keep the flag and change the policy in CPUFREQ_ADJUST only
when the change was requested by cpu_cooling, and update the current
state of cpufreq_cooling_device during CPUFREQ_NOTIFY event.
What do you think?

> 
> > Now the temperature changes and cpu_cooling should change
> > the system state to 1 (userspace component malfunctioned and is not
> > picking up this change) but since the cur_state is already at 1
> > cpu_cooling will not do anything since it believes it is in the
> > correct state. Hope this explains it better
> 

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

[toc] | [prev] | [next] | [standalone]

#1197980

On 31-07-15, 08:30, Radivoje Jovanovic wrote:
> I just looked over the notifier in the current upstream (my patch was
> made on our production kernel which is 3.14 and has old notifier
> implementation with notifier_device in place) and I see your point.

That's disappointing. You were expected to check if the same problem
exists in mainline.

> I agree with you that this patch is trivial for the current
> implementation since the notifier, as it is currently, will enforce
> cpu_cooling policy change at every CPUFREQ_ADJUST which would cause
> problems in our current implementation. In our implementation there is
> a cpufreq driver that will also change policies during CPUFREQ_ADJUST,
> once the request comes from the underlying FW so there would be a fight
> who gets there first since cpu_cooling will change the policy in
> CPUFREQ_ADJUST notifier_chain and the driver would do the same thing.
> It seems to me that better implementation of the cpu_cooling notifer
> would be to keep the flag and change the policy in CPUFREQ_ADJUST only
> when the change was requested by cpu_cooling, and update the current
> state of cpufreq_cooling_device during CPUFREQ_NOTIFY event.
> What do you think?

I think the way cpu-cooling is written today, is an *ugly* hack. We hack
the notifier to change policy->max and no one is notified for it.

That's crap.

I would rather get some help from cpufreq core on that. Which can
provide some APIs to take care of thermal considerations.

Okay, I push that to my todo list. Will keep you all posted.

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

[toc] | [prev] | [next] | [standalone]

#1198518

On 01-08-15, 17:04, Viresh Kumar wrote:
> On 31-07-15, 08:30, Radivoje Jovanovic wrote:

> > I agree with you that this patch is trivial for the current
> > implementation since the notifier, as it is currently, will enforce
> > cpu_cooling policy change at every CPUFREQ_ADJUST which would cause
> > problems in our current implementation. In our implementation there is
> > a cpufreq driver that will also change policies during CPUFREQ_ADJUST,
> > once the request comes from the underlying FW so there would be a fight
> > who gets there first since cpu_cooling will change the policy in
> > CPUFREQ_ADJUST notifier_chain and the driver would do the same thing.

Okay, I had a detailed look this morning. cpufreq-notifier is designed
this way that policy->max can be updated by drivers.. So, that's fine.

Now coming to your problem. So, there are two users: fw and thermal,
which can affect policy->max. Now, both of them need to respect the
limits set by others and only decrease policy->max from the notifier
if it doesn't suit them.

I think it should work pretty well, unless you know you have triggered
a corner case somewhere, that I am not able to imagine.

Please let me know in case I am wrong.

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

[toc] | [prev] | [next] | [standalone]

#1199220 — Re: [PATCH] thermal/cpu_cooling: remove local cooling state variable

On Mon, 3 Aug 2015 08:43:25 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:

> On 01-08-15, 17:04, Viresh Kumar wrote:
> > On 31-07-15, 08:30, Radivoje Jovanovic wrote:
> 
> > > I agree with you that this patch is trivial for the current
> > > implementation since the notifier, as it is currently, will
> > > enforce cpu_cooling policy change at every CPUFREQ_ADJUST which
> > > would cause problems in our current implementation. In our
> > > implementation there is a cpufreq driver that will also change
> > > policies during CPUFREQ_ADJUST, once the request comes from the
> > > underlying FW so there would be a fight who gets there first
> > > since cpu_cooling will change the policy in CPUFREQ_ADJUST
> > > notifier_chain and the driver would do the same thing.
> 
> Okay, I had a detailed look this morning. cpufreq-notifier is designed
> this way that policy->max can be updated by drivers.. So, that's fine.
> 
> Now coming to your problem. So, there are two users: fw and thermal,
> which can affect policy->max. Now, both of them need to respect the
> limits set by others and only decrease policy->max from the notifier
> if it doesn't suit them.
> 
> I think it should work pretty well, unless you know you have triggered
> a corner case somewhere, that I am not able to imagine.
> 
> Please let me know in case I am wrong.
>
I will port the upstream driver to our platfrom, test for all corner
cases and update this thread once I have the data
Thank you for all the help
 

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web