From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751926AbdGRNip (ORCPT ); Tue, 18 Jul 2017 09:38:45 -0400 Received: from mga05.intel.com ([192.55.52.43]:34301 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751440AbdGRNio (ORCPT ); Tue, 18 Jul 2017 09:38:44 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,378,1496127600"; d="scan'208";a="109272493" Subject: Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context To: Alexander Shishkin , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo Cc: Andi Kleen , Kan Liang , Dmitri Prokhorov , Valery Cherepennikov , Mark Rutland , David Carrillo-Cisneros , Stephane Eranian , linux-kernel References: <871spdx6jj.fsf@ashishki-desk.ger.corp.intel.com> From: Alexey Budankov Organization: Intel Corp. Message-ID: Date: Tue, 18 Jul 2017 16:38:39 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <871spdx6jj.fsf@ashishki-desk.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 18.07.2017 15:40, Alexander Shishkin wrote: > Alexey Budankov 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