* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Peter Zijlstra @ 2014-07-23 6:57 UTC (permalink / raw)
To: Mike Galbraith
Cc: Andi Kleen, Waiman Long, Thomas Gleixner, Ingo Molnar,
Darren Hart, Davidlohr Bueso, Heiko Carstens, linux-kernel,
linux-api, linux-doc, Jason Low, Scott J Norton
In-Reply-To: <1406091303.5076.33.camel@marge.simpson.net>
On Wed, Jul 23, 2014 at 06:55:03AM +0200, Mike Galbraith wrote:
> On Mon, 2014-07-21 at 09:42 -0700, Andi Kleen wrote:
>
> > FWIW the main problem is currently that switch-through-idle is so
> > slow. I think improving that would give a boost to far more
> > situations.
>
> Two high frequency idle enter/exit suckage spots:
>
> 1) nohz (tick) - it's expensive to start/stop tick on every micro-idle,
> throttle it or something.
Yeah, so the idea was to use the cpuidle idle guestimator to control
this, and now that we've moved it somewhat closer to the scheduler that
might become possible.
> 2) ondemand governor - tweak silly default settings to reflect the
> reality that we routinely schedule communicating threads cross core.
Yeah, so the plan is to shoot cpufreq in the head and base the
replacement on smp aware metrics ;-) Its on a todo list somewhere..
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Mike Galbraith @ 2014-07-23 4:55 UTC (permalink / raw)
To: Andi Kleen
Cc: Waiman Long, Thomas Gleixner, Ingo Molnar, Peter Zijlstra,
Darren Hart, Davidlohr Bueso, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Jason Low, Scott J Norton
In-Reply-To: <8761iq3bp3.fsf-KWJ+5VKanrL29G5dvP0v1laTQe2KTcn/@public.gmane.org>
On Mon, 2014-07-21 at 09:42 -0700, Andi Kleen wrote:
> FWIW the main problem is currently that switch-through-idle is so
> slow. I think improving that would give a boost to far more
> situations.
Two high frequency idle enter/exit suckage spots:
1) nohz (tick) - it's expensive to start/stop tick on every micro-idle,
throttle it or something.
2) ondemand governor - tweak silly default settings to reflect the
reality that we routinely schedule communicating threads cross core.
(3. seek/destroy fastpath cycles, damn things multiply over time)
-Mike
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-22 21:03 UTC (permalink / raw)
To: Waiman Long
Cc: Darren Hart, Andy Lutomirski, Peter Zijlstra, Andi Kleen,
Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton,
Steven Rostedt
In-Reply-To: <53CEC7B3.5030109@hp.com>
On Tue, 22 Jul 2014, Waiman Long wrote:
> On 07/21/2014 09:01 PM, Thomas Gleixner wrote:
> > So before anyone comes up with a "solution" for all of this tomorrow
> > afternoon in form of another half baken patch, please sit back mull it
> > in your head and lets have a proper discussion about the approach
> > first.
> >
> > Thanks,
> >
> > tglx
>
> Thank for your thorough analysis and suggestions on what to do to support
> spinning futexes. You certainly know more about the internal working of futex
> than most of us. I can live with what you have suggested. My patch is just a
> proof of concept piece to demonstrate optimistic spinning on futex is
> something worthwhile to do. I think I have achieved my goal of stirring
We knew that already as Darren has proven that optimistic spinning in
a simpler form provides a huge benefit. So you do not have achieved
anything except annoying people with your sloppiness.
> interest in this area. My next step will be to look into the direction of what
> you have suggested and figure out what actual code changes will be needed.
You really can do what you want with your time. You can either reread
AND understand the paragraph, which I left as a quote from my previous
mail, AND act acoordingly or just stay away from futex.c.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 1/5] futex: add new exclusive lock & unlock command codes
From: Thomas Gleixner @ 2014-07-22 21:00 UTC (permalink / raw)
To: Waiman Long
Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
Heiko Carstens, LKML, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Jason Low, Scott J Norton
In-Reply-To: <53CEABD7.3030509-VXdhtT5mjnY@public.gmane.org>
On Tue, 22 Jul 2014, Waiman Long wrote:
> On 07/21/2014 12:42 PM, Thomas Gleixner wrote:
> > > + /*
> > > + * The unlocker may have cleared the TID value and another task may
> > > + * steal it. However, if its TID is still set, we need to clear
> > > + * it as well as the FUTEX_WAITERS bit.
> >
> > No, that's complete and utter crap. The unlocker is current and it may
> > not have cleared anything.
> >
> > Your design or the lack thereof is a complete disaster.
>
> In patch 5, the documentation and the sample unlock does clear the TID before
> going in. The code here is just a safety measure in case the unlocker doesn't
> follow the recommendation.
I don't care about patch 5 at all. I'm already fed up reading 1/5. The
code is no safety measure it's a completely disastrous workaround.
We do not care whether user space follows recommendations. We set
rules and if the rules are violated then we act accordingly. Did you
even take the time to read the git history of futex.c? Did you notice
that we had a major security incident related to that code which we
fixed not long ago?
No, you obviously did not, otherwise you would not come up with
hackery which is going to explode in your face if exposed to a simple
syscall fuzzer, not to talk about a competent hacker. Without even
looking too deep I found two simple ways to leak kernel state and one
to corrupt kernel state. Brilliant stuff, really!
> > Sit down first and define the exact semantics of the new opcode. That
> > includes user and kernel space and the interaction with robust list,
> > which you happily ignored.
> >
> > What are the semantics of uval? When can it be changed in kernel and
> > in user space? How do we deal with corruption of the user space value?
>
> The semantics of the uval is the same as that of PI and robust futex where the
> TID portion contains the thread ID of the lock owner. It is my intention to
> make it works with the robust futex mechanism before it can be merged. This
> RPC patch series is for soliciting feedbacks and make the necessary changes
> that make the patch acceptable before I go deep into making it works with
> robust futex.
No, the semantics are not the same. PI and robust futexes have
different semantics, but you did not notice that at all.
Your so called semantics are really well thought out as as you have
proven with completely uncomprehensible hackeries, which you call
"safety measures".
And I do not care about your intention to make it work with robustness
etc. If you do not design it upfront to do so, then this code is going
to be a complete disaster. But to do that you need to sit down and
provide a proper design document and that wants to be patch 1/x not
the last one. And the code actually needs to follow that design.
> > How are faults handled?
>
> As you have a lot more experience working with futexes than me, any
> suggestions on what kind of faults will happen and what are the best practices
> to handle them will be highly appreciated.
So shall I fly over and read you the source code of futex.c as a
bedtime story?
You did not even try to understand how futexes work and what corner
cases they have by studying the existing code and reading the git
history.
No, you simply cobbled something together and created random
performance numbers and because they are so wonderful, you expect that
I and the other people who worked hard on that code do your homework.
No, that's not how it works.
Your numbers are completely useless because they just measure the fast
path by omitting all the required security measures and corner case
handling.
Darren had his version of spinning done years ago, but we had to
ground it due to not resolvable issues at that point. I'm quite sure,
that you did not even try to figure out whether people had looked into
that issue before and tried to understand why it failed, otherwise you
would have been clever enough to provide a reference and explain why
your approach is better or solves the underlying problems.
So your RFC is not impressive at all. It's just an inferior
implementation of something we are aware of for a very long time.
I'm already tired of this, really. Unless you start to understand the
problem of futexes in the first place and you should ask your coworker
how mind boggling that can be, do not even bother to send another half
arsed patch. Spare the electrons and the time of everyone involved.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-22 20:52 UTC (permalink / raw)
To: Waiman Long
Cc: Peter Zijlstra, Steven Rostedt, Darren Hart, Andy Lutomirski,
Andi Kleen, Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton, Robert Haas
In-Reply-To: <53CEC8AC.7020700@hp.com>
On Tue, 22 Jul 2014, Waiman Long wrote:
> On 07/22/2014 05:59 AM, Thomas Gleixner wrote:
> > On Tue, 22 Jul 2014, Peter Zijlstra wrote:
> > > On Tue, Jul 22, 2014 at 10:39:17AM +0200, Thomas Gleixner wrote:
> > > > On Tue, 22 Jul 2014, Peter Zijlstra wrote:
> > > > > Anyway, there is one big fail in the entire futex stack that we 'need'
> > > > > to sort some day and that is NUMA. Some people (again database people)
> > > > > explicitly do not use futexes and instead use sysvsem because of this.
> > > > >
> > > > > The problem with numa futexes is that because they're vaddr based
> > > > > there
> > > > > is no (persistent) node information. You always end up having to fall
> > > > > back to looking in all nodes before you can guarantee there is no
> > > > > matching futex.
> > > > >
> > > > > One way to achieve it is by extending the futex value to include a
> > > > > node
> > > > > number, but that's obviously a complete ABI break. Then again, it
> > > > > should
> > > > > be pretty straight fwd, since the node number doesn't need to be part
> > > > > of
> > > > > the actual atomic update part, just part of the userspace storage.
> > > > So you want per node hash buckets, right? Fair enough, but how do you
> > > > make sure, that no thread/process on a different node is fiddling with
> > > > that "node bound" futex as well?
> > > You don't and that should work just as well, just slower. But since the
> > > node id is in the futex 'value' we'll always end up in the right
> > > node-hash, even if its a remote one.
> > >
> > > So yes, per node hashes, and a persistent futex->node map.
> > Which works fine as long as you only have the futex_q on the stack of
> > the blocked task. If user space is lying to you, then you just end up
> > with a bunch of threads sleeping forever. Who cares?
> >
> > But if you create independent kernel state, which we have with
> > pi_state and which you need for finegrained locking and further
> > spinning fun, you open up another can of worms. Simply because this
> > would enable rogue user space to create multiple instances of the
> > kernel internal state. I can predict the CVEs resulting from that
> > even without using a crystal ball.
> >
> > Thanks,
> >
> > tglx
>
> I think NUMA futex, if implemented, is a completely independent piece that
> have no direct relationship with optimistic spinning futex. It should be a
> separate patch and not mixing with optimistic spinning patch which will only
> make the latter one more complicated.
Bullshit. Of course it handles separate issues, but Peter is
completely right, that the NUMA aspect is a far bigger issue than the
optimistic spinning stuff. Do you have an idea what the costs of cross
node memory access and cacheline bouncing are? Obviously not, as you
only interest seems to be to slap optimistic spinning to every place
which deals with locking.
And if you had tried to read _AND_ understand the discussion above,
you might have noticed that providing NUMA awareness requires a lot of
the functionality which is needed for optimistic spinning as well.
But no, you did not even take the time to think about it, you just
claim that it makes your optimistic stuff more complicated. Just get
it, there is a world outside of optimistic spinning.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Waiman Long @ 2014-07-22 20:25 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Peter Zijlstra, Steven Rostedt, Darren Hart, Andy Lutomirski,
Andi Kleen, Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton, Robert Haas
In-Reply-To: <alpine.DEB.2.10.1407221057230.20847@nanos>
On 07/22/2014 05:59 AM, Thomas Gleixner wrote:
> On Tue, 22 Jul 2014, Peter Zijlstra wrote:
>> On Tue, Jul 22, 2014 at 10:39:17AM +0200, Thomas Gleixner wrote:
>>> On Tue, 22 Jul 2014, Peter Zijlstra wrote:
>>>> Anyway, there is one big fail in the entire futex stack that we 'need'
>>>> to sort some day and that is NUMA. Some people (again database people)
>>>> explicitly do not use futexes and instead use sysvsem because of this.
>>>>
>>>> The problem with numa futexes is that because they're vaddr based there
>>>> is no (persistent) node information. You always end up having to fall
>>>> back to looking in all nodes before you can guarantee there is no
>>>> matching futex.
>>>>
>>>> One way to achieve it is by extending the futex value to include a node
>>>> number, but that's obviously a complete ABI break. Then again, it should
>>>> be pretty straight fwd, since the node number doesn't need to be part of
>>>> the actual atomic update part, just part of the userspace storage.
>>> So you want per node hash buckets, right? Fair enough, but how do you
>>> make sure, that no thread/process on a different node is fiddling with
>>> that "node bound" futex as well?
>> You don't and that should work just as well, just slower. But since the
>> node id is in the futex 'value' we'll always end up in the right
>> node-hash, even if its a remote one.
>>
>> So yes, per node hashes, and a persistent futex->node map.
> Which works fine as long as you only have the futex_q on the stack of
> the blocked task. If user space is lying to you, then you just end up
> with a bunch of threads sleeping forever. Who cares?
>
> But if you create independent kernel state, which we have with
> pi_state and which you need for finegrained locking and further
> spinning fun, you open up another can of worms. Simply because this
> would enable rogue user space to create multiple instances of the
> kernel internal state. I can predict the CVEs resulting from that
> even without using a crystal ball.
>
> Thanks,
>
> tglx
I think NUMA futex, if implemented, is a completely independent piece
that have no direct relationship with optimistic spinning futex. It
should be a separate patch and not mixing with optimistic spinning patch
which will only make the latter one more complicated.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Waiman Long @ 2014-07-22 20:21 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Darren Hart, Andy Lutomirski, Peter Zijlstra, Andi Kleen,
Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton,
Steven Rostedt
In-Reply-To: <alpine.DEB.2.10.1407220047490.20847@nanos>
On 07/21/2014 09:01 PM, Thomas Gleixner wrote:
> On Mon, 21 Jul 2014, Darren Hart wrote:
>> On 7/21/14, 14:47, "Thomas Gleixner"<tglx@linutronix.de> wrote:
>>
>>> On Mon, 21 Jul 2014, Andy Lutomirski wrote:
>>>> On Mon, Jul 21, 2014 at 2:27 PM, Peter Zijlstra<peterz@infradead.org>
>>>> wrote:
>>>>> All this is predicated on the fact that syscalls are 'expensive'.
>>>>> Weren't syscalls only 100s of cycles? All this bitmap mucking is far
>>>>> more expensive due to cacheline misses, which due to the size of the
>>>>> things is almost guaranteed.
>>>> 120 - 300 cycles for me, unless tracing happens, and I'm working on
>>>> reducing the incidence of tracing.
>>> So it's a non issue indeed and definitely not worth the trouble of
>>> that extra storage, the scheduler overhead, etc.
>>>
>>> Summary: Once you can't take it atomically in user space, you've lost
>>> anyway. And we are better off to do the magic spinning in
>>> kernel where we have all the information accessible already.
>> And we have such an implementation with the FUTEX_LOCK_ADAPTIVE code we
>> discussed back in Oct 2010 (purely kernel, no VDSO), updated with some of
>> your and other's comments:
>>
>> http://git.infradead.org/users/dvhart/linux.git/shortlog/refs/heads/futex/f
>> utex-lock/v7
>>
>>
>> I can work on forward porting this series to current mainline (post recent
>> security fixes) and cleaning up the commentary and such if people are
>> interested in seeing this implementation (based on Peter Z's spinning
>> mutex work iirc) resurrected...
> No. We really want to avoid more magic hackery in the futex code.
>
> Lets sit down and think about what we need:
>
> 1) Support for all existing features
>
> 2) More fine grained locking
>
> 3) Optimistic spinning
>
> @1: No discussion about that. Period.
>
> We are not going to introduce some new futex opcode, which is
> optimized for a microbenchmark and ignoring all of the pain we
> went through in the last 10 years. No way, especially after the
> recent security disaster.
>
> @2 and @3: Yes, we want that.
>
> But again, we don't add fine grained locking just for some half
> baken new opcode. No, we are adding it where it makes the most
> sense and lets us reuse most of the code.
>
> I can predict your question, how that should work :)
>
> If we want to have fine grained locking beyond the futex hash
> buckets and that's something we discussed before, you need a state
> representation of the user space futex in the kernel. That's what
> Waiman added as well, just in a way that's beyond repair.
>
> The charm of the futex hash buckets is that they do not create
> kernel state because the futex_q which is enqueued into the bucket
> is on the task stack of the task which is blocked on the futex.
>
> So much for the theory, because that stopped to be true when we
> added support for PI futexes. They already create kernel internal
> state which is not magically removed when the thread exits the
> syscall. It's called pi_state.
>
> Now the question is how is this related to the non PI magic
> spinning optimization? Very much so.
>
> Simply because the handling of pi_state already covers ALL the
> corner cases which come with the extra kernel internal state and
> they provide the full function set of futexes required by the
> various (ab)use cases including the requeue functionality which is
> required for condvars. It even supports caching of the pi_state
> struct in a way which makes sense, rather than blindly slapping a
> cached entry onto each hash bucket.
>
> So it's not a really mind boggling thought experiment to think
> about this magic new feature as a subset of the already existing
> PI futex code. It is a subset, just minus the PI portion plus the
> more fine grained locking.
>
> So the right solution is to rework the existing code.
>
> 1) Make pi_state the entity which gets enqueued into the hash bucket
> and manage the waiters in a pi_state internal queue.
>
> Straight forward problem as it is still protected by the hash
> bucket lock.
>
> 2) Add private locking to the pi_state to reduce the lock contention
> on the hash bucket lock.
>
> Straight forward conversion as well
>
> 3) Make mutex a subset of rt_mutex
>
> That makes a lot of sense, simply because a mutex is the same as a
> rtmutex just without the PI part.
>
> Just for the record before anyone cries murder: The original mutex
> implemention was modeled after the rtmutex one in the first place
> and the optimistic spinning was first introduced on rtmutexes by
> Gregory Haskins et. al. in the context of the RT tree. Back then it
> was too early or we were simply too stupid to see the similarities
> and opted for a complete separate implementation.
>
> Yes, I'm aware that this is a non-trivial problem, but if you look
> at the details, then the non-trivial part is in the slow path were
> stuff actually goes to sleep. The fast path of mutex and rt_mutex
> is simply the same. So there is no reason to keep them separate. An
> extra conditional in the slow path is not going to hurt at all.
>
> Surely, you might say, that I'm an egoistic bastard, who just wants
> to have the benefit of MCS for rtmutex for free. Right you are, but
> there is a whole bunch of reasons why this makes tons of sense:
>
> - As I said above, it's almost the same, except for the slow path
> details, where a few extra conditionals do not matter
>
> - Sharing the code has the obvious benefits. And it's not only
> sharing between rtmutex and mutex, it's also sharing the
> underlying optimizations of MCS for the kernel internal users and
> the user space exposed ones, i.e. futexes.
>
> - Once unified the desired futex functionality just works with a
> minimal impact on the futex code itself.
>
> The futex code does not care whether the underlying primitive has
> PI semantics or not, it just cares that all the required features
> like proxy locking etc. are in place. And that everything like
> timeouts, requeue etc. is just supported.
>
> - The semantics of the new futex functionality has not to be
> defined again. We can simply reuse the rather well defined and
> well tested straight forward semantics of the existing PI
> opcodes.
>
> - That minimizes the risk massively, because all state and error
> handling is common code.
>
> - Any update to the underlying primitives will benefit ALL usage
> sites and reduces the copy and paste induced hell of MCS code
> sprinkled all over the kernel.
>
> 4) Enable user space to make use of it.
>
> Whether this will be a separate opcode or just a new flag to the
> existing PI opcodes is going to be just a bikeshedding question at
> that point. :)
>
> The important point is, that we do NOT grow any new unmaintainable
> warts to the futex code. And the 650+ new lines of hackery which come
> with the proposed patch set are in no way acceptable, especially as
> they only cover the minimalistic functionality set of futexes. Not to
> talk about the other issues I've observed on the first glance.
>
> So before anyone comes up with a "solution" for all of this tomorrow
> afternoon in form of another half baken patch, please sit back mull it
> in your head and lets have a proper discussion about the approach
> first.
>
> Thanks,
>
> tglx
Thank for your thorough analysis and suggestions on what to do to
support spinning futexes. You certainly know more about the internal
working of futex than most of us. I can live with what you have
suggested. My patch is just a proof of concept piece to demonstrate
optimistic spinning on futex is something worthwhile to do. I think I
have achieved my goal of stirring interest in this area. My next step
will be to look into the direction of what you have suggested and figure
out what actual code changes will be needed.
Thank again for your help.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Waiman Long @ 2014-07-22 19:36 UTC (permalink / raw)
To: Ingo Molnar
Cc: Thomas Gleixner, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
Heiko Carstens, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Jason Low, Scott J Norton
In-Reply-To: <20140721211801.GA12149-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 07/21/2014 05:18 PM, Ingo Molnar wrote:
> * Waiman Long<Waiman.Long-VXdhtT5mjnY@public.gmane.org> wrote:
>
>> Testing done on a 4-socket Westmere-EX boxes with 40 cores (HT off)
>> showed the following performance data (average kops/s) with various
>> load factor (number of pause instructions) used in the critical
>> section using an userspace mutex microbenchmark.
>>
>> Threads Load Waiting Futex Spinning Futex %Change
>> ------- ---- ------------- -------------- -------
>> 256 1 6894 8883 +29%
>> 256 10 3656 4912 +34%
>> 256 50 1332 4358 +227%
>> 256 100 792 2753 +248%
>> 10 1 6382 4838 -24%
>> 10 10 3614 4748 +31%
>> 10 50 1319 3900 +196%
>> 10 100 782 2459 +214%
>> 2 1 7905 7194 -9.0%
>> 2 10 4556 4717 +3.5%
>> 2 50 2191 4167 +90%
>> 2 100 1767 2407 +36%
> So the numbers look interesting - but it would be _really_ important
> to provide noise/sttdev figures in a sixth column as well (denoted in
> percentage units, not in benchmark units), so that we know how
> significant a particular speedup (or slowdown) is.
>
> Thanks,
>
> Ingo
The performance can varies quite a bit depending on what other processes
are running at the test execution time. I will include stddev data in
the next iteration of the patch.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 2/5] futex: add optimistic spinning to FUTEX_SPIN_LOCK
From: Waiman Long @ 2014-07-22 19:34 UTC (permalink / raw)
To: Jason Low
Cc: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Heiko Carstens, linux-kernel, linux-api,
linux-doc, Scott J Norton
In-Reply-To: <1405973838.4677.11.camel@j-VirtualBox>
On 07/21/2014 04:17 PM, Jason Low wrote:
> On Mon, 2014-07-21 at 11:24 -0400, Waiman Long wrote:
>> This patch adds code to do optimistic spinning for the FUTEX_SPIN_LOCK
>> primitive on the futex value when the lock owner is running. It is
>> the same optimistic spinning technique that is done in the mutex and
>> rw semaphore code to improve their performance especially on large
>> systems with large number of CPUs. When the lock owner is not running,
>> the spinning tasks will go to sleep.
> Perhaps we could introduce a "CONFIG_FUTEX_SPIN_ON_OWNER" that depends
> on SMP and ARCH_SUPPORTS_ATOMIC_RMW?
The new futex opcode depends on the ability to do cmpxchg() in the futex
context. The code will be disabled if futex cmpxchg is not supported. I
guess that should be enough to limit it to just a handful of architectures.
>> There is 2 major advantages of doing optimistic spinning here:
>> 1) It eliminates the context switching latency and overhead (at
>> least a few us) associated with sleeping and wakeup.
>> 2) It eliminates most of the need to call futex(2) with
>> FUTEX_SPIN_UNLOCK as spinning is done without the need to set
>> the FUTEX_WAITERS bit.
>> struct futex_q_head {
>> struct list_head hnode;
>> struct list_head waitq;
>> union futex_key key;
>> + struct optimistic_spin_queue *osq;
> And this would have to be updated to
>
> + struct optimistic_spin_queue osq;
>
> given the recent changes to the osq lock.
Yes, I will make the change in the next iteration of the patch.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 2/5] futex: add optimistic spinning to FUTEX_SPIN_LOCK
From: Waiman Long @ 2014-07-22 18:46 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Heiko Carstens, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Jason Low, Scott J Norton
In-Reply-To: <1405962929.11927.19.camel-5JQ4ckphU/8SZAcGdq5asR6epYMZPwEe5NbjCUgZEJk@public.gmane.org>
On 07/21/2014 01:15 PM, Davidlohr Bueso wrote:
> On Mon, 2014-07-21 at 11:24 -0400, Waiman Long wrote:
>> This patch adds code to do optimistic spinning for the FUTEX_SPIN_LOCK
>> primitive on the futex value when the lock owner is running. It is
>> the same optimistic spinning technique that is done in the mutex and
>> rw semaphore code to improve their performance especially on large
>> systems with large number of CPUs. When the lock owner is not running,
>> the spinning tasks will go to sleep.
>>
>> There is 2 major advantages of doing optimistic spinning here:
>> 1) It eliminates the context switching latency and overhead (at
>> least a few us) associated with sleeping and wakeup.
>> 2) It eliminates most of the need to call futex(2) with
>> FUTEX_SPIN_UNLOCK as spinning is done without the need to set
>> the FUTEX_WAITERS bit.
> I think this belongs with Patch 1: optimistic spinning feature should be
> in the same patch when you add the new futex commands.
I broke the spinning code out in patch 2 in order to make patch 1
smaller and easier to review.
>> Active spinning, however, does consume time in the current quantum of
>> time slice, make it a bit more likely to be preempted while running
>> in the critcal section due to time slice expiration. The heavy spinlock
>> contention of a wait-wake futex has the same effect. So it is not
>> specific
>> to this new primitive.
>>
>> With the addition of optimistic spinning, it can significantly speed
>> up the amount of mutex operations that can be done in a certain unit
>> of time. With a userspace mutex microbenchmark running 10 million
>> mutex operations with 256 threads on a 4-socket 40-core server, the
>> spinning futex can achieve a rate of about 4.9 Mops/s with a critical
>> section load of 10 pause instructions. Whereas the wait-wake futex can
>> only achieve a rate of 3.7 Mops/s. When increasing the load to 100,
>> the corresponding rates become 2.8 Mops/s and 0.8 Mops/s respectively.
>>
>> Signed-off-by: Waiman Long<Waiman.Long-VXdhtT5mjnY@public.gmane.org>
>> ---
>> kernel/futex.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>> 1 files changed, 172 insertions(+), 18 deletions(-)
>>
>> diff --git a/kernel/futex.c b/kernel/futex.c
>> index ec9b6ee..ddc24d1 100644
>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -71,6 +71,7 @@
>> #include<asm/futex.h>
>>
>> #include "locking/rtmutex_common.h"
>> +#include "locking/mcs_spinlock.h"
>>
>> /*
>> * READ this before attempting to hack on futexes!
>> @@ -2995,30 +2996,51 @@ void exit_robust_list(struct task_struct *curr)
>> #define FUTEX_TID(u) (pid_t)((u)& FUTEX_TID_MASK)
>> #define FUTEX_HAS_WAITERS(u) ((u)& FUTEX_WAITERS)
>>
>> +/*
>> + * Bit usage of the locker count:
>> + * bit 0-23: number of lockers (spinners + waiters)
>> + * bit 24-30: number of spinners
>> + */
>> +#define FUTEX_SPINCNT_MAX 64 /* Maximum # of spinners */
>> +#define FUTEX_SPINCNT_SHIFT 24
>> +#define FUTEX_SPINCNT_BIAS (1U<< FUTEX_SPINCNT_SHIFT)
>> +#define FUTEX_LOCKCNT_MASK (FUTEX_SPINCNT_BIAS - 1)
>> +#define FUTEX_LOCKCNT(qh) (atomic_read(&(qh)->lcnt)& FUTEX_LOCKCNT_MASK)
>> +#define FUTEX_SPINCNT(qh) (atomic_read(&(qh)->lcnt)>>FUTEX_SPINCNT_SHIFT)
> Both FUTEX_LOCKCNT and FUTEX_SPINCNT should be static inline functions.
I will change them into static inline functions.
>
>> /**
>> * struct futex_q_head - head of the optspin futex queue, one per unique key
>> * @hnode: list entry from the hash bucket
>> * @waitq: a list of waiting tasks
>> * @key: the key the futex is hashed on
>> + * @osq: pointer to optimisitic spinning queue
>> + * @owner: task_struct pointer of the futex owner
>> + * @otid: TID of the futex owner
>> * @wlock: spinlock for managing wait queue
>> - * @lcnt: locker count
>> + * @lcnt: locker count (spinners + waiters)
>> *
>> * Locking sequence
>> * ----------------
>> * 1) Lock hash bucket spinlock, locate the futex queue head
>> * 2) Inc lcnt (lock) or read lcnt (unlock), release hash bucket spinlock
>> - * 3) For waiter:
>> + * 3) For spinner:
>> + * - enqueue into the spinner queue and wait for its turn.
>> + * 4) For waiter:
>> * - lock futex queue head spinlock
>> * - enqueue into the wait queue
>> * - release the lock& sleep
>> - * 4) For unlocker:
>> + * 5) For unlocker:
>> * - locate the queue head just like a locker does
>> - * - Take the queue head lock and wake up the first waiter there.
>> + * - clear the owner field if it is the current owner
>> + * - if the locker count is not 0& osq is empty, take the queue head lock
>> + * and wake up the first waiter.
>> */
>> struct futex_q_head {
>> struct list_head hnode;
>> struct list_head waitq;
>> union futex_key key;
>> + struct optimistic_spin_queue *osq;
>> + struct task_struct *owner;
>> pid_t otid;
>> spinlock_t wlock;
>> atomic_t lcnt;
>> @@ -3034,6 +3056,13 @@ struct futex_q_node {
>> struct task_struct *task;
>> };
>>
>> +/*
>> + * The maximum number of tasks that can be a futex spin queue
>> + *
>> + * It is set to the lesser of half of the total number of CPUs and
>> + * FUTEX_SPINCNT_MAX to avoid locking up all the CPUs in spinning.
>> + */
>> +static int __read_mostly futex_spincnt_max;
>>
>> /*
>> * find_qhead - Find a queue head structure with the matching key
>> @@ -3061,7 +3090,7 @@ find_qhead(struct futex_hash_bucket *hb, union futex_key *key)
>> * contention with no hash bucket collision.
>> */
>> static inline struct futex_q_head *
>> -qhead_alloc_init(struct futex_hash_bucket *hb, union futex_key *key)
>> +qhead_alloc_init(struct futex_hash_bucket *hb, union futex_key *key, u32 uval)
>> {
>> struct futex_q_head *qh = NULL;
>> static const struct futex_q_head qh0 = { { 0 } };
>> @@ -3073,10 +3102,16 @@ qhead_alloc_init(struct futex_hash_bucket *hb, union futex_key *key)
>>
>> /*
>> * Initialize the queue head structure
>> + * The lock owner field may be NULL if the task has released the lock
>> + * and exit.
>> */
>> if (qh) {
>> - *qh = qh0;
>> - qh->key = *key;
>> + *qh = qh0;
>> + qh->key = *key;
>> + qh->otid = FUTEX_TID(uval);
>> + qh->owner = futex_find_get_task(qh->otid);
>> + if (unlikely(!qh->owner))
>> + qh->otid = 0;
>> atomic_set(&qh->lcnt, 1);
>> INIT_LIST_HEAD(&qh->waitq);
>> spin_lock_init(&qh->wlock);
> All this can be a single qh setup function.
This code is already in a separate allocation and initialization
function. I don't see a big advantage in further breaking them up into 2
unless there are cases where each can be called independently without
the other.
>> @@ -3120,9 +3155,11 @@ qhead_free(struct futex_q_head *qh, struct futex_hash_bucket *hb)
>> /*
>> * Free the queue head structure
>> */
>> - BUG_ON(!list_empty(&qh->waitq));
>> + BUG_ON(!list_empty(&qh->waitq) || qh->osq);
>> list_del(&qh->hnode);
>> spin_unlock(&hb->lock);
>> + if (qh->owner)
>> + put_task_struct(qh->owner);
>>
>> if (!hb->qhcache&& (cmpxchg(&hb->qhcache, NULL, qh) == NULL))
>> return;
>> @@ -3134,14 +3171,19 @@ qhead_free(struct futex_q_head *qh, struct futex_hash_bucket *hb)
>> * Return: 1 if successful or an error happen
>> * 0 otherwise
>> *
>> + * Optimistic spinning is done without holding lock, but with page fault
>> + * explicitly disabled. So different functions need to be used to access
>> + * the userspace futex value.
>> + *
>> * Side effect: The uval and ret will be updated.
>> */
>> static inline int futex_spin_trylock(u32 __user *uaddr, u32 *puval,
>> - int *pret, u32 vpid)
>> + int *pret, u32 vpid, bool spinning)
>> {
>> - u32 old;
>> + u32 old;
>>
>> - *pret = get_futex_value_locked(puval, uaddr);
>> + *pret = spinning ? __copy_from_user_inatomic(puval, uaddr, sizeof(u32))
>> + : get_futex_value_locked(puval, uaddr);
>> if (*pret)
>> return 1;
>>
>> @@ -3150,18 +3192,102 @@ static inline int futex_spin_trylock(u32 __user *uaddr, u32 *puval,
>>
>> old = *puval;
>>
>> - *pret = cmpxchg_futex_value_locked(puval, uaddr, old, vpid | old);
>> + *pret = spinning
>> + ? futex_atomic_cmpxchg_inatomic(puval, uaddr, old, vpid)
>> + : cmpxchg_futex_value_locked(puval, uaddr, old, vpid | old);
>> +
>> if (*pret)
>> return 1;
>> if (*puval == old) {
>> /* Adjust uval to reflect current value */
>> - *puval = vpid | old;
>> + *puval = spinning ? vpid : (vpid | old);
>> return 1;
>> }
>> return 0;
>> }
>>
>> /*
>> + * futex_optspin - optimistic spinning loop
>> + * Return: 1 if lock successfully acquired
>> + * 0 if need to fall back to waiting
>> + *
>> + * Page fault and preemption are disabled in the optimistic spinning
>> + * loop. Preemption should have been disabled before calling this function.
>> + *
>> + * The number of spinners may temporarily exceed the threshold due to
>> + * racing (the spin count check and add aren't atomic), but that shouldn't
>> + * be a big problem.
>> + */
>> +static inline int futex_optspin(struct futex_q_head *qh,
>> + struct futex_q_node *qn,
>> + u32 __user *uaddr,
>> + u32 vpid)
>> +{
>> + u32 uval;
>> + int ret, gotlock = false;
>> + struct task_struct *owner;
>> +
>> + /*
>> + * Increment the spinner count
>> + */
>> + atomic_add(FUTEX_SPINCNT_BIAS,&qh->lcnt);
>> + if (!osq_lock(&qh->osq)) {
>> + atomic_sub(FUTEX_SPINCNT_BIAS,&qh->lcnt);
>> + return gotlock;
>> + }
>> + pagefault_disable();
> How about a comment to why pf is disabled.
When page fault happens, there is a chance that the task can be switched
to a different CPU so all the OSQ magic fails to work even with
preempt_disable(). This is a bug that caused me a day or so to figure
out. I will add a comment to document that.
>> + for (;; cpu_relax()) {
> while(true) {
>
>> + if (futex_spin_trylock(uaddr,&uval,&ret, vpid, true)) {
>> + /*
>> + * Fall back to waiting if an error happen
>> + */
>> + if (ret)
>> + break;
>> + qh->otid = vpid;
>> + owner = xchg(&qh->owner, qn->task);
>> + get_task_struct(qn->task);
>> + if (owner)
>> + put_task_struct(owner);
>> + gotlock = true;
>> + break;
>> + } else if (unlikely(FUTEX_HAS_WAITERS(uval))) {
> Branch predictions have a time and place, please do not use
> likely/unlikely just for anything.
Sure. I may have overused them.
>
>> + /*
>> + * Try to turn off the waiter bit as it now has a
>> + * spinner. It doesn't matter if it fails as it will
>> + * try again in the next iteration.
>> + */
>> + (void)futex_atomic_cmpxchg_inatomic
>> + (&uval, uaddr, uval, uval& ~FUTEX_WAITERS);
>> + }
>> +
>> + if (unlikely(FUTEX_TID(uval) != qh->otid)) {
>> + /*
>> + * Owner has changed
>> + */
>> + qh->otid = FUTEX_TID(uval);
>> + owner = xchg(&qh->owner, futex_find_get_task(qh->otid));
>> + if (owner)
>> + put_task_struct(owner);
>> + }
>> + owner = ACCESS_ONCE(qh->owner);
>> + if ((owner&& !ACCESS_ONCE(owner->on_cpu)) || need_resched())
>> + break;
>> + }
>> + pagefault_enable();
>> + osq_unlock(&qh->osq);
>> + atomic_sub(FUTEX_SPINCNT_BIAS,&qh->lcnt);
>> +
>> + /*
>> + * If we fell out of the spin path because of need_resched(),
>> + * reschedule now.
>> + */
>> + if (!gotlock&& need_resched())
>> + schedule_preempt_disabled();
>> +
>> + return gotlock;
>> +}
>> +
>> +/*
>> * futex_spin_lock
>> */
>> static noinline int futex_spin_lock(u32 __user *uaddr, unsigned int flags)
>> @@ -3170,6 +3296,7 @@ static noinline int futex_spin_lock(u32 __user *uaddr, unsigned int flags)
>> struct futex_q_head *qh = NULL;
>> struct futex_q_node qnode;
>> union futex_key key;
>> + struct task_struct *owner;
>> bool gotlock;
>> int ret, cnt;
>> u32 uval, vpid, old;
>> @@ -3193,7 +3320,7 @@ static noinline int futex_spin_lock(u32 __user *uaddr, unsigned int flags)
>> * Check the futex value under the hash bucket lock as it might
>> * be changed.
>> */
>> - if (futex_spin_trylock(uaddr,&uval,&ret, vpid))
>> + if (futex_spin_trylock(uaddr,&uval,&ret, vpid, false))
>> goto hbunlock_out;
>>
>> if (!qh) {
>> @@ -3201,7 +3328,7 @@ static noinline int futex_spin_lock(u32 __user *uaddr, unsigned int flags)
>> * First waiter:
>> * Allocate a queue head structure& initialize it
>> */
>> - qh = qhead_alloc_init(hb,&key);
>> + qh = qhead_alloc_init(hb,&key, uval);
>> if (unlikely(!qh)) {
>> ret = -ENOMEM;
>> goto hbunlock_out;
>> @@ -3212,9 +3339,18 @@ static noinline int futex_spin_lock(u32 __user *uaddr, unsigned int flags)
>> spin_unlock(&hb->lock);
>>
>> /*
>> - * Put the task into the wait queue and sleep
>> + * Perform optimisitic spinning if the owner is running.
>> */
>> preempt_disable();
>> + owner = ACCESS_ONCE(qh->owner);
>> + if ((FUTEX_SPINCNT(qh)< futex_spincnt_max)&&
>> + (!owner || owner->on_cpu))
>> + if (futex_optspin(qh,&qnode, uaddr, vpid))
>> + goto penable_out;
>> +
>> + /*
>> + * Put the task into the wait queue and sleep
>> + */
>> get_task_struct(qnode.task);
>> spin_lock(&qh->wlock);
>> list_add_tail(&qnode.wnode,&qh->waitq);
>> @@ -3238,6 +3374,11 @@ static noinline int futex_spin_lock(u32 __user *uaddr, unsigned int flags)
>> goto dequeue;
>> } else if (uval == old) {
>> gotlock = true;
>> + qh->otid = vpid;
>> + owner = xchg(&qh->owner, qnode.task);
>> + get_task_struct(qnode.task);
>> + if (owner)
>> + put_task_struct(owner);
>> goto dequeue;
>> }
>> continue;
>> @@ -3286,15 +3427,17 @@ dequeue:
>> }
>> }
>> spin_unlock(&qh->wlock);
>> +
>> +penable_out:
>> preempt_enable();
>>
>> cnt = atomic_dec_return(&qh->lcnt);
>> if (cnt == 0)
>> qhead_free(qh, hb);
>> /*
>> - * Need to set the waiters bit there are still waiters
>> + * Need to set the waiters bit there no spinner running
>> */
>> - else if (!ret)
>> + else if (!ret&& ((cnt>> FUTEX_SPINCNT_SHIFT) == 0))
>> ret = put_user(vpid | FUTEX_WAITERS, uaddr);
>> out:
>> put_futex_key(&key);
>> @@ -3356,6 +3499,13 @@ static noinline int futex_spin_unlock(u32 __user *uaddr, unsigned int flags)
>> }
>>
>> /*
>> + * Clear the owner field
>> + */
>> + if ((qh->owner == current)&&
>> + (cmpxchg(&qh->owner, current, NULL) == current))
>> + put_task_struct(current);
>> +
>> + /*
>> * The hash bucket lock is being hold while retrieving the task
>> * structure from the queue head to make sure that the qh structure
>> * won't go away under the hood.
>> @@ -3520,6 +3670,10 @@ static int __init futex_init(void)
>>
>> futex_detect_cmpxchg();
>>
>> + futex_spincnt_max = num_possible_cpus()/2;
>> + if (futex_spincnt_max> FUTEX_SPINCNT_MAX)
>> + futex_spincnt_max = FUTEX_SPINCNT_MAX;
> This threshold needs commenting as well.
>
There is comment up where FUTEX_SPINCNT_MAX is defined. Will add a
comment here as well.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Waiman Long @ 2014-07-22 18:35 UTC (permalink / raw)
To: Andi Kleen
Cc: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Heiko Carstens, linux-kernel, linux-api,
linux-doc, Jason Low, Scott J Norton
In-Reply-To: <871tte3bjw.fsf@tassilo.jf.intel.com>
On 07/21/2014 12:45 PM, Andi Kleen wrote:
> Andi Kleen<andi@firstfloor.org> writes:
>
>> Waiman Long<Waiman.Long@hp.com> writes:
>>
>>> This patch series introduces two new futex command codes to support
>>> a new optimistic spinning futex for implementing an exclusive lock
>>> primitive that should perform better than the same primitive using
>>> the wait-wake futex in cases where the lock owner is actively working
>>> instead of waiting for I/O completion.
>> How would you distinguish those two cases automatically?
> Also BTW traditionally the spinning was just done in user space.
>
> This would be always even more efficient, because it would
> even avoid the syscall entry path.
Spinning directly in userspace is useful if the critical section is
really short and the chance of heavy contention is small. However, if
the lock can be heavily contended and the critical section is not short,
these are the conditions where a spinning futex proposed by this patch
series can help.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Waiman Long @ 2014-07-22 18:28 UTC (permalink / raw)
To: Andi Kleen
Cc: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Heiko Carstens, linux-kernel, linux-api,
linux-doc, Jason Low, Scott J Norton
In-Reply-To: <8761iq3bp3.fsf@tassilo.jf.intel.com>
On 07/21/2014 12:42 PM, Andi Kleen wrote:
> Waiman Long<Waiman.Long@hp.com> writes:
>
>> This patch series introduces two new futex command codes to support
>> a new optimistic spinning futex for implementing an exclusive lock
>> primitive that should perform better than the same primitive using
>> the wait-wake futex in cases where the lock owner is actively working
>> instead of waiting for I/O completion.
> How would you distinguish those two cases automatically?
I don't really distinguish between these two. The purpose of this
paragraph is to show the best use cases for the spinning futexes is when
the lock owner is not likely to sleep while in the critical section.
>> This patch series improves futex performance on two different fronts:
>> 1) Reducing the amount of the futex spinlock contention by using 2
>> different spinlocks instead of just one for the wait-wake futex.
>> 2) Eliminating the context switching overhead and latency due to the
>> sleeping and the waking of the waiting tasks.
> FWIW the main problem is currently that switch-through-idle is so
> slow. I think improving that would give a boost to far more
> situations.
If we can improve the context switching overhead, that can certainly
help a lot of applications.
>
>> Patch 4 changes sleeping queue from a simple FIFO list to an rbtree
>> sorted by process priority as well as the sequeunce the tasks enter
>> the kernel.
> This seems to mix new functionality with performance improvements?
>
> Generally adding new ordering in an user interface is risky because
> often user programs have been tuned for specific old ordering.
Not really, this patch is to emulate the current behavior of the
wait-wake futex which is queued in a priority queue. Because of
spinning, tasks may enter into sleep state in different order than when
they enter into the kernel. That is why I keep a sequence number to
track who go into the kernel first.
-Longman
^ permalink raw reply
* Re: [RFC PATCH 1/5] futex: add new exclusive lock & unlock command codes
From: Waiman Long @ 2014-07-22 18:22 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Peter Zijlstra, Darren Hart, Davidlohr Bueso,
Heiko Carstens, LKML, linux-api, linux-doc, Jason Low,
Scott J Norton
In-Reply-To: <alpine.DEB.2.10.1407211811360.20847@nanos>
On 07/21/2014 12:42 PM, Thomas Gleixner wrote:
> On Mon, 21 Jul 2014, Waiman Long wrote:
>
>> +#define FUTEX_TID(u) (pid_t)((u)& FUTEX_TID_MASK)
>> +#define FUTEX_HAS_WAITERS(u) ((u)& FUTEX_WAITERS)
> You love ugly macros, right?
>
Not really, I just have a tendency to overuse it sometimes. I could take
those macros out.
>> +/*
>> + * futex_spin_trylock - attempt to take the lock
>> + * Return: 1 if successful or an error happen
>> + * 0 otherwise
>> + *
>> + * Side effect: The uval and ret will be updated.
>> + */
>> +static inline int futex_spin_trylock(u32 __user *uaddr, u32 *puval,
>> + int *pret, u32 vpid)
>> +{
>> + u32 old;
>> +
>> + *pret = get_futex_value_locked(puval, uaddr);
>> + if (*pret)
>> + return 1;
>> +
>> + if (FUTEX_TID(*puval))
>> + return 0; /* The mutex is not free */
>> +
>> + old = *puval;
>> +
>> + *pret = cmpxchg_futex_value_locked(puval, uaddr, old, vpid | old);
>> + if (*pret)
>> + return 1;
>> + if (*puval == old) {
>> + /* Adjust uval to reflect current value */
>> + *puval = vpid | old;
>> + return 1;
>> + }
>> + return 0;
> What's the point if all of this?
>
> A simple cmpxchg_futex_value_locked() does all of this, just less ugly
> and without all these extra indirections and totally uncomprehensible
> conditionals.
>
Yes, the trylock function is somewhat unwieldy. Will integrate it back
to the corresponding place. As a trylock, we usually do a read first to
make sure that it is ready before doing cmpxchg. Blindly doing a cmpxhg
unconditionally may hinder performance.
>> +}
>> +
>> +/*
>> + * futex_spin_lock
>> + */
>> +static noinline int futex_spin_lock(u32 __user *uaddr, unsigned int flags)
>> +{
> So this lacks a timeout. If we provide this, then we need to have the
> timeout supported as well.
Yes, a timeout isn't supported yet. This is a RFC and I want to get a
sense of how important a timeout will be before I add it in. I could
certainly add that in if people think it is an important feature to have.
>> + struct futex_hash_bucket *hb;
>> + struct futex_q_head *qh = NULL;
>> + struct futex_q_node qnode;
>> + union futex_key key;
>> + bool gotlock;
>> + int ret, cnt;
>> + u32 uval, vpid, old;
>> +
>> + qnode.task = current;
>> + vpid = task_pid_vnr(qnode.task);
>> +
>> + ret = get_futex_key(uaddr, flags& FLAGS_SHARED,&key, VERIFY_WRITE);
>> + if (unlikely(ret))
> Stop sprinkling the code with unlikelys
Sure. Will remove those unlikely() calls.
>> + return ret;
>> +
>> + hb = hash_futex(&key);
>> + spin_lock(&hb->lock);
>> +
>> + /*
>> + * Locate the queue head for the given key
>> + */
> Brilliant comment. If you'd comment the stuff which really matters and
> leave out the obvious, then your code might be readable some day.
That comment was before I extracted the code out into a separate
function. Will remove it.
>> + qh = find_qhead(hb,&key);
>> +
>> + /*
>> + * Check the futex value under the hash bucket lock as it might
>> + * be changed.
>> + */
> What might have changed? You enter the function with uaddr, but no
> uval. So what changed?
If there is contention, the spin_lock() call may take a while. Unlike a
wait-wake futex, the only uval that will be of interest is when the TID
portion is 0. So we don't really need to pass in an uval. The uval is
not 0 when the lock function is called. However, the lock owner may have
released the lock by the time we check the futex value there before we
go into spinning or waiting.
>
>
>> + if (futex_spin_trylock(uaddr,&uval,&ret, vpid))
>> + goto hbunlock_out;
>> +
>> + if (!qh) {
>> + /*
>> + * First waiter:
>> + * Allocate a queue head structure& initialize it
>> + */
>> + qh = qhead_alloc_init(hb,&key);
>> + if (unlikely(!qh)) {
>> + ret = -ENOMEM;
>> + goto hbunlock_out;
>> + }
>> + } else {
>> + atomic_inc(&qh->lcnt);
>> + }
>> + spin_unlock(&hb->lock);
>> +
>> + /*
>> + * Put the task into the wait queue and sleep
>> + */
>> + preempt_disable();
> Why?
I just follow what has been done in the mutex code where preemption is
disabled even in the sleeping loop.
>
>> + get_task_struct(qnode.task);
> So you get a task reference on current? What the heck is this for?
Because the task is going to sleep and a queue node with the task
pointer is going to be enqueued into the wait queue.
>> + spin_lock(&qh->wlock);
>> + list_add_tail(&qnode.wnode,&qh->waitq);
>> + __set_current_state(TASK_INTERRUPTIBLE);
>> + spin_unlock(&qh->wlock);
>> + gotlock = false;
>> + for (;;) {
>> + ret = get_user(uval, uaddr);
>> + if (ret)
>> + break;
> So you let user space handle EFAULT?
This is a good question. Do you have any suggestion on how to better
handle error when get_user fails?
>
>> +dequeue:
>> + __set_current_state(TASK_RUNNING);
>> + /*
>> + * Remove itself from the wait queue and go back to optimistic
>> + * spinning if it hasn't got the lock yet.
>> + */
>> + put_task_struct(qnode.task);
>> + spin_lock(&qh->wlock);
>> + list_del(&qnode.wnode);
>> +
>> + /*
>> + * Try to clear the waiter bit if the wait queue is empty
>> + */
>> + if (list_empty(&qh->waitq)) {
>> + int retval = get_futex_value_locked(&uval, uaddr);
>> +
>> + if (!retval&& FUTEX_HAS_WAITERS(uval)) {
>> + old = uval;
>> + uval&= ~FUTEX_WAITERS;
>> + (void)cmpxchg_futex_value_locked(&uval, uaddr, old,
>> + uval);
>> + }
>> + }
>> + spin_unlock(&qh->wlock);
>> + preempt_enable();
>> +
>> + cnt = atomic_dec_return(&qh->lcnt);
>> + if (cnt == 0)
>> + qhead_free(qh, hb);
>> + /*
>> + * Need to set the waiters bit there are still waiters
>> + */
>> + else if (!ret)
>> + ret = put_user(vpid | FUTEX_WAITERS, uaddr);
> WTF? You fiddle with the uaddr completely unprotected.
The get_futex_key(...., VERIFY_WRITE) call has check to make sure that
the location is writeable and get_user() call has happened without
error. What additional protection do you think we need here?
>> +out:
>> + put_futex_key(&key);
>> + return ret;
>> +
>> +hbunlock_out:
>> + spin_unlock(&hb->lock);
>> + goto out;
>> +}
>> +
>> +/*
>> + * futex_spin_unlock
>> + */
>> +static noinline int futex_spin_unlock(u32 __user *uaddr, unsigned int flags)
>> +{
>> + struct futex_hash_bucket *hb;
>> + struct futex_q_head *qh;
>> + union futex_key key;
>> + struct task_struct *wtask; /* Task to be woken */
>> + int ret, lcnt;
>> + u32 uval, old, vpid = task_pid_vnr(current);
>> +
>> + ret = get_user(uval, uaddr);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * The unlocker may have cleared the TID value and another task may
>> + * steal it. However, if its TID is still set, we need to clear
>> + * it as well as the FUTEX_WAITERS bit.
> No, that's complete and utter crap. The unlocker is current and it may
> not have cleared anything.
>
> Your design or the lack thereof is a complete disaster.
In patch 5, the documentation and the sample unlock does clear the TID
before going in. The code here is just a safety measure in case the
unlocker doesn't follow the recommendation.
> Sit down first and define the exact semantics of the new opcode. That
> includes user and kernel space and the interaction with robust list,
> which you happily ignored.
>
> What are the semantics of uval? When can it be changed in kernel and
> in user space? How do we deal with corruption of the user space value?
The semantics of the uval is the same as that of PI and robust futex
where the TID portion contains the thread ID of the lock owner. It is my
intention to make it works with the robust futex mechanism before it can
be merged. This RPC patch series is for soliciting feedbacks and make
the necessary changes that make the patch acceptable before I go deep
into making it works with robust futex.
>
> How does that new opcode provide robustness?
>
> How are faults handled?
As you have a lot more experience working with futexes than me, any
suggestions on what kind of faults will happen and what are the best
practices to handle them will be highly appreciated.
-Longman
^ permalink raw reply
* Re: [PATCH -v4] random: introduce getrandom(2) system call
From: Rolf Eike Beer @ 2014-07-22 14:36 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: linux-kernel, linux-api, linux-crypto, beck, deraadt
In-Reply-To: <1405718127-30042-1-git-send-email-tytso@mit.edu>
> EAGAIN The requested entropy was not available, and the
> getentropy(2) would have blocked if GRND_BLOCK flag
> was set.
I think either "and the call to getentropy(2)" or "and getentropy(2)" here.
Greetings,
Eike
^ permalink raw reply
* [PATCH 5/5] drm: panel: simple-panel: add bus format information for foxlink panel
From: Boris BREZILLON @ 2014-07-22 12:23 UTC (permalink / raw)
To: Thierry Reding, Laurent Pinchart
Cc: linux-api, linux-kernel, dri-devel, Mauro Carvalho Chehab,
linux-media
In-Reply-To: <1406031827-12432-1-git-send-email-boris.brezillon@free-electrons.com>
Foxlink's fl500wvr00-a0t supports RGB888 format.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
drivers/gpu/drm/panel/panel-simple.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 42fd6d1..f1e49fd 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -428,6 +428,7 @@ static const struct panel_desc foxlink_fl500wvr00_a0t = {
.width = 108,
.height = 65,
},
+ .bus_format = VIDEO_BUS_FMT_RGB888_1X24,
};
static const struct drm_display_mode lg_lp129qe_mode = {
--
1.8.3.2
^ permalink raw reply related
* [PATCH 4/5] drm: panel: simple-panel: add support for bus_format retrieval
From: Boris BREZILLON @ 2014-07-22 12:23 UTC (permalink / raw)
To: Thierry Reding, Laurent Pinchart
Cc: linux-api, linux-kernel, dri-devel, Mauro Carvalho Chehab,
linux-media
In-Reply-To: <1406031827-12432-1-git-send-email-boris.brezillon@free-electrons.com>
Provide a way to specify panel requirement in terms of supported media bus
format (particularly useful for panels connected to an RGB or LVDS bus).
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
drivers/gpu/drm/panel/panel-simple.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 3f76944..42fd6d1 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -41,6 +41,8 @@ struct panel_desc {
unsigned int width;
unsigned int height;
} size;
+
+ enum video_bus_format bus_format;
};
struct panel_simple {
@@ -89,6 +91,9 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
connector->display_info.width_mm = panel->desc->size.width;
connector->display_info.height_mm = panel->desc->size.height;
+ if (panel->desc->bus_format)
+ drm_display_info_set_bus_formats(&connector->display_info,
+ &panel->desc->bus_format, 1);
return num;
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH 3/5] drm: add bus_formats and nbus_formats fields to drm_display_info
From: Boris BREZILLON @ 2014-07-22 12:23 UTC (permalink / raw)
To: Thierry Reding, Laurent Pinchart
Cc: linux-api, linux-kernel, dri-devel, Mauro Carvalho Chehab,
linux-media
In-Reply-To: <1406031827-12432-1-git-send-email-boris.brezillon@free-electrons.com>
Add bus_formats and nbus_formats fields and
drm_display_info_set_bus_formats helper function to specify the bus
formats supported by a given display.
This information can be used by display controller drivers to configure
the output interface appropriately (i.e. RGB565, RGB666 or RGB888 on raw
RGB or LVDS busses).
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
drivers/gpu/drm/drm_crtc.c | 28 ++++++++++++++++++++++++++++
include/drm/drm_crtc.h | 8 ++++++++
2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c808a09..50c8395 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -825,6 +825,34 @@ static void drm_mode_remove(struct drm_connector *connector,
drm_mode_destroy(connector->dev, mode);
}
+/*
+ * drm_display_info_set_bus_formats - set the supported bus formats
+ * @info: display info to store bus formats in
+ * @fmts: array containing the supported bus formats
+ * @nfmts: the number of entries in the fmts array
+ *
+ * Store the suppported bus formats in display info structure.
+ */
+int drm_display_info_set_bus_formats(struct drm_display_info *info,
+ const enum video_bus_format *fmts,
+ int nfmts)
+{
+ enum video_bus_format *formats = NULL;
+
+ if (fmts && nfmts) {
+ formats = kmemdup(fmts, sizeof(*fmts) * nfmts, GFP_KERNEL);
+ if (!formats)
+ return -ENOMEM;
+ }
+
+ kfree(info->bus_formats);
+ info->bus_formats = formats;
+ info->nbus_formats = formats ? nfmts : 0;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_display_info_set_bus_formats);
+
/**
* drm_connector_init - Init a preallocated connector
* @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e529b68..957729b 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -31,6 +31,7 @@
#include <linux/idr.h>
#include <linux/fb.h>
#include <linux/hdmi.h>
+#include <linux/video-bus-format.h>
#include <drm/drm_mode.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_modeset_lock.h>
@@ -121,6 +122,9 @@ struct drm_display_info {
enum subpixel_order subpixel_order;
u32 color_formats;
+ const enum video_bus_format *bus_formats;
+ int nbus_formats;
+
/* Mask of supported hdmi deep color modes */
u8 edid_hdmi_dc_modes;
@@ -964,6 +968,10 @@ extern int drm_mode_connector_set_path_property(struct drm_connector *connector,
extern int drm_mode_connector_update_edid_property(struct drm_connector *connector,
struct edid *edid);
+extern int drm_display_info_set_bus_formats(struct drm_display_info *info,
+ const enum video_bus_format *fmts,
+ int nfmts);
+
static inline bool drm_property_type_is(struct drm_property *property,
uint32_t type)
{
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/5] video: add RGB444_1X12 and RGB565_1X16 bus formats
From: Boris BREZILLON @ 2014-07-22 12:23 UTC (permalink / raw)
To: Thierry Reding, Laurent Pinchart
Cc: linux-api, linux-kernel, dri-devel, Mauro Carvalho Chehab,
linux-media
In-Reply-To: <1406031827-12432-1-git-send-email-boris.brezillon@free-electrons.com>
Add RGB444 format using a 12 bits bus and RGB565 using a 16 bits bus.
These formats will later be used by atmel-hlcdc driver.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
include/uapi/linux/v4l2-mediabus.h | 2 ++
include/uapi/linux/video-bus-format.h | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
index 8c31f11..319f860 100644
--- a/include/uapi/linux/v4l2-mediabus.h
+++ b/include/uapi/linux/v4l2-mediabus.h
@@ -30,6 +30,8 @@
#define V4L2_MBUS_FMT_RGB888_2X12_BE VIDEO_BUS_FMT_RGB888_2X12_BE
#define V4L2_MBUS_FMT_RGB888_2X12_LE VIDEO_BUS_FMT_RGB888_2X12_LE
#define V4L2_MBUS_FMT_ARGB8888_1X32 VIDEO_BUS_FMT_ARGB8888_1X32
+#define V4L2_BUS_FMT_RGB444_1X12 VIDEO_BUS_FMT_RGB444_1X12
+#define V4L2_BUS_FMT_RGB565_1X16 VIDEO_BUS_FMT_RGB565_1X16
#define V4L2_MBUS_FMT_Y8_1X8 VIDEO_BUS_FMT_Y8_1X8
#define V4L2_MBUS_FMT_UV8_1X8 VIDEO_BUS_FMT_UV8_1X8
diff --git a/include/uapi/linux/video-bus-format.h b/include/uapi/linux/video-bus-format.h
index 4abbd5d..f85f7ee 100644
--- a/include/uapi/linux/video-bus-format.h
+++ b/include/uapi/linux/video-bus-format.h
@@ -34,7 +34,7 @@
enum video_bus_format {
VIDEO_BUS_FMT_FIXED = 0x0001,
- /* RGB - next is 0x100e */
+ /* RGB - next is 0x1010 */
VIDEO_BUS_FMT_RGB444_2X8_PADHI_BE = 0x1001,
VIDEO_BUS_FMT_RGB444_2X8_PADHI_LE = 0x1002,
VIDEO_BUS_FMT_RGB555_2X8_PADHI_BE = 0x1003,
@@ -48,6 +48,8 @@ enum video_bus_format {
VIDEO_BUS_FMT_RGB888_2X12_BE = 0x100b,
VIDEO_BUS_FMT_RGB888_2X12_LE = 0x100c,
VIDEO_BUS_FMT_ARGB8888_1X32 = 0x100d,
+ VIDEO_BUS_FMT_RGB444_1X12 = 0x100e,
+ VIDEO_BUS_FMT_RGB565_1X16 = 0x100f,
/* YUV (including grey) - next is 0x2024 */
VIDEO_BUS_FMT_Y8_1X8 = 0x2001,
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/5] video: move mediabus format definition to a more standard place
From: Boris BREZILLON @ 2014-07-22 12:23 UTC (permalink / raw)
To: Thierry Reding, Laurent Pinchart
Cc: linux-api, linux-kernel, dri-devel, Mauro Carvalho Chehab,
linux-media
In-Reply-To: <1406031827-12432-1-git-send-email-boris.brezillon@free-electrons.com>
Rename mediabus formats and move the enum into a separate header file so
that it can be used by DRM/KMS subsystem without any reference to the V4L2
subsystem.
Old V4L2_MBUS_FMT_ definitions are now macros that points to VIDEO_BUS_FMT_
definitions.
Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/v4l2-mediabus.h | 183 ++++++++++++++--------------------
include/uapi/linux/video-bus-format.h | 127 +++++++++++++++++++++++
3 files changed, 205 insertions(+), 106 deletions(-)
create mode 100644 include/uapi/linux/video-bus-format.h
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 24e9033..371874b 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -408,6 +408,7 @@ header-y += veth.h
header-y += vfio.h
header-y += vhost.h
header-y += videodev2.h
+header-y += video-bus-format.h
header-y += virtio_9p.h
header-y += virtio_balloon.h
header-y += virtio_blk.h
diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
index 1445e85..8c31f11 100644
--- a/include/uapi/linux/v4l2-mediabus.h
+++ b/include/uapi/linux/v4l2-mediabus.h
@@ -13,119 +13,90 @@
#include <linux/types.h>
#include <linux/videodev2.h>
+#include <linux/video-bus-format.h>
-/*
- * These pixel codes uniquely identify data formats on the media bus. Mostly
- * they correspond to similarly named V4L2_PIX_FMT_* formats, format 0 is
- * reserved, V4L2_MBUS_FMT_FIXED shall be used by host-client pairs, where the
- * data format is fixed. Additionally, "2X8" means that one pixel is transferred
- * in two 8-bit samples, "BE" or "LE" specify in which order those samples are
- * transferred over the bus: "LE" means that the least significant bits are
- * transferred first, "BE" means that the most significant bits are transferred
- * first, and "PADHI" and "PADLO" define which bits - low or high, in the
- * incomplete high byte, are filled with padding bits.
- *
- * The pixel codes are grouped by type, bus_width, bits per component, samples
- * per pixel and order of subsamples. Numerical values are sorted using generic
- * numerical sort order (8 thus comes before 10).
- *
- * As their value can't change when a new pixel code is inserted in the
- * enumeration, the pixel codes are explicitly given a numerical value. The next
- * free values for each category are listed below, update them when inserting
- * new pixel codes.
- */
-enum v4l2_mbus_pixelcode {
- V4L2_MBUS_FMT_FIXED = 0x0001,
-
- /* RGB - next is 0x100e */
- V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE = 0x1001,
- V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE = 0x1002,
- V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE = 0x1003,
- V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE = 0x1004,
- V4L2_MBUS_FMT_BGR565_2X8_BE = 0x1005,
- V4L2_MBUS_FMT_BGR565_2X8_LE = 0x1006,
- V4L2_MBUS_FMT_RGB565_2X8_BE = 0x1007,
- V4L2_MBUS_FMT_RGB565_2X8_LE = 0x1008,
- V4L2_MBUS_FMT_RGB666_1X18 = 0x1009,
- V4L2_MBUS_FMT_RGB888_1X24 = 0x100a,
- V4L2_MBUS_FMT_RGB888_2X12_BE = 0x100b,
- V4L2_MBUS_FMT_RGB888_2X12_LE = 0x100c,
- V4L2_MBUS_FMT_ARGB8888_1X32 = 0x100d,
+#define V4L2_MBUS_FMT_FIXED VIDEO_BUS_FMT_FIXED
- /* YUV (including grey) - next is 0x2024 */
- V4L2_MBUS_FMT_Y8_1X8 = 0x2001,
- V4L2_MBUS_FMT_UV8_1X8 = 0x2015,
- V4L2_MBUS_FMT_UYVY8_1_5X8 = 0x2002,
- V4L2_MBUS_FMT_VYUY8_1_5X8 = 0x2003,
- V4L2_MBUS_FMT_YUYV8_1_5X8 = 0x2004,
- V4L2_MBUS_FMT_YVYU8_1_5X8 = 0x2005,
- V4L2_MBUS_FMT_UYVY8_2X8 = 0x2006,
- V4L2_MBUS_FMT_VYUY8_2X8 = 0x2007,
- V4L2_MBUS_FMT_YUYV8_2X8 = 0x2008,
- V4L2_MBUS_FMT_YVYU8_2X8 = 0x2009,
- V4L2_MBUS_FMT_Y10_1X10 = 0x200a,
- V4L2_MBUS_FMT_UYVY10_2X10 = 0x2018,
- V4L2_MBUS_FMT_VYUY10_2X10 = 0x2019,
- V4L2_MBUS_FMT_YUYV10_2X10 = 0x200b,
- V4L2_MBUS_FMT_YVYU10_2X10 = 0x200c,
- V4L2_MBUS_FMT_Y12_1X12 = 0x2013,
- V4L2_MBUS_FMT_UYVY8_1X16 = 0x200f,
- V4L2_MBUS_FMT_VYUY8_1X16 = 0x2010,
- V4L2_MBUS_FMT_YUYV8_1X16 = 0x2011,
- V4L2_MBUS_FMT_YVYU8_1X16 = 0x2012,
- V4L2_MBUS_FMT_YDYUYDYV8_1X16 = 0x2014,
- V4L2_MBUS_FMT_UYVY10_1X20 = 0x201a,
- V4L2_MBUS_FMT_VYUY10_1X20 = 0x201b,
- V4L2_MBUS_FMT_YUYV10_1X20 = 0x200d,
- V4L2_MBUS_FMT_YVYU10_1X20 = 0x200e,
- V4L2_MBUS_FMT_YUV10_1X30 = 0x2016,
- V4L2_MBUS_FMT_AYUV8_1X32 = 0x2017,
- V4L2_MBUS_FMT_UYVY12_2X12 = 0x201c,
- V4L2_MBUS_FMT_VYUY12_2X12 = 0x201d,
- V4L2_MBUS_FMT_YUYV12_2X12 = 0x201e,
- V4L2_MBUS_FMT_YVYU12_2X12 = 0x201f,
- V4L2_MBUS_FMT_UYVY12_1X24 = 0x2020,
- V4L2_MBUS_FMT_VYUY12_1X24 = 0x2021,
- V4L2_MBUS_FMT_YUYV12_1X24 = 0x2022,
- V4L2_MBUS_FMT_YVYU12_1X24 = 0x2023,
+#define V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE VIDEO_BUS_FMT_RGB444_2X8_PADHI_BE
+#define V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE VIDEO_BUS_FMT_RGB444_2X8_PADHI_LE
+#define V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE VIDEO_BUS_FMT_RGB555_2X8_PADHI_BE
+#define V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE VIDEO_BUS_FMT_RGB555_2X8_PADHI_LE
+#define V4L2_MBUS_FMT_BGR565_2X8_BE VIDEO_BUS_FMT_BGR565_2X8_BE
+#define V4L2_MBUS_FMT_BGR565_2X8_LE VIDEO_BUS_FMT_BGR565_2X8_LE
+#define V4L2_MBUS_FMT_RGB565_2X8_BE VIDEO_BUS_FMT_RGB565_2X8_BE
+#define V4L2_MBUS_FMT_RGB565_2X8_LE VIDEO_BUS_FMT_RGB565_2X8_LE
+#define V4L2_MBUS_FMT_RGB666_1X18 VIDEO_BUS_FMT_RGB666_1X18
+#define V4L2_MBUS_FMT_RGB888_1X24 VIDEO_BUS_FMT_RGB888_1X24
+#define V4L2_MBUS_FMT_RGB888_2X12_BE VIDEO_BUS_FMT_RGB888_2X12_BE
+#define V4L2_MBUS_FMT_RGB888_2X12_LE VIDEO_BUS_FMT_RGB888_2X12_LE
+#define V4L2_MBUS_FMT_ARGB8888_1X32 VIDEO_BUS_FMT_ARGB8888_1X32
- /* Bayer - next is 0x3019 */
- V4L2_MBUS_FMT_SBGGR8_1X8 = 0x3001,
- V4L2_MBUS_FMT_SGBRG8_1X8 = 0x3013,
- V4L2_MBUS_FMT_SGRBG8_1X8 = 0x3002,
- V4L2_MBUS_FMT_SRGGB8_1X8 = 0x3014,
- V4L2_MBUS_FMT_SBGGR10_ALAW8_1X8 = 0x3015,
- V4L2_MBUS_FMT_SGBRG10_ALAW8_1X8 = 0x3016,
- V4L2_MBUS_FMT_SGRBG10_ALAW8_1X8 = 0x3017,
- V4L2_MBUS_FMT_SRGGB10_ALAW8_1X8 = 0x3018,
- V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b,
- V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c,
- V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009,
- V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8 = 0x300d,
- V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE = 0x3003,
- V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE = 0x3004,
- V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_BE = 0x3005,
- V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_LE = 0x3006,
- V4L2_MBUS_FMT_SBGGR10_1X10 = 0x3007,
- V4L2_MBUS_FMT_SGBRG10_1X10 = 0x300e,
- V4L2_MBUS_FMT_SGRBG10_1X10 = 0x300a,
- V4L2_MBUS_FMT_SRGGB10_1X10 = 0x300f,
- V4L2_MBUS_FMT_SBGGR12_1X12 = 0x3008,
- V4L2_MBUS_FMT_SGBRG12_1X12 = 0x3010,
- V4L2_MBUS_FMT_SGRBG12_1X12 = 0x3011,
- V4L2_MBUS_FMT_SRGGB12_1X12 = 0x3012,
+#define V4L2_MBUS_FMT_Y8_1X8 VIDEO_BUS_FMT_Y8_1X8
+#define V4L2_MBUS_FMT_UV8_1X8 VIDEO_BUS_FMT_UV8_1X8
+#define V4L2_MBUS_FMT_UYVY8_1_5X8 VIDEO_BUS_FMT_UYVY8_1_5X8
+#define V4L2_MBUS_FMT_VYUY8_1_5X8 VIDEO_BUS_FMT_VYUY8_1_5X8
+#define V4L2_MBUS_FMT_YUYV8_1_5X8 VIDEO_BUS_FMT_YUYV8_1_5X8
+#define V4L2_MBUS_FMT_YVYU8_1_5X8 VIDEO_BUS_FMT_YVYU8_1_5X8
+#define V4L2_MBUS_FMT_UYVY8_2X8 VIDEO_BUS_FMT_UYVY8_2X8
+#define V4L2_MBUS_FMT_VYUY8_2X8 VIDEO_BUS_FMT_VYUY8_2X8
+#define V4L2_MBUS_FMT_YUYV8_2X8 VIDEO_BUS_FMT_YUYV8_2X8
+#define V4L2_MBUS_FMT_YVYU8_2X8 VIDEO_BUS_FMT_YVYU8_2X8
+#define V4L2_MBUS_FMT_Y10_1X10 VIDEO_BUS_FMT_Y10_1X10
+#define V4L2_MBUS_FMT_UYVY10_2X10 VIDEO_BUS_FMT_UYVY10_2X10
+#define V4L2_MBUS_FMT_VYUY10_2X10 VIDEO_BUS_FMT_VYUY10_2X10
+#define V4L2_MBUS_FMT_YUYV10_2X10 VIDEO_BUS_FMT_YUYV10_2X10
+#define V4L2_MBUS_FMT_YVYU10_2X10 VIDEO_BUS_FMT_YVYU10_2X10
+#define V4L2_MBUS_FMT_Y12_1X12 VIDEO_BUS_FMT_Y12_1X12
+#define V4L2_MBUS_FMT_UYVY8_1X16 VIDEO_BUS_FMT_UYVY8_1X16
+#define V4L2_MBUS_FMT_VYUY8_1X16 VIDEO_BUS_FMT_VYUY8_1X16
+#define V4L2_MBUS_FMT_YUYV8_1X16 VIDEO_BUS_FMT_YUYV8_1X16
+#define V4L2_MBUS_FMT_YVYU8_1X16 VIDEO_BUS_FMT_YVYU8_1X16
+#define V4L2_MBUS_FMT_YDYUYDYV8_1X16 VIDEO_BUS_FMT_YDYUYDYV8_1X16
+#define V4L2_MBUS_FMT_UYVY10_1X20 VIDEO_BUS_FMT_UYVY10_1X20
+#define V4L2_MBUS_FMT_VYUY10_1X20 VIDEO_BUS_FMT_VYUY10_1X20
+#define V4L2_MBUS_FMT_YUYV10_1X20 VIDEO_BUS_FMT_YUYV10_1X20
+#define V4L2_MBUS_FMT_YVYU10_1X20 VIDEO_BUS_FMT_YVYU10_1X20
+#define V4L2_MBUS_FMT_YUV10_1X30 VIDEO_BUS_FMT_YUV10_1X30
+#define V4L2_MBUS_FMT_AYUV8_1X32 VIDEO_BUS_FMT_AYUV8_1X32
+#define V4L2_MBUS_FMT_UYVY12_2X12 VIDEO_BUS_FMT_UYVY12_2X12
+#define V4L2_MBUS_FMT_VYUY12_2X12 VIDEO_BUS_FMT_VYUY12_2X12
+#define V4L2_MBUS_FMT_YUYV12_2X12 VIDEO_BUS_FMT_YUYV12_2X12
+#define V4L2_MBUS_FMT_YVYU12_2X12 VIDEO_BUS_FMT_YVYU12_2X12
+#define V4L2_MBUS_FMT_UYVY12_1X24 VIDEO_BUS_FMT_UYVY12_1X24
+#define V4L2_MBUS_FMT_VYUY12_1X24 VIDEO_BUS_FMT_VYUY12_1X24
+#define V4L2_MBUS_FMT_YUYV12_1X24 VIDEO_BUS_FMT_YUYV12_1X24
+#define V4L2_MBUS_FMT_YVYU12_1X24 VIDEO_BUS_FMT_YVYU12_1X24
- /* JPEG compressed formats - next is 0x4002 */
- V4L2_MBUS_FMT_JPEG_1X8 = 0x4001,
+#define V4L2_MBUS_FMT_SBGGR8_1X8 VIDEO_BUS_FMT_SBGGR8_1X8
+#define V4L2_MBUS_FMT_SGBRG8_1X8 VIDEO_BUS_FMT_SGBRG8_1X8
+#define V4L2_MBUS_FMT_SGRBG8_1X8 VIDEO_BUS_FMT_SGRBG8_1X8
+#define V4L2_MBUS_FMT_SRGGB8_1X8 VIDEO_BUS_FMT_SRGGB8_1X8
+#define V4L2_MBUS_FMT_SBGGR10_ALAW8_1X8 VIDEO_BUS_FMT_SBGGR10_ALAW8_1X8
+#define V4L2_MBUS_FMT_SGBRG10_ALAW8_1X8 VIDEO_BUS_FMT_SGBRG10_ALAW8_1X8
+#define V4L2_MBUS_FMT_SGRBG10_ALAW8_1X8 VIDEO_BUS_FMT_SGRBG10_ALAW8_1X8
+#define V4L2_MBUS_FMT_SRGGB10_ALAW8_1X8 VIDEO_BUS_FMT_SRGGB10_ALAW8_1X8
+#define V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8 VIDEO_BUS_FMT_SBGGR10_DPCM8_1X8
+#define V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8 VIDEO_BUS_FMT_SGBRG10_DPCM8_1X8
+#define V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8 VIDEO_BUS_FMT_SGRBG10_DPCM8_1X8
+#define V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8 VIDEO_BUS_FMT_SRGGB10_DPCM8_1X8
+#define V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE VIDEO_BUS_FMT_SBGGR10_2X8_PADHI_BE
+#define V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE VIDEO_BUS_FMT_SBGGR10_2X8_PADHI_LE
+#define V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_BE VIDEO_BUS_FMT_SBGGR10_2X8_PADLO_BE
+#define V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_LE VIDEO_BUS_FMT_SBGGR10_2X8_PADLO_LE
+#define V4L2_MBUS_FMT_SBGGR10_1X10 VIDEO_BUS_FMT_SBGGR10_1X10
+#define V4L2_MBUS_FMT_SGBRG10_1X10 VIDEO_BUS_FMT_SGBRG10_1X10
+#define V4L2_MBUS_FMT_SGRBG10_1X10 VIDEO_BUS_FMT_SGRBG10_1X10
+#define V4L2_MBUS_FMT_SRGGB10_1X10 VIDEO_BUS_FMT_SRGGB10_1X10
+#define V4L2_MBUS_FMT_SBGGR12_1X12 VIDEO_BUS_FMT_SBGGR12_1X12
+#define V4L2_MBUS_FMT_SGBRG12_1X12 VIDEO_BUS_FMT_SGBRG12_1X12
+#define V4L2_MBUS_FMT_SGRBG12_1X12 VIDEO_BUS_FMT_SGRBG12_1X12
+#define V4L2_MBUS_FMT_SRGGB12_1X12 VIDEO_BUS_FMT_SRGGB12_1X12
- /* Vendor specific formats - next is 0x5002 */
+#define V4L2_MBUS_FMT_JPEG_1X8 VIDEO_BUS_FMT_JPEG_1X8
- /* S5C73M3 sensor specific interleaved UYVY and JPEG */
- V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8 = 0x5001,
+#define V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8 VIDEO_BUS_FMT_S5C_UYVY_JPEG_1X8
- /* HSV - next is 0x6002 */
- V4L2_MBUS_FMT_AHSV8888_1X32 = 0x6001,
-};
+#define V4L2_MBUS_FMT_AHSV8888_1X32 VIDEO_BUS_FMT_AHSV8888_1X32
/**
* struct v4l2_mbus_framefmt - frame format on the media bus
diff --git a/include/uapi/linux/video-bus-format.h b/include/uapi/linux/video-bus-format.h
new file mode 100644
index 0000000..4abbd5d
--- /dev/null
+++ b/include/uapi/linux/video-bus-format.h
@@ -0,0 +1,127 @@
+/*
+ * Video Bus API header
+ *
+ * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_VIDEO_BUS_FORMAT_H
+#define __LINUX_VIDEO_BUS_FORMAT_H
+
+/*
+ * These bus formats uniquely identify data formats on the data bus. Mostly
+ * they correspond to similarly named VIDEO_PIX_FMT_* formats, format 0 is
+ * reserved, VIDEO_BUS_FMT_FIXED shall be used by host-client pairs, where the
+ * data format is fixed. Additionally, "2X8" means that one pixel is transferred
+ * in two 8-bit samples, "BE" or "LE" specify in which order those samples are
+ * transferred over the bus: "LE" means that the least significant bits are
+ * transferred first, "BE" means that the most significant bits are transferred
+ * first, and "PADHI" and "PADLO" define which bits - low or high, in the
+ * incomplete high byte, are filled with padding bits.
+ *
+ * The bus formats are grouped by type, bus_width, bits per component, samples
+ * per pixel and order of subsamples. Numerical values are sorted using generic
+ * numerical sort order (8 thus comes before 10).
+ *
+ * As their value can't change when a new bus format is inserted in the
+ * enumeration, the bus formats are explicitly given a numerical value. The next
+ * free values for each category are listed below, update them when inserting
+ * new pixel codes.
+ */
+enum video_bus_format {
+ VIDEO_BUS_FMT_FIXED = 0x0001,
+
+ /* RGB - next is 0x100e */
+ VIDEO_BUS_FMT_RGB444_2X8_PADHI_BE = 0x1001,
+ VIDEO_BUS_FMT_RGB444_2X8_PADHI_LE = 0x1002,
+ VIDEO_BUS_FMT_RGB555_2X8_PADHI_BE = 0x1003,
+ VIDEO_BUS_FMT_RGB555_2X8_PADHI_LE = 0x1004,
+ VIDEO_BUS_FMT_BGR565_2X8_BE = 0x1005,
+ VIDEO_BUS_FMT_BGR565_2X8_LE = 0x1006,
+ VIDEO_BUS_FMT_RGB565_2X8_BE = 0x1007,
+ VIDEO_BUS_FMT_RGB565_2X8_LE = 0x1008,
+ VIDEO_BUS_FMT_RGB666_1X18 = 0x1009,
+ VIDEO_BUS_FMT_RGB888_1X24 = 0x100a,
+ VIDEO_BUS_FMT_RGB888_2X12_BE = 0x100b,
+ VIDEO_BUS_FMT_RGB888_2X12_LE = 0x100c,
+ VIDEO_BUS_FMT_ARGB8888_1X32 = 0x100d,
+
+ /* YUV (including grey) - next is 0x2024 */
+ VIDEO_BUS_FMT_Y8_1X8 = 0x2001,
+ VIDEO_BUS_FMT_UV8_1X8 = 0x2015,
+ VIDEO_BUS_FMT_UYVY8_1_5X8 = 0x2002,
+ VIDEO_BUS_FMT_VYUY8_1_5X8 = 0x2003,
+ VIDEO_BUS_FMT_YUYV8_1_5X8 = 0x2004,
+ VIDEO_BUS_FMT_YVYU8_1_5X8 = 0x2005,
+ VIDEO_BUS_FMT_UYVY8_2X8 = 0x2006,
+ VIDEO_BUS_FMT_VYUY8_2X8 = 0x2007,
+ VIDEO_BUS_FMT_YUYV8_2X8 = 0x2008,
+ VIDEO_BUS_FMT_YVYU8_2X8 = 0x2009,
+ VIDEO_BUS_FMT_Y10_1X10 = 0x200a,
+ VIDEO_BUS_FMT_UYVY10_2X10 = 0x2018,
+ VIDEO_BUS_FMT_VYUY10_2X10 = 0x2019,
+ VIDEO_BUS_FMT_YUYV10_2X10 = 0x200b,
+ VIDEO_BUS_FMT_YVYU10_2X10 = 0x200c,
+ VIDEO_BUS_FMT_Y12_1X12 = 0x2013,
+ VIDEO_BUS_FMT_UYVY8_1X16 = 0x200f,
+ VIDEO_BUS_FMT_VYUY8_1X16 = 0x2010,
+ VIDEO_BUS_FMT_YUYV8_1X16 = 0x2011,
+ VIDEO_BUS_FMT_YVYU8_1X16 = 0x2012,
+ VIDEO_BUS_FMT_YDYUYDYV8_1X16 = 0x2014,
+ VIDEO_BUS_FMT_UYVY10_1X20 = 0x201a,
+ VIDEO_BUS_FMT_VYUY10_1X20 = 0x201b,
+ VIDEO_BUS_FMT_YUYV10_1X20 = 0x200d,
+ VIDEO_BUS_FMT_YVYU10_1X20 = 0x200e,
+ VIDEO_BUS_FMT_YUV10_1X30 = 0x2016,
+ VIDEO_BUS_FMT_AYUV8_1X32 = 0x2017,
+ VIDEO_BUS_FMT_UYVY12_2X12 = 0x201c,
+ VIDEO_BUS_FMT_VYUY12_2X12 = 0x201d,
+ VIDEO_BUS_FMT_YUYV12_2X12 = 0x201e,
+ VIDEO_BUS_FMT_YVYU12_2X12 = 0x201f,
+ VIDEO_BUS_FMT_UYVY12_1X24 = 0x2020,
+ VIDEO_BUS_FMT_VYUY12_1X24 = 0x2021,
+ VIDEO_BUS_FMT_YUYV12_1X24 = 0x2022,
+ VIDEO_BUS_FMT_YVYU12_1X24 = 0x2023,
+
+ /* Bayer - next is 0x3019 */
+ VIDEO_BUS_FMT_SBGGR8_1X8 = 0x3001,
+ VIDEO_BUS_FMT_SGBRG8_1X8 = 0x3013,
+ VIDEO_BUS_FMT_SGRBG8_1X8 = 0x3002,
+ VIDEO_BUS_FMT_SRGGB8_1X8 = 0x3014,
+ VIDEO_BUS_FMT_SBGGR10_ALAW8_1X8 = 0x3015,
+ VIDEO_BUS_FMT_SGBRG10_ALAW8_1X8 = 0x3016,
+ VIDEO_BUS_FMT_SGRBG10_ALAW8_1X8 = 0x3017,
+ VIDEO_BUS_FMT_SRGGB10_ALAW8_1X8 = 0x3018,
+ VIDEO_BUS_FMT_SBGGR10_DPCM8_1X8 = 0x300b,
+ VIDEO_BUS_FMT_SGBRG10_DPCM8_1X8 = 0x300c,
+ VIDEO_BUS_FMT_SGRBG10_DPCM8_1X8 = 0x3009,
+ VIDEO_BUS_FMT_SRGGB10_DPCM8_1X8 = 0x300d,
+ VIDEO_BUS_FMT_SBGGR10_2X8_PADHI_BE = 0x3003,
+ VIDEO_BUS_FMT_SBGGR10_2X8_PADHI_LE = 0x3004,
+ VIDEO_BUS_FMT_SBGGR10_2X8_PADLO_BE = 0x3005,
+ VIDEO_BUS_FMT_SBGGR10_2X8_PADLO_LE = 0x3006,
+ VIDEO_BUS_FMT_SBGGR10_1X10 = 0x3007,
+ VIDEO_BUS_FMT_SGBRG10_1X10 = 0x300e,
+ VIDEO_BUS_FMT_SGRBG10_1X10 = 0x300a,
+ VIDEO_BUS_FMT_SRGGB10_1X10 = 0x300f,
+ VIDEO_BUS_FMT_SBGGR12_1X12 = 0x3008,
+ VIDEO_BUS_FMT_SGBRG12_1X12 = 0x3010,
+ VIDEO_BUS_FMT_SGRBG12_1X12 = 0x3011,
+ VIDEO_BUS_FMT_SRGGB12_1X12 = 0x3012,
+
+ /* JPEG compressed formats - next is 0x4002 */
+ VIDEO_BUS_FMT_JPEG_1X8 = 0x4001,
+
+ /* Vendor specific formats - next is 0x5002 */
+
+ /* S5C73M3 sensor specific interleaved UYVY and JPEG */
+ VIDEO_BUS_FMT_S5C_UYVY_JPEG_1X8 = 0x5001,
+
+ /* HSV - next is 0x6002 */
+ VIDEO_BUS_FMT_AHSV8888_1X32 = 0x6001,
+};
+
+#endif /* __LINUX_VIDEO_BUS_FORMAT_H */
--
1.8.3.2
^ permalink raw reply related
* [PATCH 0/5] video: describe data bus formats
From: Boris BREZILLON @ 2014-07-22 12:23 UTC (permalink / raw)
To: Thierry Reding, Laurent Pinchart
Cc: David Airlie, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Mauro Carvalho Chehab,
linux-media-u79uwXL29TY76Z2rM5mHXA, Boris BREZILLON
Hello,
This patch series is a proposal to describe the different data formats used
by HW components to connect with each other.
This is just a copy of the existing V4L2_MBUS_FMT defintions with a neutral
name so that it can be used by V4L2 and DRM/KMS subsystem.
This series also makes use of this video_bus_format enum in the DRM/KMS
subsystem to define the data fomats supported on the connector <-> device
link.
The video bus formats are not documented yet (and I don't know where this doc
should be stored), but I'm pretty sure this version won't be the last one ;-).
Best Regards,
Boris
Boris BREZILLON (5):
video: move mediabus format definition to a more standard place
video: add RGB444_1X12 and RGB565_1X16 bus formats
drm: add bus_formats and nbus_formats fields to drm_display_info
drm: panel: simple-panel: add support for bus_format retrieval
drm: panel: simple-panel: add bus format information for foxlink panel
drivers/gpu/drm/drm_crtc.c | 28 +++++
drivers/gpu/drm/panel/panel-simple.c | 6 ++
include/drm/drm_crtc.h | 8 ++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/v4l2-mediabus.h | 185 +++++++++++++++-------------------
include/uapi/linux/video-bus-format.h | 129 ++++++++++++++++++++++++
6 files changed, 251 insertions(+), 106 deletions(-)
create mode 100644 include/uapi/linux/video-bus-format.h
--
1.8.3.2
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-22 9:59 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Steven Rostedt, Darren Hart, Andy Lutomirski, Andi Kleen,
Waiman Long, Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Low,
Scott J Norton, Robert Haas
In-Reply-To: <20140722084842.GZ3935@laptop>
On Tue, 22 Jul 2014, Peter Zijlstra wrote:
> On Tue, Jul 22, 2014 at 10:39:17AM +0200, Thomas Gleixner wrote:
> > On Tue, 22 Jul 2014, Peter Zijlstra wrote:
> > > Anyway, there is one big fail in the entire futex stack that we 'need'
> > > to sort some day and that is NUMA. Some people (again database people)
> > > explicitly do not use futexes and instead use sysvsem because of this.
> > >
> > > The problem with numa futexes is that because they're vaddr based there
> > > is no (persistent) node information. You always end up having to fall
> > > back to looking in all nodes before you can guarantee there is no
> > > matching futex.
> > >
> > > One way to achieve it is by extending the futex value to include a node
> > > number, but that's obviously a complete ABI break. Then again, it should
> > > be pretty straight fwd, since the node number doesn't need to be part of
> > > the actual atomic update part, just part of the userspace storage.
> >
> > So you want per node hash buckets, right? Fair enough, but how do you
> > make sure, that no thread/process on a different node is fiddling with
> > that "node bound" futex as well?
>
> You don't and that should work just as well, just slower. But since the
> node id is in the futex 'value' we'll always end up in the right
> node-hash, even if its a remote one.
>
> So yes, per node hashes, and a persistent futex->node map.
Which works fine as long as you only have the futex_q on the stack of
the blocked task. If user space is lying to you, then you just end up
with a bunch of threads sleeping forever. Who cares?
But if you create independent kernel state, which we have with
pi_state and which you need for finegrained locking and further
spinning fun, you open up another can of worms. Simply because this
would enable rogue user space to create multiple instances of the
kernel internal state. I can predict the CVEs resulting from that
even without using a crystal ball.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Peter Zijlstra @ 2014-07-22 8:48 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Steven Rostedt, Darren Hart, Andy Lutomirski, Andi Kleen,
Waiman Long, Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel@vger.kernel.org, Linux API,
linux-doc@vger.kernel.org, Jason Low, Scott J Norton, Robert Haas
In-Reply-To: <alpine.DEB.2.10.1407221034260.20847@nanos>
On Tue, Jul 22, 2014 at 10:39:17AM +0200, Thomas Gleixner wrote:
> On Tue, 22 Jul 2014, Peter Zijlstra wrote:
> > Anyway, there is one big fail in the entire futex stack that we 'need'
> > to sort some day and that is NUMA. Some people (again database people)
> > explicitly do not use futexes and instead use sysvsem because of this.
> >
> > The problem with numa futexes is that because they're vaddr based there
> > is no (persistent) node information. You always end up having to fall
> > back to looking in all nodes before you can guarantee there is no
> > matching futex.
> >
> > One way to achieve it is by extending the futex value to include a node
> > number, but that's obviously a complete ABI break. Then again, it should
> > be pretty straight fwd, since the node number doesn't need to be part of
> > the actual atomic update part, just part of the userspace storage.
>
> So you want per node hash buckets, right? Fair enough, but how do you
> make sure, that no thread/process on a different node is fiddling with
> that "node bound" futex as well?
You don't and that should work just as well, just slower. But since the
node id is in the futex 'value' we'll always end up in the right
node-hash, even if its a remote one.
So yes, per node hashes, and a persistent futex->node map.
And before people start talking about mempol and using that to bind
memory to nodes and such, remember that private futexes do not have a
vma lookup and therefore mempols are impossible to use.
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Thomas Gleixner @ 2014-07-22 8:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Steven Rostedt, Darren Hart, Andy Lutomirski, Andi Kleen,
Waiman Long, Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Low,
Scott J Norton, Robert Haas
In-Reply-To: <20140722074719.GV3935@laptop>
On Tue, 22 Jul 2014, Peter Zijlstra wrote:
> Anyway, there is one big fail in the entire futex stack that we 'need'
> to sort some day and that is NUMA. Some people (again database people)
> explicitly do not use futexes and instead use sysvsem because of this.
>
> The problem with numa futexes is that because they're vaddr based there
> is no (persistent) node information. You always end up having to fall
> back to looking in all nodes before you can guarantee there is no
> matching futex.
>
> One way to achieve it is by extending the futex value to include a node
> number, but that's obviously a complete ABI break. Then again, it should
> be pretty straight fwd, since the node number doesn't need to be part of
> the actual atomic update part, just part of the userspace storage.
So you want per node hash buckets, right? Fair enough, but how do you
make sure, that no thread/process on a different node is fiddling with
that "node bound" futex as well?
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH 0/5] futex: introduce an optimistic spinning futex
From: Peter Zijlstra @ 2014-07-22 7:47 UTC (permalink / raw)
To: Steven Rostedt
Cc: Thomas Gleixner, Darren Hart, Andy Lutomirski, Andi Kleen,
Waiman Long, Ingo Molnar, Davidlohr Bueso, Heiko Carstens,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Low,
Scott J Norton, Robert Haas
In-Reply-To: <20140721213457.46623e2f-f9ZlEuEWxVcJvu8Pb33WZ0EMvNT87kid@public.gmane.org>
On Mon, Jul 21, 2014 at 09:34:57PM -0400, Steven Rostedt wrote:
> I just want to point out that I was having a very nice conversation
> with Robert Haas (Cc'd) in Napa Valley at Linux Collaboration about
> this very topic. Robert is a PostgeSQL developer who told me that they
> implement their spin locks completely in userspace (no futex, just raw
> spinning on shared memory). This is because the sleep on contention of a
> futex has shown to be very expensive in their benchmarks. His work is
> not a micro benchmark but for a very popular database where locking is
> crucial.
Userspace spinlocks are a clusterfuck. Its impossible to solve the
priority inversion trainwrecks they cause _ever_.
We've had -- as I think Mike already pointed out -- tons of 'fun' with
psql exactly because its doing this :-(
> I was telling Robert that if futexes get optimistic spinning, he should
> reconsider their use of userspace spinlocks in favor of this, because
> I'm pretty sure that they will see a great improvement.
>
> Now Robert will be the best one to answer if the system call is indeed
> more expensive than doing full spins in userspace. If the spin is done
> in the kernel and they still get better performance by just spinning
> blindly in userspace even if the owner is asleep, I think we will have
> our answer.
No, the best way is to measure the exact syscall cost. If he still gets
better performance we need to analyze why, there might be something else
hiding there.
> Note, I believe they only care about shared threads, and this
> optimistic spinning does not need to be something done between
> processes.
There's no reason not to provide it for shared futexes, in fact I
suspect not doing it for shared futexes is going to make the code
uglier.
Anyway, there is one big fail in the entire futex stack that we 'need'
to sort some day and that is NUMA. Some people (again database people)
explicitly do not use futexes and instead use sysvsem because of this.
The problem with numa futexes is that because they're vaddr based there
is no (persistent) node information. You always end up having to fall
back to looking in all nodes before you can guarantee there is no
matching futex.
One way to achieve it is by extending the futex value to include a node
number, but that's obviously a complete ABI break. Then again, it should
be pretty straight fwd, since the node number doesn't need to be part of
the actual atomic update part, just part of the userspace storage.
^ permalink raw reply
* [RESEND PATCH v3 6/6] Documentation: charger: max14577: Document exported sysfs entry
From: Krzysztof Kozlowski @ 2014-07-22 7:41 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov, David Woodhouse, linux-kernel, linux-api
Cc: Samuel Ortiz, Lee Jones, Liam Girdwood, Mark Brown, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
In-Reply-To: <1406014866-17744-1-git-send-email-k.kozlowski@samsung.com>
Document the 'fast charge timer' setting exported by max14577 driver
through sysfs entry.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
Documentation/ABI/testing/sysfs-class-power | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index 78c7baca3587..83ee67ebf0e9 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -18,3 +18,17 @@ Description:
This file is writeable and can be used to set the assumed
battery 'full level'. As batteries age, this value has to be
amended over time.
+
+What: /sys/class/power_supply/max14577-charger/device/fast_charge_timer
+Date: July 2014
+KernelVersion: 3.17.0
+Contact: Krzysztof Kozlowski <k.kozlowski@samsung.com>
+Description:
+ This entry shows and sets the maximum time the max14577
+ charger operates in fast-charge mode. When the timer expires
+ the device will terminate fast-charge mode (charging current
+ will drop to 0 A) and will trigger interrupt.
+
+ Valid values:
+ - 5, 6 or 7 (hours),
+ - 0: disabled.
--
1.9.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox