From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755371AbcHXBOP (ORCPT <rfc822;w@1wt.eu>);
        Tue, 23 Aug 2016 21:14:15 -0400
Received: from mail-pa0-f65.google.com ([209.85.220.65]:35334 "EHLO
        mail-pa0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755246AbcHXBOO (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 23 Aug 2016 21:14:14 -0400
Date: Wed, 24 Aug 2016 10:14:20 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Jan Kara <jack@suse.cz>, Kay Sievers <kay@vrfy.org>,
        Tejun Heo <tj@kernel.org>, Calvin Owens <calvinowens@fb.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [PATCH][RFC] printk: make pr_cont buffer per-cpu
Message-ID: <20160824011420.GA452@swordfish>
References: <20160822154030.2715-1-sergey.senozhatsky@gmail.com>
 <20160823051831.GA423@swordfish>
 <20160823114702.GC4866@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160823114702.GC4866@pathway.suse.cz>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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