Re: perf_event_open.2: mmap ring buffer requirement for receiving overflow notifications

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Dec 04, 2024 at 01:28:01PM +0100, Peter Zijlstra wrote:
> On Mon, Dec 02, 2024 at 12:53:56PM -0800, Namhyung Kim wrote:
> > Hi Peter and Ingo,
> > 
> > On Tue, Nov 26, 2024 at 05:13:45PM -0500, Vince Weaver wrote:
> > > On Sat, 23 Nov 2024, Mikołaj Kołek wrote:
> > > 
> > > > findings. In the MMAP layout section of the page, you can find this
> > > > sentence:
> > > > Before Linux 2.6.39, there is a bug that means you must allocate
> > > > an mmap ring buffer when sampling even if you do not plan to
> > > > access it.
> > > > Unless I'm somehow misunderstanding it, this statement does not seem
> > > > to be well worded, or alternatively this bug does not seem to be
> > > > fixed.
> > > 
> > > That text was probably written by me.
> > > 
> > > I tried looking at the 2.6.39 code, my perf_tests, and also PAPI which was 
> > > where the problem was probably noticed but I can't find a firm reference 
> > > for how the issue was fixed.
> > > 
> > > If I recall, the problem was if you were trying to create a sampling event 
> > > without mmap (say you want to get a signal every 100,000 retired 
> > > instructions, but you don't actually want any sample data).  I think 
> > > before 2.6.39 if you tried setting that up you'd get some sort of error 
> > > (an EINVAL?) when trying to start(?) the event.
> > > 
> > > It is possible this wasn't fixed.  I tried to be pretty good 
> > > about putting relevant git commits as comments in the manpage but there 
> > > doesn't seem to be one for that part of the text.  I'm guessing it was 
> > > PeterZ doing the work on this so maybe he remembers.
> > 
> > Do you remember what was the issue exactly on sampling events w/o mmap?
> 
> I can barely remember last release :/ But looking at the code, I don't
> see a reason that wouldn't work today.
> 
> Only the overflow handler should care about there being a buffer, and
> that just NOPs out if there isn't -- expensively, if this is a common
> thing, this could perhaps be optimized a little.
> 
> 
> ---
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4c6f6c286b2d..190b5c3cec10 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8117,6 +8117,8 @@ __perf_event_output(struct perf_event *event,
>  
>  	/* protect the callchain buffers */
>  	rcu_read_lock();
> +	if (unlikely(!event->rb))
> +		goto exit;

We have a similar logic in __perf_output_begin() and it checks with the
parent event due to inheritance.  But doing it here would save some
cycles for preparing samples to be dropped.

Thanks,
Namhyung

>  
>  	perf_prepare_sample(data, event, regs);
>  	perf_prepare_header(&header, data, event, regs);