Messages in this thread | | | Subject | Re: [PATCH 2/5] perf script: extend db-export api to include callchains for samples | From | Chris Phlipot <> | Date | Thu, 28 Apr 2016 01:36:50 -0700 |
| |
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
| |