From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933467AbdCaNLS (ORCPT ); Fri, 31 Mar 2017 09:11:18 -0400 Received: from mx2.suse.de ([195.135.220.15]:43373 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933419AbdCaNJy (ORCPT ); Fri, 31 Mar 2017 09:09:54 -0400 Date: Fri, 31 Mar 2017 15:09:50 +0200 From: Petr Mladek To: Sergey Senozhatsky Cc: Steven Rostedt , Jan Kara , Andrew Morton , Linus Torvalds , Peter Zijlstra , "Rafael J . Wysocki" , Eric Biederman , Greg Kroah-Hartman , Jiri Slaby , Pavel Machek , Len Brown , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [RFC][PATCHv2 1/8] printk: move printk_pending out of per-cpu Message-ID: <20170331130950.GA3452@pathway.suse.cz> References: <20170329092511.3958-1-sergey.senozhatsky@gmail.com> <20170329092511.3958-2-sergey.senozhatsky@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170329092511.3958-2-sergey.senozhatsky@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Suggested-by: Petr Mladek > --- > 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