Messages in this thread | | | Date | Tue, 23 Aug 2022 20:44:51 +0530 | From | Ravi Bangoria <> | Subject | Re: [RFC v2] perf: Rewrite core context handling |
| |
>> 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.
Got it.
> This should only be done if EVENT_TIME is not set.
Does that mean context time should be started only when context is getting scheduled I.e. ctx->is_active is 0 ?
> 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.
Correct, we can't assume that. There are cases where we call ctx_sched_out(EVENT_TIME) followed by ctx_sched_in(EVENT_TIME) when PINNED / FLEXIBLE are also set in ctx->is_active. For ex, perf_event_enable_on_exec(). In such cases, we will not advance ctx->time. Example:
child() { ... execv(); }
main() { pid = fork();
attr.enable_on_exec = 0; fd0 = perf_event_open(&attr, pid, -1, -1, 0); ... wait(NULL); }
Here execv() will cause call to ctx_sched_in() --> __update_context_time() with adv=false. I think that's fine. Sometime later we will anyway advance ctx->time.
Sorry, I've not spend enough time with this time keeping code. Please let me know if I'm talking nonsense.
Thanks, Ravi
| |