Re: [PATCH] kernel/locking/semaphore: use wake_q in up()

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Fri, May 10, 2019 at 7:50 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (05/09/19 22:06), Daniel Vetter wrote:
> [..]
> > +/* Functions for the contended case */
> > +
> > +struct semaphore_waiter {
> > +     struct list_head list;
> > +     struct task_struct *task;
> > +     bool up;
> > +};
> > +
> >  /**
> >   * up - release the semaphore
> >   * @sem: the semaphore to release
> > @@ -179,24 +187,25 @@ EXPORT_SYMBOL(down_timeout);
> >  void up(struct semaphore *sem)
> >  {
> >       unsigned long flags;
> > +     struct semaphore_waiter *waiter;
> > +     DEFINE_WAKE_Q(wake_q);
> >
> >       raw_spin_lock_irqsave(&sem->lock, flags);
> > -     if (likely(list_empty(&sem->wait_list)))
> > +     if (likely(list_empty(&sem->wait_list))) {
> >               sem->count++;
> > -     else
> > -             __up(sem);
> > +     } else {
> > +             waiter =  list_first_entry(&sem->wait_list,
> > +                                        struct semaphore_waiter, list);
> > +             list_del(&waiter->list);
> > +             waiter->up = true;
> > +             wake_q_add(&wake_q, waiter->task);
> > +     }
> >       raw_spin_unlock_irqrestore(&sem->lock, flags);
>
> So the new code still can printk/WARN under sem->lock in some buggy
> cases.
>
> E.g.
>         wake_q_add()
>          get_task_struct()
>           refcount_inc_checked()
>            WARN_ONCE()
>
> Are we fine with that?

Hm not great. It's not as bad as the one I'm trying to fix (or not the
same at least), because with the wake up chain we have a few locks in
there. Which allows lockdep to connect the loop and complain, even
when we never actually hit that specific recursion. I.e. once hitting
a WARN_ON from try_to_wake_up is enough, plus a totally separate
callchain can then close the semaphore.lock->scheduler locks part.
Your chain only goes boom if it happens from the console_lock's up.

wake_q_add_safe would be an option, but then we somehow need to
arrange for down to call get_task_struct(current) and releasing that,
but only if there's no waker who needs that task ref. Sounds tricky
... Also not sure we want to stuff that trickery into the generic
semaphore code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch