Received-SPF: pass (google.com: domain of linux-kernel+bounces-91561-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 4 Mar 2024 21:35:38 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Linux Trace Kernel
 <linux-trace-kernel@vger.kernel.org>, Masami Hiramatsu
 <mhiramat@kernel.org>, Linus Torvalds <torvalds@linux-foundation.org>,
 Sachin Sant <sachinp@linux.ibm.com>
Subject: Re: [PATCH] tracing: Have trace_marker writes be just half of
 TRACE_SEQ_SIZE
Message-ID: <20240304213538.13fe1f3b@gandalf.local.home>
In-Reply-To: <cef0b07e-f90d-4759-ae54-0f091e87edab@efficios.com>
References: <20240304192710.4c99677c@gandalf.local.home>
	<469d31a7-f358-4547-bb17-0979b3515924@efficios.com>
	<20240304203516.45b7a551@gandalf.local.home>
	<20240304204119.7503ab0b@gandalf.local.home>
	<91f27ba1-15a4-402d-8301-e2b9d23f64b0@efficios.com>
	<20240304205943.081bea96@gandalf.local.home>
	<cef0b07e-f90d-4759-ae54-0f091e87edab@efficios.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 4 Mar 2024 21:18:13 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> On 2024-03-04 20:59, Steven Rostedt wrote:
> > On Mon, 4 Mar 2024 20:42:39 -0500
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >   
> >> #define TRACE_OUTPUT_META_DATA_MAX_LEN		80
> >>
> >> and a runtime check in the code generating this header.
> >>
> >> This would avoid adding an unchecked upper limit.  
> > 
> > That would be a lot of complex code that is for debugging something that
> > has never happened in the past 16 years and Linus has already reprimanded
> > me on doing such things.  
> 
> Is it more complex than remembering the iterator position in
> print_trace_fmt() right before:
> 
>          if (tr->trace_flags & TRACE_ITER_CONTEXT_INFO) {
>                  if (iter->iter_flags & TRACE_FILE_LAT_FMT)
>                          trace_print_lat_context(iter);
>                  else
>                          trace_print_context(iter);
>          }

You forgot all of theses:

	if (iter->ent->type == TRACE_BPUTS &&
			trace_flags & TRACE_ITER_PRINTK &&
			trace_flags & TRACE_ITER_PRINTK_MSGONLY)
		return trace_print_bputs_msg_only(iter);

	if (iter->ent->type == TRACE_BPRINT &&
			trace_flags & TRACE_ITER_PRINTK &&
			trace_flags & TRACE_ITER_PRINTK_MSGONLY)
		return trace_print_bprintk_msg_only(iter);

	if (iter->ent->type == TRACE_PRINT &&
			trace_flags & TRACE_ITER_PRINTK &&
			trace_flags & TRACE_ITER_PRINTK_MSGONLY)
		return trace_print_printk_msg_only(iter);

	if (trace_flags & TRACE_ITER_BIN)
		return print_bin_fmt(iter);

	if (trace_flags & TRACE_ITER_HEX)
		return print_hex_fmt(iter);

	if (trace_flags & TRACE_ITER_RAW)
		return print_raw_fmt(iter);

	return print_trace_fmt(iter);

> 
> and then checking just after that the offset is not beyond 128
> bytes ? Perhaps there is even something internal to "iter"
> that already knows about this offset (?).
> 
> This would catch any context going beyond its planned upper
> limit early. Failing early and not just in rare corner cases
> is good.
> 
> And it's not for debugging, it's for validation of assumptions
> made about an upper bound limit defined for a compile-time
> check, so as the code evolves issues are caught early.

validating is debugging.

Seriously Mathieu. Why do we need that? The BUILD_BUG_ON() itself is
probably not even needed. Why do all this just to prevent an unlikely
situation of an event being dropped from printing?

It's not even lost (unless they are using trace_pipe, which very few people
use). If you see a "LINE TOO BIG" you can run:

  # trace-cmd extract
  # trace-cmd report

Which will pull out the raw data where the kernel trace_seq doesn't matter
and trace_cmd will handle it just fine (its trace_seq is dynamically
allocated and can increase in size when needed).

-- Steve