Re: [PATCH 6/9] printk: Release lockbuf_lock before callingconsole_trylock_for_printk()

[Steven Rostedt]

On Mon, 23 Dec 2013 21:39:27 +0100
Jan Kara <jack@xxxxxxx> wrote:

> There's no reason to hold lockbuf_lock when entering
> console_trylock_for_printk(). The first thing this function does is
> calling down_trylock(console_sem) and if that fails it immediately
> unlocks lockbuf_lock. So lockbuf_lock isn't needed for that branch.
> When down_trylock() succeeds, the rest of console_trylock() is OK
> without lockbuf_lock (it is called without it from other places), and
> the only remaining thing in console_trylock_for_printk() is
> can_use_console() call. For that call console_sem is enough (it
> iterates all consoles and checks CON_ANYTIME flag).
> 
> So we drop logbuf_lock before entering console_trylock_for_printk()
> which simplifies the code.

I'm very nervous about this change. The interlocking between console
lock and logbuf_lock seems to be very subtle. Especially the comment
where logbuf_lock is defined:

/*
 * The logbuf_lock protects kmsg buffer, indices, counters. It is also
 * used in interesting ways to provide interlocking in console_unlock();
 */

Unfortunately, it does not specify what those "interesting ways" are.


Now what I think this does is to make sure whoever wrote to the logbuf
first, does the flushing. With your change we now have:

	CPU 0				CPU 1
	-----				-----
   printk("blah");
   lock(logbuf_lock);

				printk("bazinga!");
				lock(logbuf_lock);
				<blocked>

   unlock(logbuf_lock);
   < NMI comes in delays CPU>

				<get logbuf_lock>
				unlock(logbuf_lock)
				console_trylock_for_printk()
				console_unlock();
				< dumps output >

  
Now is this a bad thing? I don't know. But the current locking will
make sure that the first writer into logbuf_lock gets to do the
dumping, and all the others will just add onto that writer.

Your change now lets the second or third or whatever writer into printk
be the one that dumps the log.

Again, this may not be a big deal, but as printk is such a core part of
the Linux kernel, and this is a very subtle change, I rather be very
cautious here and try to think what can go wrong when this happens.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/