From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752516AbdFORoj (ORCPT ); Thu, 15 Jun 2017 13:44:39 -0400 Received: from mga07.intel.com ([134.134.136.100]:51630 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751900AbdFORoi (ORCPT ); Thu, 15 Jun 2017 13:44:38 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,344,1493708400"; d="scan'208";a="115410340" Subject: Re: [PATCH]: perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi From: Alexey Budankov 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> <47dc6d8d-77db-70f5-9aa6-2aca38590e60@linux.intel.com> <87h902zr1v.fsf@ashishki-desk.ger.corp.intel.com> Organization: Intel Corp. Message-ID: <600d51c3-6a71-53be-f31d-fca99d36c097@linux.intel.com> Date: Thu, 15 Jun 2017 20:44:32 +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: 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 14.06.2017 13:07, Alexey Budankov wrote: > On 30.05.2017 11:29, Alexander Shishkin wrote: >> 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. addressed in v3. >> >>> >>>> >>>>> +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(). > > Ok. WARN_ON_ONCE() then. addressed in v3. > >> >>> >>>> >>>>> + >>>>> + 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? > > Yes, that is right. addressed in v3. > >>> >>> 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". > > Accepted. Peter already provided more specifics on this. Thanks. > >> >> Thanks, >> -- >> Alex >> >