Re: [PATCH RFC] pm_qos_requirement might sleep
From: John Kacur
Date: Tue Aug 26 2008 - 04:48:28 EST
On Mon, Aug 25, 2008 at 6:35 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Mon, 2008-08-25 at 09:34 -0700, mark gross wrote:
>> On Fri, Aug 15, 2008 at 12:51:11AM +0200, John Kacur wrote:
>> > On Thu, Aug 14, 2008 at 7:48 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>> > > On Thu, 2008-08-14 at 08:52 -0700, mark gross wrote:
>> > >
>> > >> Keeping a lock around the different "target_value"s may not be so
>> > >> important. Its just a 32bit scaler value, and perhaps we can make it an
>> > >> atomic type? That way we loose the raw_spinlock.
>> > >
>> > > My suggestion was to keep the locking for the write side - so as to
>> > > avoid stuff stomping on one another, but drop the read side as:
>> > >
>> > > spin_lock
>> > > foo = var;
>> > > spin_unlock
>> > > return foo;
>> > >
>> > > is kinda useless, it doesn't actually serialize against the usage of
>> > > foo, that is, once it gets used, var might already have acquired a new
>> > > value.
>> > >
>> > > The only thing it would protect is reading var, but since that is a
>> > > machine sized read, its atomic anyway (assuming its naturally aligned).
>> > >
>> > > So no need for atomic_t (its read-side is just a read too), just drop
>> > > the whole lock usage from pq_qos_requirement().
>> > >
>> >
>> > Thanks Peter.
>> >
>> > Mark, is the following patch ok with you? This should be applied to
>> > mainline, and then after that no special patches are necessary for
>> > real-time.
>>
>> I've been thinking about this patch and I worry that the readability
>> from making the use of this lock asymmetric WRT reads and writes to the
>> storage address is bothersome.
>>
>> I would rather make the variable an atomic. What do you think about
>> that?
>
> It would make the write side more expensive, as we already have the two
> atomic operations for the lock and unlock, this would add a third.
>
> Then again, I doubt that this is really a fast path.
>
> OTOH, a simple comment could clarify the situation for the reader.
>
> Up to you I guess ;-)
>
Personally I agree with Peter, a simple comment would clarify the
situation, it seems quite silly to me to add complexity in the name of
symmetry. This is not my definition of readability. Never-the-less I
offer up solution number 3 here if that would please everyone more.
Attached is a patch that changes the target value to an atomic
variable as suggested by Arjan. To summarize.
3 Sol'ns - all of which solve the problem.
1. Add a raw spinlock around target value only. This makes the raw
spinlock area very small, and is converted to a normal spinlock for
non-preempt-rt.
2. Remove the spinlock altogether in pm_qos_requirement since the
simple read is already atomic. Advantage - smallest patch and realtime
doesn't require a special patch once this is included in mainline. I
like this one the best.
3. make target_value atomic_t. Advantage - symmetry, some people find
this more readable. The patch is larger than the above solution but as
above, no special patch is required for realtime once this is included
in mainline. Solution three is in the attached patch. Comments are
appreciated as always.
Remove the spinlock in pm_qos_requirement by making target_value an atomic type.
This is necessary for real-time since pm_qos_requirement is called by idle and
cannot be allowed to sleep.
Signed-off-by: John Kacur <jkacur at gmail dot com>
Index: linux-2.6.26.3-rt3/kernel/pm_qos_params.c
===================================================================
--- linux-2.6.26.3-rt3.orig/kernel/pm_qos_params.c
+++ linux-2.6.26.3-rt3/kernel/pm_qos_params.c
@@ -42,7 +42,7 @@
#include <linux/uaccess.h>
/*
- * locking rule: all changes to target_value or requirements or notifiers lists
+ * locking rule: all changes to requirements or notifiers lists
* or pm_qos_object list and pm_qos_objects need to happen with pm_qos_lock
* held, taken with _irqsave. One lock to rule them all
*/
@@ -65,7 +65,7 @@ struct pm_qos_object {
struct miscdevice pm_qos_power_miscdev;
char *name;
s32 default_value;
- s32 target_value;
+ atomic_t target_value;
s32 (*comparitor)(s32, s32);
};
@@ -76,7 +76,7 @@ static struct pm_qos_object cpu_dma_pm_q
.notifiers = &cpu_dma_lat_notifier,
.name = "cpu_dma_latency",
.default_value = 2000 * USEC_PER_SEC,
- .target_value = 2000 * USEC_PER_SEC,
+ .target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
.comparitor = min_compare
};
@@ -86,7 +86,7 @@ static struct pm_qos_object network_lat_
.notifiers = &network_lat_notifier,
.name = "network_latency",
.default_value = 2000 * USEC_PER_SEC,
- .target_value = 2000 * USEC_PER_SEC,
+ .target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
.comparitor = min_compare
};
@@ -98,7 +98,7 @@ static struct pm_qos_object network_thro
.notifiers = &network_throughput_notifier,
.name = "network_throughput",
.default_value = 0,
- .target_value = 0,
+ .target_value = ATOMIC_INIT(0),
.comparitor = max_compare
};
@@ -149,13 +149,14 @@ static void update_target(int target)
extreme_value = pm_qos_array[target]->comparitor(
extreme_value, node->value);
}
- if (pm_qos_array[target]->target_value != extreme_value) {
+ spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+ if (atomic_read(&pm_qos_array[target]->target_value) != extreme_value) {
call_notifier = 1;
- pm_qos_array[target]->target_value = extreme_value;
+ atomic_set(&pm_qos_array[target]->target_value, extreme_value);
pr_debug(KERN_ERR "new target for qos %d is %d\n", target,
- pm_qos_array[target]->target_value);
+ atomic_read(&pm_qos_array[target]->target_value));
}
- spin_unlock_irqrestore(&pm_qos_lock, flags);
if (call_notifier)
blocking_notifier_call_chain(pm_qos_array[target]->notifiers,
@@ -193,11 +194,8 @@ static int find_pm_qos_object_by_minor(i
int pm_qos_requirement(int pm_qos_class)
{
int ret_val;
- unsigned long flags;
- spin_lock_irqsave(&pm_qos_lock, flags);
- ret_val = pm_qos_array[pm_qos_class]->target_value;
- spin_unlock_irqrestore(&pm_qos_lock, flags);
+ ret_val = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
return ret_val;
}