From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751050AbdE3I3G (ORCPT ); Tue, 30 May 2017 04:29:06 -0400 Received: from mga06.intel.com ([134.134.136.31]:30793 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750904AbdE3I3F (ORCPT ); Tue, 30 May 2017 04:29:05 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,417,1491289200"; d="scan'208";a="1136108507" From: Alexander Shishkin To: Alexey Budankov , 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 Subject: Re: [PATCH]: perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi In-Reply-To: <47dc6d8d-77db-70f5-9aa6-2aca38590e60@linux.intel.com> References: <1e962b59-3e39-e0d6-515d-c4fd3502edae@linux.intel.com> <87k24zzx7s.fsf@ashishki-desk.ger.corp.intel.com> <47dc6d8d-77db-70f5-9aa6-2aca38590e60@linux.intel.com> User-Agent: Notmuch/0.23.7 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Tue, 30 May 2017 11:29:00 +0300 Message-ID: <87h902zr1v.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: > 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? Sure, this is where API descriptions fit better than in commit messages. > >> >>> +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. Your call. But I'd still either drop the redundant checks or wrap them in WARN_ON_ONCE(). > >> >>> + >>> + 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. Side note: between commit message, comments and the actual code, in an ideal situation one doesn't have to 'ask' anything, because everything is already clear. Not the case here. > 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. Not sure I understand. You'd still have both. > >> >>> + } 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. Please be more specific on how scalability and performance are improved. In general, try to avoid vagues statements like "this is better for performance". Thanks, -- Alex