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.041 / U:0.040 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site