Messages in this thread |  | | Date | Tue, 4 Oct 2016 14:22:26 +0200 | From | Petr Mladek <> | Subject | Re: [RFC][PATCH 6/7] printk: use alternative printk buffers |
| |
On Sat 2016-10-01 11:48:29, Sergey Senozhatsky wrote: > On (09/30/16 13:15), Petr Mladek wrote: > > > 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
I have finally got it. You are right, there are still some other possible deadlocks.
But these are not protected at the moment. Neither printk_deferred() not logbuf_cpu prevent this. Therefore this is not a reason to keep either printk_deferred() or logbuf_cpu.
> 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.
I agree that it does not make sense to solve these other problems in this patchset.
> 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.
Yeah, so a generic solution for console_sem might be rather hard. I would not complicate this patchset with it.
> > 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'.
On the other hand, the per-CPU buffer will include only error messages from the printk() code. This is why I am fine with the extra buffer. The only way is to use printk_deferred() these days. And as you said, this was error prone and hard to maintain.
> > 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);
Yes, but we are safe to call normal printk() at this point. lockbuf_lock is released => no danger of a deadlock. console_sem is taken but this is not a problem. printk() will do console_trylock() protected by the alt_printk_enter()/exit(). Therefore the trylock will fail but it could not cause a deadlock.
We just need to replace
if (retry && console_trylock()) goto again;
with a safe variant, something like
if (retry) { local_irq_save(flags); alt_printk_enter(); lock_failed = console_trylock(); alt_printk_exit(); local_irq_restore(flags);
if (!lock_failed) goto again; }
Or do I miss anything?
> > In each case, getting rid of printk_deferred() could be > > a fantastic selling point for this patchset. > > agree.
I believe that it is doable and worth try.
I am going to look at the second version of the patchset.
Best Regards, Petr
|  |