Messages in this thread | | | Subject | Re: [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups | From | Alexey Budankov <> | Date | Wed, 23 Aug 2017 20:23:22 +0300 |
| |
On 23.08.2017 14:17, Alexander Shishkin wrote: > Alexey Budankov <alexey.budankov@linux.intel.com> writes: > >> @@ -3091,61 +3231,55 @@ static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx, >> } >> >> static void >> -ctx_pinned_sched_in(struct perf_event_context *ctx, >> - struct perf_cpu_context *cpuctx) >> +ctx_pinned_sched_in(struct perf_event *event, >> + struct perf_cpu_context *cpuctx, >> + struct perf_event_context *ctx) > > If you're doing this, you also need to rename the function, because it > now schedules in one event and not a context. But better just keep it as > is. > >> { >> - struct perf_event *event; >> - >> - list_for_each_entry(event, &ctx->pinned_groups, group_entry) { > > Why not put your new iterator here in place of the old one, instead of > moving things around? Because what follows is hard to read and is also > completely unnecessary: > >> - if (event->state <= PERF_EVENT_STATE_OFF) >> - continue; >> - if (!event_filter_match(event)) >> - continue; >> + if (event->state <= PERF_EVENT_STATE_OFF) >> + return; >> + if (!event_filter_match(event)) >> + return; > > like this, > >> >> - /* may need to reset tstamp_enabled */ >> - if (is_cgroup_event(event)) >> - perf_cgroup_mark_enabled(event, ctx); >> + /* may need to reset tstamp_enabled */ >> + if (is_cgroup_event(event)) >> + perf_cgroup_mark_enabled(event, ctx); > > or this > >> >> - if (group_can_go_on(event, cpuctx, 1)) >> - group_sched_in(event, cpuctx, ctx); >> + if (group_can_go_on(event, cpuctx, 1)) >> + group_sched_in(event, cpuctx, ctx); > > etc, etc. > >> @@ -3156,7 +3290,8 @@ ctx_sched_in(struct perf_event_context *ctx, >> struct task_struct *task) >> { >> int is_active = ctx->is_active; >> - u64 now; > > Why?
Shortened the scope/span/life time of this variable. Declared, defined and initialized closer to the place of employment.
> >> + struct perf_event *event; >> + struct rb_node *node; >> >> lockdep_assert_held(&ctx->lock); >> >> @@ -3175,7 +3310,7 @@ ctx_sched_in(struct perf_event_context *ctx, >> >> if (is_active & EVENT_TIME) { >> /* start ctx time */ >> - now = perf_clock(); >> + u64 now = perf_clock(); > > Why?> >> ctx->timestamp = now; >> perf_cgroup_set_timestamp(task, ctx); >> } >> @@ -3185,11 +3320,19 @@ ctx_sched_in(struct perf_event_context *ctx, >> * in order to give them the best chance of going on. >> */ >> if (is_active & EVENT_PINNED) >> - ctx_pinned_sched_in(ctx, cpuctx); >> + perf_event_groups_for_each(event, node, >> + &ctx->pinned_groups, group_node, >> + group_list, group_entry) >> + ctx_pinned_sched_in(event, cpuctx, ctx); > > So this perf_event_groups_for_each() can just move into > ctx_*_sched_in(), can't it?
Yes. Makes sense. Addressed it in v8. Thanks!
> > Regards, > -- > Alex >
| |