Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 12:21:29 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Jim Cromie <jim.cromie@gmail.com>, rostedt@goodmis.org,
        linux-kernel@vger.kernel.org,
        Vincent Whitchurch <vincent.whitchurch@axis.com>,
        daniel@ffwll.ch, sergey.senozhatsky@gmail.com,
        Simon Ser <contact@emersion.fr>,
        Sean Paul <seanpaul@chromium.org>
Subject: Re: [RFC PATCH 1/1] tracefs: add TP_printk_no_nl - RFC
Message-ID: <ZMeLKTzSWh3kbw8c@alley>
References: <20230730222134.54547-1-jim.cromie@gmail.com>
 <87y1iwlc80.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87y1iwlc80.fsf@jogness.linutronix.de>
Precedence: bulk

On Mon 2023-07-31 10:31:03, John Ogness wrote:
> On 2023-07-30, Jim Cromie <jim.cromie@gmail.com> wrote:
> > This variant of TP_printk() does *not* add the trailing newline.
> >
> > https://lore.kernel.org/lkml/20200825153338.17061-1-vincent.whitchurch@axis.com/
> >
> > taught dyndbg to send pr_debug() msgs to tracefs, via -x/T flag.
> >
> > It "reused" the include/trace/events/printk.h console event,
> > which does the following:
> >
> > 	TP_fast_assign(
> > 		/*
> > 		 * Each trace entry is printed in a new line.
> > 		 * If the msg finishes with '\n', cut it off
> > 		 * to avoid blank lines in the trace.
> > 		 */
> > 		if ((len > 0) && (text[len-1] == '\n'))
> > 			len -= 1;
> >
> > 		memcpy(__get_str(msg), text, len);
> > 		__get_str(msg)[len] = 0;
> > 	),
> >
> > That trim work could be avoided, *iff* all pr_debug() callers are
> > known to have no '\n' to strip.  While thats not true for *all*
> > callsites, it is 99+% true for DRM.debug callsites, and can be made
> > true for some subsets of prdbg/dyndbg callsites.
> 
> Note that the trailing '\n' in printk messages has a purpose.  It
> finalizes commitment of the message to the ringbuffer so that the
> message cannot be extended by any LOG_CONT printk's that may come along
> (from any context).

Also the message is printed on consoles only when finalized. As a
result, the last non-finalized message might not be visible on
consoles for hours or days.

> If it is not intended that a message is extended, then that message
> should include a trailing '\n'.
> 
> Grepping through the kernel, I am surprised how many messages are
> missing the trailing newline when it is obvious they are not intended to
> be extended. I consider these bugs.

I consider this bug as well.

My understanding is that this patch affects only messages printed to
the ftrace buffer so that it does not affect printk() directly.

But still. It creates a habit that is bad for printk(). Also relies
on the fact that people are aware of this macro and use it. IMHO,
it will not work in practice. Or do I miss something?

Best Regards,
Petr