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, ®s->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
