Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/13/2013 05:14 PM, Viresh Kumar wrote: > On 4 September 2013 01:10, Rafael J. Wysocki wrote: >> On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote: > >>> This doesn't solve the problem completely: it prevents the store_*() task >>> from continuing *only* when it concurrently executes the >>> __cpufreq_governor() >>> function along with the CPU offline task. But if the two calls don't >>> overlap, >>> we will still have the possibility where the store_*() task tries to acquire >>> the timer mutex after the CPU offline task has just finished destroying it. >> >> Yeah, I overlooked that. > > As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked > this unread as there were other important topics to close).. > > And he had some other code in mind and these synchronization problems > aren't there with my patch at all (as per him too).. > > Rafael, probably both me and Srivatsa are missing something that you > understood, can you please share what problem you see here with my patch? > > And yes, even with Srivatsa's patchset I found a problem: > Two threads, one changing governor from ondemand->conservative and other > one changing min/max freq.. > > First one will try to STOP governor and other one will try to change > limits of gov. > Suppose 2nd one gets to ->governor() and after this first one stops > the governor. > Now the first one tries to access lock and crashes.. > > On IRC, Srivatsa agreed about the problem.. > Actually, we had another round of discussions and decided that there is no problem here. The reason is, the top-level store() routine takes the rwsem of that policy for write; so only one store() can go at a time. So the race between changing the governor and changing the min/max freq won't happen because only one of them can execute at a given time. In fact, by extension, *any* two store()s in cpufreq shouldn't race with one another. So the conclusion is that the mainline code is fine in this aspect. Nothing needs to be reverted. That said, there are still certain low-priority/less-visible synchronization issues to be fixed in cpufreq, and we'll take them up later, once the dust is settled. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/13/2013 05:14 PM, Viresh Kumar wrote: On 4 September 2013 01:10, Rafael J. Wysocki r...@sisk.pl wrote: On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote: This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it. Yeah, I overlooked that. As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked this unread as there were other important topics to close).. And he had some other code in mind and these synchronization problems aren't there with my patch at all (as per him too).. Rafael, probably both me and Srivatsa are missing something that you understood, can you please share what problem you see here with my patch? And yes, even with Srivatsa's patchset I found a problem: Two threads, one changing governor from ondemand-conservative and other one changing min/max freq.. First one will try to STOP governor and other one will try to change limits of gov. Suppose 2nd one gets to -governor() and after this first one stops the governor. Now the first one tries to access lock and crashes.. On IRC, Srivatsa agreed about the problem.. Actually, we had another round of discussions and decided that there is no problem here. The reason is, the top-level store() routine takes the rwsem of that policy for write; so only one store() can go at a time. So the race between changing the governor and changing the min/max freq won't happen because only one of them can execute at a given time. In fact, by extension, *any* two store()s in cpufreq shouldn't race with one another. So the conclusion is that the mainline code is fine in this aspect. Nothing needs to be reverted. That said, there are still certain low-priority/less-visible synchronization issues to be fixed in cpufreq, and we'll take them up later, once the dust is settled. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 14 September 2013 05:18, Rafael J. Wysocki wrote: > OK, I'll think about that, but not today and probably not within the next > few days, because I'm heading to New Orleans in several hours and really > have other stuff to take care of now. Sorry about that. No issues.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Friday, September 13, 2013 05:14:26 PM Viresh Kumar wrote: > On 4 September 2013 01:10, Rafael J. Wysocki wrote: > > On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote: > > >> This doesn't solve the problem completely: it prevents the store_*() task > >> from continuing *only* when it concurrently executes the > >> __cpufreq_governor() > >> function along with the CPU offline task. But if the two calls don't > >> overlap, > >> we will still have the possibility where the store_*() task tries to > >> acquire > >> the timer mutex after the CPU offline task has just finished destroying it. > > > > Yeah, I overlooked that. > > As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked > this unread as there were other important topics to close).. > > And he had some other code in mind and these synchronization problems > aren't there with my patch at all (as per him too).. > > Rafael, probably both me and Srivatsa are missing something that you > understood, can you please share what problem you see here with my patch? > > And yes, even with Srivatsa's patchset I found a problem: > Two threads, one changing governor from ondemand->conservative and other > one changing min/max freq.. > > First one will try to STOP governor and other one will try to change > limits of gov. > Suppose 2nd one gets to ->governor() and after this first one stops > the governor. > Now the first one tries to access lock and crashes.. > > On IRC, Srivatsa agreed about the problem.. > > So, probably the first thing to do is to get this patch back, i.e. > revert of Srivatsa's > patch: > > commit 56d07db274b7b15ca38b60ea4a762d40de093000 > Author: Srivatsa S. Bhat > Date: Sat Sep 7 01:23:55 2013 +0530 > > cpufreq: Remove temporary fix for race between CPU hotplug and > sysfs-writes > > > Srivatsa also asked if we can get a big lock around call to ->governor() > which would create recursive locks on a call to EXIT event (as we have seen it > earlier..).. > > But he believes he can pull it off and will try this later.. So, for > now we can revert > the above patch :) OK, I'll think about that, but not today and probably not within the next few days, because I'm heading to New Orleans in several hours and really have other stuff to take care of now. Sorry about that. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 4 September 2013 01:10, Rafael J. Wysocki wrote: > On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote: >> This doesn't solve the problem completely: it prevents the store_*() task >> from continuing *only* when it concurrently executes the __cpufreq_governor() >> function along with the CPU offline task. But if the two calls don't overlap, >> we will still have the possibility where the store_*() task tries to acquire >> the timer mutex after the CPU offline task has just finished destroying it. > > Yeah, I overlooked that. As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked this unread as there were other important topics to close).. And he had some other code in mind and these synchronization problems aren't there with my patch at all (as per him too).. Rafael, probably both me and Srivatsa are missing something that you understood, can you please share what problem you see here with my patch? And yes, even with Srivatsa's patchset I found a problem: Two threads, one changing governor from ondemand->conservative and other one changing min/max freq.. First one will try to STOP governor and other one will try to change limits of gov. Suppose 2nd one gets to ->governor() and after this first one stops the governor. Now the first one tries to access lock and crashes.. On IRC, Srivatsa agreed about the problem.. So, probably the first thing to do is to get this patch back, i.e. revert of Srivatsa's patch: commit 56d07db274b7b15ca38b60ea4a762d40de093000 Author: Srivatsa S. Bhat Date: Sat Sep 7 01:23:55 2013 +0530 cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writes Srivatsa also asked if we can get a big lock around call to ->governor() which would create recursive locks on a call to EXIT event (as we have seen it earlier..).. But he believes he can pull it off and will try this later.. So, for now we can revert the above patch :) -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 4 September 2013 01:10, Rafael J. Wysocki r...@sisk.pl wrote: On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote: This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it. Yeah, I overlooked that. As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked this unread as there were other important topics to close).. And he had some other code in mind and these synchronization problems aren't there with my patch at all (as per him too).. Rafael, probably both me and Srivatsa are missing something that you understood, can you please share what problem you see here with my patch? And yes, even with Srivatsa's patchset I found a problem: Two threads, one changing governor from ondemand-conservative and other one changing min/max freq.. First one will try to STOP governor and other one will try to change limits of gov. Suppose 2nd one gets to -governor() and after this first one stops the governor. Now the first one tries to access lock and crashes.. On IRC, Srivatsa agreed about the problem.. So, probably the first thing to do is to get this patch back, i.e. revert of Srivatsa's patch: commit 56d07db274b7b15ca38b60ea4a762d40de093000 Author: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com Date: Sat Sep 7 01:23:55 2013 +0530 cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writes Srivatsa also asked if we can get a big lock around call to -governor() which would create recursive locks on a call to EXIT event (as we have seen it earlier..).. But he believes he can pull it off and will try this later.. So, for now we can revert the above patch :) -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Friday, September 13, 2013 05:14:26 PM Viresh Kumar wrote: On 4 September 2013 01:10, Rafael J. Wysocki r...@sisk.pl wrote: On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote: This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it. Yeah, I overlooked that. As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked this unread as there were other important topics to close).. And he had some other code in mind and these synchronization problems aren't there with my patch at all (as per him too).. Rafael, probably both me and Srivatsa are missing something that you understood, can you please share what problem you see here with my patch? And yes, even with Srivatsa's patchset I found a problem: Two threads, one changing governor from ondemand-conservative and other one changing min/max freq.. First one will try to STOP governor and other one will try to change limits of gov. Suppose 2nd one gets to -governor() and after this first one stops the governor. Now the first one tries to access lock and crashes.. On IRC, Srivatsa agreed about the problem.. So, probably the first thing to do is to get this patch back, i.e. revert of Srivatsa's patch: commit 56d07db274b7b15ca38b60ea4a762d40de093000 Author: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com Date: Sat Sep 7 01:23:55 2013 +0530 cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writes Srivatsa also asked if we can get a big lock around call to -governor() which would create recursive locks on a call to EXIT event (as we have seen it earlier..).. But he believes he can pull it off and will try this later.. So, for now we can revert the above patch :) OK, I'll think about that, but not today and probably not within the next few days, because I'm heading to New Orleans in several hours and really have other stuff to take care of now. Sorry about that. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 14 September 2013 05:18, Rafael J. Wysocki r...@sisk.pl wrote: OK, I'll think about that, but not today and probably not within the next few days, because I'm heading to New Orleans in several hours and really have other stuff to take care of now. Sorry about that. No issues.. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/10/2013 12:22 PM, Viresh Kumar wrote: > Back finally and I see lots of mails over cpufreq stuff.. :) > > On 3 September 2013 18:50, Srivatsa S. Bhat > wrote: >> This doesn't solve the problem completely: it prevents the store_*() task >> from continuing *only* when it concurrently executes the __cpufreq_governor() >> function along with the CPU offline task. But if the two calls don't overlap, >> we will still have the possibility where the store_*() task tries to acquire >> the timer mutex after the CPU offline task has just finished destroying it. > > How exactly? My brain is still on vacations :) > The race window is not limited to __cpufreq_governor() alone. It just starts there, but doesn't end there; it extends beyond that point. That's the main problem. And that's why serializing __cpufreq_governor() won't completely solve the issue. CPU_DOWN: __cpufreq_governor(STOP) + ... | ... | RACE ... | WINDOW ... | sysfs teardown+ In the STOP call, we destroy the timer mutex and set cur_policy = NULL. So the incorrect access to timer mutex can occur from that point until we finally unlink or teardown the sysfs files and prevent userspace from writing to those files. Thus, this window involves stuff other than the call to __cpufreq_governor() as well. So serializing __cpufreq_governor() alone is not enough. > Anyway, this was one of the problem that I tried to solve with my patch. > But there can be other race conditions where things can go wrong and so > that patch may still be useful. > I agree, but see below (its the "may be useful" part that I'm not convinced about). > Call to __cpufreq_governor() must be serialized I believe. > Sure, its a good idea to serialize __cpufreq_governor(). However, we need specific justification for that. Just a hunch that something will go wrong is not enough, IMHO. We have to *prove* that something is indeed wrong, and only then add appropriate synchronization to fix it. Besides, even the commit message appeared a bit odd. It mentions something about problems with recursive calls. What cases are those? When do we end up recursively calling __cpufreq_governor()? And hence, why can't we use plain locks and must instead use yet another quick-and-dirty variable to protect that function? [Using variables like that is bad from a lockdep perspective as well (apart from other things) : lockdep can't catch any resulting locking issues.] Other related questions that are worth answering would be: why should we add the synchronization *inside* __cpufreq_governor()? Why not add it at its call-sites? IOW, I'm all in for fixing problems, just that I'd be comfortable with the changes only when we completely understand what problem we are fixing and why that patch is the right fix for that problem. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
Back finally and I see lots of mails over cpufreq stuff.. :) On 3 September 2013 18:50, Srivatsa S. Bhat wrote: > This doesn't solve the problem completely: it prevents the store_*() task > from continuing *only* when it concurrently executes the __cpufreq_governor() > function along with the CPU offline task. But if the two calls don't overlap, > we will still have the possibility where the store_*() task tries to acquire > the timer mutex after the CPU offline task has just finished destroying it. How exactly? My brain is still on vacations :) Anyway, this was one of the problem that I tried to solve with my patch. But there can be other race conditions where things can go wrong and so that patch may still be useful. Call to __cpufreq_governor() must be serialized I believe. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
Back finally and I see lots of mails over cpufreq stuff.. :) On 3 September 2013 18:50, Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com wrote: This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it. How exactly? My brain is still on vacations :) Anyway, this was one of the problem that I tried to solve with my patch. But there can be other race conditions where things can go wrong and so that patch may still be useful. Call to __cpufreq_governor() must be serialized I believe. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/10/2013 12:22 PM, Viresh Kumar wrote: Back finally and I see lots of mails over cpufreq stuff.. :) On 3 September 2013 18:50, Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com wrote: This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it. How exactly? My brain is still on vacations :) The race window is not limited to __cpufreq_governor() alone. It just starts there, but doesn't end there; it extends beyond that point. That's the main problem. And that's why serializing __cpufreq_governor() won't completely solve the issue. CPU_DOWN: __cpufreq_governor(STOP) + ... | ... | RACE ... | WINDOW ... | sysfs teardown+ In the STOP call, we destroy the timer mutex and set cur_policy = NULL. So the incorrect access to timer mutex can occur from that point until we finally unlink or teardown the sysfs files and prevent userspace from writing to those files. Thus, this window involves stuff other than the call to __cpufreq_governor() as well. So serializing __cpufreq_governor() alone is not enough. Anyway, this was one of the problem that I tried to solve with my patch. But there can be other race conditions where things can go wrong and so that patch may still be useful. I agree, but see below (its the may be useful part that I'm not convinced about). Call to __cpufreq_governor() must be serialized I believe. Sure, its a good idea to serialize __cpufreq_governor(). However, we need specific justification for that. Just a hunch that something will go wrong is not enough, IMHO. We have to *prove* that something is indeed wrong, and only then add appropriate synchronization to fix it. Besides, even the commit message appeared a bit odd. It mentions something about problems with recursive calls. What cases are those? When do we end up recursively calling __cpufreq_governor()? And hence, why can't we use plain locks and must instead use yet another quick-and-dirty variable to protect that function? [Using variables like that is bad from a lockdep perspective as well (apart from other things) : lockdep can't catch any resulting locking issues.] Other related questions that are worth answering would be: why should we add the synchronization *inside* __cpufreq_governor()? Why not add it at its call-sites? IOW, I'm all in for fixing problems, just that I'd be comfortable with the changes only when we completely understand what problem we are fixing and why that patch is the right fix for that problem. Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Friday, September 06, 2013 11:33:55 AM Srivatsa S. Bhat wrote:
> On 09/05/2013 06:24 AM, Stephen Boyd wrote:
> > On 09/04/13 17:26, Rafael J. Wysocki wrote:
> >> On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
> >>> On 09/04/13 16:55, Rafael J. Wysocki wrote:
> Well, I'm not sure when Viresh is going to be back.
>
> Srivatsa, can you please resend this patch with a proper changelog?
>
> >>> I haven't had a chance to try this out yet, but I was just thinking
> >>> about this patch. How is it going to work? If one task opens the file
> >>> and another task is taking down the CPU wouldn't we deadlock in the
> >>> CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
> >>> grab the kobject reference and sleep on the hotplug mutex and task 2
> >>> will put the kobject and wait for the completion, but it won't happen.
> >>> At least I think that's what would happen.
> >> Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock
> >> there.
> >
> > I mean the complete in cpufreq_sysfs_release(). I don't think that will
> > ever be called because the kobject is held by the task calling store()
> > which is waiting on the hotplug lock to be released.
> >
> >>
> >> Well, I guess the Srivatsa's patch may be salvaged by making it do a
> >> "trylock"
> >> version of get_online_cpus(), but then I wonder if there's no better way.
> >
> > I think the real solution is to remove the kobject first if the CPU
> > going down is the last user of that policy. Once the completion is done
> > we can stop the governor and clean up state. For the case where there
> > are still CPUs using the policy why can't we cancel that CPU's work and
> > do nothing else? Even in the case where we have to move the cpufreq
> > directory do we need to do a STOP/START/LIMITS sequence? I hope we can
> > get away with just moving the directory and canceling that CPUs work then.
> >
>
> Conceptually, I agree that your idea of not allowing any process to take
> a new reference to the kobject while we are taking the CPU offline, is a
> sound solution.
>
> However, I am reluctant to go down that path because, handling the CPU hotplug
> sequence in the suspend/resume path might get even more tricky, if we want
> to implement the changes that you propose. Just recently we managed to
> rework the cpufreq CPU hotplug handling to retain the sysfs file permissions
> around suspend/resume, and doing that was not at all simple. Adding more
> quirks and complexity to the kobject handling in that path will only make
> things even more challenging, IMHO. That's the reason I'm trying to think
> of ways to avoid touching that fragile code, and instead solve this problem
> in some other way, without compromising on the robustness of the solution.
>
> So here is my new proposal, as a replacement to this patch[2/2]:
Actually, I've already applied patch [2/2], because while it doesn't solve the
whole problem, it narrows it down somewhat. So, please work on top of that
patch going forward.
> We note that, at CPU_DOWN_PREPARE stage, the CPU is not yet marked offline,
> whereas by the time we handle CPU_POST_DEAD, the CPU is removed from the
> cpu_online_mask, and also the cpu_hotplug lock is dropped.
>
> So, let us split up __cpu_remove_dev() into 2 parts, say:
> __cpu_remove_prepare() - invoked during CPU_DOWN_PREPARE
> __cpu_remove_finish() - invoked during CPU_POST_DEAD
>
>
> In the former function, we stop the governors, so that
> policy->governor_enabled
> gets set to false, so that patch [1/2] will return -EBUSY to any subsequent
> ->store() requests. Also, we do everything except the kobject cleanup.
>
> In the latter function, we do the remaining work, particularly the part
> where we wait for the kobject refcount to drop to zero and the subsequent
> cleanup.
>
>
> And the ->store() functions will be modified to look like this:
>
> store()
> {
> get_online_cpus();
>
> if (!cpu_online(cpu))
> goto out;
>
> /* Body of the function*/
> out:
> put_online_cpus();
> }
>
> That way, if a task tries to write to a cpufreq file during CPU offline,
> it will get blocked on get_online_cpus(), and will continue after
> CPU_DEAD (since we release the lock here). Then it will notice that the cpu
> is offline, and hence will return silently, thus dropping the kobject refcnt.
> So, when the cpufreq core comes back at the CPU_POST_DEAD stage to cleanup
> the kobject, it won't encounter any problems.
>
> Any thoughts on this approach? I'll try to code it up and post the patch
> later today.
It sounds good to me (modulo the remark above).
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/05/2013 06:24 AM, Stephen Boyd wrote:
> On 09/04/13 17:26, Rafael J. Wysocki wrote:
>> On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
>>> On 09/04/13 16:55, Rafael J. Wysocki wrote:
Well, I'm not sure when Viresh is going to be back.
Srivatsa, can you please resend this patch with a proper changelog?
>>> I haven't had a chance to try this out yet, but I was just thinking
>>> about this patch. How is it going to work? If one task opens the file
>>> and another task is taking down the CPU wouldn't we deadlock in the
>>> CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
>>> grab the kobject reference and sleep on the hotplug mutex and task 2
>>> will put the kobject and wait for the completion, but it won't happen.
>>> At least I think that's what would happen.
>> Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock
>> there.
>
> I mean the complete in cpufreq_sysfs_release(). I don't think that will
> ever be called because the kobject is held by the task calling store()
> which is waiting on the hotplug lock to be released.
>
>>
>> Well, I guess the Srivatsa's patch may be salvaged by making it do a
>> "trylock"
>> version of get_online_cpus(), but then I wonder if there's no better way.
>
> I think the real solution is to remove the kobject first if the CPU
> going down is the last user of that policy. Once the completion is done
> we can stop the governor and clean up state. For the case where there
> are still CPUs using the policy why can't we cancel that CPU's work and
> do nothing else? Even in the case where we have to move the cpufreq
> directory do we need to do a STOP/START/LIMITS sequence? I hope we can
> get away with just moving the directory and canceling that CPUs work then.
>
Conceptually, I agree that your idea of not allowing any process to take
a new reference to the kobject while we are taking the CPU offline, is a
sound solution.
However, I am reluctant to go down that path because, handling the CPU hotplug
sequence in the suspend/resume path might get even more tricky, if we want
to implement the changes that you propose. Just recently we managed to
rework the cpufreq CPU hotplug handling to retain the sysfs file permissions
around suspend/resume, and doing that was not at all simple. Adding more
quirks and complexity to the kobject handling in that path will only make
things even more challenging, IMHO. That's the reason I'm trying to think
of ways to avoid touching that fragile code, and instead solve this problem
in some other way, without compromising on the robustness of the solution.
So here is my new proposal, as a replacement to this patch[2/2]:
We note that, at CPU_DOWN_PREPARE stage, the CPU is not yet marked offline,
whereas by the time we handle CPU_POST_DEAD, the CPU is removed from the
cpu_online_mask, and also the cpu_hotplug lock is dropped.
So, let us split up __cpu_remove_dev() into 2 parts, say:
__cpu_remove_prepare() - invoked during CPU_DOWN_PREPARE
__cpu_remove_finish() - invoked during CPU_POST_DEAD
In the former function, we stop the governors, so that policy->governor_enabled
gets set to false, so that patch [1/2] will return -EBUSY to any subsequent
->store() requests. Also, we do everything except the kobject cleanup.
In the latter function, we do the remaining work, particularly the part
where we wait for the kobject refcount to drop to zero and the subsequent
cleanup.
And the ->store() functions will be modified to look like this:
store()
{
get_online_cpus();
if (!cpu_online(cpu))
goto out;
/* Body of the function*/
out:
put_online_cpus();
}
That way, if a task tries to write to a cpufreq file during CPU offline,
it will get blocked on get_online_cpus(), and will continue after
CPU_DEAD (since we release the lock here). Then it will notice that the cpu
is offline, and hence will return silently, thus dropping the kobject refcnt.
So, when the cpufreq core comes back at the CPU_POST_DEAD stage to cleanup
the kobject, it won't encounter any problems.
Any thoughts on this approach? I'll try to code it up and post the patch
later today.
Thank you!
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/05/2013 06:24 AM, Stephen Boyd wrote:
On 09/04/13 17:26, Rafael J. Wysocki wrote:
On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
On 09/04/13 16:55, Rafael J. Wysocki wrote:
Well, I'm not sure when Viresh is going to be back.
Srivatsa, can you please resend this patch with a proper changelog?
I haven't had a chance to try this out yet, but I was just thinking
about this patch. How is it going to work? If one task opens the file
and another task is taking down the CPU wouldn't we deadlock in the
CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
grab the kobject reference and sleep on the hotplug mutex and task 2
will put the kobject and wait for the completion, but it won't happen.
At least I think that's what would happen.
Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock
there.
I mean the complete in cpufreq_sysfs_release(). I don't think that will
ever be called because the kobject is held by the task calling store()
which is waiting on the hotplug lock to be released.
Well, I guess the Srivatsa's patch may be salvaged by making it do a
trylock
version of get_online_cpus(), but then I wonder if there's no better way.
I think the real solution is to remove the kobject first if the CPU
going down is the last user of that policy. Once the completion is done
we can stop the governor and clean up state. For the case where there
are still CPUs using the policy why can't we cancel that CPU's work and
do nothing else? Even in the case where we have to move the cpufreq
directory do we need to do a STOP/START/LIMITS sequence? I hope we can
get away with just moving the directory and canceling that CPUs work then.
Conceptually, I agree that your idea of not allowing any process to take
a new reference to the kobject while we are taking the CPU offline, is a
sound solution.
However, I am reluctant to go down that path because, handling the CPU hotplug
sequence in the suspend/resume path might get even more tricky, if we want
to implement the changes that you propose. Just recently we managed to
rework the cpufreq CPU hotplug handling to retain the sysfs file permissions
around suspend/resume, and doing that was not at all simple. Adding more
quirks and complexity to the kobject handling in that path will only make
things even more challenging, IMHO. That's the reason I'm trying to think
of ways to avoid touching that fragile code, and instead solve this problem
in some other way, without compromising on the robustness of the solution.
So here is my new proposal, as a replacement to this patch[2/2]:
We note that, at CPU_DOWN_PREPARE stage, the CPU is not yet marked offline,
whereas by the time we handle CPU_POST_DEAD, the CPU is removed from the
cpu_online_mask, and also the cpu_hotplug lock is dropped.
So, let us split up __cpu_remove_dev() into 2 parts, say:
__cpu_remove_prepare() - invoked during CPU_DOWN_PREPARE
__cpu_remove_finish() - invoked during CPU_POST_DEAD
In the former function, we stop the governors, so that policy-governor_enabled
gets set to false, so that patch [1/2] will return -EBUSY to any subsequent
-store() requests. Also, we do everything except the kobject cleanup.
In the latter function, we do the remaining work, particularly the part
where we wait for the kobject refcount to drop to zero and the subsequent
cleanup.
And the -store() functions will be modified to look like this:
store()
{
get_online_cpus();
if (!cpu_online(cpu))
goto out;
/* Body of the function*/
out:
put_online_cpus();
}
That way, if a task tries to write to a cpufreq file during CPU offline,
it will get blocked on get_online_cpus(), and will continue after
CPU_DEAD (since we release the lock here). Then it will notice that the cpu
is offline, and hence will return silently, thus dropping the kobject refcnt.
So, when the cpufreq core comes back at the CPU_POST_DEAD stage to cleanup
the kobject, it won't encounter any problems.
Any thoughts on this approach? I'll try to code it up and post the patch
later today.
Thank you!
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Friday, September 06, 2013 11:33:55 AM Srivatsa S. Bhat wrote:
On 09/05/2013 06:24 AM, Stephen Boyd wrote:
On 09/04/13 17:26, Rafael J. Wysocki wrote:
On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote:
On 09/04/13 16:55, Rafael J. Wysocki wrote:
Well, I'm not sure when Viresh is going to be back.
Srivatsa, can you please resend this patch with a proper changelog?
I haven't had a chance to try this out yet, but I was just thinking
about this patch. How is it going to work? If one task opens the file
and another task is taking down the CPU wouldn't we deadlock in the
CPU_DOWN notifier waiting for the kobject to be released? Task 1 will
grab the kobject reference and sleep on the hotplug mutex and task 2
will put the kobject and wait for the completion, but it won't happen.
At least I think that's what would happen.
Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock
there.
I mean the complete in cpufreq_sysfs_release(). I don't think that will
ever be called because the kobject is held by the task calling store()
which is waiting on the hotplug lock to be released.
Well, I guess the Srivatsa's patch may be salvaged by making it do a
trylock
version of get_online_cpus(), but then I wonder if there's no better way.
I think the real solution is to remove the kobject first if the CPU
going down is the last user of that policy. Once the completion is done
we can stop the governor and clean up state. For the case where there
are still CPUs using the policy why can't we cancel that CPU's work and
do nothing else? Even in the case where we have to move the cpufreq
directory do we need to do a STOP/START/LIMITS sequence? I hope we can
get away with just moving the directory and canceling that CPUs work then.
Conceptually, I agree that your idea of not allowing any process to take
a new reference to the kobject while we are taking the CPU offline, is a
sound solution.
However, I am reluctant to go down that path because, handling the CPU hotplug
sequence in the suspend/resume path might get even more tricky, if we want
to implement the changes that you propose. Just recently we managed to
rework the cpufreq CPU hotplug handling to retain the sysfs file permissions
around suspend/resume, and doing that was not at all simple. Adding more
quirks and complexity to the kobject handling in that path will only make
things even more challenging, IMHO. That's the reason I'm trying to think
of ways to avoid touching that fragile code, and instead solve this problem
in some other way, without compromising on the robustness of the solution.
So here is my new proposal, as a replacement to this patch[2/2]:
Actually, I've already applied patch [2/2], because while it doesn't solve the
whole problem, it narrows it down somewhat. So, please work on top of that
patch going forward.
We note that, at CPU_DOWN_PREPARE stage, the CPU is not yet marked offline,
whereas by the time we handle CPU_POST_DEAD, the CPU is removed from the
cpu_online_mask, and also the cpu_hotplug lock is dropped.
So, let us split up __cpu_remove_dev() into 2 parts, say:
__cpu_remove_prepare() - invoked during CPU_DOWN_PREPARE
__cpu_remove_finish() - invoked during CPU_POST_DEAD
In the former function, we stop the governors, so that
policy-governor_enabled
gets set to false, so that patch [1/2] will return -EBUSY to any subsequent
-store() requests. Also, we do everything except the kobject cleanup.
In the latter function, we do the remaining work, particularly the part
where we wait for the kobject refcount to drop to zero and the subsequent
cleanup.
And the -store() functions will be modified to look like this:
store()
{
get_online_cpus();
if (!cpu_online(cpu))
goto out;
/* Body of the function*/
out:
put_online_cpus();
}
That way, if a task tries to write to a cpufreq file during CPU offline,
it will get blocked on get_online_cpus(), and will continue after
CPU_DEAD (since we release the lock here). Then it will notice that the cpu
is offline, and hence will return silently, thus dropping the kobject refcnt.
So, when the cpufreq core comes back at the CPU_POST_DEAD stage to cleanup
the kobject, it won't encounter any problems.
Any thoughts on this approach? I'll try to code it up and post the patch
later today.
It sounds good to me (modulo the remark above).
Thanks,
Rafael
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/04/13 17:26, Rafael J. Wysocki wrote: > On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote: >> On 09/04/13 16:55, Rafael J. Wysocki wrote: >>> Well, I'm not sure when Viresh is going to be back. >>> >>> Srivatsa, can you please resend this patch with a proper changelog? >>> >> I haven't had a chance to try this out yet, but I was just thinking >> about this patch. How is it going to work? If one task opens the file >> and another task is taking down the CPU wouldn't we deadlock in the >> CPU_DOWN notifier waiting for the kobject to be released? Task 1 will >> grab the kobject reference and sleep on the hotplug mutex and task 2 >> will put the kobject and wait for the completion, but it won't happen. >> At least I think that's what would happen. > Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock > there. I mean the complete in cpufreq_sysfs_release(). I don't think that will ever be called because the kobject is held by the task calling store() which is waiting on the hotplug lock to be released. > > Well, I guess the Srivatsa's patch may be salvaged by making it do a "trylock" > version of get_online_cpus(), but then I wonder if there's no better way. I think the real solution is to remove the kobject first if the CPU going down is the last user of that policy. Once the completion is done we can stop the governor and clean up state. For the case where there are still CPUs using the policy why can't we cancel that CPU's work and do nothing else? Even in the case where we have to move the cpufreq directory do we need to do a STOP/START/LIMITS sequence? I hope we can get away with just moving the directory and canceling that CPUs work then. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote: > On 09/04/13 16:55, Rafael J. Wysocki wrote: > > > > Well, I'm not sure when Viresh is going to be back. > > > > Srivatsa, can you please resend this patch with a proper changelog? > > > > I haven't had a chance to try this out yet, but I was just thinking > about this patch. How is it going to work? If one task opens the file > and another task is taking down the CPU wouldn't we deadlock in the > CPU_DOWN notifier waiting for the kobject to be released? Task 1 will > grab the kobject reference and sleep on the hotplug mutex and task 2 > will put the kobject and wait for the completion, but it won't happen. > At least I think that's what would happen. Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock there. Well, I guess the Srivatsa's patch may be salvaged by making it do a "trylock" version of get_online_cpus(), but then I wonder if there's no better way. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/04/13 16:55, Rafael J. Wysocki wrote: > > Well, I'm not sure when Viresh is going to be back. > > Srivatsa, can you please resend this patch with a proper changelog? > I haven't had a chance to try this out yet, but I was just thinking about this patch. How is it going to work? If one task opens the file and another task is taking down the CPU wouldn't we deadlock in the CPU_DOWN notifier waiting for the kobject to be released? Task 1 will grab the kobject reference and sleep on the hotplug mutex and task 2 will put the kobject and wait for the completion, but it won't happen. At least I think that's what would happen. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
> On 09/01/2013 10:56 AM, Viresh Kumar wrote:
> > We can't take a big lock around __cpufreq_governor() as this causes
> > recursive
> > locking for some cases. But calls to this routine must be serialized for
> > every
> > policy.
> >
> > Lets introduce another variable which would guarantee serialization here.
> >
> > Signed-off-by: Viresh Kumar
> > ---
> > drivers/cpufreq/cpufreq.c | 7 ++-
> > include/linux/cpufreq.h | 1 +
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index f320a20..4d5723db 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
> > *policy,
> > policy->cpu, event);
> >
> > mutex_lock(_governor_lock);
> > - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> > + if (policy->governor_busy ||
> > + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> > (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
> >(event == CPUFREQ_GOV_STOP {
> > mutex_unlock(_governor_lock);
> > return -EBUSY;
> > }
> >
> > + policy->governor_busy = true;
> > if (event == CPUFREQ_GOV_STOP)
> > policy->governor_enabled = false;
> > else if (event == CPUFREQ_GOV_START)
> > @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
> > *policy,
> > ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> > module_put(policy->governor->owner);
> >
> > + mutex_lock(_governor_lock);
> > + policy->governor_busy = false;
> > + mutex_unlock(_governor_lock);
> > return ret;
> > }
> >
>
> This doesn't solve the problem completely: it prevents the store_*() task
> from continuing *only* when it concurrently executes the __cpufreq_governor()
> function along with the CPU offline task. But if the two calls don't overlap,
> we will still have the possibility where the store_*() task tries to acquire
> the timer mutex after the CPU offline task has just finished destroying it.
>
> So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
> store_*() thread will wait until the entire CPU offline operation is
> completed.
> After that, if it continues, it will get -EBUSY, due to patch [1], since
> policy->governor_enabled will be set to false.
>
> [1]. https://patchwork.kernel.org/patch/2852463/
>
> So here is the (completely untested) fix that I propose, as a replacement to
> this patch [2/2]:
>
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5c75e31..71c4fb2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -440,17 +440,24 @@ static ssize_t store_##file_name
> \
> unsigned int ret; \
> struct cpufreq_policy new_policy; \
> \
> + get_online_cpus(); \
> ret = cpufreq_get_policy(_policy, policy->cpu); \
> - if (ret)\
> - return -EINVAL; \
> + if (ret) { \
> + ret = -EINVAL; \
> + goto out; \
> + } \
> \
> ret = sscanf(buf, "%u", _policy.object);\
> - if (ret != 1) \
> - return -EINVAL; \
> + if (ret != 1) { \
> + ret = -EINVAL; \
> + goto out; \
> + } \
> \
> ret = __cpufreq_set_policy(policy, _policy);\
> policy->user_policy.object = policy->object;\
> \
> +out: \
> + put_online_cpus(); \
> return ret ? ret : count; \
> }
>
> @@ -494,17 +501,22 @@ static
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
On 09/01/2013 10:56 AM, Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes
recursive
locking for some cases. But calls to this routine must be serialized for
every
policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
---
drivers/cpufreq/cpufreq.c | 7 ++-
include/linux/cpufreq.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
policy-cpu, event);
mutex_lock(cpufreq_governor_lock);
- if ((policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
+ if (policy-governor_busy ||
+ (policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
(!policy-governor_enabled ((event == CPUFREQ_GOV_LIMITS) ||
(event == CPUFREQ_GOV_STOP {
mutex_unlock(cpufreq_governor_lock);
return -EBUSY;
}
+ policy-governor_busy = true;
if (event == CPUFREQ_GOV_STOP)
policy-governor_enabled = false;
else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
((event == CPUFREQ_GOV_POLICY_EXIT) !ret))
module_put(policy-governor-owner);
+ mutex_lock(cpufreq_governor_lock);
+ policy-governor_busy = false;
+ mutex_unlock(cpufreq_governor_lock);
return ret;
}
This doesn't solve the problem completely: it prevents the store_*() task
from continuing *only* when it concurrently executes the __cpufreq_governor()
function along with the CPU offline task. But if the two calls don't overlap,
we will still have the possibility where the store_*() task tries to acquire
the timer mutex after the CPU offline task has just finished destroying it.
So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
store_*() thread will wait until the entire CPU offline operation is
completed.
After that, if it continues, it will get -EBUSY, due to patch [1], since
policy-governor_enabled will be set to false.
[1]. https://patchwork.kernel.org/patch/2852463/
So here is the (completely untested) fix that I propose, as a replacement to
this patch [2/2]:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5c75e31..71c4fb2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -440,17 +440,24 @@ static ssize_t store_##file_name
\
unsigned int ret; \
struct cpufreq_policy new_policy; \
\
+ get_online_cpus(); \
ret = cpufreq_get_policy(new_policy, policy-cpu); \
- if (ret)\
- return -EINVAL; \
+ if (ret) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = sscanf(buf, %u, new_policy.object);\
- if (ret != 1) \
- return -EINVAL; \
+ if (ret != 1) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = __cpufreq_set_policy(policy, new_policy);\
policy-user_policy.object = policy-object;\
\
+out: \
+ put_online_cpus(); \
return ret ? ret : count; \
}
@@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct
cpufreq_policy *policy,
charstr_governor[16];
struct
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/04/13 16:55, Rafael J. Wysocki wrote: Well, I'm not sure when Viresh is going to be back. Srivatsa, can you please resend this patch with a proper changelog? I haven't had a chance to try this out yet, but I was just thinking about this patch. How is it going to work? If one task opens the file and another task is taking down the CPU wouldn't we deadlock in the CPU_DOWN notifier waiting for the kobject to be released? Task 1 will grab the kobject reference and sleep on the hotplug mutex and task 2 will put the kobject and wait for the completion, but it won't happen. At least I think that's what would happen. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote: On 09/04/13 16:55, Rafael J. Wysocki wrote: Well, I'm not sure when Viresh is going to be back. Srivatsa, can you please resend this patch with a proper changelog? I haven't had a chance to try this out yet, but I was just thinking about this patch. How is it going to work? If one task opens the file and another task is taking down the CPU wouldn't we deadlock in the CPU_DOWN notifier waiting for the kobject to be released? Task 1 will grab the kobject reference and sleep on the hotplug mutex and task 2 will put the kobject and wait for the completion, but it won't happen. At least I think that's what would happen. Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock there. Well, I guess the Srivatsa's patch may be salvaged by making it do a trylock version of get_online_cpus(), but then I wonder if there's no better way. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/04/13 17:26, Rafael J. Wysocki wrote: On Wednesday, September 04, 2013 04:50:01 PM Stephen Boyd wrote: On 09/04/13 16:55, Rafael J. Wysocki wrote: Well, I'm not sure when Viresh is going to be back. Srivatsa, can you please resend this patch with a proper changelog? I haven't had a chance to try this out yet, but I was just thinking about this patch. How is it going to work? If one task opens the file and another task is taking down the CPU wouldn't we deadlock in the CPU_DOWN notifier waiting for the kobject to be released? Task 1 will grab the kobject reference and sleep on the hotplug mutex and task 2 will put the kobject and wait for the completion, but it won't happen. At least I think that's what would happen. Do you mean the completion in sysfs_deactivate()? Yes, we can deadlock there. I mean the complete in cpufreq_sysfs_release(). I don't think that will ever be called because the kobject is held by the task calling store() which is waiting on the hotplug lock to be released. Well, I guess the Srivatsa's patch may be salvaged by making it do a trylock version of get_online_cpus(), but then I wonder if there's no better way. I think the real solution is to remove the kobject first if the CPU going down is the last user of that policy. Once the completion is done we can stop the governor and clean up state. For the case where there are still CPUs using the policy why can't we cancel that CPU's work and do nothing else? Even in the case where we have to move the cpufreq directory do we need to do a STOP/START/LIMITS sequence? I hope we can get away with just moving the directory and canceling that CPUs work then. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
> On 09/01/2013 10:56 AM, Viresh Kumar wrote:
> > We can't take a big lock around __cpufreq_governor() as this causes
> > recursive
> > locking for some cases. But calls to this routine must be serialized for
> > every
> > policy.
> >
> > Lets introduce another variable which would guarantee serialization here.
> >
> > Signed-off-by: Viresh Kumar
> > ---
> > drivers/cpufreq/cpufreq.c | 7 ++-
> > include/linux/cpufreq.h | 1 +
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index f320a20..4d5723db 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
> > *policy,
> > policy->cpu, event);
> >
> > mutex_lock(_governor_lock);
> > - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> > + if (policy->governor_busy ||
> > + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> > (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
> >(event == CPUFREQ_GOV_STOP {
> > mutex_unlock(_governor_lock);
> > return -EBUSY;
> > }
> >
> > + policy->governor_busy = true;
> > if (event == CPUFREQ_GOV_STOP)
> > policy->governor_enabled = false;
> > else if (event == CPUFREQ_GOV_START)
> > @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
> > *policy,
> > ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> > module_put(policy->governor->owner);
> >
> > + mutex_lock(_governor_lock);
> > + policy->governor_busy = false;
> > + mutex_unlock(_governor_lock);
> > return ret;
> > }
> >
>
> This doesn't solve the problem completely: it prevents the store_*() task
> from continuing *only* when it concurrently executes the __cpufreq_governor()
> function along with the CPU offline task. But if the two calls don't overlap,
> we will still have the possibility where the store_*() task tries to acquire
> the timer mutex after the CPU offline task has just finished destroying it.
Yeah, I overlooked that.
> So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
> store_*() thread will wait until the entire CPU offline operation is
> completed.
> After that, if it continues, it will get -EBUSY, due to patch [1], since
> policy->governor_enabled will be set to false.
>
> [1]. https://patchwork.kernel.org/patch/2852463/
>
> So here is the (completely untested) fix that I propose, as a replacement to
> this patch [2/2]:
That looks reasonable to me.
Viresh?
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 5c75e31..71c4fb2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -440,17 +440,24 @@ static ssize_t store_##file_name
> \
> unsigned int ret; \
> struct cpufreq_policy new_policy; \
> \
> + get_online_cpus(); \
> ret = cpufreq_get_policy(_policy, policy->cpu); \
> - if (ret)\
> - return -EINVAL; \
> + if (ret) { \
> + ret = -EINVAL; \
> + goto out; \
> + } \
> \
> ret = sscanf(buf, "%u", _policy.object);\
> - if (ret != 1) \
> - return -EINVAL; \
> + if (ret != 1) { \
> + ret = -EINVAL; \
> + goto out; \
> + } \
> \
> ret = __cpufreq_set_policy(policy, _policy);\
> policy->user_policy.object = policy->object;\
> \
> +out: \
> + put_online_cpus(); \
> return ret ? ret : count;
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/01/2013 10:56 AM, Viresh Kumar wrote:
> We can't take a big lock around __cpufreq_governor() as this causes recursive
> locking for some cases. But calls to this routine must be serialized for every
> policy.
>
> Lets introduce another variable which would guarantee serialization here.
>
> Signed-off-by: Viresh Kumar
> ---
> drivers/cpufreq/cpufreq.c | 7 ++-
> include/linux/cpufreq.h | 1 +
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index f320a20..4d5723db 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
> *policy,
> policy->cpu, event);
>
> mutex_lock(_governor_lock);
> - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> + if (policy->governor_busy ||
> + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
> (event == CPUFREQ_GOV_STOP {
> mutex_unlock(_governor_lock);
> return -EBUSY;
> }
>
> + policy->governor_busy = true;
> if (event == CPUFREQ_GOV_STOP)
> policy->governor_enabled = false;
> else if (event == CPUFREQ_GOV_START)
> @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
> *policy,
> ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> module_put(policy->governor->owner);
>
> + mutex_lock(_governor_lock);
> + policy->governor_busy = false;
> + mutex_unlock(_governor_lock);
> return ret;
> }
>
This doesn't solve the problem completely: it prevents the store_*() task
from continuing *only* when it concurrently executes the __cpufreq_governor()
function along with the CPU offline task. But if the two calls don't overlap,
we will still have the possibility where the store_*() task tries to acquire
the timer mutex after the CPU offline task has just finished destroying it.
So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
store_*() thread will wait until the entire CPU offline operation is completed.
After that, if it continues, it will get -EBUSY, due to patch [1], since
policy->governor_enabled will be set to false.
[1]. https://patchwork.kernel.org/patch/2852463/
So here is the (completely untested) fix that I propose, as a replacement to
this patch [2/2]:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5c75e31..71c4fb2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -440,17 +440,24 @@ static ssize_t store_##file_name
\
unsigned int ret; \
struct cpufreq_policy new_policy; \
\
+ get_online_cpus(); \
ret = cpufreq_get_policy(_policy, policy->cpu); \
- if (ret)\
- return -EINVAL; \
+ if (ret) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = sscanf(buf, "%u", _policy.object);\
- if (ret != 1) \
- return -EINVAL; \
+ if (ret != 1) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = __cpufreq_set_policy(policy, _policy);\
policy->user_policy.object = policy->object;\
\
+out: \
+ put_online_cpus(); \
return ret ? ret : count; \
}
@@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct
cpufreq_policy *policy,
charstr_governor[16];
struct cpufreq_policy new_policy;
+ get_online_cpus();
ret =
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
On 09/01/2013 10:56 AM, Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes
recursive
locking for some cases. But calls to this routine must be serialized for
every
policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
---
drivers/cpufreq/cpufreq.c | 7 ++-
include/linux/cpufreq.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
policy-cpu, event);
mutex_lock(cpufreq_governor_lock);
- if ((policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
+ if (policy-governor_busy ||
+ (policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
(!policy-governor_enabled ((event == CPUFREQ_GOV_LIMITS) ||
(event == CPUFREQ_GOV_STOP {
mutex_unlock(cpufreq_governor_lock);
return -EBUSY;
}
+ policy-governor_busy = true;
if (event == CPUFREQ_GOV_STOP)
policy-governor_enabled = false;
else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
((event == CPUFREQ_GOV_POLICY_EXIT) !ret))
module_put(policy-governor-owner);
+ mutex_lock(cpufreq_governor_lock);
+ policy-governor_busy = false;
+ mutex_unlock(cpufreq_governor_lock);
return ret;
}
This doesn't solve the problem completely: it prevents the store_*() task
from continuing *only* when it concurrently executes the __cpufreq_governor()
function along with the CPU offline task. But if the two calls don't overlap,
we will still have the possibility where the store_*() task tries to acquire
the timer mutex after the CPU offline task has just finished destroying it.
Yeah, I overlooked that.
So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
store_*() thread will wait until the entire CPU offline operation is
completed.
After that, if it continues, it will get -EBUSY, due to patch [1], since
policy-governor_enabled will be set to false.
[1]. https://patchwork.kernel.org/patch/2852463/
So here is the (completely untested) fix that I propose, as a replacement to
this patch [2/2]:
That looks reasonable to me.
Viresh?
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5c75e31..71c4fb2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -440,17 +440,24 @@ static ssize_t store_##file_name
\
unsigned int ret; \
struct cpufreq_policy new_policy; \
\
+ get_online_cpus(); \
ret = cpufreq_get_policy(new_policy, policy-cpu); \
- if (ret)\
- return -EINVAL; \
+ if (ret) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = sscanf(buf, %u, new_policy.object);\
- if (ret != 1) \
- return -EINVAL; \
+ if (ret != 1) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = __cpufreq_set_policy(policy, new_policy);\
policy-user_policy.object = policy-object;\
\
+out: \
+ put_online_cpus(); \
return ret ? ret : count; \
}
@@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct
cpufreq_policy
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 09/01/2013 10:56 AM, Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive
locking for some cases. But calls to this routine must be serialized for every
policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
---
drivers/cpufreq/cpufreq.c | 7 ++-
include/linux/cpufreq.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
policy-cpu, event);
mutex_lock(cpufreq_governor_lock);
- if ((policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
+ if (policy-governor_busy ||
+ (policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
(!policy-governor_enabled ((event == CPUFREQ_GOV_LIMITS) ||
(event == CPUFREQ_GOV_STOP {
mutex_unlock(cpufreq_governor_lock);
return -EBUSY;
}
+ policy-governor_busy = true;
if (event == CPUFREQ_GOV_STOP)
policy-governor_enabled = false;
else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
((event == CPUFREQ_GOV_POLICY_EXIT) !ret))
module_put(policy-governor-owner);
+ mutex_lock(cpufreq_governor_lock);
+ policy-governor_busy = false;
+ mutex_unlock(cpufreq_governor_lock);
return ret;
}
This doesn't solve the problem completely: it prevents the store_*() task
from continuing *only* when it concurrently executes the __cpufreq_governor()
function along with the CPU offline task. But if the two calls don't overlap,
we will still have the possibility where the store_*() task tries to acquire
the timer mutex after the CPU offline task has just finished destroying it.
So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the
store_*() thread will wait until the entire CPU offline operation is completed.
After that, if it continues, it will get -EBUSY, due to patch [1], since
policy-governor_enabled will be set to false.
[1]. https://patchwork.kernel.org/patch/2852463/
So here is the (completely untested) fix that I propose, as a replacement to
this patch [2/2]:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5c75e31..71c4fb2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -440,17 +440,24 @@ static ssize_t store_##file_name
\
unsigned int ret; \
struct cpufreq_policy new_policy; \
\
+ get_online_cpus(); \
ret = cpufreq_get_policy(new_policy, policy-cpu); \
- if (ret)\
- return -EINVAL; \
+ if (ret) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = sscanf(buf, %u, new_policy.object);\
- if (ret != 1) \
- return -EINVAL; \
+ if (ret != 1) { \
+ ret = -EINVAL; \
+ goto out; \
+ } \
\
ret = __cpufreq_set_policy(policy, new_policy);\
policy-user_policy.object = policy-object;\
\
+out: \
+ put_online_cpus(); \
return ret ? ret : count; \
}
@@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct
cpufreq_policy *policy,
charstr_governor[16];
struct cpufreq_policy new_policy;
+ get_online_cpus();
ret =
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote: > On 1 September 2013 18:58, Rafael J. Wysocki wrote: > > On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote: > >> We can't take a big lock around __cpufreq_governor() as this causes > >> recursive > >> locking for some cases. But calls to this routine must be serialized for > >> every > >> policy. > > > > Care to explain here why it must be serialized? > > Yes, added that to the attached patches (Added reported-by too): OK, the attached patches queued up for 3.12. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote: On 1 September 2013 18:58, Rafael J. Wysocki r...@sisk.pl wrote: On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote: We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy. Care to explain here why it must be serialized? Yes, added that to the attached patches (Added reported-by too): OK, the attached patches queued up for 3.12. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 2 September 2013 01:57, Rafael J. Wysocki wrote: > The second tab is one too many, I usually write such things like this: > > if (policy->governor_busy > || (policy->governor_enabled && event == CPUFREQ_GOV_START) > || ... > > Then it is much easier to distinguish the conditional code from the condition > itself. I see... I tried to do it this way but got confused again :) You fix it this time and I will understand it from that :) -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote: > On 1 September 2013 18:58, Rafael J. Wysocki wrote: > > On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote: > >> We can't take a big lock around __cpufreq_governor() as this causes > >> recursive > >> locking for some cases. But calls to this routine must be serialized for > >> every > >> policy. > > > > Care to explain here why it must be serialized? > > Yes, added that to the attached patches (Added reported-by too): > > commit dc51771506b113b998c49c3be2db0fb88bb92153 > Author: Viresh Kumar > Date: Sat Aug 31 17:48:23 2013 +0530 > > cpufreq: serialize calls to __cpufreq_governor() > > We can't take a big lock around __cpufreq_governor() as this > causes recursive > locking for some cases. But calls to this routine must be > serialized for every > policy. Otherwise we can see some unpredictable events. > > For example, consider following scenario: > > __cpufreq_remove_dev() > __cpufreq_governor(policy, CPUFREQ_GOV_STOP); >policy->governor->governor(policy, CPUFREQ_GOV_STOP); > cpufreq_governor_dbs() > case CPUFREQ_GOV_STOP: > mutex_destroy(_cdbs->timer_mutex) > cpu_cdbs->cur_policy = NULL; > > store() > __cpufreq_set_policy() > __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); > policy->governor->governor(policy, CPUFREQ_GOV_LIMITS); > case CPUFREQ_GOV_LIMITS: > mutex_lock(_cdbs->timer_mutex); <-- Warning (destroyed mutex) >if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL > > And so store() will eventually result in a crash cur_policy is already > NULL; > > Lets introduce another variable which would guarantee serialization here. > >Reported-by: Stephen Boyd >Signed-off-by: Viresh Kumar > > >> Lets introduce another variable which would guarantee serialization here. > >> > >> Signed-off-by: Viresh Kumar > >> --- > >> drivers/cpufreq/cpufreq.c | 7 ++- > >> include/linux/cpufreq.h | 1 + > >> 2 files changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > >> index f320a20..4d5723db 100644 > >> --- a/drivers/cpufreq/cpufreq.c > >> +++ b/drivers/cpufreq/cpufreq.c > >> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct > >> cpufreq_policy *policy, > >> policy->cpu, event); > >> > >> mutex_lock(_governor_lock); > >> - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) || > >> + if (policy->governor_busy || > >> + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) || > > > > Again, broken white space, but I can fix it up. > > Sorry, what exactly.. Sorry couldn't understand it :( The second tab is one too many, I usually write such things like this: if (policy->governor_busy || (policy->governor_enabled && event == CPUFREQ_GOV_START) || ... Then it is much easier to distinguish the conditional code from the condition itself. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 1 September 2013 18:58, Rafael J. Wysocki wrote: > On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote: >> We can't take a big lock around __cpufreq_governor() as this causes recursive >> locking for some cases. But calls to this routine must be serialized for >> every >> policy. > > Care to explain here why it must be serialized? Yes, added that to the attached patches (Added reported-by too): commit dc51771506b113b998c49c3be2db0fb88bb92153 Author: Viresh Kumar Date: Sat Aug 31 17:48:23 2013 +0530 cpufreq: serialize calls to __cpufreq_governor() We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy. Otherwise we can see some unpredictable events. For example, consider following scenario: __cpufreq_remove_dev() __cpufreq_governor(policy, CPUFREQ_GOV_STOP); policy->governor->governor(policy, CPUFREQ_GOV_STOP); cpufreq_governor_dbs() case CPUFREQ_GOV_STOP: mutex_destroy(_cdbs->timer_mutex) cpu_cdbs->cur_policy = NULL; store() __cpufreq_set_policy() __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); policy->governor->governor(policy, CPUFREQ_GOV_LIMITS); case CPUFREQ_GOV_LIMITS: mutex_lock(_cdbs->timer_mutex); <-- Warning (destroyed mutex) if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL And so store() will eventually result in a crash cur_policy is already NULL; Lets introduce another variable which would guarantee serialization here. Reported-by: Stephen Boyd Signed-off-by: Viresh Kumar >> Lets introduce another variable which would guarantee serialization here. >> >> Signed-off-by: Viresh Kumar >> --- >> drivers/cpufreq/cpufreq.c | 7 ++- >> include/linux/cpufreq.h | 1 + >> 2 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index f320a20..4d5723db 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy >> *policy, >> policy->cpu, event); >> >> mutex_lock(_governor_lock); >> - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) || >> + if (policy->governor_busy || >> + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) || > > Again, broken white space, but I can fix it up. Sorry, what exactly.. Sorry couldn't understand it :( 0001-cpufreq-don-t-allow-governor-limits-to-be-changed-wh.patch Description: Binary data 0002-cpufreq-serialize-calls-to-__cpufreq_governor.patch Description: Binary data
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
> We can't take a big lock around __cpufreq_governor() as this causes recursive
> locking for some cases. But calls to this routine must be serialized for every
> policy.
Care to explain here why it must be serialized?
> Lets introduce another variable which would guarantee serialization here.
>
> Signed-off-by: Viresh Kumar
> ---
> drivers/cpufreq/cpufreq.c | 7 ++-
> include/linux/cpufreq.h | 1 +
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index f320a20..4d5723db 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
> *policy,
> policy->cpu, event);
>
> mutex_lock(_governor_lock);
> - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
> + if (policy->governor_busy ||
> + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
Again, broken white space, but I can fix it up.
> (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
> (event == CPUFREQ_GOV_STOP {
> mutex_unlock(_governor_lock);
> return -EBUSY;
> }
>
> + policy->governor_busy = true;
> if (event == CPUFREQ_GOV_STOP)
> policy->governor_enabled = false;
> else if (event == CPUFREQ_GOV_START)
> @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
> *policy,
> ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
> module_put(policy->governor->owner);
>
> + mutex_lock(_governor_lock);
> + policy->governor_busy = false;
> + mutex_unlock(_governor_lock);
> return ret;
> }
>
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d568f39..cca885d 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -76,6 +76,7 @@ struct cpufreq_policy {
> struct cpufreq_governor *governor; /* see below */
> void*governor_data;
> boolgovernor_enabled; /* governor start/stop flag */
> + boolgovernor_busy;
>
> struct work_struct update; /* if update_policy() needs to be
>* called, but you're in IRQ context */
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive
locking for some cases. But calls to this routine must be serialized for every
policy.
Care to explain here why it must be serialized?
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
---
drivers/cpufreq/cpufreq.c | 7 ++-
include/linux/cpufreq.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
policy-cpu, event);
mutex_lock(cpufreq_governor_lock);
- if ((policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
+ if (policy-governor_busy ||
+ (policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
Again, broken white space, but I can fix it up.
(!policy-governor_enabled ((event == CPUFREQ_GOV_LIMITS) ||
(event == CPUFREQ_GOV_STOP {
mutex_unlock(cpufreq_governor_lock);
return -EBUSY;
}
+ policy-governor_busy = true;
if (event == CPUFREQ_GOV_STOP)
policy-governor_enabled = false;
else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
((event == CPUFREQ_GOV_POLICY_EXIT) !ret))
module_put(policy-governor-owner);
+ mutex_lock(cpufreq_governor_lock);
+ policy-governor_busy = false;
+ mutex_unlock(cpufreq_governor_lock);
return ret;
}
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..cca885d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -76,6 +76,7 @@ struct cpufreq_policy {
struct cpufreq_governor *governor; /* see below */
void*governor_data;
boolgovernor_enabled; /* governor start/stop flag */
+ boolgovernor_busy;
struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 1 September 2013 18:58, Rafael J. Wysocki r...@sisk.pl wrote: On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote: We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy. Care to explain here why it must be serialized? Yes, added that to the attached patches (Added reported-by too): commit dc51771506b113b998c49c3be2db0fb88bb92153 Author: Viresh Kumar viresh.ku...@linaro.org Date: Sat Aug 31 17:48:23 2013 +0530 cpufreq: serialize calls to __cpufreq_governor() We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy. Otherwise we can see some unpredictable events. For example, consider following scenario: __cpufreq_remove_dev() __cpufreq_governor(policy, CPUFREQ_GOV_STOP); policy-governor-governor(policy, CPUFREQ_GOV_STOP); cpufreq_governor_dbs() case CPUFREQ_GOV_STOP: mutex_destroy(cpu_cdbs-timer_mutex) cpu_cdbs-cur_policy = NULL; PREEMPT store() __cpufreq_set_policy() __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); policy-governor-governor(policy, CPUFREQ_GOV_LIMITS); case CPUFREQ_GOV_LIMITS: mutex_lock(cpu_cdbs-timer_mutex); -- Warning (destroyed mutex) if (policy-max cpu_cdbs-cur_policy-cur) - cur_policy == NULL And so store() will eventually result in a crash cur_policy is already NULL; Lets introduce another variable which would guarantee serialization here. Reported-by: Stephen Boyd sb...@codeaurora.org Signed-off-by: Viresh Kumar viresh.ku...@linaro.org Lets introduce another variable which would guarantee serialization here. Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- drivers/cpufreq/cpufreq.c | 7 ++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy-cpu, event); mutex_lock(cpufreq_governor_lock); - if ((policy-governor_enabled (event == CPUFREQ_GOV_START)) || + if (policy-governor_busy || + (policy-governor_enabled (event == CPUFREQ_GOV_START)) || Again, broken white space, but I can fix it up. Sorry, what exactly.. Sorry couldn't understand it :( 0001-cpufreq-don-t-allow-governor-limits-to-be-changed-wh.patch Description: Binary data 0002-cpufreq-serialize-calls-to-__cpufreq_governor.patch Description: Binary data
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote: On 1 September 2013 18:58, Rafael J. Wysocki r...@sisk.pl wrote: On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote: We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy. Care to explain here why it must be serialized? Yes, added that to the attached patches (Added reported-by too): commit dc51771506b113b998c49c3be2db0fb88bb92153 Author: Viresh Kumar viresh.ku...@linaro.org Date: Sat Aug 31 17:48:23 2013 +0530 cpufreq: serialize calls to __cpufreq_governor() We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy. Otherwise we can see some unpredictable events. For example, consider following scenario: __cpufreq_remove_dev() __cpufreq_governor(policy, CPUFREQ_GOV_STOP); policy-governor-governor(policy, CPUFREQ_GOV_STOP); cpufreq_governor_dbs() case CPUFREQ_GOV_STOP: mutex_destroy(cpu_cdbs-timer_mutex) cpu_cdbs-cur_policy = NULL; PREEMPT store() __cpufreq_set_policy() __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); policy-governor-governor(policy, CPUFREQ_GOV_LIMITS); case CPUFREQ_GOV_LIMITS: mutex_lock(cpu_cdbs-timer_mutex); -- Warning (destroyed mutex) if (policy-max cpu_cdbs-cur_policy-cur) - cur_policy == NULL And so store() will eventually result in a crash cur_policy is already NULL; Lets introduce another variable which would guarantee serialization here. Reported-by: Stephen Boyd sb...@codeaurora.org Signed-off-by: Viresh Kumar viresh.ku...@linaro.org Lets introduce another variable which would guarantee serialization here. Signed-off-by: Viresh Kumar viresh.ku...@linaro.org --- drivers/cpufreq/cpufreq.c | 7 ++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy-cpu, event); mutex_lock(cpufreq_governor_lock); - if ((policy-governor_enabled (event == CPUFREQ_GOV_START)) || + if (policy-governor_busy || + (policy-governor_enabled (event == CPUFREQ_GOV_START)) || Again, broken white space, but I can fix it up. Sorry, what exactly.. Sorry couldn't understand it :( The second tab is one too many, I usually write such things like this: if (policy-governor_busy || (policy-governor_enabled event == CPUFREQ_GOV_START) || ... Then it is much easier to distinguish the conditional code from the condition itself. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
On 2 September 2013 01:57, Rafael J. Wysocki r...@sisk.pl wrote: The second tab is one too many, I usually write such things like this: if (policy-governor_busy || (policy-governor_enabled event == CPUFREQ_GOV_START) || ... Then it is much easier to distinguish the conditional code from the condition itself. I see... I tried to do it this way but got confused again :) You fix it this time and I will understand it from that :) -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
We can't take a big lock around __cpufreq_governor() as this causes recursive
locking for some cases. But calls to this routine must be serialized for every
policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar
---
drivers/cpufreq/cpufreq.c | 7 ++-
include/linux/cpufreq.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
policy->cpu, event);
mutex_lock(_governor_lock);
- if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
+ if (policy->governor_busy ||
+ (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||
(event == CPUFREQ_GOV_STOP {
mutex_unlock(_governor_lock);
return -EBUSY;
}
+ policy->governor_busy = true;
if (event == CPUFREQ_GOV_STOP)
policy->governor_enabled = false;
else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
module_put(policy->governor->owner);
+ mutex_lock(_governor_lock);
+ policy->governor_busy = false;
+ mutex_unlock(_governor_lock);
return ret;
}
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..cca885d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -76,6 +76,7 @@ struct cpufreq_policy {
struct cpufreq_governor *governor; /* see below */
void*governor_data;
boolgovernor_enabled; /* governor start/stop flag */
+ boolgovernor_busy;
struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */
--
1.7.12.rc2.18.g61b472e
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()
We can't take a big lock around __cpufreq_governor() as this causes recursive
locking for some cases. But calls to this routine must be serialized for every
policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
---
drivers/cpufreq/cpufreq.c | 7 ++-
include/linux/cpufreq.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f320a20..4d5723db 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
policy-cpu, event);
mutex_lock(cpufreq_governor_lock);
- if ((policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
+ if (policy-governor_busy ||
+ (policy-governor_enabled (event == CPUFREQ_GOV_START)) ||
(!policy-governor_enabled ((event == CPUFREQ_GOV_LIMITS) ||
(event == CPUFREQ_GOV_STOP {
mutex_unlock(cpufreq_governor_lock);
return -EBUSY;
}
+ policy-governor_busy = true;
if (event == CPUFREQ_GOV_STOP)
policy-governor_enabled = false;
else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy
*policy,
((event == CPUFREQ_GOV_POLICY_EXIT) !ret))
module_put(policy-governor-owner);
+ mutex_lock(cpufreq_governor_lock);
+ policy-governor_busy = false;
+ mutex_unlock(cpufreq_governor_lock);
return ret;
}
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d568f39..cca885d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -76,6 +76,7 @@ struct cpufreq_policy {
struct cpufreq_governor *governor; /* see below */
void*governor_data;
boolgovernor_enabled; /* governor start/stop flag */
+ boolgovernor_busy;
struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */
--
1.7.12.rc2.18.g61b472e
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
