| Message ID | 1452533760-13787-12-git-send-email-juri.lelli@arm.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On 11-01-16, 17:35, Juri Lelli wrote: > __cpufreq_governor works on policy, so policy->rwsem has to be held. > Add assertion for such condition. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Juri Lelli <juri.lelli@arm.com> > --- > drivers/cpufreq/cpufreq.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index f1f9fbc..e7fc5c9 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1950,6 +1950,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, > /* Don't start any governor operations if we are entering suspend */ > if (cpufreq_suspended) > return 0; > + > + lockdep_assert_held(&policy->rwsem); > + We had an ABBA problem with the EXIT governor callback and so this rwsem is dropped just before that from set_policy().. commit 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT")
On 01/12/2016 02:20 AM, Viresh Kumar wrote: > On 11-01-16, 17:35, Juri Lelli wrote: >> __cpufreq_governor works on policy, so policy->rwsem has to be held. >> Add assertion for such condition. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Signed-off-by: Juri Lelli <juri.lelli@arm.com> >> --- >> drivers/cpufreq/cpufreq.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index f1f9fbc..e7fc5c9 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1950,6 +1950,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, >> /* Don't start any governor operations if we are entering suspend */ >> if (cpufreq_suspended) >> return 0; >> + >> + lockdep_assert_held(&policy->rwsem); >> + > > We had an ABBA problem with the EXIT governor callback and so this > rwsem is dropped just before that from set_policy().. > > commit 955ef4833574 ("cpufreq: Drop rwsem lock around > CPUFREQ_GOV_POLICY_EXIT") > AFAIR, the ABBA issue was between the sysfs lock and the policy lock. The fix for that issue should not be dropping the lock around POLICY_EXIT. The proper fix is to have the governor "export" the attributes it wants to add/remove and have the cpufreq framework do the adding/removing of the attributes from sysfs for the governor. Thanks, Saravana
On Friday, January 29, 2016 04:33:39 PM Saravana Kannan wrote: > On 01/12/2016 02:20 AM, Viresh Kumar wrote: > > On 11-01-16, 17:35, Juri Lelli wrote: > >> __cpufreq_governor works on policy, so policy->rwsem has to be held. > >> Add assertion for such condition. > >> > >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > >> Cc: Viresh Kumar <viresh.kumar@linaro.org> > >> Signed-off-by: Juri Lelli <juri.lelli@arm.com> > >> --- > >> drivers/cpufreq/cpufreq.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > >> index f1f9fbc..e7fc5c9 100644 > >> --- a/drivers/cpufreq/cpufreq.c > >> +++ b/drivers/cpufreq/cpufreq.c > >> @@ -1950,6 +1950,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, > >> /* Don't start any governor operations if we are entering suspend */ > >> if (cpufreq_suspended) > >> return 0; > >> + > >> + lockdep_assert_held(&policy->rwsem); > >> + > > > > We had an ABBA problem with the EXIT governor callback and so this > > rwsem is dropped just before that from set_policy().. > > > > commit 955ef4833574 ("cpufreq: Drop rwsem lock around > > CPUFREQ_GOV_POLICY_EXIT") > > > > AFAIR, the ABBA issue was between the sysfs lock and the policy lock. > The fix for that issue should not be dropping the lock around > POLICY_EXIT. Right. Dropping the lock is a mistake (which I overlooked, sadly). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30-01-16, 12:49, Rafael J. Wysocki wrote: > On Friday, January 29, 2016 04:33:39 PM Saravana Kannan wrote: > > AFAIR, the ABBA issue was between the sysfs lock and the policy lock. Yeah, to be precise here it is: CPU0 (sysfs read) CPU1 (exit governor) sysfs-read set_policy()-> lock policy->rwsem sysfs-active lock Remove sysfs files lock policy->rwsem sysfs-active lock Actual read > > The fix for that issue should not be dropping the lock around > > POLICY_EXIT. > > Right. Dropping the lock is a mistake (which I overlooked, sadly). I joined the party at around time of 3.10, and we had this problem and hacky solution then as well. We tried to get rid of it multiple times, but sadly failed. > > The proper fix is to have the governor "export" the > > attributes it wants to add/remove and have the cpufreq framework do > > the adding/removing of the attributes from sysfs for the governor. I failed to understand your solution, sorry. Care to explain this a bit more?
On Mon, Feb 1, 2016 at 7:09 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 30-01-16, 12:49, Rafael J. Wysocki wrote: >> On Friday, January 29, 2016 04:33:39 PM Saravana Kannan wrote: >> > AFAIR, the ABBA issue was between the sysfs lock and the policy lock. > > Yeah, to be precise here it is: > > CPU0 (sysfs read) CPU1 (exit governor) > > sysfs-read set_policy()-> lock policy->rwsem > sysfs-active lock Remove sysfs files > lock policy->rwsem sysfs-active lock > Actual read > >> > The fix for that issue should not be dropping the lock around >> > POLICY_EXIT. >> >> Right. Dropping the lock is a mistake (which I overlooked, sadly). > > I joined the party at around time of 3.10, and we had this problem and > hacky solution then as well. We tried to get rid of it multiple times, > but sadly failed. I kind of like your idea of accessing governor attributes without holding the policy rwsem. I looked at that code and it seems doable to me. The problem to solve there would be to ensure that the dbs_data pointer is valid when show/store runs for those attributes. The fact that we make the distinction between global and policy governors in there doesn't really help, but it looks like getting rid of that bit wouldn't be too much effort. Let me take a deeper look at that. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/01/2016 02:22 AM, Rafael J. Wysocki wrote: > On Mon, Feb 1, 2016 at 7:09 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> On 30-01-16, 12:49, Rafael J. Wysocki wrote: >>> On Friday, January 29, 2016 04:33:39 PM Saravana Kannan wrote: >>>> AFAIR, the ABBA issue was between the sysfs lock and the policy lock. >> >> Yeah, to be precise here it is: >> >> CPU0 (sysfs read) CPU1 (exit governor) >> >> sysfs-read set_policy()-> lock policy->rwsem >> sysfs-active lock Remove sysfs files >> lock policy->rwsem sysfs-active lock >> Actual read >> >>>> The fix for that issue should not be dropping the lock around >>>> POLICY_EXIT. >>> >>> Right. Dropping the lock is a mistake (which I overlooked, sadly). >> >> I joined the party at around time of 3.10, and we had this problem and >> hacky solution then as well. We tried to get rid of it multiple times, >> but sadly failed. > > I kind of like your idea of accessing governor attributes without > holding the policy rwsem. I'm not sure whose idea you are referring to. Viresh's (I don't think I saw his proposal) or mine. > I looked at that code and it seems doable to me. The problem to solve > there would be to ensure that the dbs_data pointer is valid when > show/store runs for those attributes. > > The fact that we make the distinction between global and policy > governors in there doesn't really help, but it looks like getting rid > of that bit wouldn't be too much effort. Let me take a deeper look at > that. > Anyway, to explain my suggestion better, I'm proposing to make it so that we don't have a need for the AB BA locking. The only reason the governor needs to even grab the sysfs lock is to add/remove the sysfs attribute files. That can be easily achieved if the policy struct has some "gov_attrs" field(s) that each governor populates. Then the framework just has to create them after POLICY_INIT is processed by the governor and remove them before POILICY_EXIT is sent to the governor. That way, we also avoid having to worry about the gov attributes accessed by the show/store disappearing while the files are being accessed. Since we remove those files before we even ask the gov to clean up, that situation can never happen. The current problem is that there is no good place for the governor to populate this "gov_attrs" field(s). Maybe the governor register might be one place for it to provide the data to the framework and the framework can later fill it up itself when switching governors. Thanks, Saravana
On Mon, Feb 1, 2016 at 9:24 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 02/01/2016 02:22 AM, Rafael J. Wysocki wrote: >> >> On Mon, Feb 1, 2016 at 7:09 AM, Viresh Kumar <viresh.kumar@linaro.org> >> wrote: >>> >>> On 30-01-16, 12:49, Rafael J. Wysocki wrote: >>>> >>>> On Friday, January 29, 2016 04:33:39 PM Saravana Kannan wrote: >>>>> >>>>> AFAIR, the ABBA issue was between the sysfs lock and the policy lock. >>> >>> >>> Yeah, to be precise here it is: >>> >>> CPU0 (sysfs read) CPU1 (exit governor) >>> >>> sysfs-read set_policy()-> lock policy->rwsem >>> sysfs-active lock Remove sysfs files >>> lock policy->rwsem sysfs-active lock >>> Actual read >>> >>>>> The fix for that issue should not be dropping the lock around >>>>> POLICY_EXIT. >>>> >>>> >>>> Right. Dropping the lock is a mistake (which I overlooked, sadly). >>> >>> >>> I joined the party at around time of 3.10, and we had this problem and >>> hacky solution then as well. We tried to get rid of it multiple times, >>> but sadly failed. >> >> >> I kind of like your idea of accessing governor attributes without >> holding the policy rwsem. > > > I'm not sure whose idea you are referring to. Viresh's (I don't think I saw > his proposal) or mine. I meant a Viresh's idea that he discussed with Preeti Murthy a while ago (or maybe just pointed her to a message where it was outlined, I can't recall ATM). >> I looked at that code and it seems doable to me. The problem to solve >> there would be to ensure that the dbs_data pointer is valid when >> show/store runs for those attributes. >> >> The fact that we make the distinction between global and policy >> governors in there doesn't really help, but it looks like getting rid >> of that bit wouldn't be too much effort. Let me take a deeper look at >> that. >> > > Anyway, to explain my suggestion better, I'm proposing to make it so that we > don't have a need for the AB BA locking. The only reason the governor needs > to even grab the sysfs lock is to add/remove the sysfs attribute files. I'm not sure what you mean by "the sysfs lock" here? The policy rwsem or something else? > That can be easily achieved if the policy struct has some "gov_attrs" > field(s) that each governor populates. Then the framework just has to create > them after POLICY_INIT is processed by the governor and remove them before > POILICY_EXIT is sent to the governor. > > That way, we also avoid having to worry about the gov attributes accessed by > the show/store disappearing while the files are being accessed. Since we > remove those files before we even ask the gov to clean up, that situation > can never happen. > > The current problem is that there is no good place for the governor to > populate this "gov_attrs" field(s). Maybe the governor register might be one > place for it to provide the data to the framework and the framework can > later fill it up itself when switching governors. Well, as I said, let me see what can be done to avoid holding the policy rwsem around governor attributes access. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01-02-16, 12:24, Saravana Kannan wrote: > On 02/01/2016 02:22 AM, Rafael J. Wysocki wrote: > I'm not sure whose idea you are referring to. Viresh's (I don't think I saw > his proposal) or mine. http://git.linaro.org/people/viresh.kumar/linux.git/commit/57714d5b1778f2f610bcc5c74d85b29ba1cc1995 > Anyway, to explain my suggestion better, I'm proposing to make it so that we > don't have a need for the AB BA locking. The only reason the governor needs > to even grab the sysfs lock is to add/remove the sysfs attribute files. > > That can be easily achieved if the policy struct has some "gov_attrs" > field(s) that each governor populates. Then the framework just has to create > them after POLICY_INIT is processed by the governor and remove them before > POILICY_EXIT is sent to the governor. What will that solve? It will stay exactly same then as well, as we would be adding/removing these attributes from within the same policy->rwsem .. > That way, we also avoid having to worry about the gov attributes accessed by > the show/store disappearing while the files are being accessed. It can't happen. S_active lock should be taking care of that, isn't it?
On 01-02-16, 22:00, Rafael J. Wysocki wrote: > I'm not sure what you mean by "the sysfs lock" here? The policy rwsem > or something else? He perhaps referred to the s_active.lock that we see in traces.
On 02/01/2016 10:34 PM, Viresh Kumar wrote: > On 01-02-16, 12:24, Saravana Kannan wrote: >> On 02/01/2016 02:22 AM, Rafael J. Wysocki wrote: >> I'm not sure whose idea you are referring to. Viresh's (I don't think I saw >> his proposal) or mine. > > http://git.linaro.org/people/viresh.kumar/linux.git/commit/57714d5b1778f2f610bcc5c74d85b29ba1cc1995 > >> Anyway, to explain my suggestion better, I'm proposing to make it so that we >> don't have a need for the AB BA locking. The only reason the governor needs >> to even grab the sysfs lock is to add/remove the sysfs attribute files. >> >> That can be easily achieved if the policy struct has some "gov_attrs" >> field(s) that each governor populates. Then the framework just has to create >> them after POLICY_INIT is processed by the governor and remove them before >> POILICY_EXIT is sent to the governor. > > What will that solve? It will stay exactly same then as well, as we > would be adding/removing these attributes from within the same > policy->rwsem .. The problem isn't that you are holding the policy rwsem. The problem is that we are trying to grab the same locks in different order. This is trying to fix that. > >> That way, we also avoid having to worry about the gov attributes accessed by >> the show/store disappearing while the files are being accessed. > > It can't happen. S_active lock should be taking care of that, isn't > it? You are right. That can't happen because we have the s_active lock. I meant to say that in general we don't have to worry about the races between a show/store needing some policy specific data within the governor to be valid but racing with governor change where it ends up being invalid. The releasing of the policy rwsem across POLICY_EXIT allows this to happen today. -Saravana
On 02/01/2016 10:36 PM, Viresh Kumar wrote: > On 01-02-16, 22:00, Rafael J. Wysocki wrote: >> I'm not sure what you mean by "the sysfs lock" here? The policy rwsem >> or something else? > > He perhaps referred to the s_active.lock that we see in traces. > Yeah, that's what I mean. I generally don't use the exact name of the lock in emails (lazy to look it up) if there isn't a lot of chance for mistaking it for another lock. -Saravana
On 02-02-16, 13:37, Saravana Kannan wrote: > On 02/01/2016 10:34 PM, Viresh Kumar wrote: > >What will that solve? It will stay exactly same then as well, as we > >would be adding/removing these attributes from within the same > >policy->rwsem .. > > The problem isn't that you are holding the policy rwsem. The problem is that > we are trying to grab the same locks in different order. This is trying to > fix that. That's exactly what I was trying to say, sorry for not being very clear. Even if you would move the sysfs file creation thing into the cpufreq core, instead of governor, we will have locks this way: CPU0 CPU1 (sysfs read) (sysfs dir remove) s_active lock policy->rwsem policy->rwsem s_active lock (hang) And so I said, nothing will change.
On 02/02/2016 06:13 PM, Viresh Kumar wrote: > On 02-02-16, 13:37, Saravana Kannan wrote: >> On 02/01/2016 10:34 PM, Viresh Kumar wrote: >>> What will that solve? It will stay exactly same then as well, as we >>> would be adding/removing these attributes from within the same >>> policy->rwsem .. >> >> The problem isn't that you are holding the policy rwsem. The problem is that >> we are trying to grab the same locks in different order. This is trying to >> fix that. > > That's exactly what I was trying to say, sorry for not being very > clear. > > Even if you would move the sysfs file creation thing into the cpufreq > core, instead of governor, we will have locks this way: > > CPU0 CPU1 > (sysfs read) (sysfs dir remove) > s_active lock policy->rwsem > policy->rwsem > s_active lock (hang) > > > And so I said, nothing will change. > What's the s_active lock in CPU1 coming from? The only reason it's there today is because of the sysfs dir remove. If you move it before the policy->rwsem, you won't have it after the policy->rwsem too. So, I think it will fix the issue. -Saravana
On 02-02-16, 20:04, Saravana Kannan wrote: > What's the s_active lock in CPU1 coming from? That's taken by sysfs core while removing the files. > The only reason it's there > today is because of the sysfs dir remove. If you move it before the > policy->rwsem, you won't have it after the policy->rwsem too. So, I think it > will fix the issue. Its complex and we will end up making ugly.. For example, EXIT can be called while switching governors. The policy->rwsem is taken at the beginning cpufreq_set_policy(). To decide if we should remove the governor sysfs directory so early (i.e. before taking rwsem) in the call, is going to be difficult. Over that the same directory might be shared across multiple policies, and all that information is present only with the governor-core.
On 02/02/2016 09:02 PM, Viresh Kumar wrote: > On 02-02-16, 20:04, Saravana Kannan wrote: >> What's the s_active lock in CPU1 coming from? > > That's taken by sysfs core while removing the files. > >> The only reason it's there >> today is because of the sysfs dir remove. If you move it before the >> policy->rwsem, you won't have it after the policy->rwsem too. So, I think it >> will fix the issue. > > Its complex and we will end up making ugly.. I disagree. I think it's way better and simpler than this patch set. It also doesn't tie into cpufreq_governor.* which is a good thing IMO since it keeps things simpler for sched-dvfs too. > For example, EXIT can be called while switching governors. The > policy->rwsem is taken at the beginning cpufreq_set_policy(). To > decide if we should remove the governor sysfs directory so early (i.e. > before taking rwsem) in the call, is going to be difficult. Just check if the governor is changing. And if it is, you just need to remove the policy specific stuff. > Over that the same directory might be shared across multiple policies, > and all that information is present only with the governor-core. That's why I said the gov needs to register the per pol and system wide attrs list separately. This will also remove the need for ever governor to do this crap. -Saravana
On 02-02-16, 21:06, Saravana Kannan wrote: > I disagree. I think it's way better and simpler than this patch set. It also > doesn't tie into cpufreq_governor.* which is a good thing IMO since it keeps > things simpler for sched-dvfs too. Lets discuss it further on the other thread ..
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f1f9fbc..e7fc5c9 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1950,6 +1950,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, /* Don't start any governor operations if we are entering suspend */ if (cpufreq_suspended) return 0; + + lockdep_assert_held(&policy->rwsem); + /* * Governor might not be initiated here if ACPI _PPC changed * notification happened, so check it.
__cpufreq_governor works on policy, so policy->rwsem has to be held. Add assertion for such condition. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Viresh Kumar <viresh.kumar@linaro.org> Signed-off-by: Juri Lelli <juri.lelli@arm.com> --- drivers/cpufreq/cpufreq.c | 3 +++ 1 file changed, 3 insertions(+)