Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690242 > unrolled thread

Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-07-18 14:50 +0200
    Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees  for pinned and flexible groups at perf_event_context Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-07-18 15:40 +0200
      Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-07-18 17:30 +0200
        Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees  for pinned and flexible groups at perf_event_context Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-07-18 17:40 +0200

#1690242 — Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context

Alexey Budankov <alexey.budankov@linux.intel.com> writes:

> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 7b2cddf..8e1967f 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -603,13 +603,6 @@ struct perf_event {
>  	 */
>  	struct list_head		group_list;
>  	/*
> -	 * Entry into the group_list list above;
> -	 * the entry may be attached to the self group_list list above
> -	 * in case the event is directly attached to the tree;
> -	 */
> -	struct list_head		group_list_entry;
> -
> -	/*
>  	 * We need storage to track the entries in perf_pmu_migrate_context; we
>  	 * cannot use the event_entry because of RCU and we want to keep the
>  	 * group in tact which avoids us using the other two entries.

You probably also want to explain this change, for example change the
@group_list description, saying that something else links into it now.

> @@ -749,15 +742,6 @@ struct perf_event {
>  #endif /* CONFIG_PERF_EVENTS */
>  };
>  
> -/*
> - * event groups keep group leader events arranged as an rb tree with
> - * event->cpu key and as a list for the whole tree iterations;
> - */
> -struct perf_event_groups {
> -	struct list_head list;
> -	struct rb_root	 tree;
> -};

Was the @list component ever used? From this patch it looks like it
wasn't and in reality you replaced the lists with trees is 1/4, but left
the lists to hang around for a while.

I think a more generic comment here is that it's difficult to review
patches that don't make sense in separation from one another. It does
make sense to make a transition across several patches, but each patch
kind of needs to make sense on its own. For example, 1/2 adds trees
while keeping the lists intact, 2/2 removes the lists.

Regards,
--
Alex

[toc] | [next] | [standalone]

#1690274 — Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context

Hi,

On 18.07.2017 15:40, Alexander Shishkin wrote:
> Alexey Budankov <alexey.budankov@linux.intel.com> writes:
> 
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index 7b2cddf..8e1967f 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -603,13 +603,6 @@ struct perf_event {
>>  	 */
>>  	struct list_head		group_list;
>>  	/*
>> -	 * Entry into the group_list list above;
>> -	 * the entry may be attached to the self group_list list above
>> -	 * in case the event is directly attached to the tree;
>> -	 */
>> -	struct list_head		group_list_entry;
>> -
>> -	/*
>>  	 * We need storage to track the entries in perf_pmu_migrate_context; we
>>  	 * cannot use the event_entry because of RCU and we want to keep the
>>  	 * group in tact which avoids us using the other two entries.
> 
> You probably also want to explain this change, for example change the
> @group_list description, saying that something else links into it now.
> 

The whole patch as a single commit, attached to patch v5 4/4, may provide
the complete view of suggested changes.

>> @@ -749,15 +742,6 @@ struct perf_event {
>>  #endif /* CONFIG_PERF_EVENTS */
>>  };
>>  
>> -/*
>> - * event groups keep group leader events arranged as an rb tree with
>> - * event->cpu key and as a list for the whole tree iterations;
>> - */
>> -struct perf_event_groups {
>> -	struct list_head list;
>> -	struct rb_root	 tree;
>> -};
> 
> Was the @list component ever used? From this patch it looks like it
> wasn't and in reality you replaced the lists with trees is 1/4, but left
> the lists to hang around for a while.

list component was used starting from patch v5 1/4 till 3/4, and 4/4 
completely switched the list to rbtree. That was initial advice made
by Peter. He also suggested organizing patches in such a way so I 
implemented it.

> 
> I think a more generic comment here is that it's difficult to review
> patches that don't make sense in separation from one another. It does
> make sense to make a transition across several patches, but each patch
> kind of needs to make sense on its own. For example, 1/2 adds trees
> while keeping the lists intact, 2/2 removes the lists.
> 

Agree. That was my intention - try to split the whole work into logically
connected steps and simplify review process and make reviewers work easier.

> Regards,
> --
> Alex
> 

Thanks,
Alexey

[toc] | [prev] | [next] | [standalone]

#1690370

Alexey Budankov <alexey.budankov@linux.intel.com> writes:

>> You probably also want to explain this change, for example change the
>> @group_list description, saying that something else links into it now.
>> 
>
> The whole patch as a single commit, attached to patch v5 4/4, may provide
> the complete view of suggested changes.

No, each patch has to be readable on its own. If one has to jump between
patches to figure out what's going on, it's already discouraging. The
point of submitting a patch, on the other hand, is to encourage the
reviewers to read on.

Regards,
--
Alex

[toc] | [prev] | [next] | [standalone]

#1690375 — Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context

On 18.07.2017 18:23, Alexander Shishkin wrote:
> Alexey Budankov <alexey.budankov@linux.intel.com> writes:
> 
>>> You probably also want to explain this change, for example change the
>>> @group_list description, saying that something else links into it now.
>>>
>>
>> The whole patch as a single commit, attached to patch v5 4/4, may provide
>> the complete view of suggested changes.
> 
> No, each patch has to be readable on its own. If one has to jump between
> patches to figure out what's going on, it's already discouraging. The
> point of submitting a patch, on the other hand, is to encourage the
> reviewers to read on.

Agree. That was my initial intention when splitting into several patches.

> 
> Regards,
> --
> Alex
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web