Re: [RFC][PATCH 6/7] printk: use alternative printk buffers

[Sergey Senozhatsky]

On (09/30/16 13:15), Petr Mladek wrote:
[..]
> > and the decision was to keep `unsigned long flags' on stack in the
> > alt_enter/exit caller. besides in most of the cases we already have
> > it (in vprintk_emit() and console_unlock()).
> 
> I would pass the pointer to flags as alt_enter() parameter.

can do. or _may be_ leave the IRQ manipulation as it is. because there
are different tricks. take a look at console_ulock() and so on.

> > do you mean that, once alt_printk is done properly, we can drop
> > printk_deferred()? I was thinking of it, but decided not to
> > mention/touch it in this patch set.
> 
> My understanding is the following:
> 
> The difference between normal printk() and printk_deferred() is
> that the other does not call console_trylock()/console_unlock().
> It means that printk_deferred() can avoid recursion only from these
> two calls.

yes.

> printk_deferred() is used only in scheduler and timekeeping code.
> Therefore it prevents only limited number of possible recursions
> and deadlocks at the moment.
> 
> This patch guards most of the two calls a more generic way.
> The redirected parts prevent recursion not only to into the
> code guarded by console_sem but also into parts guarded
> by lockbuf_lock.

yes. I'm considering to extend it to "non-recursive printk" cases
sometime in the future. it's easy to protect lockbuf_lock, but not
so easy to protect sleeping console_lock().

the cases I'm talking of are (for instance):

  devkmsg_open()
   raw_spin_lock_irq(&logbuf_lock)
    spin_dump()
     printk()
      raw_spin_lock_irq(&logbuf_lock)  << deadlock

entering to alt_printk mode each time we take the `logbuf_lock' sort
of makes sense. can be done later, don't want to overload this patch set.

addressing sleeping console_sem function is not super hard in general.
we just would have to put alt_printk_enter/exit into scheduler code, or
at least into semaphore code. which can be hard to sell. there are
gazillions of semaphores and we need to protect only one of them, but
have to do alt_printk_enter/exit for every.

> By other words, this patch is supposed to handle a superset
> of the deadlocks that are currently prevented by printk_deferred().
> If this is true, we do not longer need printk_deferred().

yes, I suppose so.
the only difference here is that printk_deferred() immediately puts the
message into logbuf (well, *if* it can lock the `logbuf_lock'), while
vprintk_alt() puts it into per-cpu buffer first and needs an irq work in
that CPU to flush it to logbuf. OTOH, vprintk_alt() does not depend on
`logbuf_lock'.

> The only question is if this patch guards enough parts of
> console_try_lock()/console_unlock() to handle the superset
> of the possible deadlocks.
> 
> I see that it does not guard two up_console_sem() calls
> from console_unlock(). But this can be fixed in the next
> version.
>
> Or is there any other catch that I do not see at the moment?

it's a bit tricky. we break from printing loop only with logbuf_lock
spin_lock locked, irqs disabled and in alt_printk mode. so everything
after the printing loop is still protected, up until

	raw_spin_lock(&logbuf_lock);
	retry = console_seq != log_next_seq;
	raw_spin_unlock(&logbuf_lock);
	alt_printk_exit();
	local_irq_restore(flags);

> In each case, getting rid of printk_deferred() could be
> a fantastic selling point for this patchset.

agree.

I also suspect that we can eliminate the recursion detection logic
in vprintk_emit().

	-ss