On Fri, 23 Jan 2015, Daniel Thompson wrote:
> +#ifdef CONFIG_ARCH_WANT_NMI_PRINTK
> +extern __printf(1, 0) int nmi_vprintk(const char *fmt, va_list args);
> +
> +struct cpumask;
> +extern int prepare_nmi_printk(struct cpumask *cpus);
> +extern void complete_nmi_printk(struct cpumask *cpus);
> +
> +/*
> + * Replace printk to write into the NMI seq.
> + *
> + * To avoid include hell this is a macro rather than an inline function
> + * (printk_func is not declared in this header file).
> + */
> +#define this_cpu_begin_nmi_printk() ({ \
> + printk_func_t __orig = this_cpu_read(printk_func); \
> + this_cpu_write(printk_func, nmi_vprintk); \
> + __orig; \
> +})
> +#define this_cpu_end_nmi_printk(fn) this_cpu_write(printk_func, fn)
Why can't we just make it a proper function in printk.c and make
DEFINE_PER_CPU(printk_func_t, printk_func) static once x86 is
converted over, thereby getting rid of the misplaced declaration in
percpu.h?
It's really not performance critical at all. If you do system wide
backtraces a function call is the least of your worries.
> +#ifdef CONFIG_ARCH_WANT_NMI_PRINTK
Why can't this simply be CONFIG_PRINTK_NMI and live at the same place
as the other printk related options?
> +int nmi_vprintk(const char *fmt, va_list args)
> +{
> + struct nmi_seq_buf *s = this_cpu_ptr(&nmi_print_seq);
> + unsigned int len = seq_buf_used(&s->seq);
> +
> + seq_buf_vprintf(&s->seq, fmt, args);
> + return seq_buf_used(&s->seq) - len;
> +}
> +EXPORT_SYMBOL_GPL(nmi_vprintk);
What's the point of these exports? This stuff is really not supposed
to be used inside random modules.
> +/*
> + * Check for concurrent usage and set up per_cpu seq_buf buffers that the
> NMIs
> + * running on the other CPUs will write to. Provides the mask of CPUs it is
> + * safe to write from (i.e. a copy of the online mask).
> + */
> +int prepare_nmi_printk(struct cpumask *cpus)
Can we please make all this proper prefixed? , i.e. printk_nmi_*
> +{
> + struct nmi_seq_buf *s;
> + int cpu;
> +
> + if (test_and_set_bit(0, &nmi_print_flag)) {
> + /*
> + * If something is already using the NMI print facility we
> + * can't allow a second one...
> + */
> + return -EBUSY;
So what's the point of saving and restoring the printk_func pointer at
the call site?
void printk_nmi_begin()
{
if (__this_cpu_inc_return(nmi_printk_nest_level) == 1)
this_cpu_write(printk_func, nmi_vprintk);
}
void printk_nmi_end()
{
if (__this_cpu_dec_return(nmi_printk_nest_level) > 0)
return;
this_cpu_write(printk_func, default_vprintk);
if (in_nmi())
irq_work_schedule();
else
printk_nmi_complete();
}
> + }
> +
> + cpumask_copy(cpus, cpu_online_mask);
Why do you need external storage for this if nesting is not allowed?
What's wrong with having a printk_nmi_mask? It's protected by the
nmi_print_flag, so the call sites do not have to take care about
protecting it until printk_nmi_complete() has been invoked.
> + for_each_cpu(cpu, cpus) {
> + s = &per_cpu(nmi_print_seq, cpu);
> + seq_buf_init(&s->seq, s->buffer, NMI_BUF_SIZE);
Why do you want to do this here? The buffers should be initialized
before the first NMI can hit and the complete code should reinit them
before the next printk_nmi_prepare() sees the nmi_print_flag cleared.
> +static void print_seq_line(struct nmi_seq_buf *s, int start, int end)
> +{
> + const char *buf = s->buffer + start;
> +
> + printk("%.*s", (end - start) + 1, buf);
> +}
> +
> +void complete_nmi_printk(struct cpumask *cpus)
> +{
> + struct nmi_seq_buf *s;
> + int len;
> + int cpu;
> + int i;
Please condense all ints to a single line, but what's worse is the
completely inconsistency versus scopes.
len and i are only used in the for_each loop. Either we put all of
them at the top of the function or we do it right.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
