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

Groups > linux.kernel > #1306541 > unrolled thread

[RFC PATCH 18/19] cpufreq: remove transition_lock

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH 18/19] cpufreq: remove transition_lock Juri Lelli <juri.lelli@arm.com> - 2016-01-11 18:40 +0100
    Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-12 12:30 +0100
      Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Michael Turquette <mturquette@baylibre.com> - 2016-01-13 02:10 +0100
        Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-13 07:40 +0100
          Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Juri Lelli <juri.lelli@arm.com> - 2016-01-14 10:50 +0100
          Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-14 11:40 +0100
            Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Juri Lelli <juri.lelli@arm.com> - 2016-01-14 15:00 +0100

#1306541 — [RFC PATCH 18/19] cpufreq: remove transition_lock

From: Michael Turquette <mturquette@baylibre.com>

transition_lock was introduced to serialize cpufreq transition
notifiers. Instead of using a different lock for protecting concurrent
modifications of policy, it is better to require that callers of
transition notifiers implement appropriate locking (this is already the
case AFAICS). Removing transition_lock also simplifies current locking
scheme.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Michael Turquette <mturquette@baylibre.com>
---
 drivers/cpufreq/cpufreq.c | 19 ++++++++++---------
 include/linux/cpufreq.h   |  1 -
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6c9bef7..78b1e2f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -421,10 +421,15 @@ static void cpufreq_notify_post_transition(struct cpufreq_policy *policy,
 	cpufreq_notify_transition(policy, freqs, CPUFREQ_POSTCHANGE);
 }
 
+/*
+ * Callers must ensure proper mutual exclusion on policy (for transition_
+ * ongoing/transition_task handling). While holding policy->rwsem is
+ * sufficient, other scheme might work as well (e.g., cpufreq_governor.c
+ * holds timer_mutex while entering the path that generates transitions).
+ */
 void cpufreq_freq_transition_begin(struct cpufreq_policy *policy,
 		struct cpufreq_freqs *freqs)
 {
-
 	/*
 	 * Catch double invocations of _begin() which lead to self-deadlock.
 	 * ASYNC_NOTIFICATION drivers are left out because the cpufreq core
@@ -439,22 +444,19 @@ void cpufreq_freq_transition_begin(struct cpufreq_policy *policy,
 wait:
 	wait_event(policy->transition_wait, !policy->transition_ongoing);
 
-	spin_lock(&policy->transition_lock);
-
-	if (unlikely(policy->transition_ongoing)) {
-		spin_unlock(&policy->transition_lock);
+	if (unlikely(policy->transition_ongoing))
 		goto wait;
-	}
 
 	policy->transition_ongoing = true;
 	policy->transition_task = current;
 
-	spin_unlock(&policy->transition_lock);
-
 	cpufreq_notify_transition(policy, freqs, CPUFREQ_PRECHANGE);
 }
 EXPORT_SYMBOL_GPL(cpufreq_freq_transition_begin);
 
+/*
+ * As above, callers must ensure proper mutual exclusion on policy.
+ */
 void cpufreq_freq_transition_end(struct cpufreq_policy *policy,
 		struct cpufreq_freqs *freqs, int transition_failed)
 {
@@ -1057,7 +1059,6 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	kobject_init(&policy->kobj, &ktype_cpufreq);
 	INIT_LIST_HEAD(&policy->policy_list);
 	init_rwsem(&policy->rwsem);
-	spin_lock_init(&policy->transition_lock);
 	init_waitqueue_head(&policy->transition_wait);
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 79b87ce..6bbb88f 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -105,7 +105,6 @@ struct cpufreq_policy {
 
 	/* Synchronization for frequency transitions */
 	bool			transition_ongoing; /* Tracks transition status */
-	spinlock_t		transition_lock;
 	wait_queue_head_t	transition_wait;
 	struct task_struct	*transition_task; /* Task which is doing the transition */
 
-- 
2.2.2

[toc] | [next] | [standalone]

#1307287

On 11-01-16, 17:35, Juri Lelli wrote:
> From: Michael Turquette <mturquette@baylibre.com>
> 
> transition_lock was introduced to serialize cpufreq transition
> notifiers. Instead of using a different lock for protecting concurrent
> modifications of policy, it is better to require that callers of
> transition notifiers implement appropriate locking (this is already the
> case AFAICS). Removing transition_lock also simplifies current locking
> scheme.

So, are you saying that the reasoning mentioned in this patch are all
wrong?

commit 12478cf0c55e ("cpufreq: Make sure frequency transitions are
serialized")

-- 
viresh

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

#1307950

Hi Viresh,

Quoting Viresh Kumar (2016-01-12 03:24:09)
> On 11-01-16, 17:35, Juri Lelli wrote:
> > From: Michael Turquette <mturquette@baylibre.com>
> > 
> > transition_lock was introduced to serialize cpufreq transition
> > notifiers. Instead of using a different lock for protecting concurrent
> > modifications of policy, it is better to require that callers of
> > transition notifiers implement appropriate locking (this is already the
> > case AFAICS). Removing transition_lock also simplifies current locking
> > scheme.
> 
> So, are you saying that the reasoning mentioned in this patch are all
> wrong?
> 
> commit 12478cf0c55e ("cpufreq: Make sure frequency transitions are
> serialized")

No, that's not what I'm saying. Quoting that patch:

"""
The key challenge is to allow drivers to begin the transition from one thread
and end it in a completely different thread (this is to enable drivers that do
asynchronous POSTCHANGE notification from bottom-halves, to also use the same
interface).

To achieve this, a 'transition_ongoing' flag, a 'transition_lock' spinlock and a
wait-queue are added per-policy. The flag and the wait-queue are used in
conjunction to create an "uninterrupted flow" from _begin() to _end(). The
spinlock is used to ensure that only one such "flow" is in flight at any given
time. Put together, this provides us all the necessary synchronization.
"""

So the transition_onging flag and wait-queue are all good. That stuff is
just great. This patch doesn't touch it.

What it does change is that it removes a superfluous spinlock that
should never have needed to exist in the first place.
cpufreq_freq_transition_begin is called directly by driver target
callbacks, and it is called by __cpufreq_driver_target.

__cpufreq_driver_target should be using a per-policy lock. Any other
behavior is just insane. I haven't gone through this thread to see if
that change has been made by Juri, but we need to get there either in
this series or the follow-up series that introduces some RCU locking.

Regards,
Mike

> 
> -- 
> viresh

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

#1308079

On 12-01-16, 16:54, Michael Turquette wrote:
> __cpufreq_driver_target should be using a per-policy lock.

It doesn't :)

-- 
viresh

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

#1309124

Hi,

On 13/01/16 10:21, Michael Turquette wrote:
> Hi Viresh,
> 
> Quoting Viresh Kumar (2016-01-12 22:31:48)
> > On 12-01-16, 16:54, Michael Turquette wrote:
> > > __cpufreq_driver_target should be using a per-policy lock.
> > 
> > It doesn't :)
> 
> It should.
> 
> A less conceited response is that a per-policy lock should be held
> around calls to __cpufreq_driver_target. This can obviously be done by
> cpufreq_driver_target (no double underscore), but there are quite a few
> drivers that call __cpufreq_driver_target, and since we're touching the
> policy structure we need a lock around it.
> 

Agree, we should enforce the rule that everything that touches policy
structure has to lock it before.

> Juri's cover letter did not explicitly state my original, full intention
> for the patches I was working on. I'll spell that out below and
> hopefully we can gather consensus around it before moving forward. Juri,
> I'm implicitly assuming that you agree with the stuff below, but please
> correct me if I am wrong.

Right. I decided to post with this RFC only a subset of the patches we
came up with because I needed to build some more confidence with the
subsystem I was going to propose changes for. Review comments received
are helping me on that front. I didn't mention at all next steps (RCU)
because I wanted to focus on understanding and documenting, and maybe
fixing where required, the current status, before we change it.

> The original idea for overhauling the locking
> in cpufreq is to use two locks:
> 
> 1) per-policy lock (my patches were using a mutex), which is the only
> lock that needs to be touched during a frequency transition. We do not
> want any lock contention between policy's during freq transition. For
> read-side operation this locking scheme should utilize RCU so that the
> scheduler can safely access the values in struct cpufreq_policy within
> it's schedule() context. [a note on RCU below]
> 
> 2) a single, framework-wide lock (my patches were using a mutex) that
> handles all of the other synchronization: governor events, driver events
> and anything else that does not happen on a per-policy basis. I don't
> think RCU is necessary for this. These operations are all slow-path ones
> so reducing the mess of 6-ish locks in cpufreq.c and friends down to a
> single mutex simplifies things greatly, eliminates the "drop the lock
> here for a few instructions" hacks and generally makes things more
> readable.
> 

This is basically what I also have on top of this series. I actually
went for RCUs also for 2, but yes, that's maybe overkilling.

A comment on 1 above, and something on which I got stuck upon for some
time, is that, if we implement RCU logic as it is supposed to be, I
think we can generate a lot of copy-update operations when changing
frequency (as policy structure needs to be changed). Also, we might read
stale data. So, I'm not sure this will pay off. However, I tried to get
around this problem and I guess we will discuss if 1 is doable in the
next RFC :-).

> A quick note on RCU and the scheduler-driven DVFS stuff: RCU only helps
> us on read-side operations. For the purposes of sched-dvfs, this means
> that when we look at capacity utilization and want to normalize
> frequency based on that, we need to access the per-policy structure in a
> lockless way. RCU makes this possible.
> 
> RCU is absolutely not a magic bullet or elixir that lets us kick off
> DVFS transitions from the schedule() context. The frequency transitions
> are write-side operations, as we invariably touch struct cpufreq_policy.
> This means that the read-side stuff can live in the schedule() context,
> but write-side needs to be kicked out to a thread.
> 

Correct. We will still need the kthread machinery even after this
changes.

Thanks for clarifying things!

Best,

- Juri

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

#1309164

On 13-01-16, 10:21, Michael Turquette wrote:
> Quoting Viresh Kumar (2016-01-12 22:31:48)
> > On 12-01-16, 16:54, Michael Turquette wrote:
> > > __cpufreq_driver_target should be using a per-policy lock.
> > 
> > It doesn't :)
> 
> It should.

I thought we wanted the routine doing DVFS to not sleep as it will be
called from scheduler ?

Looks fine otherwise. But yeah, the series is still incomplete in the
sense that there is no lock today around __cpufreq_driver_target().

-- 
viresh

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

#1309293

On 14/01/16 16:02, Viresh Kumar wrote:
> On 13-01-16, 10:21, Michael Turquette wrote:
> > Quoting Viresh Kumar (2016-01-12 22:31:48)
> > > On 12-01-16, 16:54, Michael Turquette wrote:
> > > > __cpufreq_driver_target should be using a per-policy lock.
> > > 
> > > It doesn't :)
> > 
> > It should.
> 
> I thought we wanted the routine doing DVFS to not sleep as it will be
> called from scheduler ?
> 
> Looks fine otherwise. But yeah, the series is still incomplete in the
> sense that there is no lock today around __cpufreq_driver_target().
> 

I was under the impression that the purpose of having
__cpufreq_driver_target() exported outside cpufreq.c was working due to
the fact that users implement their own locking.

That's why I put the following comment in this patch.

 /*
  * Callers must ensure proper mutual exclusion on policy (for transition_
  * ongoing/transition_task handling). While holding policy->rwsem is
  * sufficient, other schemes might work as well (e.g., cpufreq_governor.c
  * holds timer_mutex while entering the path that generates transitions).
  */

From what I can see ondemand and conservative (via governor) seem to use
timer_mutex; userspace userspace_mutex instead. Do they serve different
purposes instead? How do we currently serialize operations on policy
when using __cpufreq_driver_target() directly otherwise?

Thanks,

- Juri

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web