Re: [RFC][PATCHv2 1/8] printk: move printk_pending out of per-cpu

[Petr Mladek]

On Wed 2017-03-29 18:25:04, Sergey Senozhatsky wrote:
> Do not keep `printk_pending' in per-CPU area. We set the following bits
> of printk_pending:
> a) PRINTK_PENDING_WAKEUP
> 	when we need to wakeup klogd
> b) PRINTK_PENDING_OUTPUT
> 	when there is a pending output from deferred printk and we need
> 	to call console_unlock().
> 
> So none of the bits control/represent a state of a particular CPU and,
> basically, they should be global instead.
> 
> Besides we will use `printk_pending' to control printk kthread, so this
> patch is also a preparation work.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> Suggested-by: Petr Mladek <pmladek@xxxxxxxx>
> ---
>  kernel/printk/printk.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 2984fb0f0257..2d07678e9ff9 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -401,6 +401,14 @@ DEFINE_RAW_SPINLOCK(logbuf_lock);
>  		printk_safe_exit_irqrestore(flags);	\
>  	} while (0)
>  
> +/*
> + * Delayed printk version, for scheduler-internal messages:
> + */
> +#define PRINTK_PENDING_WAKEUP	0x01
> +#define PRINTK_PENDING_OUTPUT	0x02
> +
> +static unsigned long printk_pending;
> +
>  #ifdef CONFIG_PRINTK
>  DECLARE_WAIT_QUEUE_HEAD(log_wait);
>  /* the next printk record to read by syslog(READ) or /proc/kmsg */
> @@ -2654,25 +2662,15 @@ static int __init printk_late_init(void)
>  late_initcall(printk_late_init);
>  
>  #if defined CONFIG_PRINTK
> -/*
> - * Delayed printk version, for scheduler-internal messages:
> - */
> -#define PRINTK_PENDING_WAKEUP	0x01
> -#define PRINTK_PENDING_OUTPUT	0x02
> -
> -static DEFINE_PER_CPU(int, printk_pending);
> -
>  static void wake_up_klogd_work_func(struct irq_work *irq_work)
>  {
> -	int pending = __this_cpu_xchg(printk_pending, 0);
> -
> -	if (pending & PRINTK_PENDING_OUTPUT) {
> +	if (test_and_clear_bit(PRINTK_PENDING_OUTPUT, &printk_pending)) {
>  		/* If trylock fails, someone else is doing the printing */
>  		if (console_trylock())
>  			console_unlock();
>  	}
>  
> -	if (pending & PRINTK_PENDING_WAKEUP)
> +	if (test_and_clear_bit(PRINTK_PENDING_WAKEUP, &printk_pending))
>  		wake_up_interruptible(&log_wait);
>  }
>  
> @@ -2685,7 +2683,7 @@ void wake_up_klogd(void)
>  {
>  	preempt_disable();
>  	if (waitqueue_active(&log_wait)) {
> -		this_cpu_or(printk_pending, PRINTK_PENDING_WAKEUP);
> +		set_bit(PRINTK_PENDING_WAKEUP, &printk_pending);

We should add here a write barrier:

	/*
	 * irq_work_queue() uses cmpxchg() and implies the memory
	 * barrier only when the work is queued. An explicit barrier
	 * is needed here to make sure that wake_up_klogd_work_func()
	 * sees printk_pending set even when the work was already queued
	 * because of an other pending event.
	 */
	 smp_wmb();

>  		irq_work_queue(this_cpu_ptr(&wake_up_klogd_work));
>  	}
>  	preempt_enable();
> @@ -2701,7 +2699,7 @@ int printk_deferred(const char *fmt, ...)
>  	r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, 0, fmt, args);
>  	va_end(args);
>  
> -	__this_cpu_or(printk_pending, PRINTK_PENDING_OUTPUT);
> +	set_bit(PRINTK_PENDING_OUTPUT, &printk_pending);

Same here.

We are going to use printk_pending even more in the other patches.
I still have to check the final state. It might be useful
to define a helper if the barrier is needed on more locations.

Otherwise the change looks fine to me. It should reduce redundant
wakeups.

Best Regards,
Petr