Messages in this thread | | | Date | Fri, 30 Sep 2016 13:15:46 +0200 | From | Petr Mladek <> | Subject | Re: [RFC][PATCH 6/7] printk: use alternative printk buffers |
| |
On Fri 2016-09-30 10:15:44, Sergey Senozhatsky wrote: > On (09/29/16 15:00), Petr Mladek wrote: > [..] > > > @@ -1791,7 +1791,7 @@ asmlinkage int vprintk_emit(int facility, int level, > > > zap_locks(); > > > } > > > > > > - lockdep_off(); > > > + alt_printk_enter(); > > > > IMHO, we could not longer enter vprintk_emit() recursively. The same > > section that was guarded by logbuf_cpu is guarded by > > alt_printk_enter()/exit() now. > > you might be very right here. I'll take a look. > > > IMHO, we could remove all the logic around the recursion. Then we > > could even disable/enable irqs inside alt_printk_enter()/exit(). > > I was thinking of doing something like this; but that would require > storing 'unsigned long' flags in per-cpu data > > alt_enter() > { > unsinged long flags; > > local_irq_save(flags); > ctx = this_cpu_ptr(); > ctx->flags = flags; > ... > } > > alt_exit() > { > ctx = this_cpu_ptr(); > ... > local_irq_restore(ctx->flags); > } > > 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. > > but I can certainly hide these details in alt_enter/exit. > > > > @@ -2479,7 +2490,9 @@ void console_unlock(void) > > > */ > > > raw_spin_lock(&logbuf_lock); > > > retry = console_seq != log_next_seq; > > > - raw_spin_unlock_irqrestore(&logbuf_lock, flags); > > > + raw_spin_unlock(&logbuf_lock); > > > + alt_printk_exit(); > > > + local_irq_restore(flags); > > > > We should mention that this patch makes an obsolete artefact from > > printk_deferred(). It opens the door for another big cleanup and > > relief. > > 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.
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.
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().
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?
In each case, getting rid of printk_deferred() could be a fantastic selling point for this patchset.
Best Regards, Petr
| |