Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
Namhyung Kim writes:
> Hello,
>
> On Mon, Nov 23, 2020 at 8:00 PM Michael Ellerman wrote:
>>
>> Namhyung Kim writes:
>> > Hi Peter and Kan,
>> >
>> > (Adding PPC folks)
>> >
>> > On Tue, Nov 17, 2020 at 2:01 PM Namhyung Kim wrote:
>> >>
>> >> Hello,
>> >>
>> >> On Thu, Nov 12, 2020 at 4:54 AM Liang, Kan
>> >> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
>> >> > > On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
>> >> > >
>> >> > >> - 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
>> >> > > only set that for large pebs. Are you sure the other users (Intel LBR
>> >> > > and PowerPC BHRB) don't need it?
>> >> >
>> >> > I didn't set it for LBR, because the perf_sched_events is always enabled
>> >> > for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
>> >> > for LBR.
>> >> >
>> >> > if (has_branch_stack(event))
>> >> > inc = true;
>> >> >
>> >> > >
>> >> > > If they indeed do not require the pmu::sched_task() callback for CPU
>> >> > > events, then I still think the whole perf_sched_cb_{inc,dec}()
>> >> > > interface
>> >> >
>> >> > No, LBR requires the pmu::sched_task() callback for CPU events.
>> >> >
>> >> > Now, The LBR registers have to be reset in sched in even for CPU events.
>> >> >
>> >> > To fix the shorter LBR callstack issue for CPU events, we also need to
>> >> > save/restore LBRs in pmu::sched_task().
>> >> > https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
>> >> >
>> >> > > is confusing at best.
>> >> > >
>> >> > > Can't we do something like this instead?
>> >> > >
>> >> > I think the below patch may have two issues.
>> >> > - PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as well)
>> >> > now.
>> >> > - We may disable the large PEBS later if not all PEBS events support
>> >> > large PEBS. The PMU need a way to notify the generic code to decrease
>> >> > the nr_sched_task.
>> >>
>> >> Any updates on this? I've reviewed and tested Kan's patches
>> >> and they all look good.
>> >>
>> >> Maybe we can talk to PPC folks to confirm the BHRB case?
>> >
>> > Can we move this forward? I saw patch 3/3 also adds PERF_ATTACH_SCHED_CB
>> > for PowerPC too. But it'd be nice if ppc folks can confirm the change.
>>
>> Sorry I've read the whole thread, but I'm still not entirely sure I
>> understand the question.
>
> Thanks for your time and sorry about not being clear enough.
>
> We found per-cpu events are not calling pmu::sched_task()
> on context switches. So PERF_ATTACH_SCHED_CB was
> added to indicate the core logic that it needs to invoke the
> callback.
OK. TBH I've never thought of using branch stack with a per-cpu event,
but I guess you can do it.
I think the same logic applies as LBR, we need to read the BHRB entries
in the context of the task that they were recorded for.
> The patch 3/3 added the flag to PPC (for BHRB) with other
> changes (I think it should be split like in the patch 2/3) and
> want to get ACKs from the PPC folks.
If you post a new version with Maddy's comments addressed then he or I
can ack it.
cheers
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On 11/24/2020 12:42 AM, Madhavan Srinivasan wrote:
On 11/24/20 10:21 AM, Namhyung Kim wrote:
Hello,
On Mon, Nov 23, 2020 at 8:00 PM Michael Ellerman
wrote:
Namhyung Kim writes:
Hi Peter and Kan,
(Adding PPC folks)
On Tue, Nov 17, 2020 at 2:01 PM Namhyung Kim
wrote:
Hello,
On Thu, Nov 12, 2020 at 4:54 AM Liang, Kan
wrote:
On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
- 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB
and
only set that for large pebs. Are you sure the other users (Intel
LBR
and PowerPC BHRB) don't need it?
I didn't set it for LBR, because the perf_sched_events is always
enabled
for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
for LBR.
if (has_branch_stack(event))
inc = true;
If they indeed do not require the pmu::sched_task() callback for CPU
events, then I still think the whole perf_sched_cb_{inc,dec}()
interface
No, LBR requires the pmu::sched_task() callback for CPU events.
Now, The LBR registers have to be reset in sched in even for CPU
events.
To fix the shorter LBR callstack issue for CPU events, we also
need to
save/restore LBRs in pmu::sched_task().
https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
is confusing at best.
Can't we do something like this instead?
I think the below patch may have two issues.
- PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as
well) now.
- We may disable the large PEBS later if not all PEBS events support
large PEBS. The PMU need a way to notify the generic code to decrease
the nr_sched_task.
Any updates on this? I've reviewed and tested Kan's patches
and they all look good.
Maybe we can talk to PPC folks to confirm the BHRB case?
Can we move this forward? I saw patch 3/3 also adds
PERF_ATTACH_SCHED_CB
for PowerPC too. But it'd be nice if ppc folks can confirm the change.
Sorry I've read the whole thread, but I'm still not entirely sure I
understand the question.
Thanks for your time and sorry about not being clear enough.
We found per-cpu events are not calling pmu::sched_task()
on context switches. So PERF_ATTACH_SCHED_CB was
added to indicate the core logic that it needs to invoke the
callback.
The patch 3/3 added the flag to PPC (for BHRB) with other
changes (I think it should be split like in the patch 2/3) and
want to get ACKs from the PPC folks.
Sorry for delay.
I guess first it will be better to split the ppc change to a separate
patch,
Both PPC and X86 invokes the perf_sched_cb_inc() directly. The patch
changes the parameters of the perf_sched_cb_inc(). I think we have to
update the PPC and X86 codes together. Otherwise, there will be a
compile error, if someone may only applies the change for the
perf_sched_cb_inc() but forget to applies the changes in PPC or X86
specific codes.
secondly, we are missing the changes needed in the power_pmu_bhrb_disable()
where perf_sched_cb_dec() needs the "state" to be included.
Ah, right. The below patch should fix the issue.
diff --git a/arch/powerpc/perf/core-book3s.c
b/arch/powerpc/perf/core-book3s.c
index bced502f64a1..6756d1602a67 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -391,13 +391,18 @@ static void power_pmu_bhrb_enable(struct
perf_event *event)
static void power_pmu_bhrb_disable(struct perf_event *event)
{
struct cpu_hw_events *cpuhw = this_cpu_ptr(_hw_events);
+ int state = PERF_SCHED_CB_SW_IN;
if (!ppmu->bhrb_nr)
return;
WARN_ON_ONCE(!cpuhw->bhrb_users);
cpuhw->bhrb_users--;
- perf_sched_cb_dec(event->ctx->pmu);
+
+ if (!(event->attach_state & PERF_ATTACH_TASK))
+ state |= PERF_SCHED_CB_CPU;
+
+ perf_sched_cb_dec(event->ctx->pmu, state);
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On 11/24/20 10:21 AM, Namhyung Kim wrote:
Hello,
On Mon, Nov 23, 2020 at 8:00 PM Michael Ellerman wrote:
Namhyung Kim writes:
Hi Peter and Kan,
(Adding PPC folks)
On Tue, Nov 17, 2020 at 2:01 PM Namhyung Kim wrote:
Hello,
On Thu, Nov 12, 2020 at 4:54 AM Liang, Kan wrote:
On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
- 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
only set that for large pebs. Are you sure the other users (Intel LBR
and PowerPC BHRB) don't need it?
I didn't set it for LBR, because the perf_sched_events is always enabled
for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
for LBR.
if (has_branch_stack(event))
inc = true;
If they indeed do not require the pmu::sched_task() callback for CPU
events, then I still think the whole perf_sched_cb_{inc,dec}() interface
No, LBR requires the pmu::sched_task() callback for CPU events.
Now, The LBR registers have to be reset in sched in even for CPU events.
To fix the shorter LBR callstack issue for CPU events, we also need to
save/restore LBRs in pmu::sched_task().
https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
is confusing at best.
Can't we do something like this instead?
I think the below patch may have two issues.
- PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as well) now.
- We may disable the large PEBS later if not all PEBS events support
large PEBS. The PMU need a way to notify the generic code to decrease
the nr_sched_task.
Any updates on this? I've reviewed and tested Kan's patches
and they all look good.
Maybe we can talk to PPC folks to confirm the BHRB case?
Can we move this forward? I saw patch 3/3 also adds PERF_ATTACH_SCHED_CB
for PowerPC too. But it'd be nice if ppc folks can confirm the change.
Sorry I've read the whole thread, but I'm still not entirely sure I
understand the question.
Thanks for your time and sorry about not being clear enough.
We found per-cpu events are not calling pmu::sched_task()
on context switches. So PERF_ATTACH_SCHED_CB was
added to indicate the core logic that it needs to invoke the
callback.
The patch 3/3 added the flag to PPC (for BHRB) with other
changes (I think it should be split like in the patch 2/3) and
want to get ACKs from the PPC folks.
Sorry for delay.
I guess first it will be better to split the ppc change to a separate patch,
secondly, we are missing the changes needed in the power_pmu_bhrb_disable()
where perf_sched_cb_dec() needs the "state" to be included.
Maddy
Thanks,
Namhyung
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
Hello,
On Mon, Nov 23, 2020 at 8:00 PM Michael Ellerman wrote:
>
> Namhyung Kim writes:
> > Hi Peter and Kan,
> >
> > (Adding PPC folks)
> >
> > On Tue, Nov 17, 2020 at 2:01 PM Namhyung Kim wrote:
> >>
> >> Hello,
> >>
> >> On Thu, Nov 12, 2020 at 4:54 AM Liang, Kan
> >> wrote:
> >> >
> >> >
> >> >
> >> > On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
> >> > > On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
> >> > >
> >> > >> - 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
> >> > > only set that for large pebs. Are you sure the other users (Intel LBR
> >> > > and PowerPC BHRB) don't need it?
> >> >
> >> > I didn't set it for LBR, because the perf_sched_events is always enabled
> >> > for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
> >> > for LBR.
> >> >
> >> > if (has_branch_stack(event))
> >> > inc = true;
> >> >
> >> > >
> >> > > If they indeed do not require the pmu::sched_task() callback for CPU
> >> > > events, then I still think the whole perf_sched_cb_{inc,dec}()
> >> > > interface
> >> >
> >> > No, LBR requires the pmu::sched_task() callback for CPU events.
> >> >
> >> > Now, The LBR registers have to be reset in sched in even for CPU events.
> >> >
> >> > To fix the shorter LBR callstack issue for CPU events, we also need to
> >> > save/restore LBRs in pmu::sched_task().
> >> > https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
> >> >
> >> > > is confusing at best.
> >> > >
> >> > > Can't we do something like this instead?
> >> > >
> >> > I think the below patch may have two issues.
> >> > - PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as well)
> >> > now.
> >> > - We may disable the large PEBS later if not all PEBS events support
> >> > large PEBS. The PMU need a way to notify the generic code to decrease
> >> > the nr_sched_task.
> >>
> >> Any updates on this? I've reviewed and tested Kan's patches
> >> and they all look good.
> >>
> >> Maybe we can talk to PPC folks to confirm the BHRB case?
> >
> > Can we move this forward? I saw patch 3/3 also adds PERF_ATTACH_SCHED_CB
> > for PowerPC too. But it'd be nice if ppc folks can confirm the change.
>
> Sorry I've read the whole thread, but I'm still not entirely sure I
> understand the question.
Thanks for your time and sorry about not being clear enough.
We found per-cpu events are not calling pmu::sched_task()
on context switches. So PERF_ATTACH_SCHED_CB was
added to indicate the core logic that it needs to invoke the
callback.
The patch 3/3 added the flag to PPC (for BHRB) with other
changes (I think it should be split like in the patch 2/3) and
want to get ACKs from the PPC folks.
Thanks,
Namhyung
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
Namhyung Kim writes:
> Hi Peter and Kan,
>
> (Adding PPC folks)
>
> On Tue, Nov 17, 2020 at 2:01 PM Namhyung Kim wrote:
>>
>> Hello,
>>
>> On Thu, Nov 12, 2020 at 4:54 AM Liang, Kan wrote:
>> >
>> >
>> >
>> > On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
>> > > On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
>> > >
>> > >> - 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
>> > > only set that for large pebs. Are you sure the other users (Intel LBR
>> > > and PowerPC BHRB) don't need it?
>> >
>> > I didn't set it for LBR, because the perf_sched_events is always enabled
>> > for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
>> > for LBR.
>> >
>> > if (has_branch_stack(event))
>> > inc = true;
>> >
>> > >
>> > > If they indeed do not require the pmu::sched_task() callback for CPU
>> > > events, then I still think the whole perf_sched_cb_{inc,dec}() interface
>> >
>> > No, LBR requires the pmu::sched_task() callback for CPU events.
>> >
>> > Now, The LBR registers have to be reset in sched in even for CPU events.
>> >
>> > To fix the shorter LBR callstack issue for CPU events, we also need to
>> > save/restore LBRs in pmu::sched_task().
>> > https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
>> >
>> > > is confusing at best.
>> > >
>> > > Can't we do something like this instead?
>> > >
>> > I think the below patch may have two issues.
>> > - PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as well)
>> > now.
>> > - We may disable the large PEBS later if not all PEBS events support
>> > large PEBS. The PMU need a way to notify the generic code to decrease
>> > the nr_sched_task.
>>
>> Any updates on this? I've reviewed and tested Kan's patches
>> and they all look good.
>>
>> Maybe we can talk to PPC folks to confirm the BHRB case?
>
> Can we move this forward? I saw patch 3/3 also adds PERF_ATTACH_SCHED_CB
> for PowerPC too. But it'd be nice if ppc folks can confirm the change.
Sorry I've read the whole thread, but I'm still not entirely sure I
understand the question.
cheers
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
Hi Peter and Kan,
(Adding PPC folks)
On Tue, Nov 17, 2020 at 2:01 PM Namhyung Kim wrote:
>
> Hello,
>
> On Thu, Nov 12, 2020 at 4:54 AM Liang, Kan wrote:
> >
> >
> >
> > On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
> > > On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
> > >
> > >> - 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
> > > only set that for large pebs. Are you sure the other users (Intel LBR
> > > and PowerPC BHRB) don't need it?
> >
> > I didn't set it for LBR, because the perf_sched_events is always enabled
> > for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
> > for LBR.
> >
> > if (has_branch_stack(event))
> > inc = true;
> >
> > >
> > > If they indeed do not require the pmu::sched_task() callback for CPU
> > > events, then I still think the whole perf_sched_cb_{inc,dec}() interface
> >
> > No, LBR requires the pmu::sched_task() callback for CPU events.
> >
> > Now, The LBR registers have to be reset in sched in even for CPU events.
> >
> > To fix the shorter LBR callstack issue for CPU events, we also need to
> > save/restore LBRs in pmu::sched_task().
> > https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
> >
> > > is confusing at best.
> > >
> > > Can't we do something like this instead?
> > >
> > I think the below patch may have two issues.
> > - PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as well) now.
> > - We may disable the large PEBS later if not all PEBS events support
> > large PEBS. The PMU need a way to notify the generic code to decrease
> > the nr_sched_task.
>
> Any updates on this? I've reviewed and tested Kan's patches
> and they all look good.
>
> Maybe we can talk to PPC folks to confirm the BHRB case?
Can we move this forward? I saw patch 3/3 also adds PERF_ATTACH_SCHED_CB
for PowerPC too. But it'd be nice if ppc folks can confirm the change.
Thanks,
Namhyung
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
Hello,
On Thu, Nov 12, 2020 at 4:54 AM Liang, Kan wrote:
>
>
>
> On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
> > On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
> >
> >> - 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
> > only set that for large pebs. Are you sure the other users (Intel LBR
> > and PowerPC BHRB) don't need it?
>
> I didn't set it for LBR, because the perf_sched_events is always enabled
> for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
> for LBR.
>
> if (has_branch_stack(event))
> inc = true;
>
> >
> > If they indeed do not require the pmu::sched_task() callback for CPU
> > events, then I still think the whole perf_sched_cb_{inc,dec}() interface
>
> No, LBR requires the pmu::sched_task() callback for CPU events.
>
> Now, The LBR registers have to be reset in sched in even for CPU events.
>
> To fix the shorter LBR callstack issue for CPU events, we also need to
> save/restore LBRs in pmu::sched_task().
> https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
>
> > is confusing at best.
> >
> > Can't we do something like this instead?
> >
> I think the below patch may have two issues.
> - PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as well) now.
> - We may disable the large PEBS later if not all PEBS events support
> large PEBS. The PMU need a way to notify the generic code to decrease
> the nr_sched_task.
Any updates on this? I've reviewed and tested Kan's patches
and they all look good.
Maybe we can talk to PPC folks to confirm the BHRB case?
Thanks,
Namhyung
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On 11/11/2020 11:25 AM, Peter Zijlstra wrote:
On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
- 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
only set that for large pebs. Are you sure the other users (Intel LBR
and PowerPC BHRB) don't need it?
I didn't set it for LBR, because the perf_sched_events is always enabled
for LBR. But, yes, we should explicitly set the PERF_ATTACH_SCHED_CB
for LBR.
if (has_branch_stack(event))
inc = true;
If they indeed do not require the pmu::sched_task() callback for CPU
events, then I still think the whole perf_sched_cb_{inc,dec}() interface
No, LBR requires the pmu::sched_task() callback for CPU events.
Now, The LBR registers have to be reset in sched in even for CPU events.
To fix the shorter LBR callstack issue for CPU events, we also need to
save/restore LBRs in pmu::sched_task().
https://lore.kernel.org/lkml/1578495789-95006-4-git-send-email-kan.li...@linux.intel.com/
is confusing at best.
Can't we do something like this instead?
I think the below patch may have two issues.
- PERF_ATTACH_SCHED_CB is required for LBR (maybe PowerPC BHRB as well) now.
- We may disable the large PEBS later if not all PEBS events support
large PEBS. The PMU need a way to notify the generic code to decrease
the nr_sched_task.
The current related code in Intel PMU is as below.
static void
pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc,
struct perf_event *event, bool add)
{
struct pmu *pmu = event->ctx->pmu;
/*
* Make sure we get updated with the first PEBS
* event. It will trigger also during removal, but
* that does not hurt:
*/
bool update = cpuc->n_pebs == 1;
if (needed_cb != pebs_needs_sched_cb(cpuc)) {
if (!needed_cb)
perf_sched_cb_inc(pmu);
else
perf_sched_cb_dec(pmu);
update = true;
}
Thanks,
Kan
---
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 546cc89217bb..672d6f039fce 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3565,8 +3565,10 @@ static int intel_pmu_hw_config(struct perf_event *event)
if (!(event->attr.freq || (event->attr.wakeup_events &&
!event->attr.watermark))) {
event->hw.flags |= PERF_X86_EVENT_AUTO_RELOAD;
if (!(event->attr.sample_type &
- ~intel_pmu_large_pebs_flags(event)))
+ ~intel_pmu_large_pebs_flags(event))) {
event->hw.flags |= PERF_X86_EVENT_LARGE_PEBS;
+ event->attach_state |= PERF_ATTACH_SCHED_CB;
+ }
}
if (x86_pmu.pebs_aliases)
x86_pmu.pebs_aliases(event);
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 9a38f579bc76..af9ee539c179 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -606,6 +606,7 @@ struct swevent_hlist {
#define PERF_ATTACH_TASK 0x04
#define PERF_ATTACH_TASK_DATA 0x08
#define PERF_ATTACH_ITRACE0x10
+#define PERF_ATTACH_SCHED_CB 0x20
struct perf_cgroup;
struct perf_buffer;
@@ -817,6 +818,7 @@ struct perf_event_context {
int is_active;
int nr_stat;
int nr_freq;
+ int nr_sched_task;
int rotate_disable;
/*
* Set when nr_events != nr_active, except tolerant to events not
@@ -872,7 +874,7 @@ struct perf_cpu_context {
struct list_headcgrp_cpuctx_entry;
#endif
- intsched_cb_usage;
+
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
> - 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 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 thing is; your first two patches rely on PERF_ATTACH_SCHED_CB and
only set that for large pebs. Are you sure the other users (Intel LBR
and PowerPC BHRB) don't need it?
If they indeed do not require the pmu::sched_task() callback for CPU
events, then I still think the whole perf_sched_cb_{inc,dec}() interface
is confusing at best.
Can't we do something like this instead?
---
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 546cc89217bb..672d6f039fce 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3565,8 +3565,10 @@ static int intel_pmu_hw_config(struct perf_event *event)
if (!(event->attr.freq || (event->attr.wakeup_events &&
!event->attr.watermark))) {
event->hw.flags |= PERF_X86_EVENT_AUTO_RELOAD;
if (!(event->attr.sample_type &
- ~intel_pmu_large_pebs_flags(event)))
+ ~intel_pmu_large_pebs_flags(event))) {
event->hw.flags |= PERF_X86_EVENT_LARGE_PEBS;
+ event->attach_state |= PERF_ATTACH_SCHED_CB;
+ }
}
if (x86_pmu.pebs_aliases)
x86_pmu.pebs_aliases(event);
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 9a38f579bc76..af9ee539c179 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -606,6 +606,7 @@ struct swevent_hlist {
#define PERF_ATTACH_TASK 0x04
#define PERF_ATTACH_TASK_DATA 0x08
#define PERF_ATTACH_ITRACE 0x10
+#define PERF_ATTACH_SCHED_CB 0x20
struct perf_cgroup;
struct perf_buffer;
@@ -817,6 +818,7 @@ struct perf_event_context {
int is_active;
int nr_stat;
int nr_freq;
+ int nr_sched_task;
int rotate_disable;
/*
* Set when nr_events != nr_active, except tolerant to events not
@@ -872,7 +874,7 @@ struct perf_cpu_context {
struct list_headcgrp_cpuctx_entry;
#endif
- int sched_cb_usage;
+ struct list_headsched_cb_entry;
int online;
/*
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d2f3ca792936..0a5dfed6bb46 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -384,6 +384,7 @@ static DEFINE_MUTEX(perf_sched_mutex);
static atomic_t perf_sched_count;
static DEFINE_PER_CPU(atomic_t, perf_cgroup_events);
+static DEFINE_PER_CPU(int, perf_sched_cb_usage);
static DEFINE_PER_CPU(struct pmu_event_list, pmu_sb_events);
static atomic_t nr_mmap_events __read_mostly;
@@ -2292,6 +2293,12 @@ event_sched_out(struct perf_event *event,
perf_event_ctx_deactivate(ctx);
if (event->attr.freq && event->attr.sample_freq)
ctx->nr_freq--;
+ if (event->attach_state & PERF_ATTACH_SCHED_CB) {
+ if (!--ctx->nr_sched_task && >ctx == ctx) {
+ list_del(>sched_cb_entry);
+ this_cpu_dec(perf_sched_cb_usage);
+ }
+ }
if (event->attr.exclusive || !cpuctx->active_oncpu)
cpuctx->exclusive = 0;
@@ -2564,6 +2571,12 @@ event_sched_in(struct perf_event *event,
perf_event_ctx_activate(ctx);
if (event->attr.freq && event->attr.sample_freq)
ctx->nr_freq++;
+ if (event->attach_state & PERF_ATTACH_SCHED_CB) {
+ if (!ctx->nr_sched_task++ && >ctx == ctx) {
+ list_add(>sched_cb_entry,
this_cpu_ptr(_cb_list));
+
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On 11/9/2020 12:33 PM, Peter Zijlstra wrote:
On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
Maybe we can frob x86_pmu_enable()...
Could you please elaborate?
Something horrible like this. It will detect the first time we enable
the PMU on a new task (IOW we did a context switch) and wipe the
counters when user RDPMC is on...
Oh, you mean the RDPMC patch. It should be doable, but I think
sched_task() may be a better place, especially with the new optimization
(patch 3). We can set PERF_SCHED_CB_SW_IN bit for the case, so we only
do the check for per-task events in sched in.
It looks like the below patch has to unconditionally do the check (even
for the non-RDPMC cases), which should be unnecessary.
Anyway, I think the RDPMC patch should depend on the implementation of
the sched_task(). We may have further discussion when the design of
sched_task() is finalized.
Thanks,
Kan
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 77b963e5e70a..d862927baaef 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1289,6 +1289,15 @@ static void x86_pmu_enable(struct pmu *pmu)
perf_events_lapic_init();
}
+ if (cpuc->current != current) {
+ struct mm_struct *mm = current->mm;
+
+ cpuc->current = current;
+
+ if (mm && atomic_read(>context.perf_rdpmc_allowed))
+ wipe_dirty_counters();
+ }
+
cpuc->enabled = 1;
barrier();
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 7895cf4c59a7..d16118cb3bd0 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -248,6 +248,8 @@ struct cpu_hw_events {
unsigned inttxn_flags;
int is_fake;
+ void *current;
+
/*
* Intel DebugStore bits
*/
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote: > - 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. > There's a further optimiation possible; we can have pmu::swap_task_ctx() imply pmu::sched_task() (both in and out). It's all a bit of a mess though.. let me consider.
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On Mon, Nov 09, 2020 at 09:49:31AM -0500, Liang, Kan wrote:
> > Maybe we can frob x86_pmu_enable()...
>
> Could you please elaborate?
Something horrible like this. It will detect the first time we enable
the PMU on a new task (IOW we did a context switch) and wipe the
counters when user RDPMC is on...
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 77b963e5e70a..d862927baaef 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1289,6 +1289,15 @@ static void x86_pmu_enable(struct pmu *pmu)
perf_events_lapic_init();
}
+ if (cpuc->current != current) {
+ struct mm_struct *mm = current->mm;
+
+ cpuc->current = current;
+
+ if (mm && atomic_read(>context.perf_rdpmc_allowed))
+ wipe_dirty_counters();
+ }
+
cpuc->enabled = 1;
barrier();
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 7895cf4c59a7..d16118cb3bd0 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -248,6 +248,8 @@ struct cpu_hw_events {
unsigned inttxn_flags;
int is_fake;
+ void*current;
+
/*
* Intel DebugStore bits
*/
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
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.li...@linux.intel.com wrote:
From: Kan Liang
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
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On Mon, Nov 09, 2020 at 10:52:35AM +0100, Peter Zijlstra wrote:
> On Fri, Nov 06, 2020 at 01:29:33PM -0800, kan.li...@linux.intel.com wrote:
> > From: Kan Liang
> >
> > 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
*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.
Maybe we can frob x86_pmu_enable()...
Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
On Fri, Nov 06, 2020 at 01:29:33PM -0800, kan.li...@linux.intel.com wrote:
> From: Kan Liang
>
> 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
> +static void perf_pmu_sched_task(struct task_struct *prev,
> + struct task_struct *next,
> + bool sched_in)
> +{
> + struct perf_cpu_context *cpuctx;
> +
> + if (prev == next)
> + return;
> +
> + list_for_each_entry(cpuctx, this_cpu_ptr(_cb_list),
> sched_cb_entry) {
> + /* will be handled in perf_event_context_sched_in/out */
> + if (cpuctx->task_ctx)
> + continue;
This seems wrong; cpuctx->task_ctx merely indicates that there is a
task-ctx for this CPU. Not that the event you're interested in is in
fact there.
So consider the case where the event is on the CPU context, but we also
have a task context. Then we'll not issue this call.
> +
> + __perf_pmu_sched_task(cpuctx, sched_in);
> + }
> +}
[PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU events
From: Kan Liang
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")
Reported-by: Gabriel Marin
Reported-by: Namhyung Kim
Originally-by: Namhyung Kim
Signed-off-by: Kan Liang
---
include/linux/perf_event.h | 2 ++
kernel/events/core.c | 42 ++
2 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 0defb526cd0c..f7a84d1048b9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -606,6 +606,7 @@ struct swevent_hlist {
#define PERF_ATTACH_TASK 0x04
#define PERF_ATTACH_TASK_DATA 0x08
#define PERF_ATTACH_ITRACE 0x10
+#define PERF_ATTACH_SCHED_CB 0x20
struct perf_cgroup;
struct perf_buffer;
@@ -872,6 +873,7 @@ struct perf_cpu_context {
struct list_headcgrp_cpuctx_entry;
#endif
+ struct list_headsched_cb_entry;
int sched_cb_usage;
int online;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index dba4ea4e648b..df0df5514097 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -384,6 +384,7 @@ static DEFINE_MUTEX(perf_sched_mutex);
static atomic_t perf_sched_count;
static DEFINE_PER_CPU(atomic_t, perf_cgroup_events);
+static DEFINE_PER_CPU(int, perf_sched_cb_usages);
static DEFINE_PER_CPU(struct pmu_event_list, pmu_sb_events);
static atomic_t nr_mmap_events __read_mostly;
@@ -3481,11 +3482,16 @@ static void perf_event_context_sched_out(struct
task_struct *task, int ctxn,
}
}
+static DEFINE_PER_CPU(struct list_head, sched_cb_list);
+
void perf_sched_cb_dec(struct pmu *pmu)
{
struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
- --cpuctx->sched_cb_usage;
+ this_cpu_dec(perf_sched_cb_usages);
+
+ if (!--cpuctx->sched_cb_usage)
+ list_del(>sched_cb_entry);
}
@@ -3493,7 +3499,10 @@ void perf_sched_cb_inc(struct pmu *pmu)
{
struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
- cpuctx->sched_cb_usage++;
+ if (!cpuctx->sched_cb_usage++)
+ list_add(>sched_cb_entry, this_cpu_ptr(_cb_list));
+
+ this_cpu_inc(perf_sched_cb_usages);
}
/*
@@ -3522,6 +3531,24 @@ static void __perf_pmu_sched_task(struct
perf_cpu_context *cpuctx, bool sched_in
perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
}
+static void perf_pmu_sched_task(struct task_struct *prev,
+ struct task_struct *next,
+ bool sched_in)
+{
+ struct perf_cpu_context *cpuctx;
+
+ if (prev == next)
+ return;
+
+ list_for_each_entry(cpuctx, this_cpu_ptr(_cb_list),
sched_cb_entry) {
+ /* will be handled in perf_event_context_sched_in/out */
+ if (cpuctx->task_ctx)
+ continue;
+
+ __perf_pmu_sched_task(cpuctx, sched_in);
+ }
+}
+
static void perf_event_switch(struct task_struct *task,
struct task_struct *next_prev, bool sched_in);
@@ -3544,6 +3571,9 @@ void __perf_event_task_sched_out(struct task_struct *task,
{
int ctxn;
+ if (__this_cpu_read(perf_sched_cb_usages))
+ perf_pmu_sched_task(task, next, false);
+
if (atomic_read(_switch_events))
perf_event_switch(task, next, false);
@@ -3851,6 +3881,9 @@ void __perf_event_task_sched_in(struct task_struct *prev,
if (atomic_read(_switch_events))
perf_event_switch(task, prev, true);
+
+ if (__this_cpu_read(perf_sched_cb_usages))
+ perf_pmu_sched_task(prev, task, true);
}
static u64 perf_calculate_period(struct perf_event *event, u64 nsec, u64 count)
@@ -4675,7 +4708,7 @@ static void unaccount_event(struct perf_event *event)
if (event->parent)
return;
- if (event->attach_state & PERF_ATTACH_TASK)
+ if (event->attach_state & (PERF_ATTACH_TASK | PERF_ATTACH_SCHED_CB))
dec = true;
if (event->attr.mmap ||
