perf/core improvements and fixes from Budapest

Hi Ingo/Thomas,

Please consider pulling,

Best regards,

- Arnaldo

The following changes since commit 0cc4bd8f70d1ea2940295f1050508c663fe9eff9:

Merge branch 'core/kprobes' into perf/core, to pick up fixes (2020-01-28 07:59:05 +0100)

are available in the Git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-core-for-mingo-5.6-20200201

for you to fetch changes up to 85fc95d75970ee7dd8e01904e7fb1197c275ba6b:

perf maps: Add missing unlock to maps__insert() error case (2020-01-31 09:40:50 +0100)

----------------------------------------------------------------
perf/core improvements and fixes:

perf maps:

Cengiz Can:

- Add missing unlock to maps__insert() error case.

srcline:

Changbin Du:

- Make perf able to build with latest libbfd.

perf parse:

Leo Yan:

- Keep copy of string in perf_evsel_config_term() to fix sink terms processing in ARM CoreSight.

perf test:

Thomas Richter:

- Fix test case Merge cpu map, removing extra reference count drop that causes a segfault on s/390.

perf probe:

Thomas Richter:

- Add ustring support for perf probe command

----------------------------------------------------------------
Cengiz Can (1):
perf maps: Add missing unlock to maps__insert() error case

Changbin Du (1):
perf: Make perf able to build with latest libbfd

Leo Yan (2):
perf parse: Refactor 'struct perf_evsel_config_term' perf parse: Copy string to perf_evsel_config_term

Thomas Richter (2):
perf test: Fix test case Merge cpu map
perf probe: Add ustring support for perf probe command

 tools/perf/arch/arm/util/cs-etm.c |  2 +-
 tools/perf/tests/cpumap.c         |  1 -
 tools/perf/util/evsel.c           |  8 +++--
 tools/perf/util/evsel_config.h    |  5 ++-
 tools/perf/util/map.c             |  1 +
 tools/perf/util/parse-events.c    | 67 ++++++++++++++++++++++++++-------------
 tools/perf/util/probe-finder.c    |  3 +-
 tools/perf/util/srcline.c         | 16 +++++++++-
 8 files changed, 71 insertions(+), 32 deletions(-)

From: Leo Yan <[email protected]>

The struct perf_evsel_config_term::val is a union which contains fields 'callgraph', 'drv_cfg' and 'branch' as string pointers. This leads to the complex code logic for handling every type's string separately, and it's hard to release string as a general way.

This patch refactors the structure to add a common field 'str' in the 'val' union as string pointer and remove the other three fields 'callgraph', 'drv_cfg' and 'branch'. Without passing field name, the patch simplifies the string handling with macro ADD_CONFIG_TERM_STR() for string pointer assignment.

This patch fixes multiple warnings of line over 80 characters detected by checkpatch tool.

---
 tools/perf/arch/arm/util/cs-etm.c |  2 +-
 tools/perf/util/evsel.c           |  6 +--
 tools/perf/util/evsel_config.h    |  4 +-
 tools/perf/util/parse-events.c    | 62 ++++++++++++++++++++-----------
 4 files changed, 45 insertions(+), 29 deletions(-)

index ede040cf82ad..2898cfdf8fe1 100644
@@ -226,7 +226,7 @@ static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
 		if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
 			continue;
 
-		sink = term->val.drv_cfg;
+		sink = term->val.str;
 		snprintf(path, PATH_MAX, "sinks/%s", sink);
 
 		ret = perf_pmu__scan_file(pmu, path, "%x", &hash);

index a69e64236120..549abd43816f 100644
@@ -808,12 +808,12 @@ static void apply_config_terms(struct evsel *evsel,
 				perf_evsel__reset_sample_bit(evsel, TIME);
 			break;
 		case PERF_EVSEL__CONFIG_TERM_CALLGRAPH:
-			callgraph_buf = term->val.callgraph;
+			callgraph_buf = term->val.str;
 			break;
 		case PERF_EVSEL__CONFIG_TERM_BRANCH:
-			if (term->val.branch && strcmp(term->val.branch, "no")) {
+			if (term->val.str && strcmp(term->val.str, "no")) {
 				perf_evsel__set_sample_bit(evsel, BRANCH_STACK);
-				parse_branch_str(term->val.branch,
+				parse_branch_str(term->val.str,
 						 &attr->branch_sample_type);
 			} else
 				perf_evsel__reset_sample_bit(evsel, BRANCH_STACK);

index 1f8d2fe0b66e..b4a65201e4f7 100644
@@ -36,18 +36,16 @@ struct perf_evsel_config_term {
 		u64	      period;
 		u64	      freq;
 		bool	      time;
-		char	      *callgraph;
-		char	      *drv_cfg;
 		u64	      stack_user;
 		int	      max_stack;
 		bool	      inherit;
 		bool	      overwrite;
-		char	      *branch;
 		unsigned long max_events;
 		bool	      percore;
 		bool	      aux_output;
 		u32	      aux_sample_size;
 		u64	      cfg_chg;
+		char	      *str;
 	} val;
 	bool weak;
 };

2.21.1

From: Leo Yan <[email protected]>

perf with CoreSight fails to record trace data with command:

perf record -e cs_etm/@tmc_etr0/u --per-thread ls failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a directory)/perf/

This failure is root caused with the commit 1dc925568f01 ("perf
parse: Add a deep delete for parse event terms").

The log shows, cs_etm fails to parse the sink attribution; cs_etm event relies on the event configuration to pass sink name, but the event specific configuration data cannot be passed properly with flow:

  get_config_terms()
    ADD_CONFIG_TERM(DRV_CFG, term->val.str);
      __t->val.str = term->val.str;
        `> __t->val.str is assigned to term->val.str;

parse_events_terms__purge()

    parse_events_term__delete()
      zfree(&term->val.str);
        `> term->val.str is freed and assigned to NULL pointer;

cs_etm_set_sink_attr()
sink = __t->val.str;
`> sink string has been freed.

To fix this issue, in the function get_config_terms(), this patch changes to use strdup() for allocation a new duplicate string rather than directly assignment string pointer.

This patch addes a new field 'free_str' in the data structure perf_evsel_config_term; 'free_str' is set to true when the union is used as a string pointer; thus it can tell perf_evsel__free_config_terms() to free the string.

:# modified: tools/perf/util/evsel_config.h

---
 tools/perf/util/evsel.c        | 2 ++
 tools/perf/util/evsel_config.h | 1 +
 tools/perf/util/parse-events.c | 7 ++++++-
 3 files changed, 9 insertions(+), 1 deletion(-)

index 549abd43816f..c8dc4450884c 100644
@@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
 
 	list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
 		list_del_init(&term->list);
+		if (term->free_str)
+			zfree(&term->val.str);
 		free(term);
 	}
 }

index b4a65201e4f7..e026ab67b008 100644
@@ -32,6 +32,7 @@ enum evsel_term_type {
 struct perf_evsel_config_term {
 	struct list_head      list;
 	enum evsel_term_type  type;
+	bool		      free_str;
 	union {
 		u64	      period;
 		u64	      freq;

index f59f3c8da473..c01ba6f8fdad 100644
@@ -1240,7 +1240,12 @@ do {								\
 #define ADD_CONFIG_TERM_STR(__type, __val)			\
 do {								\
 	ADD_CONFIG_TERM(__type);				\
-	__t->val.str = __val;					\
+	__t->val.str = strdup(__val);				\
+	if (!__t->val.str) {					\
+		zfree(&__t);					\
+		return -ENOMEM;					\
+	}							\
+	__t->free_str = true;					\
 } while (0)
 
 	struct parse_events_term *term;

2.21.1

From: Thomas Richter <[email protected]>

Commit a2408a70368a ("perf evlist: Maintain evlist->all_cpus") introduces a test case for cpumap merge operation, see functions perf_cpu_map__merge() and test__cpu_map_merge().

The test case fails on s390 with this error message:

[root@m35lp76 perf]# ./perf test -Fvvvvv 52
52: Merge cpu map :
--- start ---
cpumask list: 1-2,4-5,7
perf: /root/linux/tools/include/linux/refcount.h:131:\
refcount_sub_and_test: Assertion `!(new > val)' failed.
Aborted (core dumped)
[root@m35lp76 perf]#

The root cause is in the function test__cpu_map_merge():
It creates two cpu_maps named 'a' and 'b':

  struct perf_cpu_map *a = perf_cpu_map__new("4,2,1");
  struct perf_cpu_map *b = perf_cpu_map__new("4,5,7");

and creates a third map named 'c' which is the result of the merge of maps a and b:

  struct perf_cpu_map *c = perf_cpu_map__merge(a, b);

After some verifaction of the merged cpu_map all three of them are have their reference count reduced and are freed:

   perf_cpu_map__put(a); (1)
   perf_cpu_map__put(b);
   perf_cpu_map__put(c);

The release of perf_cpu_map__put(a) is wrong. The map is already released and free'ed as part of the function

  perf_cpu_map__merge(struct perf_cpu_map *orig,
  |	              struct perf_cpu_map *other)
  +--> perf_cpu_map__put(orig);
       |
       +--> cpu_map__delete(orig)

At the end perf_cpu_map_put() is called for map 'orig' alias 'a' and since the reference count is 1, the map is deleted, as can be seen by the following gdb trace:

(gdb) where
#0 tcache_put (tc_idx=0, chunk=0x156cc30) at malloc.c:2940 #1 _int_free (av=0x3fffd49ee80 <main_arena>, p=0x156cc30,

		     have_lock=<optimized out>) at malloc.c:4222
 #2  0x00000000012d5e78 in cpu_map__delete (map=0x156cc40) at cpumap.c:31
 #3  0x00000000012d5f7a in perf_cpu_map__put (map=0x156cc40) at cpumap.c:45
 #4  0x00000000012d723a in perf_cpu_map__merge (orig=0x156cc40,
     other=0x156cc60) at cpumap.c:343
 #5  0x000000000110cdd0 in test__cpu_map_merge (
     test=0x14ea6c8 <generic_tests+2856>, subtest=-1) at tests/cpumap.c:128

Thus the perf_cpu_map__put(a) (see (1) above) frees map 'a' a second time and causes the failure. Fix this be removing that function call.

Output after:
[root@m35lp76 perf]# ./perf test -Fvvvvv 52
52: Merge cpu map :
--- start ---
cpumask list: 1-2,4-5,7
---- end ----
Merge cpu map: Ok
[root@m35lp76 perf]#

---
 tools/perf/tests/cpumap.c | 1 -
 1 file changed, 1 deletion(-)

index 4ac56741ac5f..29c793ac7d10 100644
@@ -131,7 +131,6 @@ int test__cpu_map_merge(struct test *test __maybe_unused, int subtest __maybe_un
 	TEST_ASSERT_VAL("failed to merge map: bad nr", c->nr == 5);
 	cpu_map__snprint(c, buf, sizeof(buf));
 	TEST_ASSERT_VAL("failed to merge map: bad result", !strcmp(buf, "1-2,4-5,7"));
-	perf_cpu_map__put(a);
 	perf_cpu_map__put(b);
 	perf_cpu_map__put(c);
 	return 0;

2.21.1

From: Changbin Du <[email protected]>

libbfd has changed the bfd_section_* macros to inline functions bfd_section_<field> since 2019-09-18. See below two commits:
o http://www.sourceware.org/ml/gdb-cvs/2019-09/msg00064.html
o https://www.sourceware.org/ml/gdb-cvs/2019-09/msg00072.html

This fix make perf able to build with both old and new libbfd.

---
 tools/perf/util/srcline.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

index 6ccf6f6d09df..5b7d6c16d33f 100644
@@ -193,16 +193,30 @@ static void find_address_in_section(bfd *abfd, asection *section, void *data)
 	bfd_vma pc, vma;
 	bfd_size_type size;
 	struct a2l_data *a2l = data;
+	flagword flags;
 
 	if (a2l->found)
 		return;
 
-	if ((bfd_get_section_flags(abfd, section) & SEC_ALLOC) == 0)
+#ifdef bfd_get_section_flags
+	flags = bfd_get_section_flags(abfd, section);
+#else
+	flags = bfd_section_flags(section);
+#endif
+	if ((flags & SEC_ALLOC) == 0)
 		return;
 
 	pc = a2l->addr;
+#ifdef bfd_get_section_vma
 	vma = bfd_get_section_vma(abfd, section);
+#else
+	vma = bfd_section_vma(section);
+#endif
+#ifdef bfd_get_section_size
 	size = bfd_get_section_size(section);
+#else
+	size = bfd_section_size(section);
+#endif
 
 	if (pc < vma || pc >= vma + size)
 		return;

2.21.1

From: Thomas Richter <[email protected]>

Kernel commit 88903c464321 ("tracing/probe: Add ustring type for user-space string") adds support for user-space strings when type 'ustring' is specified.

Here is an example using sysfs command line interface for kprobes:

Function to probe:
struct filename *
getname_flags(const char __user *filename, int flags, int *empty)

Setup:

  # cd /sys/kernel/debug/tracing/
  # echo 'p:tmr1 getname_flags +0(%r2):ustring' > kprobe_events
  # cat events/kprobes/tmr1/format | fgrep print
  print fmt: "(%lx) arg1=\"%s\"", REC->__probe_ip, REC->arg1
  # echo 1 > events/kprobes/tmr1/enable
  # touch /tmp/111
  # echo 0 > events/kprobes/tmr1/enable
  # cat trace|fgrep /tmp/111
  touch-5846  [005] d..2 255520.717960: tmr1:\
	  (getname_flags+0x0/0x400) arg1="/tmp/111"

Doing the same with the perf tool fails.
Using type 'string' succeeds:

 # perf probe "vfs_getname=getname_flags:72 pathname=filename:string"
 Added new event:
   probe:vfs_getname (on getname_flags:72 with pathname=filename:string)
   ....
 # perf probe -d probe:vfs_getname
 Removed event: probe:vfs_getname

However using type 'ustring' fails (output before):

 # perf probe "vfs_getname=getname_flags:72 pathname=filename:ustring"
 Failed to write event: Invalid argument
   Error: Failed to add events.
 #

Fix this by adding type 'ustring' in function
convert_variable_type().

Using ustring succeeds (output after):

 # ./perf probe "vfs_getname=getname_flags:72 pathname=filename:ustring"
 Added new event:
   probe:vfs_getname (on getname_flags:72 with pathname=filename:ustring)

You can now use it in all perf tools, such as:

perf record -e probe:vfs_getname -aR sleep 1

#

Note: This issue also exists on x86, it is not s390 specific.

---
 tools/perf/util/probe-finder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index c470c49a804f..1c817add6ca4 100644
@@ -303,7 +303,8 @@ static int convert_variable_type(Dwarf_Die *vr_die,
 	char prefix;
 
 	/* TODO: check all types */
-	if (cast && strcmp(cast, "string") != 0 && strcmp(cast, "x") != 0 &&
+	if (cast && strcmp(cast, "string") != 0 && strcmp(cast, "ustring") &&
+	    strcmp(cast, "x") != 0 &&
 	    strcmp(cast, "s") != 0 && strcmp(cast, "u") != 0) {
 		/* Non string type is OK */
 		/* and respect signedness/hexadecimal cast */

2.21.1

From: Cengiz Can <[email protected]>

tools/perf/util/map.c has a function named maps__insert that acquires a write lock if its in multithread context.

Even though this lock is released when function successfully completes, there's a branch that is executed when maps_by_name == NULL that returns from this function without releasing the write lock.

Added an up_write to release the lock when this happens.

---
 tools/perf/util/map.c | 1 +
 1 file changed, 1 insertion(+)

index fdd5bddb3075..f67960bedebb 100644
@@ -549,6 +549,7 @@ void maps__insert(struct maps *maps, struct map *map)
 
 			if (maps_by_name == NULL) {
 				__maps__free_maps_by_name(maps);
+				up_write(&maps->lock);
 				return;
 			}
 

2.21.1

Pulled, thanks a lot Arnaldo!

Ingo