lkml.org 
[lkml]   [2005]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 05/15] Generic Mutex Subsystem, mutex-core.patch
Ingo Molnar wrote:
>
> mutex implementation, core files: just the basic subsystem, no users of it.

Ingo, could you explain to me ...

> +__mutex_lock_common(struct mutex *lock, struct mutex_waiter *waiter,
> + struct thread_info *ti, struct task_struct *task,
> + unsigned long *flags, unsigned long task_state __IP_DECL__)
> +{
> + unsigned int old_val;
> +
> + debug_lock_irqsave(&debug_lock, *flags, ti);
> + DEBUG_WARN_ON(lock->magic != lock);
> +
> + spin_lock(&lock->wait_lock);
> + __add_waiter(lock, waiter, ti, task __IP__);
> + set_task_state(task, task_state);

I can't understand why __mutex_lock_common() does xchg() after
adding the waiter to the ->wait_list. We are holding ->wait_lock,
we can't race with __mutex_unlock_nonatomic() - it calls wake_up()
and sets ->count under this spinlock.

So, I think it can be simplified:

int __mutex_lock_common(lock, waiter)
{
lock(&lock->wait_lock);
ret = 1;
if (xchg(&lock->count, -1) == 1)
goto out;
__add_waiter(lock, waiter);
task->state = state;

ret = 0;
out:
unlock(&lock->wait_lock);
return ret;
}
No?

> +__mutex_wakeup_waiter(struct mutex *lock __IP_DECL__)
> +{
> + struct mutex_waiter *waiter;
> ...
> + if (!waiter->woken) {
> + waiter->woken = 1;
> + wake_up_process(waiter->ti->task);
> + }

Is it optimization? If yes - why? From mutex.h:

- only one task can hold the mutex at a time
- only the owner can unlock the mutex

So, how can this help?

> +start_mutex_timer(struct timer_list *timer, unsigned long time,
> + unsigned long *expire)
> +{
> + *expire = time + jiffies;
> + init_timer(timer);
> + timer->expires = *expire;
> + timer->data = (unsigned long)current;
> + timer->function = process_timeout;
> + add_timer(timer);
> +}

How about
setup_timer(&timer, process_timeout, (unsigned long)current);
__mod_timer(&timer, *expire);
?

> +stop_mutex_timer(struct timer_list *timer, unsigned long time,
> + unsigned long expire)
> +{
> + int ret;
> +
> + ret = (int)(expire - jiffies);
> + if (!timer_pending(timer)) {
> + del_singleshot_timer_sync(timer);
> + ret = -ETIMEDOUT;
> + }

Did you mean

if (!timer_pending(timer))
ret = -ETIMEDOUT;
del_singleshot_timer_sync(timer);
?
> +__mutex_lock_interruptible(struct mutex *lock, unsigned long time __IP_DECL__)
> +{
> + struct thread_info *ti = current_thread_info();
> + struct task_struct *task = ti->task;
> + unsigned long expire = 0, flags;
> + struct mutex_waiter waiter;
> + struct timer_list timer;
> + int ret;
> +
> +repeat:
> + if (__mutex_lock_common(lock, &waiter, ti, task, &flags,
> + TASK_INTERRUPTIBLE __IP__))
> + return 0;

I think this is wrong. We may have pending timer here if we were woken
by signal.

Oleg.
-
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/

\
 
 \ /
  Last update: 2005-12-21 16:08    [W:0.130 / U:0.152 seconds]
©2003-2011 Jasper Spaans. Advertise on this site