Re: [PATCH v3] printk: Relocate wake_klogd check close to the end of console_unlock()

[Petr Mladek]

On Mon 2018-02-26 15:37:35, Sergey Senozhatsky wrote:
> On (02/19/18 17:01), Petr Mladek wrote:
> [..]
> > -	raw_spin_lock(&logbuf_lock);
> >  	retry = console_seq != log_next_seq;
> > +	/*
> > +	 * Check whether userland needs notification. Do this only when really
> > +	 * leaving to avoid race with console_trylock_spinning().
> > +	 */
> > +	if (seen_seq != log_next_seq && !retry) {
> > +		wake_klogd = true;
> > +		seen_seq = log_next_seq;
> > +	}
> 
> Let's add the "why" part. This "!retry" might be hard to understand. We
> are looking at
> 
> - CPUa is about to leave console_unlock()
> - printk on CPUb appends a new message
> - CPUa detects that `console_seq != log_next_seq', updates `seen_seq'
> - printk on CPUb is getting preempted
> - CPUa re-takes the console_sem via retry path
> - printk CPUb is becoming TASK_RUNNING again - it now spins for console_sem,
>   since we have an active console_sem owner
> - CPUa detects that there is a console_sem waiter, so it offloads the
>   printing task, without ever waking up klogd

Yes, this is the scenario. I agree that it is very tricky.

> Either we can have that complex "seen_seq != log_next_seq && !retry"
> check - or we simply can add
> 
> 	if (console_lock_spinning_disable_and_check()) {
> 		printk_safe_exit_irqrestore(flags);
> 		if (wake_klogd)
> 			wake_up_klogd();
> 	}
> 
> to the offloading return path.
> 
> The later is *may be* simpler to follow. The rule is: every
> !console_suspend and !cant-use-consoles return path from console_unlock()
> must wake_up_klogd() [if needed].

Yes, this looks more straighforward. It might cause earlier and more
often wakeup but we want to go this way anyway. I do not see any
real problem with it.

Okay, what about the following patch for 4.16-rc4?