Messages in this thread | | | Subject | Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context | From | Alexey Budankov <> | Date | Tue, 18 Jul 2017 16:38:39 +0300 |
| |
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
| |