From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753260AbcD1Ig6 (ORCPT ); Thu, 28 Apr 2016 04:36:58 -0400 Received: from mail-pa0-f68.google.com ([209.85.220.68]:34643 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753007AbcD1Igy (ORCPT ); Thu, 28 Apr 2016 04:36:54 -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> <571AFD14.9030301@gmail.com> Cc: linux-kernel@vger.kernel.org From: Chris Phlipot Message-ID: <5721CBA2.1010805@gmail.com> Date: Thu, 28 Apr 2016 01:36:50 -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: <571AFD14.9030301@gmail.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 Hi Adrian, I have just resubmitted these changes as a new patch set, which I believe should address most of your concerns. Please review the new patch set instead of continuing with this one. https://lkml.org/lkml/2016/4/28/75 Thanks, Chris On 04/22/2016 09:41 PM, Chris Phlipot wrote: > > > 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