Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

On 03-02-16, 13:24, Rafael J. Wysocki wrote:
> I guess this was supposed to be "I am not ...".

Urg..

-- 
viresh

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 6:51 AM, Viresh Kumar  wrote:
> On 02-02-16, 22:53, Rafael J. Wysocki wrote:
>> First of all, this is effectively reverting commit 955ef4833574, so
>> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
>> lock around CPUFREQ_GOV_POLICY_EXIT)".
>>
>> There should be a Fixes: tag pointing to commit 955ef4833574 and a
>> Reported-by: for Juri.
>>
>> If there is a link to a bug report related to this, it should be
>> pointed to by a Link: tag.
>>
>> The changelog should say why the original commit was there and why the
>> way it attempted to solve the problem was incorrect.  It also should
>> say that the original problem was addressed by a previous commit, so
>> this one can be reverted without consequences.
>
> How about this:
>
> Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
>
> Earlier, when the struct freq-attr was used to represent governor
> attributes, the standard cpufreq show/store sysfs attribute callbacks
> were applied to the governor tunable attributes and they always acquire
> the policy->rwsem lock before carrying out the operation.  That could
> have resulted in an ABBA deadlock if governor tunable attributes are
> removed under policy->rwsem while one of them is being accessed
> concurrently (if sysfs attributes removal wins the race, it will wait
> for the access to complete with policy->rwsem held while the attribute
> callback will block on policy->rwsem indefinitely).
>
> We attempted to address this issue by dropping policy->rwsem around
> governor tunable attributes removal (that is, around invocations of the
> ->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
> in cpufreq_set_policy(), but that opened up race conditions that had not
> been possible with policy->rwsem held all the time.
>
> The previous commit, "cpufreq: governor: New sysfs show/store callbacks
> for governor tunables", fixed the original ABBA deadlock by adding new
> governor specific show/store callbacks.
>
> We don't have to drop rwsem around invocations of governor event
> CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
>
> Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around 
> CPUFREQ_GOV_POLICY_EXIT")
> Reported-by: Juri Lelli 
> Signed-off-by: Viresh Kumar 

Much better.

>> But I'm not going to write that changelog.  I actually am not going to
>> write any changelogs for you any more, because I'm seriously tired of
>> doing that.  Moreover, if I see a patch from you with a changelog
>> that's not acceptable to me, it will immediately go to the "not
>> applicable" trash bin no matter what the changes below look like.  You
>> *have* *to* write useful changelogs.  This isn't optional or best
>> effort.  This is mandatory and important.
>
> Will try to improve, sorry about that (again).
>
>> Now, I'm not really sure if the ordering of this patchset is right.
>> Maybe we should just revert upfront with the "we'll address the
>> original problem in the following commits" statement in the changelog
>> and fix it in a different way?
>
> Wouldn't that break things like 'git bisect'? People running kernels
> after the reverted commits may start hitting lockdeps.

Well, we have at least one bug in there before applying the whole
series anyway, regardless of the ordering.

>> It looks like patches [1-3/5] fix a
>> problem that isn't there even, but would appear after the [4/5] if
>> they were not applied previously, which doesn't sound really
>> straightforward to me.
>
> I am going to fight hard for it, if you feel 4/5 should be the first
> patch here, I will do that.

I guess this was supposed to be "I am not ...".

With the above changelog the current order of patches in the series is fine.

Thanks,
Rafael

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

On 03-02-16, 13:24, Rafael J. Wysocki wrote:
> I guess this was supposed to be "I am not ...".

Urg..

-- 
viresh

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Rafael J. Wysocki]

On Wed, Feb 3, 2016 at 6:51 AM, Viresh Kumar  wrote:
> On 02-02-16, 22:53, Rafael J. Wysocki wrote:
>> First of all, this is effectively reverting commit 955ef4833574, so
>> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
>> lock around CPUFREQ_GOV_POLICY_EXIT)".
>>
>> There should be a Fixes: tag pointing to commit 955ef4833574 and a
>> Reported-by: for Juri.
>>
>> If there is a link to a bug report related to this, it should be
>> pointed to by a Link: tag.
>>
>> The changelog should say why the original commit was there and why the
>> way it attempted to solve the problem was incorrect.  It also should
>> say that the original problem was addressed by a previous commit, so
>> this one can be reverted without consequences.
>
> How about this:
>
> Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
>
> Earlier, when the struct freq-attr was used to represent governor
> attributes, the standard cpufreq show/store sysfs attribute callbacks
> were applied to the governor tunable attributes and they always acquire
> the policy->rwsem lock before carrying out the operation.  That could
> have resulted in an ABBA deadlock if governor tunable attributes are
> removed under policy->rwsem while one of them is being accessed
> concurrently (if sysfs attributes removal wins the race, it will wait
> for the access to complete with policy->rwsem held while the attribute
> callback will block on policy->rwsem indefinitely).
>
> We attempted to address this issue by dropping policy->rwsem around
> governor tunable attributes removal (that is, around invocations of the
> ->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
> in cpufreq_set_policy(), but that opened up race conditions that had not
> been possible with policy->rwsem held all the time.
>
> The previous commit, "cpufreq: governor: New sysfs show/store callbacks
> for governor tunables", fixed the original ABBA deadlock by adding new
> governor specific show/store callbacks.
>
> We don't have to drop rwsem around invocations of governor event
> CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
>
> Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around 
> CPUFREQ_GOV_POLICY_EXIT")
> Reported-by: Juri Lelli 
> Signed-off-by: Viresh Kumar 

Much better.

>> But I'm not going to write that changelog.  I actually am not going to
>> write any changelogs for you any more, because I'm seriously tired of
>> doing that.  Moreover, if I see a patch from you with a changelog
>> that's not acceptable to me, it will immediately go to the "not
>> applicable" trash bin no matter what the changes below look like.  You
>> *have* *to* write useful changelogs.  This isn't optional or best
>> effort.  This is mandatory and important.
>
> Will try to improve, sorry about that (again).
>
>> Now, I'm not really sure if the ordering of this patchset is right.
>> Maybe we should just revert upfront with the "we'll address the
>> original problem in the following commits" statement in the changelog
>> and fix it in a different way?
>
> Wouldn't that break things like 'git bisect'? People running kernels
> after the reverted commits may start hitting lockdeps.

Well, we have at least one bug in there before applying the whole
series anyway, regardless of the ordering.

>> It looks like patches [1-3/5] fix a
>> problem that isn't there even, but would appear after the [4/5] if
>> they were not applied previously, which doesn't sound really
>> straightforward to me.
>
> I am going to fight hard for it, if you feel 4/5 should be the first
> patch here, I will do that.

I guess this was supposed to be "I am not ...".

With the above changelog the current order of patches in the series is fine.

Thanks,
Rafael

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

On 02-02-16, 22:53, Rafael J. Wysocki wrote:
> First of all, this is effectively reverting commit 955ef4833574, so
> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
> lock around CPUFREQ_GOV_POLICY_EXIT)".
> 
> There should be a Fixes: tag pointing to commit 955ef4833574 and a
> Reported-by: for Juri.
> 
> If there is a link to a bug report related to this, it should be
> pointed to by a Link: tag.
> 
> The changelog should say why the original commit was there and why the
> way it attempted to solve the problem was incorrect.  It also should
> say that the original problem was addressed by a previous commit, so
> this one can be reverted without consequences.

How about this:

Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"

Earlier, when the struct freq-attr was used to represent governor
attributes, the standard cpufreq show/store sysfs attribute callbacks
were applied to the governor tunable attributes and they always acquire
the policy->rwsem lock before carrying out the operation.  That could
have resulted in an ABBA deadlock if governor tunable attributes are
removed under policy->rwsem while one of them is being accessed
concurrently (if sysfs attributes removal wins the race, it will wait
for the access to complete with policy->rwsem held while the attribute
callback will block on policy->rwsem indefinitely).

We attempted to address this issue by dropping policy->rwsem around
governor tunable attributes removal (that is, around invocations of the
->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
in cpufreq_set_policy(), but that opened up race conditions that had not
been possible with policy->rwsem held all the time.

The previous commit, "cpufreq: governor: New sysfs show/store callbacks
for governor tunables", fixed the original ABBA deadlock by adding new
governor specific show/store callbacks.

We don't have to drop rwsem around invocations of governor event
CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.

Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around 
CPUFREQ_GOV_POLICY_EXIT")
Reported-by: Juri Lelli 
Signed-off-by: Viresh Kumar 

> But I'm not going to write that changelog.  I actually am not going to
> write any changelogs for you any more, because I'm seriously tired of
> doing that.  Moreover, if I see a patch from you with a changelog
> that's not acceptable to me, it will immediately go to the "not
> applicable" trash bin no matter what the changes below look like.  You
> *have* *to* write useful changelogs.  This isn't optional or best
> effort.  This is mandatory and important.

Will try to improve, sorry about that (again).

> Now, I'm not really sure if the ordering of this patchset is right.
> Maybe we should just revert upfront with the "we'll address the
> original problem in the following commits" statement in the changelog
> and fix it in a different way?

Wouldn't that break things like 'git bisect'? People running kernels
after the reverted commits may start hitting lockdeps.

> It looks like patches [1-3/5] fix a
> problem that isn't there even, but would appear after the [4/5] if
> they were not applied previously, which doesn't sound really
> straightforward to me.

I am going to fight hard for it, if you feel 4/5 should be the first
patch here, I will do that.

-- 
viresh

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar  wrote:
> Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
> CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
> issues.
>
> The previous commit has fixed them all and we don't need to drop these
> locks anymore.
>
> Signed-off-by: Viresh Kumar 

First of all, this is effectively reverting commit 955ef4833574, so
the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
lock around CPUFREQ_GOV_POLICY_EXIT)".

There should be a Fixes: tag pointing to commit 955ef4833574 and a
Reported-by: for Juri.

If there is a link to a bug report related to this, it should be
pointed to by a Link: tag.

The changelog should say why the original commit was there and why the
way it attempted to solve the problem was incorrect.  It also should
say that the original problem was addressed by a previous commit, so
this one can be reverted without consequences.

But I'm not going to write that changelog.  I actually am not going to
write any changelogs for you any more, because I'm seriously tired of
doing that.  Moreover, if I see a patch from you with a changelog
that's not acceptable to me, it will immediately go to the "not
applicable" trash bin no matter what the changes below look like.  You
*have* *to* write useful changelogs.  This isn't optional or best
effort.  This is mandatory and important.

Now, I'm not really sure if the ordering of this patchset is right.
Maybe we should just revert upfront with the "we'll address the
original problem in the following commits" statement in the changelog
and fix it in a different way?  It looks like patches [1-3/5] fix a
problem that isn't there even, but would appear after the [4/5] if
they were not applied previously, which doesn't sound really
straightforward to me.

Thanks,
Rafael

[PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
issues.

The previous commit has fixed them all and we don't need to drop these
locks anymore.

Signed-off-by: Viresh Kumar 
---
 drivers/cpufreq/cpufreq.c | 5 -
 include/linux/cpufreq.h   | 4 
 2 files changed, 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e979ec78b695..5f7e24567e0e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2155,10 +2155,7 @@ static int cpufreq_set_policy(struct cpufreq_policy 
*policy,
return ret;
}
 
-   up_write(>rwsem);
ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-   down_write(>rwsem);
-
if (ret) {
pr_err("%s: Failed to Exit Governor: %s (%d)\n",
   __func__, old_gov->name, ret);
@@ -2174,9 +2171,7 @@ static int cpufreq_set_policy(struct cpufreq_policy 
*policy,
if (!ret)
goto out;
 
-   up_write(>rwsem);
__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-   down_write(>rwsem);
}
 
/* new governor failed, so re-start old one */
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 88a4215125bc..79b87cebaa9c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -100,10 +100,6 @@ struct cpufreq_policy {
 * - Any routine that will write to the policy structure and/or may 
take away
 *   the policy altogether (eg. CPU hotplug), will hold this lock in 
write
 *   mode before doing so.
-*
-* Additional rules:
-* - Lock should not be held across
-* __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 */
struct rw_semaphore rwsem;
 
-- 
2.7.0.79.gdc08a19

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Rafael J. Wysocki]

On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar  wrote:
> Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
> CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
> issues.
>
> The previous commit has fixed them all and we don't need to drop these
> locks anymore.
>
> Signed-off-by: Viresh Kumar 

First of all, this is effectively reverting commit 955ef4833574, so
the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
lock around CPUFREQ_GOV_POLICY_EXIT)".

There should be a Fixes: tag pointing to commit 955ef4833574 and a
Reported-by: for Juri.

If there is a link to a bug report related to this, it should be
pointed to by a Link: tag.

The changelog should say why the original commit was there and why the
way it attempted to solve the problem was incorrect.  It also should
say that the original problem was addressed by a previous commit, so
this one can be reverted without consequences.

But I'm not going to write that changelog.  I actually am not going to
write any changelogs for you any more, because I'm seriously tired of
doing that.  Moreover, if I see a patch from you with a changelog
that's not acceptable to me, it will immediately go to the "not
applicable" trash bin no matter what the changes below look like.  You
*have* *to* write useful changelogs.  This isn't optional or best
effort.  This is mandatory and important.

Now, I'm not really sure if the ordering of this patchset is right.
Maybe we should just revert upfront with the "we'll address the
original problem in the following commits" statement in the changelog
and fix it in a different way?  It looks like patches [1-3/5] fix a
problem that isn't there even, but would appear after the [4/5] if
they were not applied previously, which doesn't sound really
straightforward to me.

Thanks,
Rafael

Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

On 02-02-16, 22:53, Rafael J. Wysocki wrote:
> First of all, this is effectively reverting commit 955ef4833574, so
> the subject should be "Revert commit 955ef4833574 (cpufreq: Drop rwsem
> lock around CPUFREQ_GOV_POLICY_EXIT)".
> 
> There should be a Fixes: tag pointing to commit 955ef4833574 and a
> Reported-by: for Juri.
> 
> If there is a link to a bug report related to this, it should be
> pointed to by a Link: tag.
> 
> The changelog should say why the original commit was there and why the
> way it attempted to solve the problem was incorrect.  It also should
> say that the original problem was addressed by a previous commit, so
> this one can be reverted without consequences.

How about this:

Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"

Earlier, when the struct freq-attr was used to represent governor
attributes, the standard cpufreq show/store sysfs attribute callbacks
were applied to the governor tunable attributes and they always acquire
the policy->rwsem lock before carrying out the operation.  That could
have resulted in an ABBA deadlock if governor tunable attributes are
removed under policy->rwsem while one of them is being accessed
concurrently (if sysfs attributes removal wins the race, it will wait
for the access to complete with policy->rwsem held while the attribute
callback will block on policy->rwsem indefinitely).

We attempted to address this issue by dropping policy->rwsem around
governor tunable attributes removal (that is, around invocations of the
->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
in cpufreq_set_policy(), but that opened up race conditions that had not
been possible with policy->rwsem held all the time.

The previous commit, "cpufreq: governor: New sysfs show/store callbacks
for governor tunables", fixed the original ABBA deadlock by adding new
governor specific show/store callbacks.

We don't have to drop rwsem around invocations of governor event
CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.

Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around 
CPUFREQ_GOV_POLICY_EXIT")
Reported-by: Juri Lelli 
Signed-off-by: Viresh Kumar 

> But I'm not going to write that changelog.  I actually am not going to
> write any changelogs for you any more, because I'm seriously tired of
> doing that.  Moreover, if I see a patch from you with a changelog
> that's not acceptable to me, it will immediately go to the "not
> applicable" trash bin no matter what the changes below look like.  You
> *have* *to* write useful changelogs.  This isn't optional or best
> effort.  This is mandatory and important.

Will try to improve, sorry about that (again).

> Now, I'm not really sure if the ordering of this patchset is right.
> Maybe we should just revert upfront with the "we'll address the
> original problem in the following commits" statement in the changelog
> and fix it in a different way?

Wouldn't that break things like 'git bisect'? People running kernels
after the reverted commits may start hitting lockdeps.

> It looks like patches [1-3/5] fix a
> problem that isn't there even, but would appear after the [4/5] if
> they were not applied previously, which doesn't sound really
> straightforward to me.

I am going to fight hard for it, if you feel 4/5 should be the first
patch here, I will do that.

-- 
viresh

[PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

Commit 955ef4833574 ("cpufreq: Drop rwsem lock around
CPUFREQ_GOV_POLICY_EXIT") dropped these because of some ABBA lockup
issues.

The previous commit has fixed them all and we don't need to drop these
locks anymore.

Signed-off-by: Viresh Kumar 
---
 drivers/cpufreq/cpufreq.c | 5 -
 include/linux/cpufreq.h   | 4 
 2 files changed, 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e979ec78b695..5f7e24567e0e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2155,10 +2155,7 @@ static int cpufreq_set_policy(struct cpufreq_policy 
*policy,
return ret;
}
 
-   up_write(>rwsem);
ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-   down_write(>rwsem);
-
if (ret) {
pr_err("%s: Failed to Exit Governor: %s (%d)\n",
   __func__, old_gov->name, ret);
@@ -2174,9 +2171,7 @@ static int cpufreq_set_policy(struct cpufreq_policy 
*policy,
if (!ret)
goto out;
 
-   up_write(>rwsem);
__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
-   down_write(>rwsem);
}
 
/* new governor failed, so re-start old one */
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 88a4215125bc..79b87cebaa9c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -100,10 +100,6 @@ struct cpufreq_policy {
 * - Any routine that will write to the policy structure and/or may 
take away
 *   the policy altogether (eg. CPU hotplug), will hold this lock in 
write
 *   mode before doing so.
-*
-* Additional rules:
-* - Lock should not be held across
-* __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
 */
struct rw_semaphore rwsem;
 
-- 
2.7.0.79.gdc08a19