From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751451AbdGRMq1 (ORCPT ); Tue, 18 Jul 2017 08:46:27 -0400 Received: from mga07.intel.com ([134.134.136.100]:48986 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751380AbdGRMq0 (ORCPT ); Tue, 18 Jul 2017 08:46:26 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,377,1496127600"; d="scan'208";a="994278876" From: Alexander Shishkin To: Alexey Budankov , 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 Subject: Re: [PATCH v5 4/4]: perf/core: complete replace of lists by rb trees for pinned and flexible groups at perf_event_context In-Reply-To: References: User-Agent: Notmuch/0.23.7 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Tue, 18 Jul 2017 15:40:00 +0300 Message-ID: <871spdx6jj.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > @@ -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