From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751063AbdE2NnR (ORCPT ); Mon, 29 May 2017 09:43:17 -0400 Received: from mga06.intel.com ([134.134.136.31]:48514 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750913AbdE2NnQ (ORCPT ); Mon, 29 May 2017 09:43:16 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,414,1491289200"; d="scan'208";a="862462503" Subject: Re: [PATCH]: perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi To: Alexander Shishkin , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo Cc: Andi Kleen , Kan Liang , Dmitri Prokhorov , Valery Cherepennikov , David Carrillo-Cisneros , Stephane Eranian , Mark Rutland , linux-kernel@vger.kernel.org References: <1e962b59-3e39-e0d6-515d-c4fd3502edae@linux.intel.com> <87k24zzx7s.fsf@ashishki-desk.ger.corp.intel.com> From: Alexey Budankov Organization: Intel Corp. Message-ID: <47dc6d8d-77db-70f5-9aa6-2aca38590e60@linux.intel.com> Date: Mon, 29 May 2017 16:43:09 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <87k24zzx7s.fsf@ashishki-desk.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed 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 On 29.05.2017 15:03, Alexander Shishkin wrote: > Alexey Budankov writes: > > Here (above the function) you could include a comment describing what > happens when this is called, locking considerations, etc. I can put the short description from the initial thread message here. Would it be sufficient? > >> +static int >> +perf_cpu_tree_insert(struct rb_root *tree, struct perf_event *event) >> +{ >> + struct rb_node **node; >> + struct rb_node *parent; >> + >> + if (!tree || !event) >> + return 0; > > I don't think this should be happening, should it? And either way you > probably don't want to return 0 here, unless you're using !0 for > success. As you might notice already, currently return codes of the tree API are not checked all other the implementation. I suggest replacing that int error code by void and simplify the stuff. > >> + >> + node = &tree->rb_node; >> + parent = *node; >> + >> + while (*node) { >> + struct perf_event *node_event = container_of(*node, >> + struct perf_event, group_node); >> + >> + parent = *node; >> + >> + if (event->cpu < node_event->cpu) { >> + node = &((*node)->rb_left); > > this would be the same as node = &parent->rb_left, right? Please ask more. node is the leaf node and parent is the parent of the node at the end of cycle. We need the both to insert a new node into a tree. > >> + } else if (event->cpu > node_event->cpu) { >> + node = &((*node)->rb_right); >> + } else { >> + list_add_tail(&event->group_list_entry, >> + &node_event->group_list); > > So why is this better than simply having per-cpu event lists plus one > for per-thread events? Good question. Choice of data structure and layout depends on the operations applied to the data so keeping groups as a tree simplifies and improves the implementation in terms of scalability and performance. Please ask more if any. > > Also, > >> + return 2; > > 2? Answered above. > >> + } >> + } >> + >> + list_add_tail(&event->group_list_entry, &event->group_list); >> + >> + rb_link_node(&event->group_node, parent, node); >> + rb_insert_color(&event->group_node, tree); >> + >> + return 1; > > Oh, you are using !0 for success. I guess it's a good thing you're not > actually checking its return code at the call site. Answered above. > > Regards, > -- > Alex >