Messages in this thread Patch in this message | | | Date | Fri, 31 Mar 2017 15:09:50 +0200 | From | Petr Mladek <> | Subject | Re: [RFC][PATCHv2 1/8] printk: move printk_pending out of per-cpu |
| |
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@gmail.com> > Suggested-by: Petr Mladek <pmladek@suse.com> > --- > 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
| |