Date:	Tue, 26 Aug 2008 10:48:13 +0200
From:	"John Kacur" <jkacur@...il.com>
To:	mgross@...ux.intel.com, LKML <linux-kernel@...r.kernel.org>,
	rt-users <linux-rt-users@...r.kernel.org>
Cc:	"Peter Zijlstra" <peterz@...radead.org>,
	"Steven Rostedt" <rostedt@...dmis.org>,
	"Ingo Molnar" <mingo@...e.hu>,
	"Thomas Gleixner" <tglx@...utronix.de>, arjan <arjan@...radead.org>
Subject: Re: [PATCH RFC] pm_qos_requirement might sleep

On Mon, Aug 25, 2008 at 6:35 PM, Peter Zijlstra <peterz@...radead.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@...radead.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.

View attachment "pm_qos_requirement.patch" of type "text/x-patch" (2980 bytes)

Powered by blists - more mailing lists