Re: [PATCH 4/9] perf kmem: Implement stat --page --caller

[Namhyung Kim <namhyung@kernel.org>]

Hi Arnaldo,

On Mon, Apr 13, 2015 at 10:40:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 06, 2015 at 02:36:11PM +0900, Namhyung Kim escreveu:
> > +static int build_alloc_func_list(void)
> > +{
> > +	int ret;
> > +	struct map *kernel_map;
> > +	struct symbol *sym;
> > +	struct rb_node *node;
> > +	struct alloc_func *func;
> > +	struct machine *machine = &kmem_session->machines.host;
> > +
> 
> Why having a blank like here?

Will remove.

> 
> > +	regex_t alloc_func_regex;
> > +	const char pattern[] = "^_?_?(alloc|get_free|get_zeroed)_pages?";
> > +
> > +	ret = regcomp(&alloc_func_regex, pattern, REG_EXTENDED);
> > +	if (ret) {
> > +		char err[BUFSIZ];
> > +
> > +		regerror(ret, &alloc_func_regex, err, sizeof(err));
> > +		pr_err("Invalid regex: %s\n%s", pattern, err);
> > +		return -EINVAL;
> > +	}
> > +
> > +	kernel_map = machine->vmlinux_maps[MAP__FUNCTION];
> > +	map__load(kernel_map, NULL);
> 
> What if the map can't be loaded?

Hmm.. yes, I need to check the return code.


> 
> > +
> > +	map__for_each_symbol(kernel_map, sym, node) {
> > +		if (regexec(&alloc_func_regex, sym->name, 0, NULL, 0))
> > +			continue;
> > +
> > +		func = realloc(alloc_func_list,
> > +			       (nr_alloc_funcs + 1) * sizeof(*func));
> > +		if (func == NULL)
> > +			return -ENOMEM;
> > +
> > +		pr_debug("alloc func: %s\n", sym->name);
> > +		func[nr_alloc_funcs].start = sym->start;
> > +		func[nr_alloc_funcs].end   = sym->end;
> > +		func[nr_alloc_funcs].name  = sym->name;
> > +
> > +		alloc_func_list = func;
> > +		nr_alloc_funcs++;
> > +	}
> > +
> > +	qsort(alloc_func_list, nr_alloc_funcs, sizeof(*func), funcmp);
> > +
> > +	regfree(&alloc_func_regex);
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Find first non-memory allocation function from callchain.
> > + * The allocation functions are in the 'alloc_func_list'.
> > + */
> > +static u64 find_callsite(struct perf_evsel *evsel, struct perf_sample *sample)
> > +{
> > +	struct addr_location al;
> > +	struct machine *machine = &kmem_session->machines.host;
> > +	struct callchain_cursor_node *node;
> > +
> > +	if (alloc_func_list == NULL)
> > +		build_alloc_func_list();

And here too..


> > +
> > +	al.thread = machine__findnew_thread(machine, sample->pid, sample->tid);
> > +	sample__resolve_callchain(sample, NULL, evsel, &al, 16);
> > +
> > +	callchain_cursor_commit(&callchain_cursor);
> > +	while (true) {
> > +		struct alloc_func key, *caller;
> > +		u64 addr;
> > +
> > +		node = callchain_cursor_current(&callchain_cursor);
> > +		if (node == NULL)
> > +			break;
> > +
> > +		key.start = key.end = node->ip;
> > +		caller = bsearch(&key, alloc_func_list, nr_alloc_funcs,
> > +				 sizeof(key), callcmp);
> > +		if (!caller) {
> > +			/* found */
> > +			if (node->map)
> > +				addr = map__unmap_ip(node->map, node->ip);
> > +			else
> > +				addr = node->ip;
> > +
> > +			return addr;
> > +		} else
> > +			pr_debug3("skipping alloc function: %s\n", caller->name);
> > +
> > +		callchain_cursor_advance(&callchain_cursor);
> > +	}
> > +
> > +	pr_debug2("unknown callsite: %"PRIx64 "\n", sample->ip);
> > +	return sample->ip;
> > +}
> > +
> > +static struct page_stat *search_page(u64 page, bool create)
> >  {
> >  	struct rb_node **node = &page_tree.rb_node;
> >  	struct rb_node *parent = NULL;
> > @@ -357,6 +491,41 @@ static struct page_stat *search_page_alloc_stat(struct page_stat *stat, bool cre
> >  	return data;
> >  }
> >  
> > +static struct page_stat *search_page_caller_stat(u64 callsite, bool create)
> > +{
> > +	struct rb_node **node = &page_caller_tree.rb_node;
> > +	struct rb_node *parent = NULL;
> > +	struct page_stat *data;
> 
> Please use the "findnew" idiom to name this function, looking at only
> its name one things it searches a tree, a read only operation, but it
> may insert elements too, a modify operation.
> 
> Since we use the findnew idiom elsewhere for operations that do that,
> i.e. optimize the "new" part of "findnew" by using the "find" part,
> please use it here as well.

OK.  Will change and resend v7 soon.

Thanks for your review!
Namhyung