[GIT PULL] printk: Support for full dynticks mode

[GIT PULL] printk: Support for full dynticks mode

[Frederic Weisbecker]

Ingo,

Please pull the support for full dynticks idle mode in printk that
can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	nohz/printk-v9

nohz/printk-v8 was the tree that I proposed to Linus. It has been left unpulled
though and we got no news from him but the tree has stayed untouched since then,
waiting in -next without any issue.

Part of nohz/printk-v8 is now in tip:irq/core, and this pull request contains
the rest. I could simply propose you the old branch but since there is a tiny
conflict with another recent commit in tip:irq/core, I just made a new branch
that contains tip:irq/core with nohz/printk-v8 merged inside which deals
with the conflict.

Thanks.
---

Frederic Weisbecker (5):
      nohz: Add API to check tick state
      irq_work: Don't stop the tick with pending works
      irq_work: Make self-IPIs optable
      printk: Wake up klogd using irq_work
      Merge branch 'nohz/printk-v8' into irq/core

Steven Rostedt (2):
      irq_work: Flush work on CPU_DYING
      irq_work: Warn if there's still work on cpu_down

 include/linux/irq_work.h |   20 ++++++++
 include/linux/printk.h   |    3 -
 include/linux/tick.h     |   17 +++++++-
 init/Kconfig             |    1 +
 kernel/irq_work.c        |  112 +++++++++++++++++++++++++++++++++++-----------
 kernel/printk.c          |   36 ++++++++-------
 kernel/time/tick-sched.c |    7 ++-
 kernel/timer.c           |    1 -
 8 files changed, 147 insertions(+), 50 deletions(-)

Re: [GIT PULL] printk: Support for full dynticks mode

[Andrew Morton]

On Tue,  5 Feb 2013 01:51:18 +0100
Frederic Weisbecker <fweisbec@gmail.com> wrote:

>       printk: Wake up klogd using irq_work

That seems reasonable.

I'm wondering if we can now remove the printk_sched() special-case. 
iirc, that was needed because wake_up(klogd) would deadlock when called
from sched internals.  But now that wakeup is punted to the timer tick,
perhaps this is now unnecessary?

(Maybe there were other problems, but the 3ccf3e8306 is poor, and its
author is unavailable.  There's a lesson there!)

Re: [GIT PULL] printk: Support for full dynticks mode

[Steven Rostedt]

On Mon, 2013-02-04 at 17:20 -0800, Andrew Morton wrote:
> On Tue,  5 Feb 2013 01:51:18 +0100
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> >       printk: Wake up klogd using irq_work
> 
> That seems reasonable.
> 
> I'm wondering if we can now remove the printk_sched() special-case. 
> iirc, that was needed because wake_up(klogd) would deadlock when called
> from sched internals.  But now that wakeup is punted to the timer tick,
> perhaps this is now unnecessary?
> 
> (Maybe there were other problems, but the 3ccf3e8306 is poor, and its
> author is unavailable.  There's a lesson there!)

I think we can nuke it with the printk irq work. I'll start looking into
that.

Thanks!

-- Steve

Re: [GIT PULL] printk: Support for full dynticks mode

[Frederic Weisbecker]

2013/2/5 Andrew Morton <akpm@linux-foundation.org>:
> On Tue,  5 Feb 2013 01:51:18 +0100
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
>>       printk: Wake up klogd using irq_work
>
> That seems reasonable.
>
> I'm wondering if we can now remove the printk_sched() special-case.
> iirc, that was needed because wake_up(klogd) would deadlock when called
> from sched internals.  But now that wakeup is punted to the timer tick,
> perhaps this is now unnecessary?

I fear irq work doesn't change much the picture in this regard. Before
irq work, printk() and printk_sched() were already relying on the tick
(printk_tick()) to do the wake up on klogd clients. I guess that the
deadlocks referenced by Peter were about printk() internals. May be
logbuf_lock against some other scheduler lock?

>
> (Maybe there were other problems, but the 3ccf3e8306 is poor, and its
> author is unavailable.  There's a lesson there!)

Re: [GIT PULL] printk: Support for full dynticks mode

[Andrew Morton]

On Tue, 5 Feb 2013 02:37:54 +0100 Frederic Weisbecker <fweisbec@gmail.com> wrote:

> 2013/2/5 Andrew Morton <akpm@linux-foundation.org>:
> > On Tue,  5 Feb 2013 01:51:18 +0100
> > Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >
> >>       printk: Wake up klogd using irq_work
> >
> > That seems reasonable.
> >
> > I'm wondering if we can now remove the printk_sched() special-case.
> > iirc, that was needed because wake_up(klogd) would deadlock when called
> > from sched internals.  But now that wakeup is punted to the timer tick,
> > perhaps this is now unnecessary?
> 
> I fear irq work doesn't change much the picture in this regard. Before
> irq work, printk() and printk_sched() were already relying on the tick
> (printk_tick()) to do the wake up on klogd clients. I guess that the
> deadlocks referenced by Peter were about printk() internals. May be
> logbuf_lock against some other scheduler lock?

I don't think so.  Conceptually printk() should be "inner" to the
scheduler and shouldn't call into sched things at all.  The (afaik
sole) exception to that was the klogd wakeup.

Traditionally the deadlock happened when calling printk() with
tasklist_lock (now q->lock) held.  printk() would call wake_up(klogd)
and wake_up() tries to take tasklist_lock and boom.  Moving the
wake_up() out to the tick "thread" fixed that.

Maybe there were other deadlock scenarios, dunno.  That knowledge
appears to be disappearing into the mists of time :(

Re: [GIT PULL] printk: Support for full dynticks mode

[Steven Rostedt]

On Mon, 2013-02-04 at 18:09 -0800, Andrew Morton wrote:

> I don't think so.  Conceptually printk() should be "inner" to the
> scheduler and shouldn't call into sched things at all.  The (afaik
> sole) exception to that was the klogd wakeup.
> 
> Traditionally the deadlock happened when calling printk() with
> tasklist_lock (now q->lock) held.  printk() would call wake_up(klogd)
> and wake_up() tries to take tasklist_lock and boom.  Moving the
> wake_up() out to the tick "thread" fixed that.
> 
> Maybe there were other deadlock scenarios, dunno.  That knowledge
> appears to be disappearing into the mists of time :(

Even without the printk irq_work the current printk method uses a
delayed wakeup anyway.

The wake_up_klogd() sets PRINTK_PENDING_WAKEUP, and the wakeup happens
at time of the tick. I don't see where there is a deadlock. I added a
printk in __sched_setscheduler() where the run queue lock is held, and
booted that with full lockdep debugging enabled. No deadlock is
detected.

Do we really even need that printk_sched()?

-- Steve

Re: [GIT PULL] printk: Support for full dynticks mode

[Andrew Morton]

On Mon, 04 Feb 2013 21:42:02 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 2013-02-04 at 18:09 -0800, Andrew Morton wrote:
> 
> > I don't think so.  Conceptually printk() should be "inner" to the
> > scheduler and shouldn't call into sched things at all.  The (afaik
> > sole) exception to that was the klogd wakeup.
> > 
> > Traditionally the deadlock happened when calling printk() with
> > tasklist_lock (now q->lock) held.  printk() would call wake_up(klogd)
> > and wake_up() tries to take tasklist_lock and boom.  Moving the
> > wake_up() out to the tick "thread" fixed that.
> > 
> > Maybe there were other deadlock scenarios, dunno.  That knowledge
> > appears to be disappearing into the mists of time :(
> 
> Even without the printk irq_work the current printk method uses a
> delayed wakeup anyway.
> 
> The wake_up_klogd() sets PRINTK_PENDING_WAKEUP, and the wakeup happens
> at time of the tick. I don't see where there is a deadlock.
>
> ...
>
> Do we really even need that printk_sched()?

3ccf3e8306156a282 ("printk/sched: Introduce special printk_sched() for
those awkward moments") was added _after_ wake_up_klogd() was switched
to using the printk_pending->printk_tick() thing.  So presumably there
were deadlocks other than around wake_up_klogd().

The printk_pending->printk_tick() thing was added by b845b517b5e
("printk: robustify printk"), four years earlier in 2008.  It says
"Avoid deadlocks against rq->lock and xtime_lock...".

So what deadlocks was the March 2012 3ccf3e830 ("printk/sched:
Introduce special printk_sched() for those awkward moments") supposed
to fix?  grr.  I searched my lkml archives for March 2012 and few
preceding months, but couldn't find any additional info.

> I added a printk in __sched_setscheduler() where the run queue lock is held,
> and booted that with full lockdep debugging enabled. No deadlock is
> detected.

Well, you'd need to enable various printk options to get full coverage.
For example, that xtime_lock deadlock would only have occurred when
timestamping is enabled.

Re: [GIT PULL] printk: Support for full dynticks mode

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> So what deadlocks was the March 2012 3ccf3e830 ("printk/sched: 
> Introduce special printk_sched() for those awkward moments") 
> supposed to fix? [...]

The wakeup in:

static int console_trylock_for_printk(unsigned int cpu)
...
        if (wake)
                up(&console_sem);

Thanks,

	Ingo

Re: [GIT PULL] printk: Support for full dynticks mode

[Steven Rostedt]

On Mon, 2013-02-04 at 18:09 -0800, Andrew Morton wrote:

> Maybe there were other deadlock scenarios, dunno.  That knowledge
> appears to be disappearing into the mists of time :(

Discussing this on IRC with Frederic, he brought up that the
up(&console_sem) can do a wake_up as well. I guess that's the issue we
have :-/

-- Steve

Re: [GIT PULL] printk: Support for full dynticks mode

[Frederic Weisbecker]

2013/2/5 Steven Rostedt <rostedt@goodmis.org>:
> On Mon, 2013-02-04 at 18:09 -0800, Andrew Morton wrote:
>
>> Maybe there were other deadlock scenarios, dunno.  That knowledge
>> appears to be disappearing into the mists of time :(
>
> Discussing this on IRC with Frederic, he brought up that the
> up(&console_sem) can do a wake_up as well. I guess that's the issue we
> have :-/

Also if we delay the wakeup on klogd but not on the console_sem
waiters, it looks like a general printk() problem. Not something that
only printk_sched() would like to avoid.

Re: [GIT PULL] printk: Support for full dynticks mode

[Steven Rostedt]

On Tue, 2013-02-05 at 04:19 +0100, Frederic Weisbecker wrote:
> 2013/2/5 Steven Rostedt <rostedt@goodmis.org>:
> > On Mon, 2013-02-04 at 18:09 -0800, Andrew Morton wrote:
> >
> >> Maybe there were other deadlock scenarios, dunno.  That knowledge
> >> appears to be disappearing into the mists of time :(
> >
> > Discussing this on IRC with Frederic, he brought up that the
> > up(&console_sem) can do a wake_up as well. I guess that's the issue we
> > have :-/
> 
> Also if we delay the wakeup on klogd but not on the console_sem
> waiters, it looks like a general printk() problem. Not something that
> only printk_sched() would like to avoid.

printk_sched() is a delay to printk() to avoid calling printk() which
might take the rq lock. It's to prevent printk() deadlocking in case it
wakes up. printk_sched() just delays the printk() to the next timer
tick.

The reason to avoid the wake up of klogd() is that it also grabs the
wait queue spinlock every time it's called. up() is fast when there's no
waiters, but wake_up_interruptible() is not so fast.

There's no issue calling wake_up_process() from printk() as long as you
don't do it while holding a rq lock.

-- Steve

Re: [GIT PULL] printk: Support for full dynticks mode

[Steven Rostedt]

On Mon, 2013-02-04 at 22:24 -0500, Steven Rostedt wrote:

> The reason to avoid the wake up of klogd() is that it also grabs the
> wait queue spinlock every time it's called. up() is fast when there's no
> waiters, but wake_up_interruptible() is not so fast.

I take that back. Seems up() takes the sem->lock every time too :-p

But it's just a matter of slowing things down. I don't see it causing
any deadlocks.

-- Steve

Re: [GIT PULL] printk: Support for full dynticks mode

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Ingo,
> 
> Please pull the support for full dynticks idle mode in printk that
> can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> 	nohz/printk-v9
> 
> nohz/printk-v8 was the tree that I proposed to Linus. It has been left unpulled
> though and we got no news from him but the tree has stayed untouched since then,
> waiting in -next without any issue.
> 
> Part of nohz/printk-v8 is now in tip:irq/core, and this pull request contains
> the rest. I could simply propose you the old branch but since there is a tiny
> conflict with another recent commit in tip:irq/core, I just made a new branch
> that contains tip:irq/core with nohz/printk-v8 merged inside which deals
> with the conflict.
> 
> Thanks.
> ---
> 
> Frederic Weisbecker (5):
>       nohz: Add API to check tick state
>       irq_work: Don't stop the tick with pending works
>       irq_work: Make self-IPIs optable
>       printk: Wake up klogd using irq_work
>       Merge branch 'nohz/printk-v8' into irq/core
> 
> Steven Rostedt (2):
>       irq_work: Flush work on CPU_DYING
>       irq_work: Warn if there's still work on cpu_down
> 
>  include/linux/irq_work.h |   20 ++++++++
>  include/linux/printk.h   |    3 -
>  include/linux/tick.h     |   17 +++++++-
>  init/Kconfig             |    1 +
>  kernel/irq_work.c        |  112 +++++++++++++++++++++++++++++++++++-----------
>  kernel/printk.c          |   36 ++++++++-------
>  kernel/time/tick-sched.c |    7 ++-
>  kernel/timer.c           |    1 -
>  8 files changed, 147 insertions(+), 50 deletions(-)

Pulled, thanks Frederic!

	Ingo