[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <469d31a7-f358-4547-bb17-0979b3515924@efficios.com>
Date: Mon, 4 Mar 2024 20:15:57 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Steven Rostedt <rostedt@...dmis.org>, LKML
<linux-kernel@...r.kernel.org>,
Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Sachin Sant <sachinp@...ux.ibm.com>
Subject: Re: [PATCH] tracing: Have trace_marker writes be just half of
TRACE_SEQ_SIZE
On 2024-03-04 19:27, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@...dmis.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@...dmis.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
Powered by blists - more mailing lists