RFC: Leave sysfs nodes alone during hotplug
The adding and removing of sysfs nodes in cpufreq causes a ton of pain. There's always some stability or deadlock issue every few weeks on our internal tree. We sync up our internal tree fairly often with the upstream cpufreq code. And more of these issues are popping up as we start exercising the cpufreq framework for b.L systems or HMP systems.
It looks like we adding a lot of unnecessary complexity by adding and removing these sysfs nodes. The other per CPU sysfs nodes like:/sys/devices/system/cpu/cpu1/power or cpuidle are left alone during hotplug. So, why are we not doing the same for cpufreq too?
Any objections to leaving them alone during hotplug? If those files are read/written to when the entire cluster is hotplugged off, we could just return an error. I'm not saying it would be impossible to fix all these deadlock and race issues in the current code -- but it seems like a lot of pointless effort to remove/add sysfs nodes.
Examples of issues caused by this:
1. Race when changing governor really quickly from userspace. The governors end up getting 2 STOP or 2 START events. This was introduced by [1] when it tried to fix another deadlock issue.
2. Incorrect policy/sysfs handling during suspend/resume. Suspend takes out CPU in the order n, n+1, n+2, etc and resume adds them back in the same order. Both sysfs and policy ownership transfer aren't handled correctly in this case. This obviously applies even outside suspend/resume if the same sequence is repeated using just hotplug.
I'd be willing to take a shot at this if there isn't any objection to this. It's a lot of work/refactor -- so I don't want to spend a lot of time on it if there's a strong case for removing these sysfs nodes.
Thoughts?
-SaravanaP.S: I always find myself sending emails to the lists close to one holiday or another. Sigh.
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Cc'ing Srivatsa and fixing Rafael's id.
Quoted 10 lines · Saravana Kannan
The adding and removing of sysfs nodes in cpufreq causes a ton of pain. There's always some stability or deadlock issue every few weeks on our internal tree. We sync up our internal tree fairly often with the upstream cpufreq code. And more of these issues are popping up as we start exercising the cpufreq framework for b.L systems or HMP systems.
It looks like we adding a lot of unnecessary complexity by adding and removing these sysfs nodes. The other per CPU sysfs nodes like:
/sys/devices/system/cpu/cpu1/poweror cpuidle are left alone during hotplug. So, why are we not doing the same for cpufreq too?
This is how it had been since ever, don't know which method is correct. Though these are the requirements I have from them:
- On hotplug files values should get reset ..
- On suspend/resume values must be retained.
Quoted 5 lines
Any objections to leaving them alone during hotplug? If those files are read/written to when the entire cluster is hotplugged off, we could just return an error. I'm not saying it would be impossible to fix all these deadlock and race issues in the current code -- but it seems like a lot of pointless effort to remove/add sysfs nodes.
Lets understand the problem first and then can take the right decision.
Quoted 4 lines
Examples of issues caused by this:
1. Race when changing governor really quickly from userspace. The governors end up getting 2 STOP or 2 START events. This was introduced by [1] when it tried to fix another deadlock issue.
I was talking about [1] offline with Srivatsa, and one of us might look in detail why [1] was actually required.
But I don't know how exactly can we get 2 STOP/START in latest mainline code. As we have enough protection against that now.
So, we would really like to see some reports against mainline for this.
Quoted 4 lines
2. Incorrect policy/sysfs handling during suspend/resume. Suspend takes out CPU in the order n, n+1, n+2, etc and resume adds them back in the same order. Both sysfs and policy ownership transfer aren't handled correctly in this case.
I know few of these, but can you please tell what you have in mind?
Quoted 2 lines
This obviously applies even outside suspend/resume if the same sequence is repeated using just hotplug.
Again, what's the issue?
Quoted 3 lines
I'd be willing to take a shot at this if there isn't any objection to this. It's a lot of work/refactor -- so I don't want to spend a lot of time on it if there's a strong case for removing these sysfs nodes.
Sure, I fully understand this but still wanna understand the issue first.
Quoted 2 lines
P.S: I always find myself sending emails to the lists close to one holiday or another. Sigh.
Sorry for being late to reply to this. I saw it on friday, but couldn't reply whole day. Was following something with ticks core. :(
Rafael's email is bounced. Looks like I used a stale ID. Fixing it in this email.
Quoted 9 lines · Viresh Kumar
Cc'ing Srivatsa and fixing Rafael's id.
Quoted 6 lines · Saravana Kannan
The adding and removing of sysfs nodes in cpufreq causes a ton of pain. There's always some stability or deadlock issue every few weeks on our internal tree. We sync up our internal tree fairly often with the upstream cpufreq code. And more of these issues are popping up as we start exercising the cpufreq framework for b.L systems or HMP systems.
Quoted 6 lines
Quoted 4 lines
It looks like we adding a lot of unnecessary complexity by adding and removing these sysfs nodes. The other per CPU sysfs nodes like:
/sys/devices/system/cpu/cpu1/poweror cpuidle are left alone during hotplug. So, why are we not doing the same for cpufreq too?This is how it had been since ever, don't know which method is correct.
Rafael, If you don't have any objections, I would like to simplify it.
Quoted 3 lines
Though these are the requirements I have from them:
- On hotplug files values should get reset ..
- On suspend/resume values must be retained.
Hmm... There's actually enough interest in NOT reseting across hotplug because it's also used by thermal when a CPU gets too hot and then it's plugged in later. So, userspace has no way to cleanly restore the values. But that's a separate topic.
Quoted 7 lines
Quoted 5 lines
Any objections to leaving them alone during hotplug? If those files are read/written to when the entire cluster is hotplugged off, we could just return an error. I'm not saying it would be impossible to fix all these deadlock and race issues in the current code -- but it seems like a lot of pointless effort to remove/add sysfs nodes.
Lets understand the problem first and then can take the right decision.
My point is more of a, do we even need to allow this problem to happen instead of trying and fixing it. I see no benefit at all in removing/adding the files during hotplug.
Quoted 8 lines
Quoted 5 lines
Examples of issues caused by this:
1. Race when changing governor really quickly from userspace. The governors end up getting 2 STOP or 2 START events. This was introduced by [1] when it tried to fix another deadlock issue.I was talking about [1] offline with Srivatsa, and one of us might look in detail why [1] was actually required.
I believe it has to do with a race that's something along these lines:
1. Echo/store into a sysfs node grabbing a sysfs lock (inside sysfs code) before trying to grab one of the cpufreq locks
2. The hotplug path grabbing one of the cpufreq locks before trying to remove the sysfs group.
The two START/STOP happens because of [1]. It can happen when userspace is quickly changes governors back to back. Or say, multiple threads trying to store the same governor. The double START/STOP happens because the 2nd request is able to slip in when you unlock the rwsem for sending the policy INIT/EXIT.
Quoted 5 lines
But I don't know how exactly can we get 2 STOP/START in latest mainline code. As we have enough protection against that now.
So, we would really like to see some reports against mainline for this.
Is the above sufficient enough? It's really easy to trigger if you have a b.L system. Just slam scaling_governor between performance and another governor (or it might have been the same value). b.L is important because if you don't have multi-cluster, you don't get POLICY_EXIT often.
Quoted 6 lines
Quoted 4 lines
2. Incorrect policy/sysfs handling during suspend/resume. Suspend takes out CPU in the order n, n+1, n+2, etc and resume adds them back in the same order. Both sysfs and policy ownership transfer aren't handled correctly in this case.
I know few of these, but can you please tell what you have in mind?
Not sure what you mean by "what you have in mind". Just simple suspend/resume is broken. Again, easier to reproduce in b.L system since you need to actually have POLICY_EXIT happen.
Quoted 4 lines
Quoted 2 lines
This obviously applies even outside suspend/resume if the same sequence is repeated using just hotplug.
Again, what's the issue?
Just hotplug all CPUs in a cluster in one order and bring it up in another. It should crash or panic about sysfs. Basically, the kobj of the real sysfs stays with the last CPU that went down, but the first CPU that comes up owns the policy without owning the real kobj.
Quoted 5 lines
Quoted 3 lines
I'd be willing to take a shot at this if there isn't any objection to this. It's a lot of work/refactor -- so I don't want to spend a lot of time on it if there's a strong case for removing these sysfs nodes.
Sure, I fully understand this but still wanna understand the issue first.
We might be able to throw around more locks, etc to fix this. But again, my main point is that all this seems pointless.
We should just leave the sysfs nodes alone. We won't break any backward compatibility since userspace can't be depending on these to be present when a CPU was OFFLINE. Anything that depended on that would be broken userspace code anyway.
Quoted 5 lines
Quoted 2 lines
P.S: I always find myself sending emails to the lists close to one holiday or another. Sigh.Sorry for being late to reply to this. I saw it on friday, but couldn't reply whole day. Was following something with ticks core. :(
No worries. This was a fast enough :)
Quoted 3 lines
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
On Mon, Jul 7, 2014 at 12:30 PM, Saravana Kannan <[email protected]> wrote:
...
Quoted 8 lines
Quoted 3 lines
Though these are the requirements I have from them:
- On hotplug files values should get reset ..
- On suspend/resume values must be retained.Hmm... There's actually enough interest in NOT reseting across hotplug because it's also used by thermal when a CPU gets too hot and then it's plugged in later. So, userspace has no way to cleanly restore the values. But that's a separate topic.
For Android's usage we're also interested in both:
1. not removing and recreating the cpufreq sysfs files for a CPU on hotplug events (we currently use hotplug uevents to reset file ownership such that power policy can be controlled by non-root).
2. not resetting the contents of policy files such as scaling_max_freq (also fixed up from uevents) or stats files (we currently keep a separate persistent time_in_state for battery accounting purposes).
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Any objections to leaving them alone during hotplug? If those files are read/written to when the entire cluster is hotplugged off, we could just return an error. I'm not saying it would be impossible to fix all these deadlock and race issues in the current code -- but it seems like a lot of
pointless effort to remove/add sysfs nodes.
No objections from our standpoint.
Thanks -- Todd
Quoted 11 lines · Todd Poynor
On Mon, Jul 7, 2014 at 12:30 PM, Saravana Kannan <[email protected]> wrote:
...Quoted 8 lines
Quoted 3 lines
Though these are the requirements I have from them:
- On hotplug files values should get reset ..
- On suspend/resume values must be retained.Hmm... There's actually enough interest in NOT reseting across hotplug because it's also used by thermal when a CPU gets too hot and then it's plugged in later. So, userspace has no way to cleanly restore the values. But that's a separate topic.
Quoted 5 lines
For Android's usage we're also interested in both:
1. not removing and recreating the cpufreq sysfs files for a CPU on hotplug events (we currently use hotplug uevents to reset file ownership such that power policy can be controlled by non-root).
Ah thanks! This is another good point towards leaving the sysfs nodes alone that I forgot about.
Quoted 4 lines
2. not resetting the contents of policy files such as
scaling_max_freq(also fixed up from uevents) or stats files (we currently keep a separate persistenttime_in_statefor battery accounting purposes).
Quoted 9 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Any objections to leaving them alone during hotplug? If those files are read/written to when the entire cluster is hotplugged off, we could just return an error. I'm not saying it would be impossible to fix all these deadlock and race issues in the current code -- but it seems like a lot of
pointless effort to remove/add sysfs nodes.No objections from our standpoint.
Thanks,
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 9 lines · Todd Poynor
For Android's usage we're also interested in both:
1. not removing and recreating the cpufreq sysfs files for a CPU on hotplug events (we currently use hotplug uevents to reset file ownership such that power policy can be controlled by non-root).
2. not resetting the contents of policy files such as
scaling_max_freq(also fixed up from uevents) or stats files (we currently keep a separate persistenttime_in_statefor battery accounting purposes).
So, we actually need to retain all the files. I will try to see this separately. Will add it in my todo list.
Quoted 7 lines · Viresh Kumar
Quoted 6 lines · Todd Poynor
For Android's usage we're also interested in both:
1. not removing and recreating the cpufreq sysfs files for a CPU on hotplug events (we currently use hotplug uevents to reset file ownership such that power policy can be controlled by non-root).
Quoted 7 lines
Quoted 3 lines
2. not resetting the contents of policy files such as
scaling_max_freq(also fixed up from uevents) or stats files (we currently keep a separate persistenttime_in_statefor battery accounting purposes).So, we actually need to retain all the files. I will try to see this separately. Will add it in my todo list.
Looks like there's enough interest. I'll also try to send out a patch of what I think a simplified CPUfreq should look like.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoted 2 lines · On 8 July 2014 09:00, <[email protected]> wrote:
Looks like there's enough interest. I'll also try to send out a patch of what I think a simplified CPUfreq should look like.
Okay, will wait for it then.
Quoted 6 lines · Viresh Kumar
Quoted 2 lines · On 8 July 2014 09:00, <[email protected]> wrote:
Looks like there's enough interest. I'll also try to send out a patch of what I think a simplified CPUfreq should look like.
Okay, will wait for it then.
Sent a super preliminary version to give an idea of what I'm going for. Future patches would be replies to that thread.
-Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Quoted 2 lines · Saravana Kannan
Rafael's email is bounced. Looks like I used a stale ID. Fixing it in this email.
Because you have the wrong address initially, which I fixed and told you below. You missed that and again added the wrong address :)
Fixing it yet again. Don't change it now :)
Quoted 10 lines · Viresh Kumar
Quoted 3 lines
Though these are the requirements I have from them:
- On hotplug files values should get reset ..
- On suspend/resume values must be retained.Hmm... There's actually enough interest in NOT reseting across hotplug because it's also used by thermal when a CPU gets too hot and then it's plugged in later. So, userspace has no way to cleanly restore the values. But that's a separate topic.
Okay, will think about it more and see what's the right thing to do apart from the requirements we have.
Quoted 3 lines
My point is more of a, do we even need to allow this problem to happen instead of trying and fixing it. I see no benefit at all in removing/adding the files during hotplug.
We can surely think separately if leaving these sysfs directories is the right thing to do, but we need to see why this bug is shown up again.
Quoted 10 lines
Quoted 2 lines
I was talking about [1] offline with Srivatsa, and one of us might look in detail why [1] was actually required.
I believe it has to do with a race that's something along these lines:
1. Echo/store into a sysfs node grabbing a sysfs lock (inside sysfs code) before trying to grab one of the cpufreq locks
2. The hotplug path grabbing one of the cpufreq locks before trying to remove the sysfs group.
Quoted 5 lines
The two START/STOP happens because of [1]. It can happen when userspace is quickly changes governors back to back. Or say, multiple threads trying to store the same governor. The double START/STOP happens because the 2nd request is able to slip in when you unlock the rwsem for sending the policy INIT/EXIT.
Yeah, we still need to see how can we avoid dropping of locks before exit.
Quoted 10 lines
Quoted 4 lines
But I don't know how exactly can we get 2 STOP/START in latest mainline code. As we have enough protection against that now.
So, we would really like to see some reports against mainline for this.
Is the above sufficient enough? It's really easy to trigger if you have a b.L system. Just slam
scaling_governorbetween performance and another governor (or it might have been the same value). b.L is important because if you don't have multi-cluster, you don't getPOLICY_EXIToften.
Hmm, b.L. isn't new to cpufreq. I did worked a lot with it over one and half year back which working on TC2, and that's when I came very close to cpufreq internals.
There were lots of problems around hotplug paths, etc, but all are fixed now and nothing pending is in my knowledge.
And inside Linaro we do have automated testing going on TC2 with hotplug/suspend/resume tests and nothing is reported since sometime. We also have Juno (64 bit b.L.) onboard now, and nothing reported on that as well.
Quoted 1 line
Not sure what you mean by "what you have in mind". Just simple
I meant the problem you are trying to mention.
Quoted 2 lines
suspend/resume is broken. Again, easier to reproduce in b.L system since you need to actually have
POLICY_EXIThappen.
Oh, I want to believe that but couldn't. This has been tested well on TC2. Can you show something against mainline to prove this issue? I tend to believe that you are working with some old version of tree..
Quoted 4 lines
Just hotplug all CPUs in a cluster in one order and bring it up in another. It should crash or panic about sysfs. Basically, the kobj of the real sysfs stays with the last CPU that went down, but the first CPU that comes up owns the policy without owning the real kobj.
These issues were already fixed, can you try mainline please?
Quoted 2 lines
We might be able to throw around more locks, etc to fix this. But again, my main point is that all this seems pointless.
No more locks. But I do agree that we need some simplicity in code here. Will address that separately, lets fix your issues first.