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 <[email protected]>
> Suggested-by: Petr Mladek <[email protected]>
> ---
> 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
