Re: [PATCH] printk: robustify printk

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 08 Aug 2008 21:21:08 +0200
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Fri, 2008-08-08 at 12:14 -0700, Andrew Morton wrote:
> > On Fri, 08 Aug 2008 20:14:28 +0200
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > >  void wake_up_klogd(void)
> > >  {
> > > -	if (!oops_in_progress && waitqueue_active(&log_wait))
> > > -		wake_up_interruptible(&log_wait);
> > > +	unsigned long flags;
> > > +	struct klogd_wakeup_state *kws;
> > > +
> > > +	if (!waitqueue_active(&log_wait))
> > > +		return;
> > > +
> > > +	local_irq_save(flags);
> > > +	kws = &__get_cpu_var(kws);
> > > +	if (!kws->pending) {
> > > +		kws->pending = 1;
> > > +		call_rcu(&kws->head, __wake_up_klogd);
> > > +	}
> > > +	local_irq_restore(flags);
> > >  }
> > 
> > Note that kernel/rcupreempt.c's flavour of call_rcu() takes
> > RCU_DATA_ME().lock, so there are still code sites from which a printk
> > can deadlock.  Only now, it is config-dependent.
> > 
> > From a quick look it appears that large amounts of kernel/rcupreempt.c
> > are now a printk-free zone.
> 
> Drad, missed that bit, I did look at the calling end, but forgot the
> call_rcu() end :-/
> 
> The initial printk_tick() based implementation didn't suffer this
> problem, should we revert to that scheme?

Dunno.  Perhaps we could convert RCU_DATA_ME's spinlock_t into an
rwlock and do read_lock() in call_rcu()?  Then we can should be able to
call printk from inside that read_lock(), but not inside write_lock(),
which, with suitable warning comments might be acceptable.

afacit everything in call_rcu()'s *rdp is cpu-local and is protected by
local_irq_save().  rcu_ctrlblk.completed and rcu_flipped need some
protection, but a) rdp->lock isn't sufficient anyway and b)
read_lock protection would suffice.  Maybe other CPUs can alter *rdp
while __rcu_advance_callbacks() is running.

Anyway, that's all handwaving.  My point is that making rcupreempt.c
more robust and more concurrent might be an alternative fix, and might
be beneficial in its own right.  Working out the details is what we
have Pauls for ;)