Messages in this thread | | | Date | Sat, 25 Mar 2006 22:07:28 -0800 | From | Andrew Morton <> | Subject | Re: [patch 05/10] PI-futex: rt-mutex core |
| |
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |