Received-SPF: pass (google.com: domain of linux-kernel+bounces-103063-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 14 Mar 2024 10:35:48 +0100
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Marco Elver <elver@google.com>
Cc: linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	Adrian Hunter <adrian.hunter@intel.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ian Rogers <irogers@google.com>, Ingo Molnar <mingo@redhat.com>,
	Jiri Olsa <jolsa@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2 2/4] perf: Enqueue SIGTRAP always via task_work.
Message-ID: <20240314093548.sVPg_3b9@linutronix.de>
References: <20240312180814.3373778-1-bigeasy@linutronix.de>
 <20240312180814.3373778-3-bigeasy@linutronix.de>
 <CANpmjNNYNxMMSdWFiPiT3i888fi8--k_H0xMX7i17VCT97vARg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CANpmjNNYNxMMSdWFiPiT3i888fi8--k_H0xMX7i17VCT97vARg@mail.gmail.com>

On 2024-03-13 15:41:18 [+0100], Marco Elver wrote:
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index c7a0274c662c8..e9926baaa1587 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -9592,14 +9572,17 @@ static int __perf_event_overflow(struct perf_event *event,
> >
> >                 if (regs)
> >                         pending_id = hash32_ptr((void *)instruction_pointer(regs)) ?: 1;
> > -               if (!event->pending_sigtrap) {
> > -                       event->pending_sigtrap = pending_id;
> > +               if (!event->pending_work) {
> > +                       event->pending_work = pending_id;
> >                         local_inc(&event->ctx->nr_pending);
> > -                       irq_work_queue(&event->pending_irq);
> > +                       WARN_ON_ONCE(!atomic_long_inc_not_zero(&event->refcount));
> > +                       task_work_add(current, &event->pending_task, TWA_RESUME);
> > +                       if (in_nmi())
> > +                               irq_work_queue(&event->pending_irq);
> 
> Some brief code comments here would help having to dig through git
> history to understand this.

Sure.

> >                 } else if (event->attr.exclude_kernel && valid_sample) {
> >                         /*
> >                          * Should not be able to return to user space without
> > -                        * consuming pending_sigtrap; with exceptions:
> > +                        * consuming pending_work; with exceptions:
> >                          *
> >                          *  1. Where !exclude_kernel, events can overflow again
> >                          *     in the kernel without returning to user space.
> > @@ -13049,6 +13032,13 @@ static void sync_child_event(struct perf_event *child_event)
> >                      &parent_event->child_total_time_running);
> >  }
> >
> > +static bool task_work_cb_match(struct callback_head *cb, void *data)
> > +{
> > +       struct perf_event *event = container_of(cb, struct perf_event, pending_task);
> > +
> > +       return event == data;
> > +}
> > +
> >  static void
> >  perf_event_exit_event(struct perf_event *event, struct perf_event_context *ctx)
> >  {
> > @@ -13088,6 +13078,11 @@ perf_event_exit_event(struct perf_event *event, struct perf_event_context *ctx)
> >                  * Kick perf_poll() for is_event_hup();
> >                  */
> >                 perf_event_wakeup(parent_event);
> > +               if (event->pending_work &&
> > +                   task_work_cancel_match(current, task_work_cb_match, event)) {
> 
> Brief comment which case this covers would be good.

Okay.

> > +                       put_event(event);
> > +                       local_dec(&event->ctx->nr_pending);
> > +               }
> >                 free_event(event);
> >                 put_event(parent_event);
> >                 return;

Sebastian