LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will.deacon@arm.com>,
	Petr Mladek <pmladek@suse.com>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	John Ogness <john.ogness@linutronix.de>,
	Chris Wilson <chris@chris-wilson.co.uk>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] RFC: console: hack up console_lock more v3
Date: Thu, 9 May 2019 15:06:09 +0200	[thread overview]
Message-ID: <CAKMK7uGuOFGEuw1m_fiBfGbAEY0eeoDEFtP7Htt8-RCzD66MGw@mail.gmail.com> (raw)
In-Reply-To: <20190509123104.GQ2589@hirez.programming.kicks-ass.net>

On Thu, May 9, 2019 at 2:31 PM Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, May 09, 2019 at 02:09:03PM +0200, Daniel Vetter wrote:
> > Fix this by creating a prinkt_safe_up() which calls wake_up_process
> > outside of the spinlock. This isn't correct in full generality, but
> > good enough for console_lock:
> >
> > - console_lock doesn't use interruptible or killable or timeout down()
> >   calls, hence an up() is the only thing that can wake up a process.
>
> Wrong :/ Any task can be woken at any random time. We must, at all
> times, assume spurious wakeups will happen.

Out of curiosity, where do these come from? I know about the races
where you need to recheck on the waiter side to avoid getting stuck,
but didn't know about this. Are these earlier (possibly spurious)
wakeups that got held up and delayed for a while, then hit the task
much later when it's already continued doing something else? Or even
more random, and even if I never put a task on a wait list or anything
else, ever, it can get woken spuriously?

> > +void printk_safe_up(struct semaphore *sem)
> > +{
> > +     unsigned long flags;
> > +     struct semaphore_waiter *waiter = NULL;
> > +
> > +     raw_spin_lock_irqsave(&sem->lock, flags);
> > +     if (likely(list_empty(&sem->wait_list))) {
> > +             sem->count++;
> > +     } else {
> > +             waiter = list_first_entry(&sem->wait_list,
> > +                                       struct semaphore_waiter, list);
> > +             list_del(&waiter->list);
> > +             waiter->up = true;
> > +     }
> > +     raw_spin_unlock_irqrestore(&sem->lock, flags);
> > +
> > +     if (waiter) /* protected by being sole wake source */
> > +             wake_up_process(waiter->task);
> > +}
> > +EXPORT_SYMBOL(printk_safe_up);
>
> Since its only used from printk, that EXPORT really isn't needed.
>
> Something like the below might work.

Yeah that looks like the proper fix. I guess semaphores are uncritical
enough that we can roll this out for everyone. Thanks for the hint.
-Daniel

>
> ---
>  kernel/locking/semaphore.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
> index 561acdd39960..ac0a67e95aac 100644
> --- a/kernel/locking/semaphore.c
> +++ b/kernel/locking/semaphore.c
> @@ -38,7 +38,6 @@ static noinline void __down(struct semaphore *sem);
>  static noinline int __down_interruptible(struct semaphore *sem);
>  static noinline int __down_killable(struct semaphore *sem);
>  static noinline int __down_timeout(struct semaphore *sem, long timeout);
> -static noinline void __up(struct semaphore *sem);
>
>  /**
>   * down - acquire the semaphore
> @@ -178,14 +177,24 @@ EXPORT_SYMBOL(down_timeout);
>   */
>  void up(struct semaphore *sem)
>  {
> +       struct semaphore_waiter *waiter;
> +       DEFINE_WAKE_Q(wake_q);
>         unsigned long flags;
>
>         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);
> +               goto unlock;
> +       }
> +
> +       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);
> +unlock:
>         raw_spin_unlock_irqrestore(&sem->lock, flags);
> +
> +       wake_up_q(&wake_q);
>  }
>  EXPORT_SYMBOL(up);
>
> @@ -252,12 +261,3 @@ static noinline int __sched __down_timeout(struct semaphore *sem, long timeout)
>  {
>         return __down_common(sem, TASK_UNINTERRUPTIBLE, timeout);
>  }
> -
> -static noinline void __sched __up(struct semaphore *sem)
> -{
> -       struct semaphore_waiter *waiter = list_first_entry(&sem->wait_list,
> -                                               struct semaphore_waiter, list);
> -       list_del(&waiter->list);
> -       waiter->up = true;
> -       wake_up_process(waiter->task);
> -}



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

  reply	other threads:[~2019-05-09 13:06 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-09 12:09 Daniel Vetter
2019-05-09 12:21 ` Chris Wilson
2019-05-09 12:31 ` Peter Zijlstra
2019-05-09 13:06   ` Daniel Vetter [this message]
2019-05-09 13:36     ` Peter Zijlstra
2019-05-09 14:56 ` Petr Mladek
2019-05-09 16:43   ` Daniel Vetter
2019-05-10  9:15     ` Petr Mladek
2019-05-10  9:51       ` Sergey Senozhatsky
2019-05-10 15:05       ` Daniel Vetter
2019-05-09 20:06 ` [PATCH] kernel/locking/semaphore: use wake_q in up() Daniel Vetter
2019-05-10  5:50   ` Sergey Senozhatsky
2019-05-10  7:51     ` Daniel Vetter
2019-05-10  9:28   ` Petr Mladek
2019-05-10 15:20     ` Daniel Vetter
2019-05-15 11:53       ` Petr Mladek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKMK7uGuOFGEuw1m_fiBfGbAEY0eeoDEFtP7Htt8-RCzD66MGw@mail.gmail.com \
    --to=daniel.vetter@ffwll.ch \
    --cc=chris@chris-wilson.co.uk \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=john.ogness@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=will.deacon@arm.com \
    --subject='Re: [PATCH] RFC: console: hack up console_lock more v3' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).