Re: [PATCH][RFC] printk: make pr_cont buffer per-cpu

[Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>]

Hello,

On (08/23/16 13:47), Petr Mladek wrote:
[..]
> >  	if (!(lflags & LOG_NEWLINE)) {
> > +		if (!this_cpu_read(cont_printing)) {
> > +			if (system_state == SYSTEM_RUNNING) {
> > +				this_cpu_write(cont_printing, true);
> > +				preempt_disable();
> > +			}
> > +		}
> 
> I am afraid that this is not acceptable. It means that printk() will have
> an unexpected side effect. The missing "\n" at the end of a printed
> string would disable preemption. See below for more.

missing '\n' must WARN about "sched while atomic" eventually, so it
shouldn't go unnoticed or stay hidden.

> I think that cont lines should be a corner case. There should be only
> a limited use of them. We should not make too complicated things to
> support them. Also printk() must not get harder to use because of them.
> I still see a messed output rather as a cosmetic problem in compare with
> possible possible deadlocks or hung tasks.

oh, I would love it if pr_cont() was never used in SMP. but this is not
the case, unfortunately. and, ironically, where pr_cont really matters
is debugging -- for instance, look at arch/x86/kernel/dumpstack_{32,64}.c
show_regs() or show_stack_log_lvl()

void show_regs(struct pt_regs *regs)
{
	...
	if (!user_mode(regs)) {
		pr_emerg("Stack:\n");
		show_stack_log_lvl(NULL, regs, &regs->sp, 0, KERN_EMERG);

		pr_emerg("Code:");

		ip = (u8 *)regs->ip - code_prologue;
		if (ip < (u8 *)PAGE_OFFSET || probe_kernel_address(ip, c)) {
			/* try starting at IP */
			ip = (u8 *)regs->ip;
			code_len = code_len - code_prologue + 1;
		}
		for (i = 0; i < code_len; i++, ip++) {
			if (ip < (u8 *)PAGE_OFFSET ||
					probe_kernel_address(ip, c)) {
				pr_cont("  Bad EIP value.");
				break;
			}
			if (ip == (u8 *)regs->ip)
				pr_cont(" <%02x>", c);
			else
				pr_cont(" %02x", c);
		}
	}
	pr_cont("\n");
}

or arch/x86/mm/kmemcheck/error.c
... or arch/arm/kernel/traps.c

static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
{
	unsigned int fp, mode;
	int ok = 1;

	printk("Backtrace: ");

	if (!tsk)
		tsk = current;

	if (regs) {
		fp = frame_pointer(regs);
		mode = processor_mode(regs);
	} else if (tsk != current) {
		fp = thread_saved_fp(tsk);
		mode = 0x10;
	} else {
		asm("mov %0, fp" : "=r" (fp) : : "cc");
		mode = 0x10;
	}

	if (!fp) {
		pr_cont("no frame pointer");
		ok = 0;
	} else if (verify_stack(fp)) {
		pr_cont("invalid frame pointer 0x%08x", fp);
		ok = 0;
	} else if (fp < (unsigned long)end_of_stack(tsk))
		pr_cont("frame pointer underflow");
	pr_cont("\n");

	if (ok)
		c_backtrace(fp, mode);
}

or arch/arm/mm/fault.c show_pte()... and so on and so forth.

well, I do understand what you mean and agree with it, but I'm
afraid pr_cont() kinda matters after all and people *probably*
expect it to be SMP safe (I'm not entirely sure whether all of
those pr_cont() calls were put there with the idea that the
output can be messed up and quite hard to read).

	-ss