Messages in this thread | | | Date | Wed, 24 Aug 2016 10:14:20 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [PATCH][RFC] printk: make pr_cont buffer per-cpu |
| |
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
| |