[BUG] perf: perf_swevent PMU should not be on rotation_list

[BUG] perf: perf_swevent PMU should not be on rotation_list

[Stephane Eranian]

Hi,

I was looking at the rotation code and I found out that when
I monitor a SW event (in my case a probe), I end up having
two PMUs on the rotation list on Intel Core: cpu and software.

I thought there was no multiplexing needed for SW events.

So why is the SW PMU on the rotation list causing extra
iterations through the rotation code?

Shouldn't we do something like:

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -771,6 +780,9 @@ static void perf_pmu_rotate_start(struct pmu *pmu)
        struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
        struct list_head *head = &__get_cpu_var(rotation_list);

+       if (pmu->type == PERF_TYPE_SOFTWARE)
+               return;
+
        WARN_ON(!irqs_disabled());

        if (list_empty(&cpuctx->rotation_list))

Re: [BUG] perf: perf_swevent PMU should not be on rotation_list

[Peter Zijlstra]

On Wed, 2012-09-05 at 16:03 +0200, Stephane Eranian wrote:
> Hi,
> 
> I was looking at the rotation code and I found out that when
> I monitor a SW event (in my case a probe), I end up having
> two PMUs on the rotation list on Intel Core: cpu and software.
> 
> I thought there was no multiplexing needed for SW events.

Correct, since programming of swevents should always succeed.

> So why is the SW PMU on the rotation list causing extra
> iterations through the rotation code?

Because... uhm.. someone (probably me) didn't think to exclude swevents.

> Shouldn't we do something like:
> 
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -771,6 +780,9 @@ static void perf_pmu_rotate_start(struct pmu *pmu)
>         struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
>         struct list_head *head = &__get_cpu_var(rotation_list);
> 
> +       if (pmu->type == PERF_TYPE_SOFTWARE)
> +               return;
> +
>         WARN_ON(!irqs_disabled());
> 
>         if (list_empty(&cpuctx->rotation_list))


Yeah, I guess that'll do, although I guess something like:

  pmu->task_ctx_nr == perf_sw_context

would be even better, since that would also work for TYPE_TRACEPOINT and
possibly any other swevent like things.

Re: [BUG] perf: perf_swevent PMU should not be on rotation_list

[Stephane Eranian]

On Wed, Sep 5, 2012 at 7:06 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2012-09-05 at 16:03 +0200, Stephane Eranian wrote:
>> Hi,
>>
>> I was looking at the rotation code and I found out that when
>> I monitor a SW event (in my case a probe), I end up having
>> two PMUs on the rotation list on Intel Core: cpu and software.
>>
>> I thought there was no multiplexing needed for SW events.
>
> Correct, since programming of swevents should always succeed.
>
>> So why is the SW PMU on the rotation list causing extra
>> iterations through the rotation code?
>
> Because... uhm.. someone (probably me) didn't think to exclude swevents.
>
>> Shouldn't we do something like:
>>
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -771,6 +780,9 @@ static void perf_pmu_rotate_start(struct pmu *pmu)
>>         struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
>>         struct list_head *head = &__get_cpu_var(rotation_list);
>>
>> +       if (pmu->type == PERF_TYPE_SOFTWARE)
>> +               return;
>> +
>>         WARN_ON(!irqs_disabled());
>>
>>         if (list_empty(&cpuctx->rotation_list))
>
>
> Yeah, I guess that'll do, although I guess something like:
>
>   pmu->task_ctx_nr == perf_sw_context
>
> would be even better, since that would also work for TYPE_TRACEPOINT and
> possibly any other swevent like things.

Yeah, that's better. Will post a patch to fix that then.

Thanks.