Re: [PATCH RFC V8 3/4] perf,tools: per-event callgraph support

[Arnaldo Carvalho de Melo]

Em Wed, Aug 05, 2015 at 03:45:27PM +0000, Liang, Kan escreveu:
> > hum, calling parse_callchain_record_opt from evsel hurts the python code:
> > 
> > 17: Try 'import perf' in python, checking link problems      :
> > --- start ---
> > test child forked, pid 25751
> > Traceback (most recent call last):
> >   File "<stdin>", line 1, in <module>
> > ImportError: python/perf.so: undefined symbol:
> > parse_callchain_record_opt test child finished with -1
> > ---- end ----
> > Try 'import perf' in python, checking link problems: FAILED!
> > 
> > 
> > not sure if we can call it from some place else (I guess not), then we'd need
> > to either put the util/callchain.c under python objects,
> 
> We cannot only put the util/callchain.c under python objects, since
> there are too many dependency for callchain.c. 
> 
> > or somehow refine
> > needed parsing code..
> 
> Could we just move the related code to util.c as below?

Looks sensible, can you resend this as a separate patch, plus the two
remaining ones? I.e. patch below is the first on this new series, then
this per event callchain selection one, then the 'perf test' patch.

The first two already are in tip.git

Thanks,

- Arnaldo
 
> ---
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 931cca8..773fe13 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -25,96 +25,9 @@
>  
>  __thread struct callchain_cursor callchain_cursor;
>  
> -#ifdef HAVE_DWARF_UNWIND_SUPPORT
> -static int get_stack_size(const char *str, unsigned long *_size)
> -{
> -	char *endptr;
> -	unsigned long size;
> -	unsigned long max_size = round_down(USHRT_MAX, sizeof(u64));
> -
> -	size = strtoul(str, &endptr, 0);
> -
> -	do {
> -		if (*endptr)
> -			break;
> -
> -		size = round_up(size, sizeof(u64));
> -		if (!size || size > max_size)
> -			break;
> -
> -		*_size = size;
> -		return 0;
> -
> -	} while (0);
> -
> -	pr_err("callchain: Incorrect stack dump size (max %ld): %s\n",
> -	       max_size, str);
> -	return -1;
> -}
> -#endif /* HAVE_DWARF_UNWIND_SUPPORT */
> -
>  int parse_callchain_record_opt(const char *arg, struct callchain_param *param)
>  {
> -	char *tok, *name, *saveptr = NULL;
> -	char *buf;
> -	int ret = -1;
> -
> -	/* We need buffer that we know we can write to. */
> -	buf = malloc(strlen(arg) + 1);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	strcpy(buf, arg);
> -
> -	tok = strtok_r((char *)buf, ",", &saveptr);
> -	name = tok ? : (char *)buf;
> -
> -	do {
> -		/* Framepointer style */
> -		if (!strncmp(name, "fp", sizeof("fp"))) {
> -			if (!strtok_r(NULL, ",", &saveptr)) {
> -				param->record_mode = CALLCHAIN_FP;
> -				ret = 0;
> -			} else
> -				pr_err("callchain: No more arguments "
> -				       "needed for --call-graph fp\n");
> -			break;
> -
> -#ifdef HAVE_DWARF_UNWIND_SUPPORT
> -		/* Dwarf style */
> -		} else if (!strncmp(name, "dwarf", sizeof("dwarf"))) {
> -			const unsigned long default_stack_dump_size = 8192;
> -
> -			ret = 0;
> -			param->record_mode = CALLCHAIN_DWARF;
> -			param->dump_size = default_stack_dump_size;
> -
> -			tok = strtok_r(NULL, ",", &saveptr);
> -			if (tok) {
> -				unsigned long size = 0;
> -
> -				ret = get_stack_size(tok, &size);
> -				param->dump_size = size;
> -			}
> -#endif /* HAVE_DWARF_UNWIND_SUPPORT */
> -		} else if (!strncmp(name, "lbr", sizeof("lbr"))) {
> -			if (!strtok_r(NULL, ",", &saveptr)) {
> -				param->record_mode = CALLCHAIN_LBR;
> -				ret = 0;
> -			} else
> -				pr_err("callchain: No more arguments "
> -					"needed for --call-graph lbr\n");
> -			break;
> -		} else {
> -			pr_err("callchain: Unknown --call-graph option "
> -			       "value: %s\n", arg);
> -			break;
> -		}
> -
> -	} while (0);
> -
> -	free(buf);
> -	return ret;
> +	return parse_callchain_record(arg, param);
>  }
>  
>  static int parse_callchain_mode(const char *value)
> diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> index 68a32c2..acee2b3 100644
> --- a/tools/perf/util/callchain.h
> +++ b/tools/perf/util/callchain.h
> @@ -177,6 +177,7 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
>  			bool hide_unresolved);
>  
>  extern const char record_callchain_help[];
> +extern int parse_callchain_record(const char *arg, struct callchain_param *param);
>  int parse_callchain_record_opt(const char *arg, struct callchain_param *param);
>  int parse_callchain_report_opt(const char *arg);
>  int perf_callchain_config(const char *var, const char *value);
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 106cd20..675d237 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -651,7 +651,7 @@ static void apply_config_terms(struct perf_evsel *evsel,
>  				param.record_mode = CALLCHAIN_NONE;
>  			} else {
>  				param.enabled = true;
> -				if (parse_callchain_record_opt(callgraph_buf, &param)) {
> +				if (parse_callchain_record(callgraph_buf, &param)) {
>  					pr_err("per-event callgraph setting for %s failed. "
>  					       "Apply callgraph global setting for it\n",
>  					       evsel->name);
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index edc2d63..f7adf12 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -566,6 +566,96 @@ unsigned long parse_tag_value(const char *str, struct parse_tag *tags)
>  	return (unsigned long) -1;
>  }
>  
> +int get_stack_size(const char *str, unsigned long *_size)
> +{
> +	char *endptr;
> +	unsigned long size;
> +	unsigned long max_size = round_down(USHRT_MAX, sizeof(u64));
> +
> +	size = strtoul(str, &endptr, 0);
> +
> +	do {
> +		if (*endptr)
> +			break;
> +
> +		size = round_up(size, sizeof(u64));
> +		if (!size || size > max_size)
> +			break;
> +
> +		*_size = size;
> +		return 0;
> +
> +	} while (0);
> +
> +	pr_err("callchain: Incorrect stack dump size (max %ld): %s\n",
> +	       max_size, str);
> +	return -1;
> +}
> +
> +int parse_callchain_record(const char *arg, struct callchain_param *param)
> +{
> +	char *tok, *name, *saveptr = NULL;
> +	char *buf;
> +	int ret = -1;
> +
> +	/* We need buffer that we know we can write to. */
> +	buf = malloc(strlen(arg) + 1);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	strcpy(buf, arg);
> +
> +	tok = strtok_r((char *)buf, ",", &saveptr);
> +	name = tok ? : (char *)buf;
> +
> +	do {
> +		/* Framepointer style */
> +		if (!strncmp(name, "fp", sizeof("fp"))) {
> +			if (!strtok_r(NULL, ",", &saveptr)) {
> +				param->record_mode = CALLCHAIN_FP;
> +				ret = 0;
> +			} else
> +				pr_err("callchain: No more arguments "
> +				       "needed for --call-graph fp\n");
> +			break;
> +
> +#ifdef HAVE_DWARF_UNWIND_SUPPORT
> +		/* Dwarf style */
> +		} else if (!strncmp(name, "dwarf", sizeof("dwarf"))) {
> +			const unsigned long default_stack_dump_size = 8192;
> +
> +			ret = 0;
> +			param->record_mode = CALLCHAIN_DWARF;
> +			param->dump_size = default_stack_dump_size;
> +
> +			tok = strtok_r(NULL, ",", &saveptr);
> +			if (tok) {
> +				unsigned long size = 0;
> +
> +				ret = get_stack_size(tok, &size);
> +				param->dump_size = size;
> +			}
> +#endif /* HAVE_DWARF_UNWIND_SUPPORT */
> +		} else if (!strncmp(name, "lbr", sizeof("lbr"))) {
> +			if (!strtok_r(NULL, ",", &saveptr)) {
> +				param->record_mode = CALLCHAIN_LBR;
> +				ret = 0;
> +			} else
> +				pr_err("callchain: No more arguments "
> +					"needed for --call-graph lbr\n");
> +			break;
> +		} else {
> +			pr_err("callchain: Unknown --call-graph option "
> +			       "value: %s\n", arg);
> +			break;
> +		}
> +
> +	} while (0);
> +
> +	free(buf);
> +	return ret;
> +}
> +
>  int filename__read_str(const char *filename, char **buf, size_t *sizep)
>  {
>  	size_t size = 0, alloc_size = 0;
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 20d625a..8148703 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -351,4 +351,6 @@ static inline char *asprintf_expr_not_in_ints(const char *var, size_t nints, int
>  	return asprintf_expr_inout_ints(var, false, nints, ints);
>  }
>  
> +int get_stack_size(const char *str, unsigned long *_size);
> +
>  #endif /* GIT_COMPAT_UTIL_H */
>  --
> 
> Thanks,
> Kan
> 
> --
> 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/
--
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/