Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Wed, Aug 24, 2016 at 10:56:59AM +0100, Will Deacon wrote:
> > + owner = atomic_long_read(&lock->owner);
> > + for (;;) {
> > + unsigned long old;
> > +
> > + old = atomic_long_cmpxchg_release(&lock->owner, owner, owner &
> > 0x03);
> > + if (old == owner)
> > + break;
> > +
> > + owner = old;
> > + }
>
> Can you rewrite this using atomic_long_fetch_and_release?
Yes, until patch 3/3.. but now that you mention it I think we can do:
owner = atomic_long_read(&lock->owner);
if (!(owner & MUTEX_FLAG_HANDOFF))
(void)atomic_long_fetch_and_release(MUTEX_FLAGS, &lock->owner);
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Wed, Aug 24, 2016 at 05:34:12PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 24, 2016 at 10:56:59AM +0100, Will Deacon wrote:
> > > + owner = atomic_long_read(&lock->owner);
> > > + for (;;) {
> > > + unsigned long old;
> > > +
> > > + old = atomic_long_cmpxchg_release(&lock->owner, owner, owner &
> > > 0x03);
> > > + if (old == owner)
> > > + break;
> > > +
> > > + owner = old;
> > > + }
> >
> > Can you rewrite this using atomic_long_fetch_and_release?
>
> Yes, until patch 3/3.. but now that you mention it I think we can do:
>
> owner = atomic_long_read(&lock->owner);
> if (!(owner & MUTEX_FLAG_HANDOFF))
> (void)atomic_long_fetch_and_release(MUTEX_FLAGS, &lock->owner);
>
And of course, x86 would very much like atomic_long_and_release() here,
such that it can do LOCK ADD instead of a LOCK CMPXCHG loop. But of
course, we don't have that ...
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Wed, Aug 24, 2016 at 06:52:44PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 24, 2016 at 05:34:12PM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 24, 2016 at 10:56:59AM +0100, Will Deacon wrote:
> > > > + owner = atomic_long_read(&lock->owner);
> > > > + for (;;) {
> > > > + unsigned long old;
> > > > +
> > > > + old = atomic_long_cmpxchg_release(&lock->owner, owner,
> > > > owner & 0x03);
> > > > + if (old == owner)
> > > > + break;
> > > > +
> > > > + owner = old;
> > > > + }
> > >
> > > Can you rewrite this using atomic_long_fetch_and_release?
> >
> > Yes, until patch 3/3.. but now that you mention it I think we can do:
> >
> > owner = atomic_long_read(&lock->owner);
> > if (!(owner & MUTEX_FLAG_HANDOFF))
> > (void)atomic_long_fetch_and_release(MUTEX_FLAGS, &lock->owner);
> >
>
> And of course, x86 would very much like atomic_long_and_release() here,
> such that it can do LOCK ADD instead of a LOCK CMPXCHG loop. But of
> course, we don't have that ...
... yeah, I noticed that. There is a curious use of atomic_and in
linux/atomic.h, but it's packed full of false promises.
Will
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Tue, Aug 23, 2016 at 02:46:18PM +0200, Peter Zijlstra wrote:
> There's a number of iffy in mutex because mutex::count and
> mutex::owner are two different fields; this too is the reason
> MUTEX_SPIN_ON_OWNER and DEBUG_MUTEX are mutually exclusive.
>
> Cure this by folding them into a single atomic_long_t field.
>
> This nessecairly kills all the architecture specific mutex code.
>
> Signed-off-by: Peter Zijlstra (Intel)
[...]
> void __sched mutex_unlock(struct mutex *lock)
> {
> - /*
> - * The unlocking fastpath is the 0->1 transition from 'locked'
> - * into 'unlocked' state:
> - */
> -#ifndef CONFIG_DEBUG_MUTEXES
> - /*
> - * When debugging is enabled we must not clear the owner before time,
> - * the slow path will always be taken, and that clears the owner field
> - * after verifying that it was indeed current.
> - */
> - mutex_clear_owner(lock);
> + unsigned long owner;
> +
> +#ifdef CONFIG_DEBUG_MUTEXES
> + DEBUG_LOCKS_WARN_ON(__mutex_owner(lock) != current);
> #endif
> - __mutex_fastpath_unlock(&lock->count, __mutex_unlock_slowpath);
> -}
>
> + owner = atomic_long_read(&lock->owner);
> + for (;;) {
> + unsigned long old;
> +
> + old = atomic_long_cmpxchg_release(&lock->owner, owner, owner &
> 0x03);
> + if (old == owner)
> + break;
> +
> + owner = old;
> + }
Can you rewrite this using atomic_long_fetch_and_release?
Will
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Tue, Aug 23, 2016 at 03:55:52PM -0400, Waiman Long wrote: > On 08/23/2016 08:46 AM, Peter Zijlstra wrote: > > I have 2 more comments about the code. > 1) There are a couple of places where you only use 0x3 in mutex.c. They > should be replaced by the symbolic name instead. > 2) We should make __mutex_lock_slowpath() a noinline function just like > __mutex_lock_killable_slowpath() or __mutex_lock_interruptible_slowpath(). 3) I broken lockdep with the fastpath changes.. we used to only take the slowpath with debugging, so only the slow paths are annotated. Now we uncondtionally use the fast paths.
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Tue, Aug 23, 2016 at 01:52:46PM -0700, Tim Chen wrote:
> On Tue, 2016-08-23 at 15:55 -0400, Waiman Long wrote:
> > On 08/23/2016 08:46 AM, Peter Zijlstra wrote:
> >
> > I have 2 more comments about the code.
> > 1) There are a couple of places where you only use 0x3 in mutex.c. They
> > should be replaced by the symbolic name instead.
>
> May be easier to read if (owner & 0x3) and
> (owner & ~0x3) are changed to something like
> _owner_flag(owner) and _owner_task(owner).
Yes that would work. Something like the below..
Note the ';' in the last "-" line that currently ensures we always take
the slow path.
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -55,7 +55,17 @@ EXPORT_SYMBOL(__mutex_init);
#define MUTEX_FLAG_WAITERS 0x01
#define MUTEX_FLAG_HANDOFF 0x02
-#define MUTEX_FLAG_ALL 0x03
+#define MUTEX_FLAGS0x03
+
+static inline struct task_struct *__owner_task(unsigned long owner)
+{
+ return (struct task_struct *)(owner & ~MUTEX_FLAGS);
+}
+
+static inline unsigned long __owner_flags(unsigned long owner)
+{
+ return owner & MUTEX_FLAGS;
+}
/*
* Atomically try to take the lock when it is available
@@ -65,7 +75,7 @@ static inline bool __mutex_trylock(struc
unsigned long owner, new_owner;
owner = atomic_long_read(&lock->owner);
- if (owner & ~0x03)
+ if (__owner_task(owner))
return false;
new_owner = owner | (unsigned long)current;
@@ -466,14 +476,14 @@ void __sched mutex_unlock(struct mutex *
if (owner & MUTEX_FLAG_HANDOFF)
break;
- old = atomic_long_cmpxchg_release(&lock->owner, owner, owner &
0x03);
+ old = atomic_long_cmpxchg_release(&lock->owner, owner,
__owner_flags(owner));
if (old == owner)
break;
owner = old;
}
- if (owner & 0x03);
+ if (__owner_flags(owner))
__mutex_unlock_slowpath(lock, owner);
}
EXPORT_SYMBOL(mutex_unlock);
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Tue, 2016-08-23 at 15:55 -0400, Waiman Long wrote: > On 08/23/2016 08:46 AM, Peter Zijlstra wrote: > > I have 2 more comments about the code. > 1) There are a couple of places where you only use 0x3 in mutex.c. They > should be replaced by the symbolic name instead. May be easier to read if (owner & 0x3) and (owner & ~0x3) are changed to something like _owner_flag(owner) and _owner_task(owner). Tim > 2) We should make __mutex_lock_slowpath() a noinline function just like > __mutex_lock_killable_slowpath() or __mutex_lock_interruptible_slowpath(). > > Cheers, > Longman
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On 08/23/2016 08:46 AM, Peter Zijlstra wrote:
/*
* Simple, straightforward mutexes with strict semantics:
@@ -48,13 +49,9 @@
* locks and tasks (and only those tasks)
*/
struct mutex {
- /* 1: unlocked, 0: locked, negative: locked, possible waiters */
- atomic_tcount;
+ atomic_long_t owner;
spinlock_t wait_lock;
struct list_headwait_list;
-#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
- struct task_struct *owner;
-#endif
#ifdef CONFIG_MUTEX_SPIN_ON_OWNER
struct optimistic_spin_queue osq; /* Spinner MCS lock */
#endif
I think you should put the wait_lock and osq next to each other to save
8 bytes in space on 64-bit machines.
Cheers,
Longman
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On Tue, Aug 23, 2016 at 04:17:54PM -0400, Waiman Long wrote:
> On 08/23/2016 08:46 AM, Peter Zijlstra wrote:
> > /*
> > * Simple, straightforward mutexes with strict semantics:
> >@@ -48,13 +49,9 @@
> > * locks and tasks (and only those tasks)
> > */
> > struct mutex {
> >-/* 1: unlocked, 0: locked, negative: locked, possible waiters */
> >-atomic_tcount;
> >+atomic_long_t owner;
> > spinlock_t wait_lock;
> > struct list_headwait_list;
> >-#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
> >-struct task_struct *owner;
> >-#endif
> > #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
> > struct optimistic_spin_queue osq; /* Spinner MCS lock */
> > #endif
>
> I think you should put the wait_lock and osq next to each other to save 8
> bytes in space on 64-bit machines.
Right you are.. didn't get around to looking at layout yet. Just barely
got it to compile and boot :-)
Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner
On 08/23/2016 08:46 AM, Peter Zijlstra wrote: I have 2 more comments about the code. 1) There are a couple of places where you only use 0x3 in mutex.c. They should be replaced by the symbolic name instead. 2) We should make __mutex_lock_slowpath() a noinline function just like __mutex_lock_killable_slowpath() or __mutex_lock_interruptible_slowpath(). Cheers, Longman
