Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1329881 > unrolled thread

[PATCH V4 0/7] cpufreq: Locking fixes and cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH V4 0/7] cpufreq: Locking fixes and cleanups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-09 04:50 +0100
    [PATCH V4 7/7] cpufreq: ondemand: Rearrange od_dbs_timer() to void updating delay Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-09 04:50 +0100
      Re: [PATCH V4 7/7] cpufreq: ondemand: Rearrange od_dbs_timer() to  void updating delay "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-10 01:30 +0100
    [PATCH V4 5/7] cpufreq: governor: No need to manage state machine now Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-09 04:50 +0100
      Re: [PATCH V4 5/7] cpufreq: governor: No need to manage state machine now "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-10 01:40 +0100
        Re: [PATCH V4 5/7] cpufreq: governor: No need to manage state  machine now Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-10 06:40 +0100
    [PATCH V4 2/7] cpufreq: Call __cpufreq_governor() with policy->rwsem held Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-09 04:50 +0100
      Re: [PATCH V4 2/7] cpufreq: Call __cpufreq_governor() with  policy->rwsem held "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-11 10:50 +0100
        Re: [PATCH V4 2/7] cpufreq: Call __cpufreq_governor() with  policy->rwsem held Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-11 13:00 +0100
    [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish routines Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-09 05:00 +0100
      Re: [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish routines "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-11 02:00 +0100
        Re: [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish routines "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-11 02:20 +0100
          Re: [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish  routines Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-11 12:50 +0100

#1329881 — [PATCH V4 0/7] cpufreq: Locking fixes and cleanups

Hi Rafael,

These are rest of the patches that fix some more locking issues with
policy->rwsem and do some minor optimization/cleanups.

These were part of the 13 patch series which was sent earlier. The last
patch is new, and does what I suggested to one of your commits.

V3->V4:
- Reordered all the patches and created a new series.
- s/global/common for common tunables
- One new patch

@Juri/Shilpa: I have added your Tested-by for the first 6 patches,
please let me know if you don't want to add that.

Viresh Kumar (7):
  cpufreq: Merge cpufreq_offline_prepare/finish routines
  cpufreq: Call __cpufreq_governor() with policy->rwsem held
  cpufreq: Remove cpufreq_governor_lock
  cpufreq: governor: Move common sysfs tunables to cpufreq_governor.c
  cpufreq: governor: No need to manage state machine now
  cpufreq: conservative: Update sample_delay_ns immediately
  cpufreq: ondemand: Rearrange od_dbs_timer() to void updating delay

 drivers/cpufreq/cpufreq.c              |  93 +++++++++---------
 drivers/cpufreq/cpufreq_conservative.c |  80 +++-------------
 drivers/cpufreq/cpufreq_governor.c     | 170 ++++++++++++++++++++++++++++-----
 drivers/cpufreq/cpufreq_governor.h     |  16 +++-
 drivers/cpufreq/cpufreq_ondemand.c     | 153 +++++------------------------
 5 files changed, 240 insertions(+), 272 deletions(-)

-- 
2.7.1.370.gb2aa7f8

[toc] | [next] | [standalone]

#1329882 — [PATCH V4 7/7] cpufreq: ondemand: Rearrange od_dbs_timer() to void updating delay

'delay' is updated properly in all paths of the routine od_dbs_timer(),
leaving just one. And can be 0 only in that case.

Move the update to 'delay' as an else part of the if block.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_ondemand.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index d9f323f150c4..388ae07ce413 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -194,7 +194,7 @@ static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 	struct dbs_data *dbs_data = policy_dbs->dbs_data;
 	struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, policy->cpu);
-	int delay = 0, sample_type = dbs_info->sample_type;
+	int delay, sample_type = dbs_info->sample_type;
 
 	/* Common NORMAL_SAMPLE setup */
 	dbs_info->sample_type = OD_NORMAL_SAMPLE;
@@ -208,13 +208,12 @@ static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
 			/* Setup timer for SUB_SAMPLE */
 			dbs_info->sample_type = OD_SUB_SAMPLE;
 			delay = dbs_info->freq_hi_jiffies;
+		} else {
+			delay = delay_for_sampling_rate(dbs_data->sampling_rate
+							* dbs_info->rate_mult);
 		}
 	}
 
-	if (!delay)
-		delay = delay_for_sampling_rate(dbs_data->sampling_rate
-				* dbs_info->rate_mult);
-
 	return delay;
 }
 
-- 
2.7.1.370.gb2aa7f8

[toc] | [prev] | [next] | [standalone]

#1330787 — Re: [PATCH V4 7/7] cpufreq: ondemand: Rearrange od_dbs_timer() to void updating delay

s/void/avoid/ in the subject.

Apart from that it looks good.

On Tue, Feb 9, 2016 at 4:46 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 'delay' is updated properly in all paths of the routine od_dbs_timer(),
> leaving just one. And can be 0 only in that case.
>
> Move the update to 'delay' as an else part of the if block.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_ondemand.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index d9f323f150c4..388ae07ce413 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -194,7 +194,7 @@ static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
>         struct policy_dbs_info *policy_dbs = policy->governor_data;
>         struct dbs_data *dbs_data = policy_dbs->dbs_data;
>         struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, policy->cpu);
> -       int delay = 0, sample_type = dbs_info->sample_type;
> +       int delay, sample_type = dbs_info->sample_type;
>
>         /* Common NORMAL_SAMPLE setup */
>         dbs_info->sample_type = OD_NORMAL_SAMPLE;
> @@ -208,13 +208,12 @@ static unsigned int od_dbs_timer(struct cpufreq_policy *policy)
>                         /* Setup timer for SUB_SAMPLE */
>                         dbs_info->sample_type = OD_SUB_SAMPLE;
>                         delay = dbs_info->freq_hi_jiffies;
> +               } else {
> +                       delay = delay_for_sampling_rate(dbs_data->sampling_rate
> +                                                       * dbs_info->rate_mult);
>                 }
>         }
>
> -       if (!delay)
> -               delay = delay_for_sampling_rate(dbs_data->sampling_rate
> -                               * dbs_info->rate_mult);
> -
>         return delay;
>  }
>
> --
> 2.7.1.370.gb2aa7f8
>
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1329883 — [PATCH V4 5/7] cpufreq: governor: No need to manage state machine now

cpufreq core now guarantees that policy->rwsem wouldn't get dropped
while calling CPUFREQ_GOV_POLICY_EXIT governor event and will be kept
acquired until the complete sequence of governor state changes has
finished.

And so we can remove the state machine checks that were put in place
earlier.

This also means that policy_dbs->policy can be initialized while
policy_dbs is allocated, to move all initialization together.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Juri Lelli <juri.lelli@arm.com>
Tested-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
 drivers/cpufreq/cpufreq_governor.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 7038ada3915d..464f346815e0 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -332,8 +332,10 @@ static inline void gov_clear_update_util(struct cpufreq_policy *policy)
 	synchronize_rcu();
 }
 
-static void gov_cancel_work(struct policy_dbs_info *policy_dbs)
+static void gov_cancel_work(struct cpufreq_policy *policy)
 {
+	struct policy_dbs_info *policy_dbs = policy->governor_data;
+
 	/* Tell dbs_update_util_handler() to skip queuing up work items. */
 	atomic_inc(&policy_dbs->skip_work);
 	/*
@@ -429,6 +431,7 @@ static struct policy_dbs_info *alloc_policy_dbs_info(struct cpufreq_policy *poli
 	if (!policy_dbs)
 		return NULL;
 
+	policy_dbs->policy = policy;
 	mutex_init(&policy_dbs->timer_mutex);
 	atomic_set(&policy_dbs->skip_work, 0);
 	init_irq_work(&policy_dbs->irq_work, dbs_irq_work);
@@ -560,10 +563,6 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy)
 	struct dbs_data *dbs_data = policy_dbs->dbs_data;
 	int count;
 
-	/* State should be equivalent to INIT */
-	if (policy_dbs->policy)
-		return -EBUSY;
-
 	mutex_lock(&dbs_data->mutex);
 	list_del(&policy_dbs->list);
 	count = dbs_data->usage_count--;
@@ -599,10 +598,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)
 	if (!policy->cur)
 		return -EINVAL;
 
-	/* State should be equivalent to INIT */
-	if (policy_dbs->policy)
-		return -EBUSY;
-
 	sampling_rate = dbs_data->sampling_rate;
 	ignore_nice = dbs_data->ignore_nice_load;
 
@@ -627,7 +622,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)
 		if (ignore_nice)
 			j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
 	}
-	policy_dbs->policy = policy;
 
 	if (gov->governor == GOV_CONSERVATIVE) {
 		struct cs_cpu_dbs_info_s *cs_dbs_info =
@@ -650,14 +644,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)
 
 static int cpufreq_governor_stop(struct cpufreq_policy *policy)
 {
-	struct policy_dbs_info *policy_dbs = policy->governor_data;
-
-	/* State should be equivalent to START */
-	if (!policy_dbs->policy)
-		return -EBUSY;
-
-	gov_cancel_work(policy_dbs);
-	policy_dbs->policy = NULL;
+	gov_cancel_work(policy);
 
 	return 0;
 }
@@ -666,10 +653,6 @@ static int cpufreq_governor_limits(struct cpufreq_policy *policy)
 {
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 
-	/* State should be equivalent to START */
-	if (!policy_dbs->policy)
-		return -EBUSY;
-
 	mutex_lock(&policy_dbs->timer_mutex);
 	if (policy->max < policy->cur)
 		__cpufreq_driver_target(policy, policy->max, CPUFREQ_RELATION_H);
-- 
2.7.1.370.gb2aa7f8

[toc] | [prev] | [next] | [standalone]

#1330807 — Re: [PATCH V4 5/7] cpufreq: governor: No need to manage state machine now

On Tue, Feb 9, 2016 at 4:46 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> cpufreq core now guarantees that policy->rwsem wouldn't get dropped

"The cpufreq core ..." and "won't be dropped"

> while calling CPUFREQ_GOV_POLICY_EXIT governor event and will be kept

"while running the ->governor callback for the CPUFREQ_GOV_POLICY_EXIT
event and will be held"

> acquired until the complete sequence of governor state changes has
> finished.
>
> And so we can remove the state machine checks that were put in place
> earlier.

"This allows governor state machine checks to be dropped from multiple
functions in cpufreq_governor.c."

>
> This also means that policy_dbs->policy can be initialized while

"initialized upfront"

> policy_dbs is allocated, to move all initialization together.

"so the entire initialization of struct policy_dbs is carried out in one place."

>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Tested-by: Juri Lelli <juri.lelli@arm.com>
> Tested-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c | 27 +++++----------------------
>  1 file changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 7038ada3915d..464f346815e0 100644

[cut]

> @@ -650,14 +644,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy)
>
>  static int cpufreq_governor_stop(struct cpufreq_policy *policy)
>  {
> -       struct policy_dbs_info *policy_dbs = policy->governor_data;
> -
> -       /* State should be equivalent to START */
> -       if (!policy_dbs->policy)
> -               return -EBUSY;
> -
> -       gov_cancel_work(policy_dbs);
> -       policy_dbs->policy = NULL;
> +       gov_cancel_work(policy);
>
>         return 0;
>  }

So maybe we can call gov_cancel_work(policy) from
cpufreq_governor_dbs() directly and get rid of this wrapper too?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1330905 — Re: [PATCH V4 5/7] cpufreq: governor: No need to manage state machine now

On 10-02-16, 01:36, Rafael J. Wysocki wrote:
> >  static int cpufreq_governor_stop(struct cpufreq_policy *policy)
> >  {
> > -       struct policy_dbs_info *policy_dbs = policy->governor_data;
> > -
> > -       /* State should be equivalent to START */
> > -       if (!policy_dbs->policy)
> > -               return -EBUSY;
> > -
> > -       gov_cancel_work(policy_dbs);
> > -       policy_dbs->policy = NULL;
> > +       gov_cancel_work(policy);
> >
> >         return 0;
> >  }
> 
> So maybe we can call gov_cancel_work(policy) from
> cpufreq_governor_dbs() directly and get rid of this wrapper too?

I thought about it, but left it for consistency. It wouldn't hurt, the
compiler will anyway make it inline I believe.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1329884 — [PATCH V4 2/7] cpufreq: Call __cpufreq_governor() with policy->rwsem held

This isn't followed properly by all parts of the core code, some follow
it, whereas others don't.

Enforcing it will also enable us to remove cpufreq_governor_lock, that
is used today because we can't guarantee that __cpufreq_governor() isn't
executed in parallel.

We should also ensure that the lock is held across state changes to the
governors.

For example, while adding a CPU to the policy on cpu-online path, we
need to stop the governor, change policy->cpus, start the governor and
then refresh its limits. The complete sequence must be guaranteed to
execute without any concurrent races. And that can be achieved using
policy->rwsem around these use cases.

Also note that cpufreq_driver->stop_cpu() and ->exit() can get called
while policy->rwsem is held. That shouldn't have any side effects
though.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Juri Lelli <juri.lelli@arm.com>
Tested-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
 drivers/cpufreq/cpufreq.c | 49 +++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 863ac26c4ecf..51fb47cd38a0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1048,30 +1048,29 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
 	if (cpumask_test_cpu(cpu, policy->cpus))
 		return 0;
 
+	down_write(&policy->rwsem);
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret) {
 			pr_err("%s: Failed to stop governor\n", __func__);
-			return ret;
+			goto unlock;
 		}
 	}
 
-	down_write(&policy->rwsem);
 	cpumask_set_cpu(cpu, policy->cpus);
-	up_write(&policy->rwsem);
 
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
 		if (!ret)
 			ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
-		if (ret) {
+		if (ret)
 			pr_err("%s: Failed to start governor\n", __func__);
-			return ret;
-		}
 	}
 
-	return 0;
+unlock:
+	up_write(&policy->rwsem);
+	return ret;
 }
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
@@ -1374,13 +1373,13 @@ static void cpufreq_offline(unsigned int cpu)
 		return;
 	}
 
+	down_write(&policy->rwsem);
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret)
 			pr_err("%s: Failed to stop governor\n", __func__);
 	}
 
-	down_write(&policy->rwsem);
 	cpumask_clear_cpu(cpu, policy->cpus);
 
 	if (policy_is_inactive(policy)) {
@@ -1393,7 +1392,6 @@ static void cpufreq_offline(unsigned int cpu)
 		/* Nominate new CPU */
 		policy->cpu = cpumask_any(policy->cpus);
 	}
-	up_write(&policy->rwsem);
 
 	/* Start governor again for active policy */
 	if (!policy_is_inactive(policy)) {
@@ -1406,7 +1404,7 @@ static void cpufreq_offline(unsigned int cpu)
 				pr_err("%s: Failed to start governor\n", __func__);
 		}
 
-		return;
+		goto unlock;
 	}
 
 	if (cpufreq_driver->stop_cpu)
@@ -1428,6 +1426,9 @@ static void cpufreq_offline(unsigned int cpu)
 		cpufreq_driver->exit(policy);
 		policy->freq_table = NULL;
 	}
+
+unlock:
+	up_write(&policy->rwsem);
 }
 
 /**
@@ -1624,6 +1625,7 @@ EXPORT_SYMBOL(cpufreq_generic_suspend);
 void cpufreq_suspend(void)
 {
 	struct cpufreq_policy *policy;
+	int ret;
 
 	if (!cpufreq_driver)
 		return;
@@ -1634,7 +1636,11 @@ void cpufreq_suspend(void)
 	pr_debug("%s: Suspending Governors\n", __func__);
 
 	for_each_active_policy(policy) {
-		if (__cpufreq_governor(policy, CPUFREQ_GOV_STOP))
+		down_write(&policy->rwsem);
+		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
+		up_write(&policy->rwsem);
+
+		if (ret)
 			pr_err("%s: Failed to stop governor for policy: %p\n",
 				__func__, policy);
 		else if (cpufreq_driver->suspend
@@ -1656,6 +1662,7 @@ void cpufreq_suspend(void)
 void cpufreq_resume(void)
 {
 	struct cpufreq_policy *policy;
+	int ret;
 
 	if (!cpufreq_driver)
 		return;
@@ -1668,13 +1675,20 @@ void cpufreq_resume(void)
 	pr_debug("%s: Resuming Governors\n", __func__);
 
 	for_each_active_policy(policy) {
-		if (cpufreq_driver->resume && cpufreq_driver->resume(policy))
+		if (cpufreq_driver->resume && cpufreq_driver->resume(policy)) {
 			pr_err("%s: Failed to resume driver: %p\n", __func__,
 				policy);
-		else if (__cpufreq_governor(policy, CPUFREQ_GOV_START)
-		    || __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS))
-			pr_err("%s: Failed to start governor for policy: %p\n",
-				__func__, policy);
+		} else {
+			down_write(&policy->rwsem);
+			ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+			if (!ret)
+				__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
+			up_write(&policy->rwsem);
+
+			if (ret)
+				pr_err("%s: Failed to start governor for policy: %p\n",
+				       __func__, policy);
+		}
 	}
 
 	/*
@@ -2325,8 +2339,11 @@ static int cpufreq_boost_set_sw(int state)
 				       __func__);
 				break;
 			}
+
+			down_write(&policy->rwsem);
 			policy->user_policy.max = policy->max;
 			__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
+			up_write(&policy->rwsem);
 		}
 	}
 
-- 
2.7.1.370.gb2aa7f8

[toc] | [prev] | [next] | [standalone]

#1331790 — Re: [PATCH V4 2/7] cpufreq: Call __cpufreq_governor() with policy->rwsem held

On Tue, Feb 9, 2016 at 4:46 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> This isn't followed properly by all parts of the core code, some follow
> it, whereas others don't.

"The cpufreq core code is not consistent with respect to invoking
__cpufreq_governor() under policy->rwsem."

> Enforcing it will also enable us to remove cpufreq_governor_lock, that
> is used today because we can't guarantee that __cpufreq_governor() isn't
> executed in parallel.

"Changing all code to always hold policy->rwsem around
__cpufreq_governor() invocations will allow us to ..."

> We should also ensure that the lock is held across state changes to the
> governors.
>
> For example, while adding a CPU to the policy on cpu-online path, we
> need to stop the governor, change policy->cpus, start the governor and
> then refresh its limits. The complete sequence must be guaranteed to
> execute without any concurrent races. And that can be achieved using
> policy->rwsem around these use cases.
>
> Also note that cpufreq_driver->stop_cpu() and ->exit() can get called
> while policy->rwsem is held. That shouldn't have any side effects
> though.

The last paragraph is unclear.

Is it supposed to mean that the change will cause
cpufreq_driver->stop_cpu() and ->exit() to be called under
policy->rwsem sometimes?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1331855 — Re: [PATCH V4 2/7] cpufreq: Call __cpufreq_governor() with policy->rwsem held

On 11-02-16, 10:48, Rafael J. Wysocki wrote:
> On Tue, Feb 9, 2016 at 4:46 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Also note that cpufreq_driver->stop_cpu() and ->exit() can get called
> > while policy->rwsem is held. That shouldn't have any side effects
> > though.
> 
> The last paragraph is unclear.
> 
> Is it supposed to mean that the change will cause
> cpufreq_driver->stop_cpu() and ->exit() to be called under
> policy->rwsem sometimes?

Yeah, reworded it a bit ..

Also note that, after this patch cpufreq_driver->stop_cpu() and ->exit()
will get called while policy->rwsem is held, which wasn't the case
earlier. That shouldn't have any side effects though.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1329892 — [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish routines

The offline routine was separated into two halves earlier by
'commit 1aee40ac9c86 ("cpufreq: Invoke __cpufreq_remove_dev_finish()
after releasing cpu_hotplug.lock");.

And the reasons cited were, race issues between accessing policy's sysfs
files and policy kobject's cleanup.

That race isn't valid anymore, as we don't remove the policy & its
kobject completely on hotplugs, but do that from ->remove() callback of
subsys framework.

These two routines can be merged back now.

This is a preparatory step for the next patch, that will enforce
policy->rwsem lock around __cpufreq_governor() routines STOP/EXIT
sequence.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Tested-by: Juri Lelli <juri.lelli@arm.com>
Tested-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
 drivers/cpufreq/cpufreq.c | 36 ++++++++++--------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 9c62bf35b9dc..863ac26c4ecf 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1361,9 +1361,10 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	return ret;
 }
 
-static void cpufreq_offline_prepare(unsigned int cpu)
+static void cpufreq_offline(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
+	int ret;
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
 
@@ -1374,7 +1375,7 @@ static void cpufreq_offline_prepare(unsigned int cpu)
 	}
 
 	if (has_target()) {
-		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
+		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret)
 			pr_err("%s: Failed to stop governor\n", __func__);
 	}
@@ -1397,34 +1398,23 @@ static void cpufreq_offline_prepare(unsigned int cpu)
 	/* Start governor again for active policy */
 	if (!policy_is_inactive(policy)) {
 		if (has_target()) {
-			int ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
+			ret = __cpufreq_governor(policy, CPUFREQ_GOV_START);
 			if (!ret)
 				ret = __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 
 			if (ret)
 				pr_err("%s: Failed to start governor\n", __func__);
 		}
-	} else if (cpufreq_driver->stop_cpu) {
-		cpufreq_driver->stop_cpu(policy);
-	}
-}
 
-static void cpufreq_offline_finish(unsigned int cpu)
-{
-	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
-
-	if (!policy) {
-		pr_debug("%s: No cpu_data found\n", __func__);
 		return;
 	}
 
-	/* Only proceed for inactive policies */
-	if (!policy_is_inactive(policy))
-		return;
+	if (cpufreq_driver->stop_cpu)
+		cpufreq_driver->stop_cpu(policy);
 
 	/* If cpu is last user of policy, free policy */
 	if (has_target()) {
-		int ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
+		ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
 		if (ret)
 			pr_err("%s: Failed to exit governor\n", __func__);
 	}
@@ -1453,10 +1443,8 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	if (!policy)
 		return;
 
-	if (cpu_online(cpu)) {
-		cpufreq_offline_prepare(cpu);
-		cpufreq_offline_finish(cpu);
-	}
+	if (cpu_online(cpu))
+		cpufreq_offline(cpu);
 
 	cpumask_clear_cpu(cpu, policy->real_cpus);
 	remove_cpu_dev_symlink(policy, cpu);
@@ -2304,11 +2292,7 @@ static int cpufreq_cpu_callback(struct notifier_block *nfb,
 		break;
 
 	case CPU_DOWN_PREPARE:
-		cpufreq_offline_prepare(cpu);
-		break;
-
-	case CPU_POST_DEAD:
-		cpufreq_offline_finish(cpu);
+		cpufreq_offline(cpu);
 		break;
 
 	case CPU_DOWN_FAILED:
-- 
2.7.1.370.gb2aa7f8

[toc] | [prev] | [next] | [standalone]

#1331617 — Re: [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish routines

On Tue, Feb 9, 2016 at 4:46 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The offline routine was separated into two halves earlier by
> 'commit 1aee40ac9c86 ("cpufreq: Invoke __cpufreq_remove_dev_finish()
> after releasing cpu_hotplug.lock");.
>
> And the reasons cited were, race issues between accessing policy's sysfs
> files and policy kobject's cleanup.
>
> That race isn't valid anymore, as we don't remove the policy & its
> kobject completely on hotplugs, but do that from ->remove() callback of
> subsys framework.

Governor sysfs attributes are still removed in
__cpufreq_governor(_EXIT), though, so had store() been used for them,
the deadlock described in the changelog of commit 1aee40ac9c86 would
have been possible.

Fortunately, we don't use store() (which still does get_online_cpus())
for those attributes now.  We use governor_store() for them and that
doesn't call get_online_cpus().  So in fact this patch is only correct
after the recent rework of the governor attributes handling.

Please modify the changelog to explain that more thoroughly.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1331630 — Re: [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish routines

On Thu, Feb 11, 2016 at 1:59 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Feb 9, 2016 at 4:46 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> The offline routine was separated into two halves earlier by
>> 'commit 1aee40ac9c86 ("cpufreq: Invoke __cpufreq_remove_dev_finish()
>> after releasing cpu_hotplug.lock");.
>>
>> And the reasons cited were, race issues between accessing policy's sysfs
>> files and policy kobject's cleanup.
>>
>> That race isn't valid anymore, as we don't remove the policy & its
>> kobject completely on hotplugs, but do that from ->remove() callback of
>> subsys framework.
>
> Governor sysfs attributes are still removed in
> __cpufreq_governor(_EXIT), though, so had store() been used for them,
> the deadlock described in the changelog of commit 1aee40ac9c86 would
> have been possible.
>
> Fortunately, we don't use store() (which still does get_online_cpus())
> for those attributes now.  We use governor_store() for them and that
> doesn't call get_online_cpus().  So in fact this patch is only correct
> after the recent rework of the governor attributes handling.
>
> Please modify the changelog to explain that more thoroughly.

And one question tangentially related to this patch: Would it be
possible to avoid calling __cpufreq_governor(_EXIT) for CPU offline?

The fact that we still carry out the whole governor teardown at that
point is slightly disturbing, as in theory it should be possible to
keep the governor attributes in place across offline/online.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1331846 — Re: [PATCH V4 1/7] cpufreq: Merge cpufreq_offline_prepare/finish routines

On 11-02-16, 02:15, Rafael J. Wysocki wrote:
> And one question tangentially related to this patch: Would it be
> possible to avoid calling __cpufreq_governor(_EXIT) for CPU offline?
> 
> The fact that we still carry out the whole governor teardown at that
> point is slightly disturbing, as in theory it should be possible to
> keep the governor attributes in place across offline/online.

Will think about that after the current code is stable a bit. It should be
possible, but need to see if it is worth it.

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web