Re: [RFC 1/2] perf/core: Enable sched_task callbacks if PMU has it

Namhyung Kim
2020-11-06T00:53:23Z

On Fri, Nov 6, 2020 at 4:01 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 11/5/2020 10:45 AM, Namhyung Kim wrote:
> > Hello,
> >
> > On Thu, Nov 5, 2020 at 11:47 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>
> >>
> >>
> >> On 11/2/2020 9:52 AM, Namhyung Kim wrote:
> >>> If an event associated with a PMU which has a sched_task callback,
> >>> it should be called regardless of cpu/task context. For example,
> >>
> >>
> >> I don't think it's necessary. We should call it when we have to.
> >> Otherwise, it just waste cycles.
> >> Shouldn't the patch 2 be enough?
> >
> > I'm not sure, without this patch __perf_event_task_sched_in/out
> > cannot be called for per-cpu events (w/o cgroups) IMHO.
> > And I could not find any other place to check the
> > perf_sched_cb_usages.
> >
>
> Yes, it should a bug for large PEBS, and it should has always been there
> since the large PEBS was introduced. I just tried some older kernels
> (before recent change). Large PEBS is not flushed with per-cpu events.
>
> But from your description, it looks like the issue is only found after
> recent change. Could you please double check if the issue can also be
> reproduced before the recent change?

Yep, actually Gabriel reported this problem on v4.4 kernel.
I'm sorry that my description was misleading.

>
>
> >>> diff --git a/kernel/events/core.c b/kernel/events/core.c
> >>> index b458ed3dc81b..aaa0155c4142 100644
> >>> --- a/kernel/events/core.c
> >>> +++ b/kernel/events/core.c
> >>> @@ -4696,6 +4696,8 @@ static void unaccount_event(struct perf_event *event)
> >>> dec = true;
> >>> if (has_branch_stack(event))
> >>> dec = true;
> >>> + if (event->pmu->sched_task)
> >>> + dec = true;
>
> I think sched_task is a too big hammer. The
> __perf_event_task_sched_in/out will be invoked even for non-pebs per-cpu
> events, which is not necessary.

Agreed.

>
> Maybe we can introduce a flag to indicate the case. How about the patch
> as below?

LGTM, but I prefer PERF_ATTACH_SCHED_CB, though. :)

Thanks
Namhyung

>
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index c79748f6921d..953a4bb98330 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_DATA;
> + }
> }
> 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 0defb526cd0c..3eef7142aa11 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_DATA 0x20
>
> struct perf_cgroup;
> struct perf_buffer;
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index dba4ea4e648b..a38133b5543a 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4675,7 +4675,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_DATA))
> dec = true;
> if (event->attr.mmap || event->attr.mmap_data)
> atomic_dec(&nr_mmap_events);
> @@ -11204,7 +11204,7 @@ static void account_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_DATA))
> inc = true;
> if (event->attr.mmap || event->attr.mmap_data)
> atomic_inc(&nr_mmap_events);
>
> Thanks,
> Kan