Re: perf_events: question about __perf_event_read()

[Stephane Eranian]

On Fri, Jan 21, 2011 at 7:18 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Fri, 2011-01-21 at 19:06 +0100, Stephane Eranian wrote:
>> Hi,
>>
>> I think the code below still has a problem in case of a per-cpu event.
>>
>> If you issue a read() on a different CPU, then you IPI to the event's cpu.
>> By the time you get there, the event may be de-scheduled in which
>> case you don't want to issue event->pmu_read() nor update context
>> timings. The function has a test but it seems to be checking the per-cpu
>> case only.
>>
>> I have seen panics on P4 with this code because it goes all the way
>> down to rdmsrl() with a bogus counter index (like -1).
>>
>> Am I missing something here?
>>
>> static void __perf_event_read(void *info)
>> {
>> Â Â Â Â struct perf_event *event = info;
>> Â Â Â Â struct perf_event_context *ctx = event->ctx;
>> Â Â Â Â struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
>>
>> Â Â Â Â /*
>> Â Â Â Â Â* If this is a task context, we need to check whether it is
>> Â Â Â Â Â* the current task context of this cpu. ÂIf not it has been
>> Â Â Â Â Â* scheduled out before the smp call arrived. ÂIn that case
>> Â Â Â Â Â* event->count would have been updated to a recent sample
>> Â Â Â Â Â* when the event was scheduled out.
>> Â Â Â Â Â*/
>> Â Â Â Â if (ctx->task && cpuctx->task_ctx != ctx)
>> Â Â Â Â Â Â Â Â return;
>>
>> Â Â Â Â raw_spin_lock(&ctx->lock);
>
> Shouldn't we re-check event->state once we hold this lock?
>
I remember checking this about a month ago and tip-x86 had
some checks in this routine, but now the're gone.

I think you need something like:

+	if (ctx->is_active)
+		update_context_time(ctx);

+	if (event->state == PERF_EVENT_STATE_ACTIVE)
+		event->pmu->read(event);

>> Â Â Â Â update_context_time(ctx);
>> Â Â Â Â update_event_times(event);
>> Â Â Â Â raw_spin_unlock(&ctx->lock);
>>
>> Â Â Â Â event->pmu->read(event);
>> }
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/