From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751824AbcDWEmB (ORCPT ); Sat, 23 Apr 2016 00:42:01 -0400 Received: from mail-pa0-f68.google.com ([209.85.220.68]:34917 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751219AbcDWEmA (ORCPT ); Sat, 23 Apr 2016 00:42:00 -0400 Subject: Re: [PATCH 2/5] perf script: extend db-export api to include callchains for samples To: Adrian Hunter , acme@kernel.org, mingo@redhat.com, peterz@infradead.org References: <1461056164-14914-1-git-send-email-cphlipot0@gmail.com> <1461056164-14914-2-git-send-email-cphlipot0@gmail.com> <5719D914.2000109@intel.com> Cc: linux-kernel@vger.kernel.org From: Chris Phlipot Message-ID: <571AFD14.9030301@gmail.com> Date: Fri, 22 Apr 2016 21:41:56 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <5719D914.2000109@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/22/2016 12:56 AM, Adrian Hunter wrote: > The call_paths table already has symbol_id which belongs uniquely to a DSO, > so why do we need dso_id as well? If the symbol_id is 0 because the IP could not be resolved to a symbol, this is not necessarily a valid assumption. Without a dso_id in the call_paths table, it is not possible to resolve the dso when symbol information is missing. the db_export api currently does not have enough information to match a DSO with an IP. It is often useful to still have the call path associated with a DSO even if there is no symbol, which i why i recommend keeping the dso_id in the call_paths table. > Why do you need a callback? Seems like the only thing you need from > thread-stack.c is the call path tree. You could move that to its own .c/.h > files and then process the call chain in db-export.c My original intent was to reuse existing code with minimal changes and conform the existing design patterns they used. Thread-stack.c, for example, currently uses a callback to populate the call_return table, so I used a callback as well to populate the call_path table. I am open to making this change if it is believed it will result in a cleaner implementation. > > Also a list of changes like the one above heavily implies you are not > obeying the one patch == one change rule. Please try to make patches that > only do one thing and also run checkpatch. While i can split this into a few smaller patches there is only really justification for applying all of them all together. If this is still preferred i can resubmit this in smaller parts. > > If you don't mind, I'll let you respond to my comments before I comment on > any other patches. > Let me know if you have any additional comments. Thanks, Chris