Re: [PATCH 2/4] perf probe: Do not rely on map__load() filter to find symbols

From: Masami Hiramatsu
Date: Wed Jan 14 2015 - 03:42:37 EST


(2015/01/14 10:45), Namhyung Kim wrote:
> On Mon, Jan 12, 2015 at 09:31:30PM +0900, Masami Hiramatsu wrote:
>> (2015/01/10 19:33), Namhyung Kim wrote:
>>> The find_probe_trace_events_from_map() searches matching symbol from a
>>> map (so from a backing dso). For uprobes, it'll create a new map (and
>>> dso) and loads it using a filter. It's a little bit inefficient in that
>>> it'll read out the symbol table everytime but works well anyway.
>>>
>>> For kprobes however, it'll reuse existing kernel map which might be
>>> loaded before. In this case map__load() just returns with no result.
>>> It makes kprobes always failed to find symbol even if it exists in the
>>> map (dso).
>>>
>>> To fix it, use map__find_symbol_by_name() instead. It'll load a map
>>> with full symbols and sorts them by name. It needs to search sibing
>>> nodes since there can be multiple (local) symbols with same name. Now
>>> resulting symbol references are saved in the funcs list.
>>>
>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>>> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
>>> ---
>>> tools/perf/util/probe-event.c | 101 ++++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 87 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>>> index 7f9b8632e433..e5af16988791 100644
>>> --- a/tools/perf/util/probe-event.c
>>> +++ b/tools/perf/util/probe-event.c
>>> @@ -2191,20 +2191,86 @@ static int __add_probe_trace_events(struct perf_probe_event *pev,
>>> return ret;
>>> }
>>>
>>> -static char *looking_function_name;
>>> -static int num_matched_functions;
>>> +struct symbol_entry {
>>> + struct list_head node;
>>> + struct symbol *sym;
>>> +};
>>>
>>> -static int probe_function_filter(struct map *map __maybe_unused,
>>> - struct symbol *sym)
>>> +/* returns 1 if symbol was added, 0 if symbol was skipped, -1 if error */
>>> +static int add_symbol_entry(struct symbol *sym, struct list_head *head)
>>> {
>>> - if ((sym->binding == STB_GLOBAL || sym->binding == STB_LOCAL) &&
>>> - strcmp(looking_function_name, sym->name) == 0) {
>>> - num_matched_functions++;
>>> + struct symbol_entry *ent;
>>> +
>>> + if (sym->binding != STB_GLOBAL && sym->binding != STB_LOCAL)
>>> return 0;
>>> - }
>>> +
>>> + ent = malloc(sizeof(*ent));
>>> + if (ent == NULL)
>>> + return -1;
>>
>> return -ENOMEM; ?
>
> Okay, will change.
>
>
>>
>>> +
>>> + ent->sym = sym;
>>> + list_add(&ent->node, head);
>>> return 1;
>>> }
>>>
>>> +static int find_probe_functions(struct map *map, char *name, struct list_head *head)
>>> +{
>>> + struct symbol *sym, *orig_sym;
>>> + struct symbol_entry *ent;
>>> + struct rb_node *node;
>>> + int found = 0;
>>> + int ret;
>>> +
>>> + sym = map__find_symbol_by_name(map, name, NULL);
>>> + if (sym == NULL)
>>> + return 0;
>>> +
>>> + ret = add_symbol_entry(sym, head);
>>> + if (ret < 0)
>>> + goto err;
>>> +
>>> + found += ret;
>>
>> If ret always be 1 in successful case, we'd better do "found++" here.
>> And it also means we can do it shorter as below.
>>
>> if (add_symbol_entry(sym, head) < 0)
>> goto err;
>> found++;
>
> But it can return 0 in successful case like STB_WEAK.. I'm not sure
> how we can handle the weak functions properly, but anyway the original
> code already ignored the weak functions.

Ah, I see... OK, it should not be changed.

>>> +
>>> + /* search back and forth to find symbols that have same name */
>>
>> Hmm, I see. but this code looks no-good sign... Can we have any generic
>> synonym handling routine?
>
> Like what? I guess we can change map__find_symbol_by_name() to return
> a list of symbols or add a new function to do it. Is it okay to you?

Yeah, one possible solution is introducing map__find_all_symbols_by_name()
for looking up all the symbols who have same name. Or, map__find_symbol_by_name()
does forward search on rbtree to find the first symbol and returns it, so that
caller just do backward search.

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@xxxxxxxxxxx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/