Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events

[Liang, Kan]

On 11/9/2020 6:04 AM, Peter Zijlstra wrote:
> On Mon, Nov 09, 2020 at 10:52:35AM +0100, Peter Zijlstra wrote:
> > On Fri, Nov 06, 2020 at 01:29:33PM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:
> > > From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > >
> > > Sometimes the PMU internal buffers have to be flushed for per-CPU events
> > > during a context switch, e.g., large PEBS. Otherwise, the perf tool may
> > > report samples in locations that do not belong to the process where the
> > > samples are processed in, because PEBS does not tag samples with PID/TID.
> > >
> > > The current code only flush the buffers for a per-task event. It doesn't
> > > check a per-CPU event.
> > >
> > > Add a new event state flag, PERF_ATTACH_SCHED_CB, to indicate that the
> > > PMU internal buffers have to be flushed for this event during a context
> > > switch.
> > >
> > > Add sched_cb_entry and perf_sched_cb_usages back to track the PMU/cpuctx
> > > which is required to be flushed.
> > >
> > > Only need to invoke the sched_task() for per-CPU events in this patch.
> > > The per-task events have been handled in perf_event_context_sched_in/out
> > > already.
> > >
> > > Fixes: 9c964efa4330 ("perf/x86/intel: Drain the PEBS buffer during context switches")
> >
> > Are you sure? In part this patch looks like a revert of:
> >
> >    44fae179ce73a26733d9e2d346da4e1a1cb94647
> >    556cccad389717d6eb4f5a24b45ff41cad3aaabf

The patch tries to fix a long term PEBS bug with per-CPU event. It's not 
a revert. I think we still want to keep the optimization for per-task 
event with 44fae179ce73 and 556cccad3897.

Here is the story.

- When the large PEBS was introduced (9c964efa4330), the sched_task() 
should be invoked to flush the PEBS buffer in each context switch. 
However, The perf_sched_events in account_event() is not updated 
accordingly. The perf_event_task_sched_* never be invoked for a pure 
per-CPU context. Only per-task event works.
   At that time, the perf_pmu_sched_task() is outside of 
perf_event_context_sched_in/out. It means that perf has to double 
perf_pmu_disable() for per-task event.


- The recent optimization (44fae179ce73 and 556cccad3897) only optimize 
the per-task event by moving the sched_task() into 
perf_event_context_sched_in/out. So perf only need to invoke 
perf_pmu_disable() once.
  But it doesn't fix the issue in a pure per-CPU context. The per-CPU 
events are still broken.


- The patch 1 tries to fix broken per-CPU events. The CPU context cannot 
be retrieved from the task->perf_event_ctxp. So it has to be tracked in 
the sched_cb_list. Yes, the code is very similar to the original codes, 
but it is actually the new code for per-CPU events. The optimization for 
per-task events is still kept.
  For the case, which has both a CPU context and a task context, yes, 
the __perf_pmu_sched_task() in this patch is not invoked. Because the 
sched_task() only need to be invoked once in a context switch. The 
sched_task() will be eventually invoked in the task context.


- The following patch 3 tries to optimize the sched_task() further.
  - We handle the per-CPU event and per-task event in different places. 
In a pure per-task context, we don't need to go through the 
sched_cb_list. We can get the information from task->perf_event_ctxp. We 
introduce a flag to distinguish them.
  - For most of the cases, we don't need to invoke the sched_task() in 
both sched in and sched out. We also introduce a flag for PMUs to avoid 
unnecessary calls.


All the implementation and optimization are generic. For example, for 
power, the BHRB entries are reset in context switch in. They can also 
benefit from the optimization.


> *groan*... I think I might've made a mistake with those two patches. I
> assumed the whole cpuctx->task_ctx thing was relevant, it is not.
>
> As per perf_sched_cb_{inc,dec}(struct pmu *), the thing we care about is
> that the *PMU* gets a context switch callback, we don't give a crap
> about the actual task context. Except that LBR code does, but I'm
> thinking that started the whole confusion -- and I'm still not sure it's
> actually correct either.
>
> Now,.. how did we end up with the above two patches anyway... /me frobs
> around in the inbox... Ah! that daft user RDPMC thing. I wanted to avoid
> yet another pmu::method().
>
> Hmm.. and the reason I proposed that change is because we'd end up with
> the sched_cb for every context switch now, not just large-pebs and or
> lbr crud. And this form avoids the double perf_pmu_disable() and all
> that.

With the patch set,  I think the double perf_pmu_disable() is still avoided.

> Maybe we can frob x86_pmu_enable()...

Could you please elaborate?

Thanks,
Kan