Re: [PATCH v4] printk: Add console owner and waiter logic to load balance console writes

[Petr Mladek]

On Fri 2017-11-24 16:54:16, Petr Mladek wrote:
> On Wed 2017-11-08 10:27:23, Steven Rostedt wrote:
> > If there is a waiter, then it breaks out of the loop, clears the waiter
> > flag (because that will release the waiter from its spin), and exits.
> > Note, it does *not* release the console semaphore. Because it is a
> > semaphore, there is no owner.
> 
> > Index: linux-trace.git/kernel/printk/printk.c
> > ===================================================================
> > --- linux-trace.git.orig/kernel/printk/printk.c
> > +++ linux-trace.git/kernel/printk/printk.c
> > @@ -86,8 +86,15 @@ EXPORT_SYMBOL_GPL(console_drivers);
> >  static struct lockdep_map console_lock_dep_map = {
> >  	.name = "console_lock"
> >  };
> > +static struct lockdep_map console_owner_dep_map = {
> > +	.name = "console_owner"
> > +};
> >  #endif
> >  
> > +static DEFINE_RAW_SPINLOCK(console_owner_lock);
> > +static struct task_struct *console_owner;
> > +static bool console_waiter;
> > +
> >  enum devkmsg_log_bits {
> >  	__DEVKMSG_LOG_BIT_ON = 0,
> >  	__DEVKMSG_LOG_BIT_OFF,
> > @@ -1753,8 +1760,56 @@ asmlinkage int vprintk_emit(int facility
> >  		 * semaphore.  The release will print out buffers and wake up
> >  		 * /dev/kmsg and syslog() users.
> >  		 */
> > -		if (console_trylock())
> > +		if (console_trylock()) {
> >  			console_unlock();
> > +		} else {
> > +			struct task_struct *owner = NULL;
> > +			bool waiter;
> > +			bool spin = false;
> > +
> > +			printk_safe_enter_irqsave(flags);
> > +
> > +			raw_spin_lock(&console_owner_lock);
> > +			owner = READ_ONCE(console_owner);
> > +			waiter = READ_ONCE(console_waiter);
> > +			if (!waiter && owner && owner != current) {
> > +				WRITE_ONCE(console_waiter, true);
> > +				spin = true;
> > +			}
> > +			raw_spin_unlock(&console_owner_lock);
> > +
> > +			/*
> > +			 * If there is an active printk() writing to the
> > +			 * consoles, instead of having it write our data too,
> > +			 * see if we can offload that load from the active
> > +			 * printer, and do some printing ourselves.
> > +			 * Go into a spin only if there isn't already a waiter
> > +			 * spinning, and there is an active printer, and
> > +			 * that active printer isn't us (recursive printk?).
> > +			 */
> > +			if (spin) {
> > +				/* We spin waiting for the owner to release us */
> > +				spin_acquire(&console_owner_dep_map, 0, 0, _THIS_IP_);
> > +				/* Owner will clear console_waiter on hand off */
> > +				while (READ_ONCE(console_waiter))
> > +					cpu_relax();
> > +
> > +				spin_release(&console_owner_dep_map, 1, _THIS_IP_);
> > +				printk_safe_exit_irqrestore(flags);
> > +
> > +				/*
> > +				 * The owner passed the console lock to us.
> > +				 * Since we did not spin on console lock, annotate
> > +				 * this as a trylock. Otherwise lockdep will
> > +				 * complain.
> > +				 */
> > +				mutex_acquire(&console_lock_dep_map, 0, 1, _THIS_IP_);
> 
> I am not sure that this correctly imitates the real lock
> dependency. The trylock flag means that we are able to skip
> this section when the lock is taken elsewhere. But it is not
> the whole truth. In fact, we are blocked in this code path
> when console_sem is taken by another process.
> 
> The use of console_owner_lock is not enough. The other
> console_sem calls a lot of code outside the section
> guarded by console_owner_lock.
> 
> I think that we have actually entered the cross-release bunch
> of problems, see https://lwn.net/Articles/709849/
> 
> IMHO, we need to use struct lockdep_map_cross for
> console_lock_dep_map. Also we need to put somewhere
> lock_commit_crosslock().
> 
> I am going to play with it. Also I add Byungchul Park into CC.
> This is why I keep most of the context in this reply (I am sorry
> for it).

See my first attempt below. I do not get any lockdep
warning but it is possible that I just did it wrong.