On Sat 2016-10-01 00:17:54, Sergey Senozhatsky wrote:
> This patch extends the idea of NMI per-cpu buffers to regions
> that may cause recursive printk() calls and possible deadlocks.
> diff --git a/kernel/printk/alt_printk.c b/kernel/printk/alt_printk.c
> index 7178661..4bc1e7d 100644
> --- a/kernel/printk/alt_printk.c
> +++ b/kernel/printk/alt_printk.c
> len = atomic_read(&s->len);
>
> - if (len >= sizeof(s->buffer)) {
> - atomic_inc(&nmi_message_lost);
> + if (len >= sizeof(s->buffer))
> return 0;
> - }
>
> /*
> * Make sure that all old data have been read before the buffer was
> @@ -240,6 +235,83 @@ void alt_printk_flush_on_panic(void)
> alt_printk_flush();
> }
>
> +/*
> + * Safe printk() for NMI context. It uses a per-CPU buffer to
> + * store the message. NMIs are not nested, so there is always only
> + * one writer running. But the buffer might get flushed from another
> + * CPU, so we need to be careful.
> + */
> +static int vprintk_nmi(const char *fmt, va_list args)
> +{
> + struct alt_printk_seq_buf *s = this_cpu_ptr(&nmi_print_seq);
> + int add;
> +
> + add = alt_printk_log_store(s, fmt, args);
> + if (!add)
> + atomic_inc(&nmi_message_lost);
This would could also empty string as an error. A solution might be
update alt_printk_log_store() to return -1 in case of lost log.
Note that vprintk_nmi() still needs to return 0 in this case to
stay compatible with printk().
> +
> + return add;
> +}
> +
> +void printk_nmi_enter(void)
> +{
> + this_cpu_or(alt_printk_ctx, ALT_PRINTK_NMI_CONTEXT_MASK);
> +}
> +
> +void printk_nmi_exit(void)
> +{
> + this_cpu_and(alt_printk_ctx, ~ALT_PRINTK_NMI_CONTEXT_MASK);
> +}
> +
> +/*
> + * Lockless printk(), to avoid deadlocks should the printk() recurse
> + * into itself. It uses a per-CPU buffer to store the message, just like
> + * NMI.
> + */
> +static int vprintk_alt(const char *fmt, va_list args)
> +{
> + struct alt_printk_seq_buf *s = this_cpu_ptr(&alt_print_seq);
> +
> + return alt_printk_log_store(s, fmt, args);
We should handle lost strings here as well. But it can be
done in a followup patch.
> +}
> +
> +/*
> + * Returns with local IRQs disabled.
> + * Can be preempted by NMI.
> + */
> +void alt_printk_enter(void)
> +{
> + unsigned long flags;
> + int entry_count;
> +
> + local_irq_save(flags);
> + if (!(this_cpu_read(alt_printk_ctx) & ALT_PRINTK_CONTEXT_MASK))
> + this_cpu_write(alt_printk_irq_flags, flags);
> + this_cpu_inc(alt_printk_ctx);
> +}
> +
> +/*
> + * Restores local IRQs state saved in alt_printk_enter().
> + * Can be preempted by NMI.
> + */
> +void alt_printk_exit(void)
> +{
> + this_cpu_dec(alt_printk_ctx);
> + if (!(this_cpu_read(alt_printk_ctx) & ALT_PRINTK_CONTEXT_MASK))
> + local_irq_restore(this_cpu_read(alt_printk_irq_flags));
> +}
I will discuss this in your replay that explains the details.
Anyway, it looks much easier now.
Best Regards,
Petr
