Received-SPF: pass (google.com: domain of linux-kernel+bounces-26607-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 16 Jan 2024 08:48:33 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Vincent Donnefort <vdonnefort@google.com>, Masami Hiramatsu
 <mhiramat@kernel.org>, linux-kernel@vger.kernel.org,
 linux-trace-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
 kernel-team@android.com
Subject: Re: [PATCH v11 2/5] ring-buffer: Introducing ring-buffer mapping
 functions
Message-Id: <20240116084833.f5537b00bae88afb0249492f@kernel.org>
In-Reply-To: <20240115112359.65dcecbf@rorschach.local.home>
References: <20240111161712.1480333-1-vdonnefort@google.com>
	<20240111161712.1480333-3-vdonnefort@google.com>
	<20240115134303.1a673e37b8e7d35a33d8df52@kernel.org>
	<ZaVRO76JxaHjPwCi@google.com>
	<20240115110938.613380ca@rorschach.local.home>
	<20240115112359.65dcecbf@rorschach.local.home>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 15 Jan 2024 11:23:59 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 15 Jan 2024 11:09:38 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > No. The ring buffer logic should not care if the user of it is swapping
> > the entire ring buffer or not. It only cares if parts of the ring
> > buffer is being swapped or not. That's not the level of scope it should
> > care about. If we do not want a swap to happen in update_max_tr()
> > that's not ring_buffer.c's problem. The code to prevent that from
> > happening should be 100% in trace.c.
> 
> What needs to be done, and feel free to add this as a separate patch,
> is to have checks where snapshot is used.
> 
>   (All errors return -EBUSY)
> 
> Before allowing mapping, check to see if:
> 
>  1) the current tracer has "use_max_tr" set.
>  2) any event has a "snapshot" trigger set
>  3) Any function has a "snapshot" command set
> 
> Fail if any of the above is true.
> 
> Also in reverse, if the buffer is mapped, then fail:
> 
>  1) a tracer being set that has "use_max_tr" set.
>  2) a "snapshot" command being set on a function
>  3) a "snapshot" trigger being set on an event.
> 
> For the last two, we may be able to get away with just a below as well.
> Adding the tr->flags bit. We could also add a tr->snapshot count to
> keep track of everything that is using a snapshot, and if that count is
> non-zero, mapping fails.

BTW, if we allow mapping per-cpu ring buffer, we may need "tr->mapped"
counter in addition to per-cpu mapped bit. Then we can just check
tr->snapshot at mapping, and tr->mapped at preparing snapshot.

Thank you,

> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 2a7c6fd934e9..f534f74ae80f 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1175,6 +1175,12 @@ static void tracing_snapshot_instance_cond(struct trace_array *tr,
>  		return;
>  	}
>  
> +	if (tr->flags & TRACE_ARRAY_FL_MAPPED) {
> +		trace_array_puts(tr, "*** BUFFER IS MEMORY MAPPED ***\n");
> +		trace_array_puts(tr, "*** Can not use snapshot (sorry) ***\n");
> +		return;
> +	}
> +
>  	local_irq_save(flags);
>  	update_max_tr(tr, current, smp_processor_id(), cond_data);
>  	local_irq_restore(flags);
> 
> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>