Re: [patch 05/15] Generic Mutex Subsystem, mutex-core.patch

[Oleg Nesterov <oleg@tv-sign.ru>]

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.