Re: [patch 05/10] PI-futex: rt-mutex core

[Andrew Morton <akpm@osdl.org>]

Ingo Molnar <mingo@elte.hu> wrote:
>
> From: Ingo Molnar <mingo@elte.hu>
> 
> +/*
> + * The rt_mutex structure
> + *
> + * @wait_lock:	spinlock to protect the structure
> + * @wait_list:	pilist head to enqueue waiters in priority order
> + * @owner:	the mutex owner
> + */
> +struct rt_mutex {
> +	spinlock_t		wait_lock;
> +	struct plist_head	wait_list;
> +	struct task_struct	*owner;
> +# ifdef CONFIG_DEBUG_RT_MUTEXES
> +	int			save_state;
> +	struct list_head	held_list;
> +	unsigned long		acquire_ip;
> +	const char 		*name, *file;
> +	int			line;
> +	void			*magic;
> +# endif
> +};

The indented #-statments make some sense when we're using nested #ifs
(although I tend to accidentally-on-purpose delete them).  But the above
ones aren't even nested..

> +extern void fastcall __rt_mutex_init(struct rt_mutex *lock, const char *name);
> +extern void fastcall rt_mutex_destroy(struct rt_mutex *lock);
> +
> +extern void fastcall rt_mutex_lock(struct rt_mutex *lock);
> +extern int fastcall rt_mutex_lock_interruptible(struct rt_mutex *lock,
> +						int detect_deadlock);
> +extern int fastcall rt_mutex_timed_lock(struct rt_mutex *lock,
> +					struct hrtimer_sleeper *timeout,
> +					int detect_deadlock);
> +
> +extern int fastcall rt_mutex_trylock(struct rt_mutex *lock);
> +
> +extern void fastcall rt_mutex_unlock(struct rt_mutex *lock);

Does fastcall actually do any good?  Isn't CONFIG_REGPARM equivalent to
that anyway?   It's a bit of an eyesore.

> +#ifdef CONFIG_RT_MUTEXES
> +# define rt_mutex_init_task(p)						\
> + do {									\
> +	spin_lock_init(&p->pi_lock);					\
> +	plist_head_init(&p->pi_waiters);				\
> +	p->pi_blocked_on = NULL;					\
> +	p->pi_locked_by = NULL;						\
> +	INIT_LIST_HEAD(&p->pi_lock_chain);				\
> + } while (0)

Somewhere in there is a C function struggling to escape.

> Index: linux-pi-futex.mm.q/include/linux/rtmutex_internal.h

Perhaps this could go in kernel/.  If you think that's valuable.

> +/*
> + * Plist wrapper macros
> + */
> +#define rt_mutex_has_waiters(lock)	(!plist_head_empty(&lock->wait_list))
> +
> +#define rt_mutex_top_waiter(lock) 	\
> +({ struct rt_mutex_waiter *__w = plist_first_entry(&lock->wait_list, \
> +					struct rt_mutex_waiter, list_entry); \
> +	BUG_ON(__w->lock != lock);	\
> +	__w;				\
> +})
> +
> +#define task_has_pi_waiters(task)	(!plist_head_empty(&task->pi_waiters))
> +
> +#define task_top_pi_waiter(task) 	\
> +	plist_first_entry(&task->pi_waiters, struct rt_mutex_waiter, pi_list_entry)

All of these can become C functions, yes?

> +#define rt_mutex_owner(lock)						\
> +({									\
> +	typecheck(struct rt_mutex *,(lock));				\
> + 	((struct task_struct *)((unsigned long)((lock)->owner) & ~RT_MUTEX_OWNER_MASKALL)); \
> +})
> +
> +#define rt_mutex_real_owner(lock)					\
> +({									\
> +	typecheck(struct rt_mutex *,(lock));				\
> + 	((struct task_struct *)((unsigned long)((lock)->owner) & ~RT_MUTEX_HAS_WAITERS)); \
> +})
> +
> +#define rt_mutex_owner_pending(lock)					\
> +({									\
> +	typecheck(struct rt_mutex *,(lock));				\
> +	((unsigned long)((lock)->owner) & RT_MUTEX_OWNER_PENDING);	\
> +})

Bizarre.  The `typecheck' thingies were added, I assume, because these
macros really wanted to be C functions?

> +static inline void rt_mutex_set_owner(struct rt_mutex *lock, struct task_struct *owner,
> +				      unsigned long msk)
> +{
> +	unsigned long val = ((unsigned long) owner) | msk;
> +
> +	if (rt_mutex_has_waiters(lock))
> +		val |= RT_MUTEX_HAS_WAITERS;
> +
> +	lock->owner = (struct task_struct *)(val);
> +}

Might be getting a bit large for inlining.

> +/*
> + * Lock the full boosting chain.
> + *
> + * If 'try' is set, we have to backout if we hit a owner who is
> + * running its own pi chain operation. We go back and take the slow
> + * path via the pi_conflicts_lock.
> + */
> +static int lock_pi_chain(struct rt_mutex *act_lock,
> +			 struct rt_mutex_waiter *waiter,
> +			 struct list_head *lock_chain,
> +			 int try, int detect_deadlock)
> +{
> +	struct task_struct *owner;
> +	struct rt_mutex *nextlock, *lock = act_lock;
> +	struct rt_mutex_waiter *nextwaiter;
> +
> +	/*
> +	 * Debugging might turn deadlock detection on, unconditionally:
> +	 */
> +	detect_deadlock = debug_rt_mutex_detect_deadlock(detect_deadlock);
> +
> +	for (;;) {
> +		owner = rt_mutex_owner(lock);
> +
> +		/* Check for circular dependencies */
> +		if (unlikely(owner->pi_locked_by == current)) {
> +			debug_rt_mutex_deadlock(detect_deadlock, waiter, lock);
> +			return detect_deadlock ? -EDEADLK : 0;
> +		}
> +
> +		while (!spin_trylock(&owner->pi_lock)) {
> +			/*
> +			 * Owner runs its own chain. Go back and take
> +			 * the slow path
> +			 */
> +			if (try && owner->pi_locked_by == owner)
> +				return -EBUSY;
> +			cpu_relax();
> +		}
> +
> +		BUG_ON(owner->pi_locked_by);
> +		owner->pi_locked_by = current;
> +		BUG_ON(!list_empty(&owner->pi_lock_chain));
> +		list_add(&owner->pi_lock_chain, lock_chain);
> +
> +		/*
> +		 * When the owner is blocked on a lock, try to take
> +		 * the lock:
> +		 */
> +		nextwaiter = owner->pi_blocked_on;
> +
> +		/* End of chain? */
> +		if (!nextwaiter)
> +			return 0;

We return zero with the spinlock held?  I guess that's the point of the
whole function.


> +		nextlock = nextwaiter->lock;
> +
> +		/* Check for circular dependencies: */
> +		if (unlikely(nextlock == act_lock ||
> +			     rt_mutex_owner(nextlock) == current)) {
> +			debug_rt_mutex_deadlock(detect_deadlock, waiter,
> +						nextlock);
> +			list_del_init(&owner->pi_lock_chain);
> +			owner->pi_locked_by = NULL;
> +			spin_unlock(&owner->pi_lock);
> +			return detect_deadlock ? -EDEADLK : 0;
> +		}

But here we can return zero without having locked anything.  How does the
caller know what locks are held?

This function needs a better covering description, IMO.

> +		/* Try to get nextlock->wait_lock: */
> +		if (unlikely(!spin_trylock(&nextlock->wait_lock))) {
> +			list_del_init(&owner->pi_lock_chain);
> +			owner->pi_locked_by = NULL;
> +			spin_unlock(&owner->pi_lock);
> +			cpu_relax();
> +			continue;
> +		}

All these trylocks and cpu_relaxes are a worry.

> +/*
> + * Do the priority (un)boosting along the chain:
> + */
> +static void adjust_pi_chain(struct rt_mutex *lock,
> +			    struct rt_mutex_waiter *waiter,
> +			    struct rt_mutex_waiter *top_waiter,
> +			    struct list_head *lock_chain)
> +{
> +	struct task_struct *owner = rt_mutex_owner(lock);
> +	struct list_head *curr = lock_chain->prev;
> +
> +	for (;;) {
> +		if (top_waiter)
> +			plist_del(&top_waiter->pi_list_entry,
> +				  &owner->pi_waiters);
> +
> +		if (waiter && waiter == rt_mutex_top_waiter(lock)) {

rt_mutex_top_waiter() can never return NULL, so the test for NULL could be
removed.

> +/*
> + * Slow path lock function:
> + */
> +static int fastcall noinline __sched
> +rt_mutex_slowlock(struct rt_mutex *lock, int state,
> +		  struct hrtimer_sleeper *timeout,
> +		  int detect_deadlock __IP_DECL__)
> +{

heh, fastcall slowpath.  Why's it noinline?

> +	struct rt_mutex_waiter waiter;
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	debug_rt_mutex_init_waiter(&waiter);
> +	waiter.task = NULL;
> +
> +	spin_lock_irqsave(&lock->wait_lock, flags);
> +
> +	/* Try to acquire the lock again: */
> +	if (try_to_take_rt_mutex(lock __IP__)) {
> +		spin_unlock_irqrestore(&lock->wait_lock, flags);
> +		return 0;
> +	}
> +
> +	BUG_ON(rt_mutex_owner(lock) == current);
> +
> +	set_task_state(current, state);

set_current_state()  (Several more below)

> +void fastcall __rt_mutex_init(struct rt_mutex *lock, const char *name)
> +{
> +	lock->owner = NULL;
> +	spin_lock_init(&lock->wait_lock);
> +	plist_head_init(&lock->wait_list);
> +
> +	debug_rt_mutex_init(lock, name);
> +}
> +EXPORT_SYMBOL_GPL(__rt_mutex_init);

What's the export for?


Anyway.  Tricky-looking stuff.