* [PATCH] cpufreq: Move cancelling of policy update work just after removing notifiers
@ 2019-10-21 13:28 Sudeep Holla
2019-10-22 2:25 ` Viresh Kumar
0 siblings, 1 reply; 5+ messages in thread
From: Sudeep Holla @ 2019-10-21 13:28 UTC (permalink / raw)
To: viresh.kumar; +Cc: Sudeep Holla, linux-kernel, linux-pm, rjw
Commit 099967699ad9 ("cpufreq: Cancel policy update work scheduled before freeing")
added cancel_work_sync(policy->update) after the frequency QoS were
removed. We can cancel the work just after taking the last CPU in the
policy offline and unregistering the notifiers as policy->update cannot
be scheduled from anywhere at this point.
However, due to other bugs, doing so still triggered the race between
freeing of policy and scheduled policy update work. Now that all those
issues are resolved, we can move this cancelling of any scheduled policy
update work just after removing min/max notifiers.
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/cpufreq/cpufreq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Hi Rafael,
Based on Viresh's suggestion, I am posting a patch to move this
cancel_work_sync earlier though it's not a must have change.
I will leave it up to your preference.
Regards,
Sudeep
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 829a3764df1b..48a224a6b178 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1268,6 +1268,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
freq_qos_remove_notifier(&policy->constraints, FREQ_QOS_MIN,
&policy->nb_min);
+ /* Cancel any pending policy->update work before freeing the policy. */
+ cancel_work_sync(&policy->update);
+
if (policy->max_freq_req) {
/*
* CPUFREQ_CREATE_POLICY notification is sent only after
@@ -1279,8 +1282,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
}
freq_qos_remove_request(policy->min_freq_req);
- /* Cancel any pending policy->update work before freeing the policy. */
- cancel_work_sync(&policy->update);
kfree(policy->min_freq_req);
cpufreq_policy_put_kobj(policy);
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: Move cancelling of policy update work just after removing notifiers
2019-10-21 13:28 [PATCH] cpufreq: Move cancelling of policy update work just after removing notifiers Sudeep Holla
@ 2019-10-22 2:25 ` Viresh Kumar
2019-10-22 9:46 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Viresh Kumar @ 2019-10-22 2:25 UTC (permalink / raw)
To: Sudeep Holla; +Cc: linux-kernel, linux-pm, rjw
On 21-10-19, 14:28, Sudeep Holla wrote:
> Commit 099967699ad9 ("cpufreq: Cancel policy update work scheduled before freeing")
> added cancel_work_sync(policy->update) after the frequency QoS were
> removed. We can cancel the work just after taking the last CPU in the
> policy offline and unregistering the notifiers as policy->update cannot
> be scheduled from anywhere at this point.
>
> However, due to other bugs, doing so still triggered the race between
> freeing of policy and scheduled policy update work. Now that all those
> issues are resolved, we can move this cancelling of any scheduled policy
> update work just after removing min/max notifiers.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/cpufreq/cpufreq.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Hi Rafael,
>
> Based on Viresh's suggestion, I am posting a patch to move this
> cancel_work_sync earlier though it's not a must have change.
For me it is :)
> I will leave it up to your preference.
>
> Regards,
> Sudeep
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 829a3764df1b..48a224a6b178 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1268,6 +1268,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> freq_qos_remove_notifier(&policy->constraints, FREQ_QOS_MIN,
> &policy->nb_min);
>
> + /* Cancel any pending policy->update work before freeing the policy. */
> + cancel_work_sync(&policy->update);
> +
> if (policy->max_freq_req) {
> /*
> * CPUFREQ_CREATE_POLICY notification is sent only after
> @@ -1279,8 +1282,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> }
>
> freq_qos_remove_request(policy->min_freq_req);
> - /* Cancel any pending policy->update work before freeing the policy. */
> - cancel_work_sync(&policy->update);
> kfree(policy->min_freq_req);
>
> cpufreq_policy_put_kobj(policy);
Thanks for doing this.
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: Move cancelling of policy update work just after removing notifiers
2019-10-22 2:25 ` Viresh Kumar
@ 2019-10-22 9:46 ` Rafael J. Wysocki
2019-10-22 10:07 ` Viresh Kumar
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-10-22 9:46 UTC (permalink / raw)
To: Viresh Kumar, Sudeep Holla
Cc: Linux Kernel Mailing List, Linux PM, Rafael J. Wysocki
On Tue, Oct 22, 2019 at 4:25 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 21-10-19, 14:28, Sudeep Holla wrote:
> > Commit 099967699ad9 ("cpufreq: Cancel policy update work scheduled before freeing")
> > added cancel_work_sync(policy->update) after the frequency QoS were
> > removed. We can cancel the work just after taking the last CPU in the
> > policy offline and unregistering the notifiers as policy->update cannot
> > be scheduled from anywhere at this point.
> >
> > However, due to other bugs, doing so still triggered the race between
> > freeing of policy and scheduled policy update work. Now that all those
> > issues are resolved, we can move this cancelling of any scheduled policy
> > update work just after removing min/max notifiers.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > drivers/cpufreq/cpufreq.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > Hi Rafael,
> >
> > Based on Viresh's suggestion, I am posting a patch to move this
> > cancel_work_sync earlier though it's not a must have change.
>
> For me it is :)
>
> > I will leave it up to your preference.
> >
> > Regards,
> > Sudeep
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 829a3764df1b..48a224a6b178 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1268,6 +1268,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > freq_qos_remove_notifier(&policy->constraints, FREQ_QOS_MIN,
> > &policy->nb_min);
> >
> > + /* Cancel any pending policy->update work before freeing the policy. */
> > + cancel_work_sync(&policy->update);
> > +
> > if (policy->max_freq_req) {
> > /*
> > * CPUFREQ_CREATE_POLICY notification is sent only after
> > @@ -1279,8 +1282,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > }
> >
> > freq_qos_remove_request(policy->min_freq_req);
> > - /* Cancel any pending policy->update work before freeing the policy. */
> > - cancel_work_sync(&policy->update);
> > kfree(policy->min_freq_req);
> >
> > cpufreq_policy_put_kobj(policy);
>
> Thanks for doing this.
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Folded into the previous patch and applied.
Please double check the result in the current linux-next branch in my tree.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: Move cancelling of policy update work just after removing notifiers
2019-10-22 9:46 ` Rafael J. Wysocki
@ 2019-10-22 10:07 ` Viresh Kumar
2019-10-22 16:11 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Viresh Kumar @ 2019-10-22 10:07 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Sudeep Holla, Linux Kernel Mailing List, Linux PM, Rafael J. Wysocki
On 22-10-19, 11:46, Rafael J. Wysocki wrote:
> On Tue, Oct 22, 2019 at 4:25 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 21-10-19, 14:28, Sudeep Holla wrote:
> > > Commit 099967699ad9 ("cpufreq: Cancel policy update work scheduled before freeing")
> > > added cancel_work_sync(policy->update) after the frequency QoS were
> > > removed. We can cancel the work just after taking the last CPU in the
> > > policy offline and unregistering the notifiers as policy->update cannot
> > > be scheduled from anywhere at this point.
> > >
> > > However, due to other bugs, doing so still triggered the race between
> > > freeing of policy and scheduled policy update work. Now that all those
> > > issues are resolved, we can move this cancelling of any scheduled policy
> > > update work just after removing min/max notifiers.
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > > drivers/cpufreq/cpufreq.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > Hi Rafael,
> > >
> > > Based on Viresh's suggestion, I am posting a patch to move this
> > > cancel_work_sync earlier though it's not a must have change.
> >
> > For me it is :)
> >
> > > I will leave it up to your preference.
> > >
> > > Regards,
> > > Sudeep
> > >
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 829a3764df1b..48a224a6b178 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1268,6 +1268,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > > freq_qos_remove_notifier(&policy->constraints, FREQ_QOS_MIN,
> > > &policy->nb_min);
> > >
> > > + /* Cancel any pending policy->update work before freeing the policy. */
> > > + cancel_work_sync(&policy->update);
> > > +
> > > if (policy->max_freq_req) {
> > > /*
> > > * CPUFREQ_CREATE_POLICY notification is sent only after
> > > @@ -1279,8 +1282,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > > }
> > >
> > > freq_qos_remove_request(policy->min_freq_req);
> > > - /* Cancel any pending policy->update work before freeing the policy. */
> > > - cancel_work_sync(&policy->update);
> > > kfree(policy->min_freq_req);
> > >
> > > cpufreq_policy_put_kobj(policy);
> >
> > Thanks for doing this.
> >
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Folded into the previous patch and applied.
>
> Please double check the result in the current linux-next branch in my tree.
I would have kept the blank line after cancel_work_sync() which isn't
there anymore.
--
viresh
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: Move cancelling of policy update work just after removing notifiers
2019-10-22 10:07 ` Viresh Kumar
@ 2019-10-22 16:11 ` Rafael J. Wysocki
0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2019-10-22 16:11 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Sudeep Holla, Linux Kernel Mailing List,
Linux PM, Rafael J. Wysocki
On Tue, Oct 22, 2019 at 12:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 22-10-19, 11:46, Rafael J. Wysocki wrote:
> > On Tue, Oct 22, 2019 at 4:25 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 21-10-19, 14:28, Sudeep Holla wrote:
> > > > Commit 099967699ad9 ("cpufreq: Cancel policy update work scheduled before freeing")
> > > > added cancel_work_sync(policy->update) after the frequency QoS were
> > > > removed. We can cancel the work just after taking the last CPU in the
> > > > policy offline and unregistering the notifiers as policy->update cannot
> > > > be scheduled from anywhere at this point.
> > > >
> > > > However, due to other bugs, doing so still triggered the race between
> > > > freeing of policy and scheduled policy update work. Now that all those
> > > > issues are resolved, we can move this cancelling of any scheduled policy
> > > > update work just after removing min/max notifiers.
> > > >
> > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > ---
> > > > drivers/cpufreq/cpufreq.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > Hi Rafael,
> > > >
> > > > Based on Viresh's suggestion, I am posting a patch to move this
> > > > cancel_work_sync earlier though it's not a must have change.
> > >
> > > For me it is :)
> > >
> > > > I will leave it up to your preference.
> > > >
> > > > Regards,
> > > > Sudeep
> > > >
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index 829a3764df1b..48a224a6b178 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -1268,6 +1268,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > > > freq_qos_remove_notifier(&policy->constraints, FREQ_QOS_MIN,
> > > > &policy->nb_min);
> > > >
> > > > + /* Cancel any pending policy->update work before freeing the policy. */
> > > > + cancel_work_sync(&policy->update);
> > > > +
> > > > if (policy->max_freq_req) {
> > > > /*
> > > > * CPUFREQ_CREATE_POLICY notification is sent only after
> > > > @@ -1279,8 +1282,6 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > > > }
> > > >
> > > > freq_qos_remove_request(policy->min_freq_req);
> > > > - /* Cancel any pending policy->update work before freeing the policy. */
> > > > - cancel_work_sync(&policy->update);
> > > > kfree(policy->min_freq_req);
> > > >
> > > > cpufreq_policy_put_kobj(policy);
> > >
> > > Thanks for doing this.
> > >
> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> >
> > Folded into the previous patch and applied.
> >
> > Please double check the result in the current linux-next branch in my tree.
>
> I would have kept the blank line after cancel_work_sync() which isn't
> there anymore.
OK, it looks better with the extra blank line, so updated. Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-22 16:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-21 13:28 [PATCH] cpufreq: Move cancelling of policy update work just after removing notifiers Sudeep Holla
2019-10-22 2:25 ` Viresh Kumar
2019-10-22 9:46 ` Rafael J. Wysocki
2019-10-22 10:07 ` Viresh Kumar
2019-10-22 16:11 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).