Re: [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups

[Alexey Budankov]

On 23.08.2017 14:17, Alexander Shishkin wrote:
> Alexey Budankov <alexey.budankov@xxxxxxxxxxxxxxx> 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
>