From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2993484AbbEEQLk (ORCPT ); Tue, 5 May 2015 12:11:40 -0400 Received: from mail.kernel.org ([198.145.29.136]:37249 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2993493AbbEEQA7 (ORCPT ); Tue, 5 May 2015 12:00:59 -0400 Date: Tue, 5 May 2015 13:00:54 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern Subject: Re: [PATCH v3 10/10] perf tools: Move TUI-specific fields out of map_symbol Message-ID: <20150505160054.GX10475@kernel.org> References: <20150505142626.GN10475@kernel.org> <1430837746-5439-1-git-send-email-namhyung@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430837746-5439-1-git-send-email-namhyung@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Tue, May 05, 2015 at 11:55:46PM +0900, Namhyung Kim escreveu: > The has_children and unfolded fields don't belong to struct map_symbol > since they're used by TUI only. Move those fields out of map_symbol > since the struct is also used by other places. > > This will also help to compact the sizeof struct hist_entry. So this fixes that problem, but to test this patchkit a bit more before commiting it, I tried the following: 1) perf record -a -g sleep 2 2) perf report 3) press E to expand all callchains 4) press P to save a perf.hist.0 "screenshot" of all the hist entries and its callchains, in their current state, i.e. expanded Ok I did this with and without your patchset, and there are differences in the result, that I don't know if are unintended fixes or how to account them for, could you try reproducing these steps to see if you notice this difference too? Ok, now I understand, its a bug, when collecting callchains we need to process them all, marking the DSOs that had hits _in the callchains_ and save the build-id for those in the perf.data header, in addition to saving the DSO in the buildid cache. What happened was that the different entries I noticed where all in the callchains, and the perf tool got rebuilt, and all the samples, in my case, were in kernel functions, that were called by the perf binary.... So for something like: --- perf.hist.0.WITHOUT 2015-05-05 12:49:06.557802910 -0300 +++ perf.hist.0.WITH_NAMHYUNG_PATCHSET 2015-05-05 12:48:13.871216253 -0300 @@ -167,26 +167,26 @@ - 0.07% 0.00% perf perf [.] main main __libc_start_main -- 0.07% 0.00% perf perf [.] browser_command_config - browser_command_config +- 0.07% 0.00% perf perf [.] run_builtin And I collected the perf.data with your patchset applied, the correct is to appear "run_builtin", not like here: - 0.07% 0.00% perf perf [.] browser_command_config browser_command_config main This is all because the perf binary used got lost, replaced by the new one, as it wasn't saved in the perf.data file header/~/.debug/ cache: [root@ssdandy linux]# perf buildid-list cd1d2cf9f473d0cac668e3afee32866da4540bd4 /lib/modules/4.0.0-rc6+/build/vmlinux 2c07e0dc941be12d3db663ea617c08cf10dbbfa1 /lib/modules/4.0.0-rc6+/kernel/drivers/net/wireless/iwlwifi/iwlwifi.ko [root@ssdandy linux]# The fix probably will make the buildid collection at the end of a 'perf record' session more expensive, will try to fix this... But your patch is not responsible for this, so thanks for the fixes and I'm applying it. - Arnaldo