Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
On Tue, Jul 17, 2007 at 04:25:42PM +0200, Andi Kleen wrote:
>
>
> When you revamp everything I guess it would make the locks
> easier to read to just put them into a .S file? They're
> out of line anyways.
It is a bit tricky because of the way kernel/spinlock.c uses
the the inline asm (that, and unlocks are inlined), and some
of the debugging code.
Also, I'm not exactly sure what is the best way to clean up
spinlocks, if it is even possible. I'm afraid to touch it
more than I have to :)
>
> In general they look ok.
>
> >
> > static inline void __raw_spin_unlock(raw_spinlock_t *lock)
> > {
> > - asm volatile("movl $1,%0" :"=m" (lock->slock) :: "memory");
> > + __asm__ __volatile__(
>
> Minor nit: please don't use these underlined keywords. They just look
> ugly and asm volatile works as well.
OK, I like that better too. I didn't even realise I changed this
because it is mechanical to do the __. That's good to know and I'll
change my habit now.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
When you revamp everything I guess it would make the locks
easier to read to just put them into a .S file? They're
out of line anyways.
In general they look ok.
>
> static inline void __raw_spin_unlock(raw_spinlock_t *lock)
> {
> - asm volatile("movl $1,%0" :"=m" (lock->slock) :: "memory");
> + __asm__ __volatile__(
Minor nit: please don't use these underlined keywords. They just look
ugly and asm volatile works as well.
-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
On Mon, Jul 16, 2007 at 11:49:40AM +0200, Ingo Molnar wrote:
>
> * Nick Piggin <[EMAIL PROTECTED]> wrote:
>
> > On Mon, Jul 16, 2007 at 11:26:46AM +0200, Ingo Molnar wrote:
> > >
> > > * Nick Piggin <[EMAIL PROTECTED]> wrote:
> > >
> > > > [...] trylock is more significantly slower, but they are relatively
> > > > rare.
> > >
> > > trylock is the main thing that the spinlock debugging code uses, and
> > > SPINLOCK_DEBUG is frequently enabled by distro kernels. OTOH, the cost
> > > looks like to be +5 instructions, right? Still ...
> >
> > Which trylocks do you mean? The lockbreak spinlocks use trylock, but
> > those are not used with the ticket version.
>
> the trylocks in lib/spinlock-debug.c:
>
> static void __spin_lock_debug(spinlock_t *lock)
> {
> ...
> if (__raw_spin_trylock(&lock->raw_lock))
> return;
> ...
> void _raw_spin_lock(spinlock_t *lock)
> {
> debug_spin_lock_before(lock);
> if (unlikely(!__raw_spin_trylock(&lock->raw_lock)))
> __spin_lock_debug(lock);
> debug_spin_lock_after(lock);
> }
>
> am i missing something?
No, I missed that. Yeah, that would get a bit slower, but I'm not sure
if it would be a problem on a kernel where you have spinlock debuggin
on anyway.
If it becomes a problem, we could perhaps do a version for ticket locks
that first takes a ticket, and then is for up to a second before
printing the stuck lock message. That would make the performance hit go away.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
* Nick Piggin <[EMAIL PROTECTED]> wrote:
> On Mon, Jul 16, 2007 at 11:26:46AM +0200, Ingo Molnar wrote:
> >
> > * Nick Piggin <[EMAIL PROTECTED]> wrote:
> >
> > > [...] trylock is more significantly slower, but they are relatively
> > > rare.
> >
> > trylock is the main thing that the spinlock debugging code uses, and
> > SPINLOCK_DEBUG is frequently enabled by distro kernels. OTOH, the cost
> > looks like to be +5 instructions, right? Still ...
>
> Which trylocks do you mean? The lockbreak spinlocks use trylock, but
> those are not used with the ticket version.
the trylocks in lib/spinlock-debug.c:
static void __spin_lock_debug(spinlock_t *lock)
{
...
if (__raw_spin_trylock(&lock->raw_lock))
return;
...
void _raw_spin_lock(spinlock_t *lock)
{
debug_spin_lock_before(lock);
if (unlikely(!__raw_spin_trylock(&lock->raw_lock)))
__spin_lock_debug(lock);
debug_spin_lock_after(lock);
}
am i missing something?
Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
On Mon, Jul 16, 2007 at 11:26:46AM +0200, Ingo Molnar wrote: > > * Nick Piggin <[EMAIL PROTECTED]> wrote: > > > [...] trylock is more significantly slower, but they are relatively > > rare. > > trylock is the main thing that the spinlock debugging code uses, and > SPINLOCK_DEBUG is frequently enabled by distro kernels. OTOH, the cost > looks like to be +5 instructions, right? Still ... Which trylocks do you mean? The lockbreak spinlocks use trylock, but those are not used with the ticket version. I wouldn't be against adding an option between either of the lock types, if there was value in it. But I would like to default ticket locks to "y", at least in -rc kernels, in order to see if performance goes down anywhere. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
* Nick Piggin <[EMAIL PROTECTED]> wrote: > [...] trylock is more significantly slower, but they are relatively > rare. trylock is the main thing that the spinlock debugging code uses, and SPINLOCK_DEBUG is frequently enabled by distro kernels. OTOH, the cost looks like to be +5 instructions, right? Still ... Ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
On Mon, Jul 16, 2007 at 10:16:54AM +0100, Alan Cox wrote: > > however the difference is quite small on Core2 and Opteron when working out > > of > > cache, and becomes almost insignificant even on P4 when the lock misses > > cache. > > trylock is more significantly slower, but they are relatively rare. > > This has a 255 processor limit (worst case). That should probably be > clearly documented for the future. Yeah good point. I added an #error for it. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc][patch 2/2] x86_64: FIFO ticket spinlocks
> however the difference is quite small on Core2 and Opteron when working out of > cache, and becomes almost insignificant even on P4 when the lock misses cache. > trylock is more significantly slower, but they are relatively rare. This has a 255 processor limit (worst case). That should probably be clearly documented for the future. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
