Received-SPF: pass (google.com: domain of linux-kernel+bounces-91487-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <469d31a7-f358-4547-bb17-0979b3515924@efficios.com>
Date: Mon, 4 Mar 2024 20:15:57 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] tracing: Have trace_marker writes be just half of
 TRACE_SEQ_SIZE
Content-Language: en-US
To: Steven Rostedt <rostedt@goodmis.org>, LKML
 <linux-kernel@vger.kernel.org>,
 Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>,
 Linus Torvalds <torvalds@linux-foundation.org>,
 Sachin Sant <sachinp@linux.ibm.com>
References: <20240304192710.4c99677c@gandalf.local.home>
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
In-Reply-To: <20240304192710.4c99677c@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2024-03-04 19:27, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Since the size of trace_seq's buffer is the max an event can output, have
> the trace_marker be half of the entire TRACE_SEQ_SIZE, which is 4K. That
> will keep writes that has meta data written from being dropped (but
> reported), because the total output of the print event is greater than
> what the trace_seq can hold.

Defining the trace_mark limit in terms of "TRACE_SEQ_SIZE / 2"
seems backwards. It's basically using a define of the maximum
buffer size for the pretty-printing output and defining the maximum
input size of a system call to half of that.

I'd rather see, in a header file shared between tracing mark
write implementation and output implementation:

#define TRACING_MARK_MAX_SIZE	4096

and then a static validation that this input fits within your
pretty printing output in the output implementation file:

BUILD_BUG_ON(TRACING_MARK_MAX_SIZE + sizeof(meta data stuff...) > TRACE_SEQ_SIZE);

This way we clearly document that the tracing mark write
input limit is 4kB, rather than something derived from
the size of an output buffer.

Thanks,

Mathieu

> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>   kernel/trace/trace.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 8198bfc54b58..d68544aef65f 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7320,6 +7320,17 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>   	if ((ssize_t)cnt < 0)
>   		return -EINVAL;
>   
> +	/*
> +	 * TRACE_SEQ_SIZE is the total size of trace_seq buffer used
> +	 * for output. As the print event outputs more than just
> +	 * the string written, keep it smaller than the trace_seq
> +	 * as it could drop the event if the extra data makes it bigger
> +	 * than what the trace_seq can hold. Half he TRACE_SEQ_SIZE
> +	 * is more than enough.
> +	 */
> +	if (cnt > TRACE_SEQ_SIZE / 2)
> +		cnt = TRACE_SEQ_SIZE / 2;
> +
>   	meta_size = sizeof(*entry) + 2;  /* add '\0' and possible '\n' */
>    again:
>   	size = cnt + meta_size;
> @@ -7328,11 +7339,6 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>   	if (cnt < FAULTED_SIZE)
>   		size += FAULTED_SIZE - cnt;
>   
> -	if (size > TRACE_SEQ_BUFFER_SIZE) {
> -		cnt -= size - TRACE_SEQ_BUFFER_SIZE;
> -		goto again;
> -	}
> -
>   	buffer = tr->array_buffer.buffer;
>   	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
>   					    tracing_gen_ctx());

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com