Re: [PATCH RFC] pm_qos_requirement might sleep

mark gross
2008-08-28T19:37:51Z

On Tue, Aug 26, 2008 at 07:45:37PM +0200, John Kacur wrote:
> On Tue, Aug 26, 2008 at 6:18 PM, mark gross <mgross@linux.intel.com> wrote:
> > On Tue, Aug 26, 2008 at 10:48:13AM +0200, John Kacur wrote:
> >> On Mon, Aug 25, 2008 at 6:35 PM, Peter Zijlstra <peterz@infradead.org> 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@infradead.org> 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.
> >
> > Thank you! FWIW I'm really on the fence between option 2 and 3.
> >
> >> 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);
> >> +
> >
> > do we want to move the unlock before the setting of the target_value?
> > This feels wrong to me, the option 2 patch didn't do this.
> >
> > couldn't we have a race from 2 cpu's hitting update_target at the same
> > time with different values if we drop the lock before the target_value
> > is set?
>
> I think you are right since atomicity doesn't have anything to do with
> ordering, good catch, putting the the unlock back where it was before,
> new patch attached. (also shortened-up pm_qos_requirement)
>
> ---SNIP----
>
> John

looks good, its running without error on the non-rt kernel.

Signed-off-by: mark gross <mgross@linux.intel.com>

FWIW I'll send this as patch to Andrew in a moment.

--mgross

> 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,11 +149,11 @@ 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) {
> + 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);
>
> @@ -192,14 +192,7 @@ 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);
> -
> - return ret_val;
> + return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> }
> EXPORT_SYMBOL_GPL(pm_qos_requirement);
>