Received-SPF: pass (google.com: domain of linux-kernel+bounces-129982-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 3 Apr 2024 15:39:44 +0100
From: Vincent Donnefort <vdonnefort@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: mhiramat@kernel.org, linux-kernel@vger.kernel.org,
	linux-trace-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
	kernel-team@android.com, linux-mm@kvack.org
Subject: Re: [PATCH v19 RESEND 3/5] tracing: Allow user-space mapping of the
 ring-buffer
Message-ID: <Zg1qMI7JYhdeY_z7@google.com>
References: <20240326100830.1326610-1-vdonnefort@google.com>
 <20240326100830.1326610-4-vdonnefort@google.com>
 <20240329144055.0ae2dd4b@gandalf.local.home>
 <20240403101352.08c83aa4@gandalf.local.home>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240403101352.08c83aa4@gandalf.local.home>

On Wed, Apr 03, 2024 at 10:13:52AM -0400, Steven Rostedt wrote:
> On Fri, 29 Mar 2024 14:40:55 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > +static vm_fault_t tracing_buffers_mmap_fault(struct vm_fault *vmf)
> > > +{
> > > +	return VM_FAULT_SIGBUS;
> > > +}  
> > 
> > If this is all it does, I don't believe it's needed.
> > 
> > > +
> > > +#ifdef CONFIG_TRACER_MAX_TRACE
> > > +static int get_snapshot_map(struct trace_array *tr)
> > > +{
> > > +	int err = 0;
> > > +
> > > +	/*
> > > +	 * Called with mmap_lock held. lockdep would be unhappy if we would now
> > > +	 * take trace_types_lock. Instead use the specific
> > > +	 * snapshot_trigger_lock.
> > > +	 */
> > > +	spin_lock(&tr->snapshot_trigger_lock);
> > > +
> > > +	if (tr->snapshot || tr->mapped == UINT_MAX)
> > > +		err = -EBUSY;
> > > +	else
> > > +		tr->mapped++;
> > > +
> > > +	spin_unlock(&tr->snapshot_trigger_lock);
> > > +
> > > +	/* Wait for update_max_tr() to observe iter->tr->mapped */
> > > +	if (tr->mapped == 1)
> > > +		synchronize_rcu();
> > > +
> > > +	return err;
> > > +
> > > +}
> > > +static void put_snapshot_map(struct trace_array *tr)
> > > +{
> > > +	spin_lock(&tr->snapshot_trigger_lock);
> > > +	if (!WARN_ON(!tr->mapped))
> > > +		tr->mapped--;
> > > +	spin_unlock(&tr->snapshot_trigger_lock);
> > > +}
> > > +#else
> > > +static inline int get_snapshot_map(struct trace_array *tr) { return 0; }
> > > +static inline void put_snapshot_map(struct trace_array *tr) { }
> > > +#endif
> > > +
> > > +static void tracing_buffers_mmap_close(struct vm_area_struct *vma)
> > > +{
> > > +	struct ftrace_buffer_info *info = vma->vm_file->private_data;
> > > +	struct trace_iterator *iter = &info->iter;
> > > +
> > > +	WARN_ON(ring_buffer_unmap(iter->array_buffer->buffer, iter->cpu_file));
> > > +	put_snapshot_map(iter->tr);
> > > +}
> > > +
> > > +static void tracing_buffers_mmap_open(struct vm_area_struct *vma) { }  
> > 
> > Same for the open.
> > 
> > 
> > > +
> > > +static const struct vm_operations_struct tracing_buffers_vmops = {
> > > +	.open		= tracing_buffers_mmap_open,
> > > +	.close		= tracing_buffers_mmap_close,
> > > +	.fault		= tracing_buffers_mmap_fault,
> > > +};  
> > 
> > I replaced this with:
> > 
> > static const struct vm_operations_struct tracing_buffers_vmops = {
> > 	.close		= tracing_buffers_mmap_close,
> > };
> > 
> > And it appears to work just fine. The mm code handles the NULL cases for
> > .open and .fault.
> > 
> > Is there any reason to do something different than the mm defaults?

No other reason here than my own ignorance. I will remove.

> 
> Hi Vincent,
> 
> Do you plan on sending out a v20 series?

Of course, let me spin that this week! Got also few typos to fix in the doc and
I believe an include missing for riscv.

> 
> -- Steve