Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676610 > unrolled thread

Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Petr Mladek <pmladek@suse.com> - 2017-06-28 14:20 +0200
    Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Andreas Mohr <andi@lisas.de> - 2017-06-29 08:30 +0200
      Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Andreas Mohr <andi@lisas.de> - 2017-06-29 09:10 +0200
    Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-29 09:40 +0200
      Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-29 13:40 +0200
      Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-30 09:10 +0200
        Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-30 12:30 +0200
          Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-30 14:00 +0200
            Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-30 14:50 +0200
              Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-30 15:00 +0200
        Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Petr Mladek <pmladek@suse.com> - 2017-06-30 15:20 +0200
          Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-30 15:40 +0200
            Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-07-03 13:20 +0200
          Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-30 16:20 +0200
      Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Petr Mladek <pmladek@suse.com> - 2017-06-30 14:00 +0200
        Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-30 14:50 +0200
          Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Steven Rostedt <rostedt@goodmis.org> - 2017-06-30 15:40 +0200
            Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-06-30 16:30 +0200
              Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Steven Rostedt <rostedt@goodmis.org> - 2017-06-30 16:50 +0200
                Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2017-07-01 04:00 +0200

#1676610 — Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread

On Wed 2017-05-31 16:22:33, Sergey Senozhatsky wrote:
> Hello Petr,
> 
> On (05/29/17 11:29), Petr Mladek wrote:
> [..]
> > > +static inline bool console_offload_printing(void)
> > > +{
> > > +	 * A trivial emergency enforcement - give up on printk_kthread if
> > > +	 * we can't wake it up. This assumes that `atomic_print_limit' is
> > > +	 * reasonably and sufficiently large.
> > > +	 */
> > > +	if (lines_printed > 10 * (unsigned long long)atomic_print_limit &&
> > > +			saved_csw == (current->nvcsw + current->nivcsw)) {
> > > +		printk_enforce_emergency = true;
> > > +		pr_crit("Declaring printk emergency mode.\n");
> > > +		return false;
> > > +	}
> > 
> > It might work only if the waken printk kthread is scheduled in a predictable
> > time period.
> 
> > You try to achieve this by setting real time priority
> 
> yes, that's the reason.
> 
> > (4th patch of this patchset). The question is how this would work:
> 
> > First, the real time priority is questionable on its own. Logging
> > is important but the real time priority is dangerous. Any "flood"
> > of messages will starve all other processes with normal priority.
> > It is better than a softlockup but it might cause problems as well.
> 
> so I try to minimize the negative impact of RT prio here. printk_kthread
> is not special any more. it's an auxiliary kthread that we sometimes
> wake_up. the thing is that printk_kthread also must offload at some
> point, basically the same `atomic_print_limit' limit applies to it as
> well.

You might call cond_resched() outside console_unlock(). But you have
to keep printk_kthread in runnable state as long as there are pending
messages. Then scheduler will always prefer this RT task over non-RT
tasks. Or am I wrong?

The more I think about it, the more I am persuaded that RT priority
is no no for printk_kthread.


> > Third, if the console_trylock() fails below, it is not guaranteed that
> > the other waiter will really continue printing. The console_trylock()
> > would fail because someone else called console_lock(), failed to get it,
> > and went into sleep. There is no guarantee that it will be waken
> > once again.
> 
> please explain, why wouldn't it? we call up() after every line we print
> once the task exceeds `atomic_print_limit'. you mean there probably
> won't be any tasks in console_sem wait list? just for that case we have
> 'if (current == printk_kthread)' branch. `printk_kthread' can indeed be
> the one and only task to down() the console_sem.

I think that I was unclear and talked only about one possibility.

console_trylock() fails if anyone else already owns the lock
or if anyone else is in the wait list. I talked about the 2nd
possibility. But both ways look tricky:


1. We are talking about flood of messages. printk() does
   console_trylock() and does not add itself into the wait list.

   It means that another printk() could take over the console
   stuff only when its console_trylock() is called in the small
   window when console_unlock() releases the lock:

   CPU0				CPU1

   console_unlock()		printk()
    //handle few lines		// store message into log buffer
    up_console_sem()
				console_trylock()

    if (pending_output)
      console_trylock()

   The printk() on CPU1 got console_lock only because it called
   console_trylock() in the right time. This looks too tricky
   to me.

   Note that the interleaving of the two processes is complicated
   also because they both take logbug_lock. It might affect the
   chances as well (in possive or negative way).


2. There are only few functions that call console_lock() and
   add itself into the wait queue.

   They will get the console_lock directly only when they race
   the same way as printk() described above.

   The more likely scenario is that they will fail to get
   console_lock because someone else is in console_unlock().
   They will add itself into the wait queue and get into
   sleep.

   Then up_console_sem() will set TASK_RUNNING for the waiting
   process and add it to the run queue. The console_trylock()
   in console_unlock() will fail because the lock is assigned
   to the waiter. But the waiter is not running. It is just
   in the run queue. There is no quarantee how soon it will
   get scheduled. It might not happen at all if the system
   is in bad state.


> >       + slow => async =>
> > 		+ might be far behind
> > 		+ loosing messages
> > 		+ showing only ones with high log level
> > 		+ possible softlockup
> > 		+ not guaranteed when dying
> 
> yeah. good summary. the previous async printk implementation...
> we officially hate it now.
> 
> [..]
> > The patch tries to reduce the negative impact by detecting when
> > the negative impact is visible and switching back to the sync mode.
> 
> um, sorry, no. or I simply don't follow. the patch keeps the existing sync
> mode and switches to async mode when the problem probably can show up. we
> don't work in async mode by default. only when task reaches atomic_print_limit.

I talked about situation where the async mode was active and we tried
to detect emergency mode and forced the sync mode again.


> [..]
> >   + I would suggest to detect flood of messages by a difference
> >     between log_next_seq and console_seq. We could take into
> >     account the number of messages in the buffer
> >     (log_next_seq - log_first_seq)
> 
> yeah, I think I already posted a patch that would do exactly this
> thing. the problem with printk flood is ... if you have at least 2 CPUs
> flooding logbuf ... other CPU is still enjoying ... just log_store()

Just thinking loudly. I wonder if we could somehow detect this and
throttle such processes in printk(). I mean to make them spin there
for a bit ;-) Well, I would leave this for later optimizations.


> > In addition, I think that a semi-sync-async mode might better
> > handle the sudden dead. What is it? I have already talked about
> > something similar last time.
> 
> but this is what the patch does. but default we are in sync mode.
> assuming that `atomic_print_limit' is large enough it can take
> seconds before we offload printing to printk_kthread. so that
> semi-sync-async is what we are trying to have. am I missing something?

Perfect. I am really sorry, I was too deep in thinking and missed how
close the two proposals actually were.


> and that's why I'm *a bit* (just *really a bit*) less concerned about cases
> that we missed. I expect that `atomic_print_limit' usually will be around
> thousands of lines, if not tens of thousands, giving enough time to sync
> printing.

Well, the limit must be low enough to prevent soft lockup. In fact, it
should be low enough to keep almost normal scheduling rythm. It does
not make sense to block "innoncent" processes for seconds when they do
important job and just by chance printed their rare message.

On the other hand, it should be big enough to allow flushing
some average log, e.g. lockdep, sysrq report.

I might be too optimistic. Maybe it is not possible to tune it
this way.

> > Normal printk() should do something like:
> > 
> >   printk()
> >     vprintk_emit()
> >       log_store()
> >       if (!deferred)
> > 	console_trylock()
> > 	console_unlock()
> > 	  while (num_lines < atomic_print_limit)
> > 	    handle message
> > 	  up_console_sem()
> > 	  if (pending_messages)
> > 	    wakeup(printk_kthread)
> 
> this is close to what we do.

I see it now.

> at the same we have better guarantees.
> we don't just wakeup(printk_kthread) and leave. we wait for any other
> process to re-take the console_sem. until this happens we can't leave
> console_unlock().

And this is my problem. I am scared of the waiting. It is very hard
to predict, especially without RT priority. But it is tricky anyway,
see above.

It makes it very hard to tune the emergency situation detection.
If we enable the emergency mode too easily, the soft lockups will
be back again.

In addition, it adds another dependecy on the scheduler behavior.
It is a can of worms as explained by Jan Kara in another mail.

> 
> > and printk_kthread should do:
> > 
> >    while() {
> > 
> >        if (!pending_messages)
> > 	 sleep()
> > 
> >        console_lock()
> > 
> >        preempt_disable()
> >        console_unlock()
> > 	 while (pending_message && !need_resched)
> > 	   handle message
> > 	 up_console_sem()
> >        preempt_enable()
> >     }
> 
> hm. I don't want printk_kthread to be special. just because there are cases
> when printk_kthread won't be there. we had too many problems with relying on
> printk_kthread in all the corner cases. I want printk_kthread to be just one
> extra process that can do the printing for us.
> if we have X tasks sleeping in UNINTERRUPTIBLE on console_sem then
> we better use them; keeping them in UNINTERRUPTIBLE as long as
> printk_kthread has pending messages does no good.

I am a bit confused by this paragraph. What exactly makes the
printk_kthread special in my proposal? Is the main problem
that it checks need_resched instead of atomic_print_limit?

You are right that it should not block other waiters. And I forgot
to take other console_lock() callers into account.

My motivation was exactly the opposite. I wanted to reduce
unnecessary up_console_sem()/down_console_sem() dance
and slow down console output by this. The printk_kthread
is special after all because its only dute is to flush the console.
Also it is the only process where we know for sure that it does
the job in normal context. It looked to me that it made sense
to use its assigned time slot as much as possible.

Anyway, this is just an optimization. Let's handle printk_kthread
like any other process for now.


Summary:

Our two proposals are very close after all. I suggest to make
the following changes in your patch:

  + Remove the waiting for another console_lock owner. It is
    too tricky.

  + Instead try to reduce sleeping with console_lock taken.
    Remove that cond_resched() from console_unlock(). Maybe
    even call console_trylock()/console_unlock() with
    disabled preemtion again. It will increase the chance
    that anyone else will continue handling the console.

  + keep the code to force sync mode in known emergency
    situations (halt, suspend, ...).


This way we should be good in all situations:

  + sudden death because we are in sync mode until atomic_limit
    is reached

  + flood of messages because printk() does not sleep with
    console_lock taken. Either someone is flushing console
    or any printk() call could continue flushing the console.

  + critical situations because we force the sync mode
    explicitely

How does that sound, please?

Best Regards,
Petr

PS: I am sorry for the late reply. I was deep in other things.
I wanted to anwer this with a clean head.

[toc] | [next] | [standalone]

#1677431

On Wed, Jun 28, 2017 at 02:19:25PM +0200, Petr Mladek wrote:
> On Wed 2017-05-31 16:22:33, Sergey Senozhatsky wrote:
> > so I try to minimize the negative impact of RT prio here. printk_kthread
> > is not special any more. it's an auxiliary kthread that we sometimes
> > wake_up. the thing is that printk_kthread also must offload at some
> > point, basically the same `atomic_print_limit' limit applies to it as
> > well.
> 
> You might call cond_resched() outside console_unlock(). But you have
> to keep printk_kthread in runnable state as long as there are pending
> messages. Then scheduler will always prefer this RT task over non-RT
> tasks. Or am I wrong?

Not sure whether I mentioned this advice before, but:
I believe we should strive to achieve a design where
cond_resched() etc. is *not* needed -
cond_resched() / sleep() etc. likely are signs of extended code smell:
one should strive to achieve handling which has
a properly *precisely*/*strictly* handshaked
request/response (producer/consumer) communication protocol.
I.e., IPC mechanism objects (mutex etc.).
That way, one avoids
the polling-type, imprecise-type "are we there yet? is it our job now?" handling
and instead uses
properly precise (thus, *not* needlessly inefficient!)
scheduler wakeup mechanisms.

Thus, it's "merely" (hah!) a matter of
designing handling where responsibilities / transitions are clearly spelt out,
thus end up as
properly precisely implemented notifications via IPC mechanisms.


For a very simple setup (which quite possibly cannot be done this easily!),
things could be:

printk_work_wakeup_one_worker()
{
  reliably_notify_only_one_available_computing_hardware_resource(); <-- kernel mechanism available, I think
}

void printk_work_dump_my_package(work_count_max)
{
  while (++work_count < work_count_max)
    printk_work_dump_element();
}


void printk_work_dump_handler(work_count_max)
{
  --> added mutex to have continuation check be done within *inner* atomic handling section (avoid race window).
  printk_work_dump_my_package(work_count_max);

  take_mutex();
  bool all_done = !have_payload_remain;
  bool need_continuation = !(all_done);
  if (need_continuation)
    printk_work_wakeup_one_worker();
  release_mutex();
}


worker thread frame function:
WORKER printk_work_worker()
{
  for (;;)
  {
    switch(select())
      case dump_requested:
        printk_work_dump_handler(printk_work_count_max_kthread);
      case termination_requested:
        return;
  }
}

/* tasked_cpus = all_cpus; */
tasked_cpus = all_cpus / 4 */
for(tasked_cpus)
{
  new_kthread(printk_work_worker());
}



printk_impl()
{
  printk_queue_element(...);
  printk_work_dump_handler(printk_work_count_max_immediate);
}

Very Q&D thoughts, but might be helpful...


(ermm, however launching #cpus workers most likely is useless,
since schedulable decision-making does *not* depend on #cpus -
if any computing resource is available, this *can* be executed, thus
quite likely only one worker is needed anyway - no, two, since
one worker needs to be able to wake up precisely *one* other
to have things precisely continue on *another* computing resource!)

Oh, and this doesn't implement
(and especially not reliably/atomically race-window-less)
the case of
having another activity trigger another printk queuing
and thus (potentially - but not necessarily!!) another package dump activity
while some worker activity already is ongoing.
I.e. we've got a race window in the (multi-)use of printk_work_dump_handler()
(which should be solvable, though).

Oh, and rather than doing some specific work_count limit comparisons,
it might actually be more elegant instead to
bundle worker work packages in *advance*,
to achieve simple/isolated/separate submission to
whichever worker one would prefer; inner handling would then simply do:

work_handler()
{
  while (!my_package_done)
    dump_package_element();
}

But splitting into separate work packages bears the risk of
illegal reordering of printk payload elements, thus
most likely one should *not* do this and instead keep doing
simple work_count checks on a *global*/*shared* printk queue payload
(insertion submission of further elements into this queue
must still be possible at any time
without (excessive) blocking, though, of course).

while (can_continue_dumping)
{
payload_mutex_begin();
element = grab_element();
payload_mutex_end();
dump_element(element);
}



payload_mutex_begin();
queue_element(element);
payload_mutex_end();

HTH,

Andreas Mohr

[toc] | [prev] | [next] | [standalone]

#1677451

On Thu, Jun 29, 2017 at 08:26:20AM +0200, Andreas Mohr wrote:
> On Wed, Jun 28, 2017 at 02:19:25PM +0200, Petr Mladek wrote:
> > On Wed 2017-05-31 16:22:33, Sergey Senozhatsky wrote:
> > > so I try to minimize the negative impact of RT prio here. printk_kthread
> > > is not special any more. it's an auxiliary kthread that we sometimes
> > > wake_up. the thing is that printk_kthread also must offload at some
> > > point, basically the same `atomic_print_limit' limit applies to it as
> > > well.
> > 
> > You might call cond_resched() outside console_unlock(). But you have
> > to keep printk_kthread in runnable state as long as there are pending
> > messages. Then scheduler will always prefer this RT task over non-RT
> > tasks. Or am I wrong?
> 
> Not sure whether I mentioned this advice before, but:
> I believe we should strive to achieve a design where
> cond_resched() etc. is *not* needed -
> cond_resched() / sleep() etc. likely are signs of extended code smell:
> one should strive to achieve handling which has
> a properly *precisely*/*strictly* handshaked
> request/response (producer/consumer) communication protocol.
> I.e., IPC mechanism objects (mutex etc.).
> That way, one avoids
> the polling-type, imprecise-type "are we there yet? is it our job now?" handling
> and instead uses
> properly precise (thus, *not* needlessly inefficient!)
> scheduler wakeup mechanisms.
> 
> Thus, it's "merely" (hah!) a matter of
> designing handling where responsibilities / transitions are clearly spelt out,
> thus end up as
> properly precisely implemented notifications via IPC mechanisms.

Big picture:

IMHO (I might be missing something, or even more)
this still is a simple producer/consumer type scenario:
- you've got any producer inserting payload elements into a *shared* (*global*) store
  shared store --> store mutex needed
- reordering is very undesirable --> keep as one *shared*/*global* data store
- producer side(!): in case of hitting a max limit, queuing of elements
  is discarded completely (emergency situation), BUT with a printk marker element LIMIT EXCEEDED
- consumer side:
  - you've got one central handler which
    simply knows nothing other (SEPARATION OF CONCERNS!) than
    how to reliably dequeue and dump (to console) elements,
    in *any* processing context
- producer side (as a special shortcut handling) will *also* subsequently turn to some consumption stuff
  "dequeue elements until a limit is reached"
  (here: with its special direct-context limit count)
- consumption side is specially constructed
  to have properly atomic inner handling of
  decision-making who is the one to dequeue the store
  at a particular moment in time


Thus, you want:
- reliable insertion at-any-moment
- one consistent/global/shared store
- properly precise scheduler decision-making on how the consumer side is done
- and of course keep concerns (printk user-side handling, printk payload
  element store handling, *thread-specific* worker context handling) properly
implementation-separate

I currently don't see any other complications to this.

HTH,

Andreas Mohr

[toc] | [prev] | [next] | [standalone]

#1677487

On (06/28/17 14:19), Petr Mladek wrote:
[..]
> > so I try to minimize the negative impact of RT prio here. printk_kthread
> > is not special any more. it's an auxiliary kthread that we sometimes
> > wake_up. the thing is that printk_kthread also must offload at some
> > point, basically the same `atomic_print_limit' limit applies to it as
> > well.
> 
> You might call cond_resched() outside console_unlock(). But you have
> to keep printk_kthread in runnable state as long as there are pending
> messages. Then scheduler will always prefer this RT task over non-RT
> tasks. Or am I wrong?

if we try to offload from IRQ->console_unlock() (or with preemption
disabled, etc. etc.) and scheduler decides to enqueue printk_kthread
on the same CPU, then no offloading will take place. I can reproduce
it on my system. we need to play some affinity games, I think. but
there are corner cases, once again.


> The more I think about it, the more I am persuaded that RT priority
> is no no for printk_kthread.

yeah, in fact, it didn't work as expected. so I dropped that idea
some time ago.

[..]
> > at the same we have better guarantees.
> > we don't just wakeup(printk_kthread) and leave. we wait for any other
> > process to re-take the console_sem. until this happens we can't leave
> > console_unlock().
> 
> And this is my problem. I am scared of the waiting. It is very hard
> to predict, especially without RT priority. But it is tricky anyway,
> see above.

but.....
the opposite possibility is that messages either won't be printed
soon (until next printk or console_unlock()) or won't be printed
ever at all (in case of sudden system death). I don't think it's
a good alternative.

[..]
> > hm. I don't want printk_kthread to be special. just because there are cases
> > when printk_kthread won't be there. we had too many problems with relying on
> > printk_kthread in all the corner cases. I want printk_kthread to be just one
> > extra process that can do the printing for us.
> > if we have X tasks sleeping in UNINTERRUPTIBLE on console_sem then
> > we better use them; keeping them in UNINTERRUPTIBLE as long as
> > printk_kthread has pending messages does no good.
> 
> I am a bit confused by this paragraph. What exactly makes the
> printk_kthread special in my proposal?

that fact that we rely on it and rely on the scheduler. may be I'm
simply misunderstanding you, sorry if so, but seems that you both want
and don't want to depend on the scheduler at the same time. I choose
not to depend on it. and for this choice to become reasonable we need
to preserve the existing 'direct' print out guarantees. and yes, I
understand that sometimes it may cause good old lockups.



> In addition, it adds another dependecy on the scheduler behavior.
> It is a can of worms as explained by Jan Kara in another mail.

and that's exactly why "wake_up() and leave console_unlock()" is not
going to fly, is it? what am I missing?


[..]
> Our two proposals are very close after all. I suggest to make
> the following changes in your patch:
> 
>   + Remove the waiting for another console_lock owner. It is
>     too tricky.

we lose the printing guarantees this way. what if printk_kthread
doesn't wake up after all? the whole point of this design twist
(and previous discussions) was that people spoke up and said that
they want printk to do the thing it was doing for decades. even if
it would cause lockup reports sometimes (but it doesn't seem to be
such a common problem after all. how many people see printk lockup
reports more or less regularly?).

>   + Instead try to reduce sleeping with console_lock taken.
>     Remove that cond_resched() from console_unlock(). Maybe
>     even call console_trylock()/console_unlock() with
>     disabled preemtion again. It will increase the chance
>     that anyone else will continue handling the console.

console_unlock() should run with the preemption disable, yes.

>   + keep the code to force sync mode in known emergency
>     situations (halt, suspend, ...).
> 
> 
> This way we should be good in all situations:
> 
>   + sudden death because we are in sync mode until atomic_limit
>     is reached

what if sudden death happens right after wake_up(printk_kthread)?
we can't just leave console_unlock().

>   + flood of messages because printk() does not sleep with
>     console_lock taken. Either someone is flushing console
>     or any printk() call could continue flushing the console.
> 
>   + critical situations because we force the sync mode
>     explicitely

	-ss

[toc] | [prev] | [next] | [standalone]

#1677632

Sergey Senozhatsky wrote:
> On (06/28/17 14:19), Petr Mladek wrote:
> > Our two proposals are very close after all. I suggest to make
> > the following changes in your patch:
> > 
> >   + Remove the waiting for another console_lock owner. It is
> >     too tricky.
> 
> we lose the printing guarantees this way. what if printk_kthread
> doesn't wake up after all? the whole point of this design twist
> (and previous discussions) was that people spoke up and said that
> they want printk to do the thing it was doing for decades. even if
> it would cause lockup reports sometimes (but it doesn't seem to be
> such a common problem after all. how many people see printk lockup
> reports more or less regularly?).

I do see printk lockup regularly because warn_alloc() from __alloc_pages_may_oom()
is basically doing

   while (1) {
     dump_stack();
     cond_resched();
   }

as long as somebody is looping inside console_unlock() with oom_lock mutex held.
Since warn_alloc() is currently a local DoS attack vector, I'm waiting for this
printk offloading patch to be merged into mainline, but it seems that this
offloading might not be able to become a solution to warn_alloc() lockup problem.

[toc] | [prev] | [next] | [standalone]

#1678545

Hello,

On (06/29/17 16:33), Sergey Senozhatsky wrote:
[..]
> On (06/28/17 14:19), Petr Mladek wrote:
> [..]
> > > so I try to minimize the negative impact of RT prio here. printk_kthread
> > > is not special any more. it's an auxiliary kthread that we sometimes
> > > wake_up. the thing is that printk_kthread also must offload at some
> > > point, basically the same `atomic_print_limit' limit applies to it as
> > > well.
> > 
> > You might call cond_resched() outside console_unlock(). But you have
> > to keep printk_kthread in runnable state as long as there are pending
> > messages. Then scheduler will always prefer this RT task over non-RT
> > tasks. Or am I wrong?
> 
> if we try to offload from IRQ->console_unlock() (or with preemption
> disabled, etc. etc.) and scheduler decides to enqueue printk_kthread
> on the same CPU, then no offloading will take place. I can reproduce
> it on my system. we need to play some affinity games, I think. but
> there are corner cases, once again.

something like this (in console_offload_printing()). try to keep
printk_kthread out of this_cpu->rq, so we (hopefully) wake it up
on some other CPU (if there are other CPUs online), that is not
in console_unlock() now:

---
        if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
                wake_up_process(printk_kthread);
                return true;
        }

        cpumask_copy(cpus_allowed, cpu_online_mask);
        cpumask_clear_cpu(smp_processor_id(), cpus_allowed);

        /*
         * If this_cpu is the only one online, then try to wake up
         * `printk_kthread' on it. what else we can do...
         */
        if (cpumask_empty(cpus_allowed))
                cpumask_set_cpu(smp_processor_id(), cpus_allowed);

        set_cpus_allowed_ptr(printk_kthread, cpus_allowed);
        wake_up_process(printk_kthread);

        free_cpumask_var(cpus_allowed);
---

once again, this is merely for testing purposes. I haven't done
many tests yet, will continue testing; but looks promising. there
are some corner cases here.

JFYI, pushed updated patch set to my tree
https://github.com/sergey-senozhatsky/linux-next-ss/commits/printk-kthread-affine

it also contains a debugging patch that I'm using to track
printk_kthread behaviour.


I'm still thinking about Steven's proposals; but we will need offloading
anyways, so the bits we are talking about here are important regardless
the direction printk design will take, I think.


> [..]
> > Our two proposals are very close after all. I suggest to make
> > the following changes in your patch:
> > 
> >   + Remove the waiting for another console_lock owner. It is
> >     too tricky.
> 
> we lose the printing guarantees this way. what if printk_kthread
> doesn't wake up after all? the whole point of this design twist
> (and previous discussions) was that people spoke up and said that
> they want printk to do the thing it was doing for decades. even if
> it would cause lockup reports sometimes

to be clear on this.


we are doing our best in order to avoid lockups caused by console_unlock(),
but the top priority remains messages print out. If we can't guarantee that
anything will take over and print the messages, we continue printing from
the current process, even though it may result in lockups.


this is based on my own experience with the previous "wake_up and forget
about it" async printk patch set (v12) which we rolled out to our fleet
of developers' boards; responses we received from the community; and
somehow it also aligned with the recent Linus' reply

 : If those two things aren't the absolutely primary goals, the whole
 : thing is pointless to even discuss. No amount of cool features,
 : performance, or theoretical deadlock avoidance matters ONE WHIT
 : compared to the two things above.

// the two things above were -- messages on the screen and dmesg.


so, offloading is cool and avoiding lockups is also very much desired,
but we need to print out the messages in the first place.

	-ss

[toc] | [prev] | [next] | [standalone]

#1678676

Sergey Senozhatsky wrote:
>         if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
>                 wake_up_process(printk_kthread);
>                 return true;
>         }

Please avoid memory allocations when trying to print something.
__GFP_DIRECT_RECLAIM allocations (e.g. GFP_KERNEL) can sleep for
unpredictable duration. Allocations without __GFP_NOWARN will cause
e.g. memory allocation failure messages. Even with __GFP_NOWARN,
some messages might be still printed (e.g. serious problem).

> I'm still thinking about Steven's proposals; but we will need offloading
> anyways, so the bits we are talking about here are important regardless
> the direction printk design will take, I think.

Is there a chance that printk() waits for only data queued by that printk()
call (exception will be printk() from NMI). If we carry penalty for printk()
(charge delay according to amount of data queued by that printk()), users
will stop doing stupid flooding with printk() based on an assumption that
offloaded kernel thread will manage magically with guarantee of being
printed out (i.e. users has to become careful).

[toc] | [prev] | [next] | [standalone]

#1678730

On (06/30/17 19:18), Tetsuo Handa wrote:
> Sergey Senozhatsky wrote:
> >         if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
> >                 wake_up_process(printk_kthread);
> >                 return true;
> >         }
> 
> Please avoid memory allocations when trying to print something.
> __GFP_DIRECT_RECLAIM allocations (e.g. GFP_KERNEL) can sleep for
> unpredictable duration. Allocations without __GFP_NOWARN will cause
> e.g. memory allocation failure messages. Even with __GFP_NOWARN,
> some messages might be still printed (e.g. serious problem).

wow... dammit, what a stupid mistake. you are 100% right, thanks!
it's sooo unsafe and dumb, console_unlock() and, thus, offloading
can happen from IRQ.

thanks again.

> > I'm still thinking about Steven's proposals; but we will need offloading
> > anyways, so the bits we are talking about here are important regardless
> > the direction printk design will take, I think.
> 
> Is there a chance that printk() waits for only data queued by that printk()
> call (exception will be printk() from NMI).

hm, I don't think this can be done easily... consider

	console_lock();
	printk();
	printk();
	...			-> this guys will wait forever. nothing
				   flushes the logbuf.
	printk();
	console_unlock();


> If we carry penalty for printk() (charge delay according to amount of
> data queued by that printk()), users will stop doing stupid flooding
> with printk() based on an assumption that offloaded kernel thread will
> manage magically with guarantee of being printed out (i.e. users has
> to become careful).

ratelimiting is on my list. but it's a bit tricky... what should happen
if one does

	CPU1

	-> IRQ
		printk()
			console_unlock();
		printk()
			console_unlock();
		...
		printk()
			ratelimit();
			console_unlock();

	-> NMI panic()


need to think more.

	-ss

[toc] | [prev] | [next] | [standalone]

#1678778

Sergey Senozhatsky wrote:
> On (06/30/17 19:18), Tetsuo Handa wrote:
> > > I'm still thinking about Steven's proposals; but we will need offloading
> > > anyways, so the bits we are talking about here are important regardless
> > > the direction printk design will take, I think.
> > 
> > Is there a chance that printk() waits for only data queued by that printk()
> > call (exception will be printk() from NMI).
> 
> hm, I don't think this can be done easily... consider
> 
> 	console_lock();
> 	printk();
> 	printk();
> 	...			-> this guys will wait forever. nothing
> 				   flushes the logbuf.
> 	printk();
> 	console_unlock();

Can't we remove console_lock()/console_unlock() from printk() ?
I think that printk() depends on console_unlock() is complicating.

[toc] | [prev] | [next] | [standalone]

#1678798

On (06/30/17 21:35), Tetsuo Handa wrote:
> Date: Fri, 30 Jun 2017 21:35:28 +0900
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> To: sergey.senozhatsky.work@gmail.com
> Cc: pmladek@suse.com, sergey.senozhatsky@gmail.com, rostedt@goodmis.org,
>  jack@suse.cz, akpm@linux-foundation.org, peterz@infradead.org,
>  rjw@rjwysocki.net, ebiederm@xmission.com, gregkh@linuxfoundation.org,
>  jslaby@suse.com, pavel@ucw.cz, andi@lisas.de, linux-kernel@vger.kernel.org
> Subject: Re: [RFC][PATCHv3 2/5] printk: introduce printing kernel thread
> X-Mailer: Winbiff [Version 2.51 PL2]
> 
> Sergey Senozhatsky wrote:
> > On (06/30/17 19:18), Tetsuo Handa wrote:
> > > > I'm still thinking about Steven's proposals; but we will need offloading
> > > > anyways, so the bits we are talking about here are important regardless
> > > > the direction printk design will take, I think.
> > > 
> > > Is there a chance that printk() waits for only data queued by that printk()
> > > call (exception will be printk() from NMI).
> > 
> > hm, I don't think this can be done easily... consider
> > 
> > 	console_lock();
> > 	printk();
> > 	printk();
> > 	...			-> this guys will wait forever. nothing
> > 				   flushes the logbuf.
> > 	printk();
> > 	console_unlock();
> 
> Can't we remove console_lock()/console_unlock() from printk() ?

we can't... well, we can... and there are some ideas but we are years
away from all it becoming reality. console_sem is the giant and major
lock that fbcon, drm, tty and so on are using. please see a sub-thread
starting from here:

https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-June/004389.html

> I think that printk() depends on console_unlock() is complicating.

it absolutely is.

	-ss

[toc] | [prev] | [next] | [standalone]

#1678820

On Fri 2017-06-30 16:01:31, Sergey Senozhatsky wrote:
> we are doing our best in order to avoid lockups caused by console_unlock(),
> but the top priority remains messages print out. If we can't guarantee that
> anything will take over and print the messages, we continue printing from
> the current process, even though it may result in lockups.
>
> this is based on my own experience with the previous "wake_up and forget
> about it" async printk patch set (v12) which we rolled out to our fleet
> of developers' boards;

Well, v12 completely failed when there was a sudden death. Also
printk_kthread slept with console_lock() taken. Therefore it was
much less effective during printk() floods.


> responses we received from the community; and
> somehow it also aligned with the recent Linus' reply
> 
>  : If those two things aren't the absolutely primary goals, the whole
>  : thing is pointless to even discuss. No amount of cool features,
>  : performance, or theoretical deadlock avoidance matters ONE WHIT
>  : compared to the two things above.
> 
> // the two things above were -- messages on the screen and dmesg.

Sure, this sounds cool but things are not black and white.

We have the offload patches in SUSE for years because some big
machines did not boot without them. On the other hand, AFAIK,
we have zero bug reports on losing messages caused by a flood
of messages. To be fair, we often look into crash dumps for
these messages.

I remember that you mentioned loosing messages in several threads.
I wonder if it is caused by different configuration, use case,
or extra patches. Anyway, it might suggest that you use the
printk() system (buffers, throughput) on the edge or even beyond
of its capacity.


Anyway, the handshake during offloading might be pretty
problematic. To be honest, I do not have much experience
with it. I have shared some my fears in the other mail[1].
Jan Kara spent a lot of time on this and probably could
say more.

Maybe, we could try to look into the throotling path. Slowing down
massive printk() callers looks necessary when things gets
out of control.

I wonder if I could add some counter into task_struct.
It might be configurable. I am not sure if people would
want this enabled on production systems where the level
of messages should be lower anyway.

[1] https://lkml.kernel.org/r/20170630115457.GE23069@pathway.suse.cz

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1678831

On (06/30/17 15:16), Petr Mladek wrote:
> Anyway, the handshake during offloading might be pretty
> problematic. To be honest, I do not have much experience
> with it. I have shared some my fears in the other mail[1].
> Jan Kara spent a lot of time on this and probably could
> say more.
> 
> Maybe, we could try to look into the throotling path. Slowing down
> massive printk() callers looks necessary when things gets
> out of control.

throttling, in some form, is already there. I think.

there is a printk_delay() function. which we can silently activate
when things don't look cool anymore. and printk_delay() is already
getting called on every vprintk_emit() entry. the question is -- how
big should be our delay value, and... when do we need to activate
printk_delay()?

when the distance between console_seq and log_next_seq... suggests
that we will drop (overwrite) un-flushed messages sooner than console_seq
reaches log_next_seq? so log_next_seq is closer to log_first_seq than
console_seq to log_next_seq. or something like this... I'm a bit tired
after a long week; need more time to think about it.


the same printk_delay() *may be* can be used in console_unlock(), to
give potential new console_sem owner more time. just an idea.

> I wonder if I could add some counter into task_struct.
> It might be configurable. I am not sure if people would
> want this enabled on production systems where the level
> of messages should be lower anyway.

	-ss

[toc] | [prev] | [next] | [standalone]

#1679899

On (06/30/17 22:38), Sergey Senozhatsky wrote:
> On (06/30/17 15:16), Petr Mladek wrote:
> > Anyway, the handshake during offloading might be pretty
> > problematic. To be honest, I do not have much experience
> > with it. I have shared some my fears in the other mail[1].
> > Jan Kara spent a lot of time on this and probably could
> > say more.
> > 
> > Maybe, we could try to look into the throotling path. Slowing down
> > massive printk() callers looks necessary when things gets
> > out of control.
> 
> throttling, in some form, is already there. I think.
> 
> there is a printk_delay() function. which we can silently activate
> when things don't look cool anymore. and printk_delay() is already
> getting called on every vprintk_emit() entry. the question is -- how
> big should be our delay value, and... when do we need to activate
> printk_delay()?
> 
> when the distance between console_seq and log_next_seq... suggests
> that we will drop (overwrite) un-flushed messages sooner than console_seq
> reaches log_next_seq? so log_next_seq is closer to log_first_seq than
> console_seq to log_next_seq.

something like below, may be. a sketch, just to demonstrate the
idea. but, once polished, can go to printk out of series.

===8<===8<===8<===

Throttle printk() callers when we detect that consoles are
far behind the logbuf: we printed to the consoles 4 times
less messages than we still have to print.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f24d3789faa0..fd546bd95207 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1758,17 +1758,43 @@ static void call_console_drivers(const char *ext_text, size_t ext_len,
 	}
 }
 
+#define PRINTK_FLOOD_DEFAULT_DELAY	10
+
 int printk_delay_msec __read_mostly;
 
+static inline void __printk_delay(int m)
+{
+	while (m--) {
+		mdelay(1);
+		touch_nmi_watchdog();
+	}
+}
+
 static inline void printk_delay(void)
 {
-	if (unlikely(printk_delay_msec)) {
-		int m = printk_delay_msec;
+	unsigned long flags;
+	u64 console_seen = 0, console_to_see;
 
-		while (m--) {
-			mdelay(1);
-			touch_nmi_watchdog();
-		}
+	if (printk_delay_msec) {
+		__printk_delay(printk_delay_msec);
+		return;
+	}
+
+	/*
+	 * Check if consoles are far behind the loguf head and
+	 * throttle printk() callers if so.
+	 */
+	logbuf_lock_irqsave(flags);
+	if (console_seq > log_first_seq)
+		console_seen = console_seq - log_first_seq;
+	console_to_see = log_next_seq - console_seq;
+	logbuf_unlock_irqrestore(flags);
+
+	if (console_seen < 4 * console_to_see) {
+		if (printk_delay_msec)
+			__printk_delay(printk_delay_msec);
+		else
+			__printk_delay(PRINTK_FLOOD_DEFAULT_DELAY);
 	}
 }
 
-- 
2.13.2

[toc] | [prev] | [next] | [standalone]

#1678865

On (06/30/17 15:16), Petr Mladek wrote:
[..]
>
> I wonder if I could add some counter into task_struct.
> It might be configurable. I am not sure if people would
> want this enabled on production systems where the level
> of messages should be lower anyway.

I don't think it really matters which one of the tasks has issued way
too many printk()-s. it's the state of the logbuf that matters after
all. was it one task or 5 tasks, or 15 that pushed logbuf far ahead of
console_seq, we need to throttle all of new printk() calls anyway.
just because logbuf is already too far and we need to give console a
chache to catch up. so let's activate throttling based on the logbuf
state (those seq numbers).

	-ss

[toc] | [prev] | [next] | [standalone]

#1678726

On Thu 2017-06-29 16:33:22, Sergey Senozhatsky wrote:
> On (06/28/17 14:19), Petr Mladek wrote:
> [..]
> > > at the same we have better guarantees.
> > > we don't just wakeup(printk_kthread) and leave. we wait for any other
> > > process to re-take the console_sem. until this happens we can't leave
> > > console_unlock().
> > 
> > And this is my problem. I am scared of the waiting. It is very hard
> > to predict, especially without RT priority. But it is tricky anyway,
> > see above.
> 
> but.....
> the opposite possibility is that messages either won't be printed
> soon (until next printk or console_unlock()) or won't be printed
> ever at all (in case of sudden system death). I don't think it's
> a good alternative.

I see it like a weighing machine. There is a "guaranteed" output on
one side and a softlockups prevention on the other side. The more
we prevent the softlockups the less we guarantee the output.

We do not have the same opinion about the balance. My solution
completely prevents softlockups. Your one tries to be more
conservative. It might look that a compromise is better but
we need to consider how it is achieved, what the effect
and side-effects are.


My main unresolved doubts about this patchset are:

1. It gives other processes only very small change to take
   over the job. They either need to call console_trylock()
   in very small "race" window or they need to call
   console_lock(). Where console_lock() only signalizes
   that the caller is willing to take the job and puts
   him into sleep.

   Another detailed description of this problem can be found
   in my previous mail, see
   https://lkml.kernel.org/r/20170628121925.GN1538@pathway.suse.cz


2. It adds rather complex dependency on the scheduler. I know
   that my simplified solution do this as well but another way[*]
   Let me explain. I would split the dependency on the code
   and behavior relation.

   From the code side: The current printk() calls wake_up_process()
   and we need to use printk_deferred() in the related scheduler code.
   This patch does this as well, so there is no win and no lose.
   Well, you talk about changing the affinity and other tricks
   in the other mails. This might add more locations where
   printk_deferred() would be needed.

   From the behavior side: The current code wakes the process
   and is done. The code in this patch wakes the process and
   waits until it[**] gets CPU and really runs. It switches to
   the emergency mode when the other process does not run in time.
   By other words, this patch depends on more actions done
   by the scheduler and changes behavior based on it. IMHO,
   this will be very hard to code, tune, and debug.
   A proper solution might require more code dependency.

   [*] My solution depends on the scheduler in the sense
       that messages will get lost when nobody else will take
       over the console job. The logic is simple, no scheduler
       => only atomic_print_limit messages. It might sound
       drastic but see below. The main win is that it is "simple".

   [**] It is enough when any other process takes over the
       console_lock. But this is tricky, see my 1st point above.


3. The prevention of soft-lockups is questionable. If you are in
   soft-lockup prone situation, the only prevention is to do an
   offload. But if you switch to the emergency mode and give
   up offloading too early, the new code stops preventing
   the softlockup.

   Of course, the patchset does not make it worse but the question
   is how much it really helps. It would be bad to add a lot of
   code/complexity with almost no gain.


IMHO, if we try to solve the 1st problem (chance of offloading),
it might add a risk of deadlocks and/or make the 2nd problem
(dependency on scheduler) worse. Also I am afraid that we would
repeat many dead ways already tried by Jan Kara.

If you will try to improve 3rd problem and make some guaranties
of the soft-lockup prevention, it would make the 2nd problem
(dependency on scheduler) worse. Also the code might be
very hard to understand and tune.


This is why I look for a rather simple solution. IMHO, we both
agree that:

   +  the offload will be activated only when there is
      a flood of messages

   + the only reason to wait for the other handler is to
     better handle sudden death where panic() is not called.

IMHO, the only one who brought the problem of sudden death
was Pavel Machek. AFAIK, he works on embedded systems and
hardware enablement. I guess the combination of the flood
of messages and sudden death is rare there. Also I doubt
that the current code handle it well. The flood is badly
handed in general. In each case, I wonder how long we could
survive flushing messages when there is sudden death
and scheduling does not work.

One problem here is that some questions/doubts are hard to
answer/prove without wide testing.

A compromise might be to start with the simple code
and disable the offloading by default. I am sure that
there will be volunteers that would want to play with it,
e.g. Tetsuo. We would enable it in SUSE as well because
there should not be any regression against what we have
used for years now. We could make it always more complex
according to the feedback and eventually enable it
by default.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1678774

Hello,

On (06/30/17 13:54), Petr Mladek wrote:
> > but.....
> > the opposite possibility is that messages either won't be printed
> > soon (until next printk or console_unlock()) or won't be printed
> > ever at all (in case of sudden system death). I don't think it's
> > a good alternative.
> 
> I see it like a weighing machine. There is a "guaranteed" output on
> one side and a softlockups prevention on the other side. The more
> we prevent the softlockups the less we guarantee the output.

I apply a very simple litmus test. if the answer to the question
"so we leave console_unlock() and there are pending messages,
who and when is going to flush the remaining messages?" is
"something sometime in the future" then it's a no-no.

"something sometime in the future" is equal to "no one".

we must stay and continue printing. because it gives the right
answer - "current process and right now. until someone else
(+printk_kthread) takes over".


> We do not have the same opinion about the balance. My solution
> completely prevents softlockups.

not at all costs. especially if we talk about possibility of losing
messages. if we consider such possibility, then let's just
unconditionally drop logbuf entries every time console_lock() is
looping for too long.

> Your one tries to be more conservative.

Linus wrote:

: If those two things aren't the absolutely primary goals, the whole
: thing is pointless to even discuss. No amount of cool features,
: performance, or theoretical deadlock avoidance matters ONE WHIT
: compared to the two things above.

and

Linus wrote:

: But not having any messages at all, because we were trying so hard to
: abstract things out and put them in buffers so that we couldn't
: deadlock with the IO routines, and the timer or workqueue that was
: supposed to do it is never going to happen any more because of the bug
: that is trying to be printed out?
:
: THAT is bad.


I think our priorities should be quite clear here.


> My main unresolved doubts about this patchset are:
> 
> 1. It gives other processes only very small change to take
>    over the job. They either need to call console_trylock()
>    in very small "race" window or they need to call
>    console_lock(). Where console_lock() only signalizes
>    that the caller is willing to take the job and puts
>    him into sleep.

printk_kthread does console_lock().

we may (and need to) improve the retry path in console_unlock().
but we must not leave it until other process locks the console_sem.

>    Another detailed description of this problem can be found
>    in my previous mail, see
>    https://lkml.kernel.org/r/20170628121925.GN1538@pathway.suse.cz
> 
> 
> 2. It adds rather complex dependency on the scheduler. I know
>    that my simplified solution do this as well but another way[*]
>    Let me explain. I would split the dependency on the code
>    and behavior relation.
> 
>    From the code side: The current printk() calls wake_up_process()
>    and we need to use printk_deferred() in the related scheduler code.
>    This patch does this as well, so there is no win and no lose.
>    Well, you talk about changing the affinity and other tricks
>    in the other mails. This might add more locations where
>    printk_deferred() would be needed.

we are in printk_safe all the way through console_offload_printing(),
the context is derived from console_unlock().

why we would need printk_deferred()?

>    From the behavior side: The current code wakes the process
>    and is done. The code in this patch wakes the process and
>    waits until it[**] gets CPU and really runs. It switches to
>    the emergency mode when the other process does not run in time.
>    By other words, this patch depends on more actions done
>    by the scheduler and changes behavior based on it. IMHO,
>    this will be very hard to code, tune, and debug.
>    A proper solution might require more code dependency.
> 
>    [*] My solution depends on the scheduler in the sense
>        that messages will get lost when nobody else will take
>        over the console job.

which is precisely and exactly the thing that we should never
let to happen. there is no _win_, because we _lost_ the messages.


> 3. The prevention of soft-lockups is questionable. If you are in
>    soft-lockup prone situation, the only prevention is to do an
>    offload. But if you switch to the emergency mode and give
>    up offloading too early, the new code stops preventing
>    the softlockup.
> 
>    Of course, the patchset does not make it worse but the question
>    is how much it really helps. It would be bad to add a lot of
>    code/complexity with almost no gain.
> 
> 
> IMHO, if we try to solve the 1st problem (chance of offloading),
> it might add a risk of deadlocks and/or make the 2nd problem
> (dependency on scheduler) worse. Also I am afraid that we would
> repeat many dead ways already tried by Jan Kara.

what deadlock?


> If you will try to improve 3rd problem and make some guaranties
> of the soft-lockup prevention, it would make the 2nd problem
> (dependency on scheduler) worse. Also the code might be
> very hard to understand and tune.
> 
> 
> This is why I look for a rather simple solution. IMHO, we both
> agree that:
> 
>    +  the offload will be activated only when there is
>       a flood of messages
> 
>    + the only reason to wait for the other handler is to
>      better handle sudden death where panic() is not called.
> 
> IMHO, the only one who brought the problem of sudden death
> was Pavel Machek.

we gave up on printk-async. the last bug report was titled "blah blah
missing backtrace". and I really would rather prefer to see that
backtrace + soft lockup or even hard lockup. still would have been
better than seeing nothing at all.

	-ss

> AFAIK, he works on embedded systems and hardware enablement.
> I guess the combination of the flood of messages and sudden
> death is rare there. Also I doubt that the current code handle
> it well. The flood is badly handed in general. In each case,
> I wonder how long we could survive flushing messages when there
> is sudden death and scheduling does not work.
> 
> One problem here is that some questions/doubts are hard to
> answer/prove without wide testing.
> 
> A compromise might be to start with the simple code
> and disable the offloading by default. I am sure that
> there will be volunteers that would want to play with it,
> e.g. Tetsuo. We would enable it in SUSE as well because
> there should not be any regression against what we have
> used for years now. We could make it always more complex
> according to the feedback and eventually enable it
> by default.
> 
> Best Regards,
> Petr
> 

[toc] | [prev] | [next] | [standalone]

#1678832

On Fri, 30 Jun 2017 21:42:24 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> I apply a very simple litmus test. if the answer to the question
> "so we leave console_unlock() and there are pending messages,
> who and when is going to flush the remaining messages?" is
> "something sometime in the future" then it's a no-no.

I totally agree with this, but...

> 
> "something sometime in the future" is equal to "no one".
> 
> we must stay and continue printing. because it gives the right
> answer - "current process and right now. until someone else
> (+printk_kthread) takes over".

Would it be acceptable to have a user knob that allows for it not to
happen? That is, let the user of the kernel decide if they care about
critical prints or not? If a knob says, "only print X, then offload"
would that be allowed. Of course the default would be "only print ALL
OF IT" to keep the current behavior.

A lot of times the console isn't recorded to debug hard lock ups. I
know most desktops running a GUI do not. When ever my workstation locks
up, and it has no serial, I don't get to see the dmesg at all. In this
situation, I don't care if the prints are offloaded or not.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1678877

On (06/30/17 09:33), Steven Rostedt wrote:
[..]
> > "something sometime in the future" is equal to "no one".
> > 
> > we must stay and continue printing. because it gives the right
> > answer - "current process and right now. until someone else
> > (+printk_kthread) takes over".
> 
> Would it be acceptable to have a user knob that allows for it not to
> happen? That is, let the user of the kernel decide if they care about
> critical prints or not? If a knob says, "only print X, then offload"
> would that be allowed. Of course the default would be "only print ALL
> OF IT" to keep the current behavior.

I'm sorry, I'm not sure I got your question right (I need some rest
probably).

I guess the question was, a knob that would determine what happens after
current wakes up printk_kthread -- does it stay in console_unlock() and
wait for new console_sem owner, printing the messages in the meantime,
or goes all in and expects printk_kthread or anything else to lock
console_sem at some point and flush the remaining messages. is that
correct? we can do this (well, I'm absolutely not in position to say
"we can't do this" :) ). I guess the sort of a problem we have now is
that we can't guarantee that wake_up() will actually wake_up printk_kthread.
but if user requests it, then well... it might be easier to adjust watchdog
timeout value ;) just kidding. or may be I misunderstood your question.


or... another silly idea... shall we start touching the lockup
watchdog per-cpu counters on the CPU that has:

a) woken up printk_kthread
b) but is still in console_unlock() waiting for the new console_sem owner?

...

or we, may be, can add a new sysrq that would flush logbuf messages...
the same way as console_flush_on_panic() does. just in case if wake_up
didn't work.... or is there already a sysrq for that?

	-ss

> A lot of times the console isn't recorded to debug hard lock ups. I
> know most desktops running a GUI do not. When ever my workstation locks
> up, and it has no serial, I don't get to see the dmesg at all. In this
> situation, I don't care if the prints are offloaded or not.
> 
> -- Steve
> 

[toc] | [prev] | [next] | [standalone]

#1678887

On Fri, 30 Jun 2017 23:28:51 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> I guess the question was, a knob that would determine what happens after
> current wakes up printk_kthread -- does it stay in console_unlock() and
> wait for new console_sem owner, printing the messages in the meantime,
> or goes all in and expects printk_kthread or anything else to lock
> console_sem at some point and flush the remaining messages. is that
> correct? we can do this (well, I'm absolutely not in position to say
> "we can't do this" :) ). I guess the sort of a problem we have now is
> that we can't guarantee that wake_up() will actually wake_up printk_kthread.
> but if user requests it, then well... it might be easier to adjust watchdog
> timeout value ;) just kidding. or may be I misunderstood your question.

Actually, what I was thinking of was just setting a hard limit at how
much printk() can write without pushing it off to a helper thread. Kind
of like how softirq works. If there's too much work, it then pushes off
the rest to ksoftirqd.

If we have a kprintd, have a knob that says how much printk can write
under console_lock, and after it hits a limit (default being no limit),
then to simply wake up kprintd to do the rest. This should allow those
that care to limit how much time printk can stop a CPU by, with the
knowledge that if the kernel were to crash, there's a good chance that
the necessary debug info will not be printed.

Now as I have said, the default would be to keep things as is, and let
printk take up as much of the CPU it needs till it gets all the pending
prints out.

I would also like (in my wish list), a mechanism, that when an oops in
progress happens, to set an emergency flag. If something already has
console_lock, then we can let it do what it does today, and have the
critical thread write its data to the logbuf, and let the current
printer hopefully output it (I think that's what we do today). But if
there's no holder of console lock, the critical thread takes control
and dumps the buffer to get everything out. Once that emergency flag is
set, there will be no more offloading. All prints will be in critical
mode, and try to get the data out if possible.

While I'm giving you my Christmas list, I would also like a way to have
an NMI safe lock in printk (like Peter Zijlstra and I have patches for
with early-printk). Which checks if the current CPU owns the spin lock,
and if it doesn't it is safe to take the lock even in NMI mode. But if
the current CPU has the lock, then the NMI handle does the printing
without locking, not caring if it screws up the thread that is currently
printing.

Of course we need to be careful about various consoles that have their
own locks. Maybe make an NMI lock primitive that all consoles use?

This is just wishful thinking, but hopefully we can come up with
something that can solve all issues reasonably.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1679232

On (06/30/17 10:45), Steven Rostedt wrote:
> On Fri, 30 Jun 2017 23:28:51 +0900
> Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> 
> > I guess the question was, a knob that would determine what happens after
> > current wakes up printk_kthread -- does it stay in console_unlock() and
> > wait for new console_sem owner, printing the messages in the meantime,
> > or goes all in and expects printk_kthread or anything else to lock
> > console_sem at some point and flush the remaining messages. is that
> > correct? we can do this (well, I'm absolutely not in position to say
> > "we can't do this" :) ). I guess the sort of a problem we have now is
> > that we can't guarantee that wake_up() will actually wake_up printk_kthread.
> > but if user requests it, then well... it might be easier to adjust watchdog
> > timeout value ;) just kidding. or may be I misunderstood your question.
> 
> Actually, what I was thinking of was just setting a hard limit at how
> much printk() can write without pushing it off to a helper thread. Kind
> of like how softirq works. If there's too much work, it then pushes off
> the rest to ksoftirqd.

oh, OK. this is exactly the way it works now. we've got a sysfs knob

 echo 150 > /sys/module/printk/parameters/atomic_print_limit

which sets the limit on the number of lines we print in direct (old
printk mode). as soon as current prints more than atomic_print_limit
lines it wakes up printk_kthread.

the default value for atomic_print_limit is 0, which means no limit.
IOW, no offloading at all, aka the old behaviour.

> If we have a kprintd, have a knob that says how much printk can write
> under console_lock, and after it hits a limit (default being no limit),
> then to simply wake up kprintd to do the rest. This should allow those
> that care to limit how much time printk can stop a CPU by, with the
> knowledge that if the kernel were to crash, there's a good chance that
> the necessary debug info will not be printed.

yes. this is how it's done in the patch set. with some additional tricks.

> Now as I have said, the default would be to keep things as is, and let
> printk take up as much of the CPU it needs till it gets all the pending
> prints out.

yes. this is the way it's done in the patch set.

> I would also like (in my wish list), a mechanism, that when an oops in
> progress happens, to set an emergency flag. If something already has
> console_lock, then we can let it do what it does today, and have the
> critical thread write its data to the logbuf, and let the current
> printer hopefully output it (I think that's what we do today). But if
> there's no holder of console lock, the critical thread takes control
> and dumps the buffer to get everything out. Once that emergency flag is
> set, there will be no more offloading. All prints will be in critical
> mode, and try to get the data out if possible.

yes. this is what we are trying to do in the patch set.

when we decide if we need to offload printing, we call this guy first

static inline bool printk_offloading_enabled(void)
{
	return atomic_print_limit &&
		printk_enforce_emergency == 0 &&
		printk_kthread &&
		atomic_read(&printk_emergency) == 0;
}

which checks printk internal emergency flags, etc.. for example,
printk_enforce_emergency is set automatically when we printk()
LOGLEVEL_EMERG message the first time.


then we check the system wide flags

	if (system_state != SYSTEM_RUNNING || oops_in_progress)
		return false;


if all checks pass (no oops, no emergency, nothing) then we continue
with the offloading thing. increment the counter which tracks the number
of lines printed already printed and offload printing if that counter
goes above user defined atomic_print_limit threshold.

otherwise, we just stay and continue printing, because we are in oops or
emergency, or whatever.

> While I'm giving you my Christmas list, I would also like a way to have
> an NMI safe lock in printk (like Peter Zijlstra and I have patches for
> with early-printk). Which checks if the current CPU owns the spin lock,
> and if it doesn't it is safe to take the lock even in NMI mode. But if
> the current CPU has the lock, then the NMI handle does the printing
> without locking, not caring if it screws up the thread that is currently
> printing.
> 
> Of course we need to be careful about various consoles that have their
> own locks. Maybe make an NMI lock primitive that all consoles use?
> 
> This is just wishful thinking, but hopefully we can come up with
> something that can solve all issues reasonably.

ok, the NMI thing is not in the current patch set. we can do it a bit
later. the early-printk patch set is also interesting. and I hope that
Petr will take a look on it once we settle down the offloading.

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web