From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too
Date: Thu, 12 Sep 2013 00:43:47 +0000 [thread overview]
Message-ID: <52310E43.1090808@intel.com> (raw)
In-Reply-To: <CAKohpokEZcwF-zL6rTXYyC_x=SJzGFvdvxqvmvyhJHa8nPXogQ@mail.gmail.com>
On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()
Which platform?
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)
Yes, if you can point to a specific driver having this problem.
Thanks,
Rafael
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
WARNING: multiple messages have this Message-ID (diff)
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
Greg KH <greg@kroah.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"cpufreq@vger.kernel.org" <cpufreq@vger.kernel.org>,
SH-Linux <linux-sh@vger.kernel.org>,
Magnus Damm <magnus.damm@gmail.com>
Subject: Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too
Date: Thu, 12 Sep 2013 02:43:47 +0200 [thread overview]
Message-ID: <52310E43.1090808@intel.com> (raw)
In-Reply-To: <CAKohpokEZcwF-zL6rTXYyC_x=SJzGFvdvxqvmvyhJHa8nPXogQ@mail.gmail.com>
On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()
Which platform?
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)
Yes, if you can point to a specific driver having this problem.
Thanks,
Rafael
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
WARNING: multiple messages have this Message-ID (diff)
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
Greg KH <greg@kroah.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"cpufreq@vger.kernel.org" <cpufreq@vger.kernel.org>,
SH-Linux <linux-sh@vger.kernel.org>,
Magnus Damm <magnus.damm@gmail.com>
Subject: Re: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too
Date: Thu, 12 Sep 2013 02:43:47 +0200 [thread overview]
Message-ID: <52310E43.1090808@intel.com> (raw)
In-Reply-To: <CAKohpokEZcwF-zL6rTXYyC_x=SJzGFvdvxqvmvyhJHa8nPXogQ@mail.gmail.com>
On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()
Which platform?
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)
Yes, if you can point to a specific driver having this problem.
Thanks,
Rafael
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
WARNING: multiple messages have this Message-ID (diff)
From: rafael.j.wysocki@intel.com (Rafael J. Wysocki)
To: linux-arm-kernel@lists.infradead.org
Subject: "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too
Date: Thu, 12 Sep 2013 02:43:47 +0200 [thread overview]
Message-ID: <52310E43.1090808@intel.com> (raw)
In-Reply-To: <CAKohpokEZcwF-zL6rTXYyC_x=SJzGFvdvxqvmvyhJHa8nPXogQ@mail.gmail.com>
On 9/12/2013 2:39 AM, Viresh Kumar wrote:
> On 11 September 2013 18:48, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, September 11, 2013 02:08:44 PM Viresh Kumar wrote:
>>> That looked like a straight forward issue/bug to me and so I haven't
>>> gotten deep into it..
>> Which you should always do when you're going to deal with concurrency issues.
>> Even if they appear to be obvious, they often are far from that, like in this
>> case.
> /me Nods
>
>>> Scenario 2:
>>> --------------
>>> Governor is changing freq and has called __cpufreq_driver_target().
>>> At the same time we are changing scaling_{min|max}_freq from
>>> sysfs, which would eventually end up calling governors:
>>> CPUFREQ_GOV_LIMITS notification, that will also call:
>>> __cpufreq_driver_target()..
>>>
>>> So, we eventually have two concurrent calls to ->target() and we
>>> don't really know how hardware will behave in this case.. Most of
>>> the implementations of ->target() routines just go and change
>>> freq/voltage without checking if we are already in progress of doing
>>> that (i.e. based on expectation that this call is not re entrant)..
>>>
>>> Now anything can happen at hardware level, which I don't have
>>> all insight of :(
>> That is more theoretical, however.
> Maybe we can get more deeper into it then :)
> Platform have something like this in their target()
Which platform?
> A. If new freq is more than old: Increase voltage
> B. Change freq
> C. If new freq is less than old: decrease voltage
>
> Now, two concurrent calls to target are X and Y, where X is trying to increase
> freq and Y is trying to decrease it..
>
> And this is the sequence that followed due to races..
>
> X.A: voltage increased for larger freq
> Y.A: nothing happened here
> Y.B: freq decreased
> Y.C: voltage decreased
> X.B: freq increased
> X.C: nothing happened..
>
> We ended up setting a freq which is not supported by the voltage we have
> set.. That will probably make clock to CPU unstable and system wouldn't
> be workable anymore...
>
> And so I think even this case must also get some space in the changelog :)
Yes, if you can point to a specific driver having this problem.
Thanks,
Rafael
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
next prev parent reply other threads:[~2013-09-12 0:43 UTC|newest]
Thread overview: 103+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-09 15:11 "cpufreq: fix serialization issues with freq change notifiers" breaks cpufreq too Guennadi Liakhovetski
2013-09-09 15:11 ` Guennadi Liakhovetski
2013-09-09 15:11 ` Guennadi Liakhovetski
2013-09-09 15:11 ` Guennadi Liakhovetski
2013-09-09 20:57 ` Rafael J. Wysocki
2013-09-09 21:08 ` Rafael J. Wysocki
2013-09-09 21:08 ` Rafael J. Wysocki
2013-09-09 21:42 ` Guennadi Liakhovetski
2013-09-09 21:42 ` Guennadi Liakhovetski
2013-09-09 21:42 ` Guennadi Liakhovetski
2013-09-09 23:12 ` Rafael J. Wysocki
2013-09-09 23:12 ` Rafael J. Wysocki
2013-09-09 23:12 ` Rafael J. Wysocki
2013-09-10 1:46 ` Rafael J. Wysocki
2013-09-10 1:46 ` Rafael J. Wysocki
2013-09-10 11:29 ` Viresh Kumar
2013-09-10 11:41 ` Viresh Kumar
2013-09-10 11:29 ` Viresh Kumar
2013-09-10 11:29 ` Viresh Kumar
2013-09-10 11:49 ` Rafael J. Wysocki
2013-09-10 11:49 ` Rafael J. Wysocki
2013-09-10 11:49 ` Rafael J. Wysocki
2013-09-10 11:49 ` Rafael J. Wysocki
2013-09-10 15:14 ` Viresh Kumar
2013-09-10 15:26 ` Viresh Kumar
2013-09-10 15:14 ` Viresh Kumar
2013-09-10 15:14 ` Viresh Kumar
2013-09-10 19:46 ` Rafael J. Wysocki
2013-09-10 19:46 ` Rafael J. Wysocki
2013-09-10 19:46 ` Rafael J. Wysocki
2013-09-10 19:46 ` Rafael J. Wysocki
2013-09-11 8:38 ` Viresh Kumar
2013-09-11 8:50 ` Viresh Kumar
2013-09-11 8:38 ` Viresh Kumar
2013-09-11 8:38 ` Viresh Kumar
2013-09-11 13:18 ` Rafael J. Wysocki
2013-09-11 13:18 ` Rafael J. Wysocki
2013-09-11 13:18 ` Rafael J. Wysocki
2013-09-11 13:18 ` Rafael J. Wysocki
2013-09-12 0:39 ` Viresh Kumar
2013-09-12 0:51 ` Viresh Kumar
2013-09-12 0:39 ` Viresh Kumar
2013-09-12 0:39 ` Viresh Kumar
2013-09-12 0:43 ` Rafael J. Wysocki [this message]
2013-09-12 0:43 ` Rafael J. Wysocki
2013-09-12 0:43 ` Rafael J. Wysocki
2013-09-12 0:43 ` Rafael J. Wysocki
2013-09-12 5:36 ` Viresh Kumar
2013-09-12 5:48 ` Viresh Kumar
2013-09-12 5:36 ` Viresh Kumar
2013-09-12 5:36 ` Viresh Kumar
2013-09-12 10:50 ` Rafael J. Wysocki
2013-09-12 11:01 ` Rafael J. Wysocki
2013-09-12 11:01 ` Rafael J. Wysocki
2013-09-12 11:01 ` Rafael J. Wysocki
2013-09-12 10:52 ` Viresh Kumar
2013-09-12 10:52 ` Viresh Kumar
2013-09-12 10:52 ` Viresh Kumar
2013-09-12 10:52 ` Viresh Kumar
2013-09-10 15:12 ` Guennadi Liakhovetski
2013-09-10 15:12 ` Guennadi Liakhovetski
2013-09-10 15:12 ` Guennadi Liakhovetski
2013-09-10 15:12 ` Guennadi Liakhovetski
2013-09-10 15:26 ` Viresh Kumar
2013-09-10 15:38 ` Viresh Kumar
2013-09-10 15:26 ` Viresh Kumar
2013-09-10 15:26 ` Viresh Kumar
2013-09-10 16:22 ` Guennadi Liakhovetski
2013-09-10 16:22 ` Guennadi Liakhovetski
2013-09-10 16:22 ` Guennadi Liakhovetski
2013-09-10 16:22 ` Guennadi Liakhovetski
2013-09-10 16:34 ` Viresh Kumar
2013-09-10 16:46 ` Viresh Kumar
2013-09-10 16:34 ` Viresh Kumar
2013-09-10 16:34 ` Viresh Kumar
2013-09-10 17:07 ` Guennadi Liakhovetski
2013-09-10 17:07 ` Guennadi Liakhovetski
2013-09-10 17:07 ` Guennadi Liakhovetski
2013-09-10 17:07 ` Guennadi Liakhovetski
2013-09-11 8:06 ` Viresh Kumar
2013-09-11 8:18 ` Viresh Kumar
2013-09-11 8:06 ` Viresh Kumar
2013-09-11 8:06 ` Viresh Kumar
2013-09-11 8:15 ` Guennadi Liakhovetski
2013-09-11 8:15 ` Guennadi Liakhovetski
2013-09-11 8:15 ` Guennadi Liakhovetski
2013-09-11 8:15 ` Guennadi Liakhovetski
2013-09-11 8:39 ` Viresh Kumar
2013-09-11 8:51 ` Viresh Kumar
2013-09-11 8:39 ` Viresh Kumar
2013-09-11 8:39 ` Viresh Kumar
2013-09-11 13:28 ` Rafael J. Wysocki
2013-09-11 13:28 ` Rafael J. Wysocki
2013-09-11 13:28 ` Rafael J. Wysocki
2013-09-11 13:28 ` Rafael J. Wysocki
2013-09-12 7:47 ` Guennadi Liakhovetski
2013-09-12 7:47 ` Guennadi Liakhovetski
2013-09-12 7:47 ` Guennadi Liakhovetski
2013-09-12 7:47 ` Guennadi Liakhovetski
2013-09-12 7:51 ` Viresh Kumar
2013-09-12 7:51 ` Viresh Kumar
2013-09-12 7:51 ` Viresh Kumar
2013-09-12 7:51 ` Viresh Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52310E43.1090808@intel.com \
--to=rafael.j.wysocki@intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.