Messages in this thread | | | Date | Tue, 23 Aug 2022 09:26:20 +0200 | From | Peter Zijlstra <> | Subject | Re: [RFC v2] perf: Rewrite core context handling |
| |
On Tue, Aug 02, 2022 at 11:47:24AM +0530, Ravi Bangoria wrote: > [...] > > > static void > > -ctx_sched_in(struct perf_event_context *ctx, > > - struct perf_cpu_context *cpuctx, > > - enum event_type_t event_type, > > +ctx_sched_in(struct perf_event_context *ctx, enum event_type_t event_type, > > struct task_struct *task) > > { > > + struct perf_cpu_context *cpuctx = this_cpu_ptr(&cpu_context); > > int is_active = ctx->is_active; > > u64 now; > > > > @@ -3818,6 +3905,7 @@ ctx_sched_in(struct perf_event_context *ctx, > > /* start ctx time */ > > now = perf_clock(); > > ctx->timestamp = now; > > + // XXX ctx->task =? task > > Couldn't get this XXX, it's from your original patch. If you can recall, it > would be helpful.
No memories at all; but looking at it; it seems to worry if ctx->task is up-to-date; in this context the only thing that relies on the task is the cgroup for which we update the timestamp in the next statement:
> > perf_cgroup_set_timestamp(task, ctx);
I suppose I should really write less cryptic notes; then again, I never imagined this would take that many years to complete :/
> > } > > Also, this hunk is under if (is_active ^ EVENT_TIME), which effectively is > (is_active != EVENT_TIME). I'm assuming it should be (is_active & EVENT_TIME)?
So that code is identical to what it currently is upstream; but yes that looks somewhat dodgy.
So the code itself (does as the comment says) starts time. This should only be done if EVENT_TIME is not set. That is, I'm thinking it should be something like:
!(is_active & EVENT_TIME)
which happens to be the same as:
is_active ^ EVENT_TIME
under the assumption is_active contains no other bits -- which I don't think is a valid assumption.
| |