[PATCH v5 1/3] perf lock: Fix parse_lock_type which only retrieve one lock flag

[PATCH v5 1/3] perf lock: Fix parse_lock_type which only retrieve one lock flag

[Chun-Tse Shao]

`parse_lock_type` can only add the first lock flag in `lock_type_table`
given input `str`. For example, for `Y rwlock`, it only adds `rwlock:R`
into this perf session. Another example is for `-Y mutex`, it only adds
the mutex without `LCB_F_SPIN` flag. The patch fixes this issue, makes
sure both `rwlock:R` and `rwlock:W` will be added with `-Y rwlock`, and
so on.

Testing:
  $ ./perf lock con -ab -Y mutex,rwlock -- perf bench sched pipe
  # Running 'sched/pipe' benchmark:
  # Executed 1000000 pipe operations between two processes

       Total time: 9.313 [sec]

         9.313976 usecs/op
           107365 ops/sec
   contended   total wait     max wait     avg wait         type   caller

         176      1.65 ms     19.43 us      9.38 us        mutex   pipe_read+0x57
          34    180.14 us     10.93 us      5.30 us        mutex   pipe_write+0x50
           7     77.48 us     16.09 us     11.07 us        mutex   do_epoll_wait+0x24d
           7     74.70 us     13.50 us     10.67 us        mutex   do_epoll_wait+0x24d
           3     35.97 us     14.44 us     11.99 us     rwlock:W   ep_done_scan+0x2d
           3     35.00 us     12.23 us     11.66 us     rwlock:W   do_epoll_wait+0x255
           2     15.88 us     11.96 us      7.94 us     rwlock:W   do_epoll_wait+0x47c
           1     15.23 us     15.23 us     15.23 us     rwlock:W   do_epoll_wait+0x4d0
           1     14.26 us     14.26 us     14.26 us     rwlock:W   ep_done_scan+0x2d
           2     14.00 us      7.99 us      7.00 us        mutex   pipe_read+0x282
           1     12.29 us     12.29 us     12.29 us     rwlock:R   ep_poll_callback+0x35
           1     12.02 us     12.02 us     12.02 us     rwlock:W   do_epoll_ctl+0xb65
           1     10.25 us     10.25 us     10.25 us     rwlock:R   ep_poll_callback+0x35
           1      7.86 us      7.86 us      7.86 us        mutex   do_epoll_ctl+0x6c1
           1      5.04 us      5.04 us      5.04 us        mutex   do_epoll_ctl+0x3d4

Fixes: d783ea8f62c4 ("perf lock contention: Simplify parse_lock_type()")
Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 tools/perf/builtin-lock.c | 64 ++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 062e2b56a2ab..7e36bbe3cb80 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -1591,8 +1591,6 @@ static const struct {
 	{ LCB_F_PERCPU | LCB_F_WRITE,	"pcpu-sem:W",	"percpu-rwsem" },
 	{ LCB_F_MUTEX,			"mutex",	"mutex" },
 	{ LCB_F_MUTEX | LCB_F_SPIN,	"mutex",	"mutex" },
-	/* alias for get_type_flag() */
-	{ LCB_F_MUTEX | LCB_F_SPIN,	"mutex-spin",	"mutex" },
 };
 
 static const char *get_type_str(unsigned int flags)
@@ -1617,19 +1615,6 @@ static const char *get_type_name(unsigned int flags)
 	return "unknown";
 }
 
-static unsigned int get_type_flag(const char *str)
-{
-	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
-		if (!strcmp(lock_type_table[i].name, str))
-			return lock_type_table[i].flags;
-	}
-	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
-		if (!strcmp(lock_type_table[i].str, str))
-			return lock_type_table[i].flags;
-	}
-	return UINT_MAX;
-}
-
 static void lock_filter_finish(void)
 {
 	zfree(&filters.types);
@@ -2350,29 +2335,58 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
 			   int unset __maybe_unused)
 {
 	char *s, *tmp, *tok;
-	int ret = 0;
 
 	s = strdup(str);
 	if (s == NULL)
 		return -1;
 
 	for (tok = strtok_r(s, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
-		unsigned int flags = get_type_flag(tok);
+		bool found = false;
 
-		if (flags == -1U) {
-			pr_err("Unknown lock flags: %s\n", tok);
-			ret = -1;
-			break;
+		/* `tok` is `str` in `lock_type_table` if it contains ':'. */
+		if (strchr(tok, ':')) {
+			for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
+				if (!strcmp(lock_type_table[i].str, tok) &&
+				    add_lock_type(lock_type_table[i].flags)) {
+					found = true;
+					break;
+				}
+			}
+
+			if (!found) {
+				pr_err("Unknown lock flags name: %s\n", tok);
+				free(s);
+				return -1;
+			}
+
+			continue;
 		}
 
-		if (!add_lock_type(flags)) {
-			ret = -1;
-			break;
+		/*
+		 * Otherwise `tok` is `name` in `lock_type_table`.
+		 * Single lock name could contain multiple flags.
+		 */
+		for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
+			if (!strcmp(lock_type_table[i].name, tok)) {
+				if (add_lock_type(lock_type_table[i].flags)) {
+					found = true;
+				} else {
+					free(s);
+					return -1;
+				}
+			}
 		}
+
+		if (!found) {
+			pr_err("Unknown lock name: %s\n", tok);
+			free(s);
+			return -1;
+		}
+
 	}
 
 	free(s);
-	return ret;
+	return 0;
 }
 
 static bool add_lock_addr(unsigned long addr)
-- 
2.47.1.545.g3c1d2e2a6a-goog

[PATCH v5 2/3] perf lock: Add percpu-rwsem for type filter

[Chun-Tse Shao]

percpu-rwsem was missing in man page. And for backward compatibility,
replace `pcpu-sem` with `percpu-rwsem` before parsing lock name.
Tested `./perf lock con -ab -Y pcpu-sem` and `./perf lock con -ab -Y
percpu-rwsem`

Fixes: 4f701063bfa2 ("perf lock contention: Show lock type with address")
Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 tools/perf/Documentation/perf-lock.txt | 4 ++--
 tools/perf/builtin-lock.c              | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
index 57a940399de0..d3793054f7d3 100644
--- a/tools/perf/Documentation/perf-lock.txt
+++ b/tools/perf/Documentation/perf-lock.txt
@@ -187,8 +187,8 @@ CONTENTION OPTIONS
 	Show lock contention only for given lock types (comma separated list).
 	Available values are:
 	  semaphore, spinlock, rwlock, rwlock:R, rwlock:W, rwsem, rwsem:R, rwsem:W,
-	  rtmutex, rwlock-rt, rwlock-rt:R, rwlock-rt:W, pcpu-sem, pcpu-sem:R, pcpu-sem:W,
-	  mutex
+	  rtmutex, rwlock-rt, rwlock-rt:R, rwlock-rt:W, percpu-rwmem, pcpu-sem,
+	  pcpu-sem:R, pcpu-sem:W, mutex
 
 	Note that RW-variant of locks have :R and :W suffix.  Names without the
 	suffix are shortcuts for the both variants.  Ex) rwsem = rwsem:R + rwsem:W.
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 7e36bbe3cb80..50630551adad 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -2365,7 +2365,10 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
 		/*
 		 * Otherwise `tok` is `name` in `lock_type_table`.
 		 * Single lock name could contain multiple flags.
+		 * Replace alias `pcpu-sem` with actual name `percpu-rwsem.
 		 */
+		if (!strcmp(tok, "pcpu-sem"))
+			tok = (char *)"percpu-rwsem";
 		for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
 			if (!strcmp(lock_type_table[i].name, tok)) {
 				if (add_lock_type(lock_type_table[i].flags)) {
-- 
2.47.1.545.g3c1d2e2a6a-goog

[PATCH v5 3/3] perf lock: Rename fields in lock_type_table

[Chun-Tse Shao]

`lock_type_table` contains `name` and `str` which can be confusing.
Rename them to `flags_name` and `lock_name` and add descriptions to
enhance understanding.
Tested by building perf for x86.

Signed-off-by: Chun-Tse Shao <ctshao@google.com>
---
 tools/perf/builtin-lock.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 50630551adad..74085c8e32d3 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -1575,8 +1575,13 @@ static void sort_result(void)
 
 static const struct {
 	unsigned int flags;
-	const char *str;
-	const char *name;
+	/*
+	 * Name of the lock flags (access), with delimeter ':'.
+	 * For example, rwsem:R of rwsem:W.
+	 */
+	const char *flags_name;
+	/* Name of the lock (type), for example, rwlock or rwsem. */
+	const char *lock_name;
 } lock_type_table[] = {
 	{ 0,				"semaphore",	"semaphore" },
 	{ LCB_F_SPIN,			"spinlock",	"spinlock" },
@@ -1593,24 +1598,24 @@ static const struct {
 	{ LCB_F_MUTEX | LCB_F_SPIN,	"mutex",	"mutex" },
 };
 
-static const char *get_type_str(unsigned int flags)
+static const char *get_type_flags_name(unsigned int flags)
 {
 	flags &= LCB_F_MAX_FLAGS - 1;
 
 	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
 		if (lock_type_table[i].flags == flags)
-			return lock_type_table[i].str;
+			return lock_type_table[i].flags_name;
 	}
 	return "unknown";
 }
 
-static const char *get_type_name(unsigned int flags)
+static const char *get_type_lock_name(unsigned int flags)
 {
 	flags &= LCB_F_MAX_FLAGS - 1;
 
 	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
 		if (lock_type_table[i].flags == flags)
-			return lock_type_table[i].name;
+			return lock_type_table[i].lock_name;
 	}
 	return "unknown";
 }
@@ -1717,7 +1722,7 @@ static void print_lock_stat_stdio(struct lock_contention *con, struct lock_stat
 
 	switch (aggr_mode) {
 	case LOCK_AGGR_CALLER:
-		fprintf(lock_output, "  %10s   %s\n", get_type_str(st->flags), st->name);
+		fprintf(lock_output, "  %10s   %s\n", get_type_flags_name(st->flags), st->name);
 		break;
 	case LOCK_AGGR_TASK:
 		pid = st->addr;
@@ -1727,7 +1732,7 @@ static void print_lock_stat_stdio(struct lock_contention *con, struct lock_stat
 		break;
 	case LOCK_AGGR_ADDR:
 		fprintf(lock_output, "  %016llx   %s (%s)\n", (unsigned long long)st->addr,
-			st->name, get_type_name(st->flags));
+			st->name, get_type_lock_name(st->flags));
 		break;
 	case LOCK_AGGR_CGROUP:
 		fprintf(lock_output, "  %s\n", st->name);
@@ -1768,7 +1773,7 @@ static void print_lock_stat_csv(struct lock_contention *con, struct lock_stat *s
 
 	switch (aggr_mode) {
 	case LOCK_AGGR_CALLER:
-		fprintf(lock_output, "%s%s %s", get_type_str(st->flags), sep, st->name);
+		fprintf(lock_output, "%s%s %s", get_type_flags_name(st->flags), sep, st->name);
 		if (verbose <= 0)
 			fprintf(lock_output, "\n");
 		break;
@@ -1780,7 +1785,7 @@ static void print_lock_stat_csv(struct lock_contention *con, struct lock_stat *s
 		break;
 	case LOCK_AGGR_ADDR:
 		fprintf(lock_output, "%llx%s %s%s %s\n", (unsigned long long)st->addr, sep,
-			st->name, sep, get_type_name(st->flags));
+			st->name, sep, get_type_lock_name(st->flags));
 		break;
 	case LOCK_AGGR_CGROUP:
 		fprintf(lock_output, "%s\n",st->name);
@@ -2343,10 +2348,10 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
 	for (tok = strtok_r(s, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
 		bool found = false;
 
-		/* `tok` is `str` in `lock_type_table` if it contains ':'. */
+		/* `tok` is a flags name if it contains ':'. */
 		if (strchr(tok, ':')) {
 			for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
-				if (!strcmp(lock_type_table[i].str, tok) &&
+				if (!strcmp(lock_type_table[i].flags_name, tok) &&
 				    add_lock_type(lock_type_table[i].flags)) {
 					found = true;
 					break;
@@ -2363,14 +2368,14 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
 		}
 
 		/*
-		 * Otherwise `tok` is `name` in `lock_type_table`.
+		 * Otherwise `tok` is a lock name.
 		 * Single lock name could contain multiple flags.
 		 * Replace alias `pcpu-sem` with actual name `percpu-rwsem.
 		 */
 		if (!strcmp(tok, "pcpu-sem"))
 			tok = (char *)"percpu-rwsem";
 		for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
-			if (!strcmp(lock_type_table[i].name, tok)) {
+			if (!strcmp(lock_type_table[i].lock_name, tok)) {
 				if (add_lock_type(lock_type_table[i].flags)) {
 					found = true;
 				} else {
-- 
2.47.1.545.g3c1d2e2a6a-goog

Re: [PATCH v5 1/3] perf lock: Fix parse_lock_type which only retrieve one lock flag

[Namhyung Kim]

On Tue, Dec 10, 2024 at 12:08:20PM -0800, Chun-Tse Shao wrote:
> `parse_lock_type` can only add the first lock flag in `lock_type_table`
> given input `str`. For example, for `Y rwlock`, it only adds `rwlock:R`
> into this perf session. Another example is for `-Y mutex`, it only adds
> the mutex without `LCB_F_SPIN` flag. The patch fixes this issue, makes
> sure both `rwlock:R` and `rwlock:W` will be added with `-Y rwlock`, and
> so on.
> 
> Testing:
>   $ ./perf lock con -ab -Y mutex,rwlock -- perf bench sched pipe
>   # Running 'sched/pipe' benchmark:
>   # Executed 1000000 pipe operations between two processes
> 
>        Total time: 9.313 [sec]
> 
>          9.313976 usecs/op
>            107365 ops/sec
>    contended   total wait     max wait     avg wait         type   caller
> 
>          176      1.65 ms     19.43 us      9.38 us        mutex   pipe_read+0x57
>           34    180.14 us     10.93 us      5.30 us        mutex   pipe_write+0x50
>            7     77.48 us     16.09 us     11.07 us        mutex   do_epoll_wait+0x24d
>            7     74.70 us     13.50 us     10.67 us        mutex   do_epoll_wait+0x24d
>            3     35.97 us     14.44 us     11.99 us     rwlock:W   ep_done_scan+0x2d
>            3     35.00 us     12.23 us     11.66 us     rwlock:W   do_epoll_wait+0x255
>            2     15.88 us     11.96 us      7.94 us     rwlock:W   do_epoll_wait+0x47c
>            1     15.23 us     15.23 us     15.23 us     rwlock:W   do_epoll_wait+0x4d0
>            1     14.26 us     14.26 us     14.26 us     rwlock:W   ep_done_scan+0x2d
>            2     14.00 us      7.99 us      7.00 us        mutex   pipe_read+0x282
>            1     12.29 us     12.29 us     12.29 us     rwlock:R   ep_poll_callback+0x35
>            1     12.02 us     12.02 us     12.02 us     rwlock:W   do_epoll_ctl+0xb65
>            1     10.25 us     10.25 us     10.25 us     rwlock:R   ep_poll_callback+0x35
>            1      7.86 us      7.86 us      7.86 us        mutex   do_epoll_ctl+0x6c1
>            1      5.04 us      5.04 us      5.04 us        mutex   do_epoll_ctl+0x3d4
> 
> Fixes: d783ea8f62c4 ("perf lock contention: Simplify parse_lock_type()")
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
>  tools/perf/builtin-lock.c | 64 ++++++++++++++++++++++++---------------
>  1 file changed, 39 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 062e2b56a2ab..7e36bbe3cb80 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -1591,8 +1591,6 @@ static const struct {
>  	{ LCB_F_PERCPU | LCB_F_WRITE,	"pcpu-sem:W",	"percpu-rwsem" },
>  	{ LCB_F_MUTEX,			"mutex",	"mutex" },
>  	{ LCB_F_MUTEX | LCB_F_SPIN,	"mutex",	"mutex" },
> -	/* alias for get_type_flag() */
> -	{ LCB_F_MUTEX | LCB_F_SPIN,	"mutex-spin",	"mutex" },
>  };
>  
>  static const char *get_type_str(unsigned int flags)
> @@ -1617,19 +1615,6 @@ static const char *get_type_name(unsigned int flags)
>  	return "unknown";
>  }
>  
> -static unsigned int get_type_flag(const char *str)
> -{
> -	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
> -		if (!strcmp(lock_type_table[i].name, str))
> -			return lock_type_table[i].flags;
> -	}
> -	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
> -		if (!strcmp(lock_type_table[i].str, str))
> -			return lock_type_table[i].flags;
> -	}
> -	return UINT_MAX;
> -}
> -
>  static void lock_filter_finish(void)
>  {
>  	zfree(&filters.types);
> @@ -2350,29 +2335,58 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
>  			   int unset __maybe_unused)
>  {
>  	char *s, *tmp, *tok;
> -	int ret = 0;
>  
>  	s = strdup(str);
>  	if (s == NULL)
>  		return -1;
>  
>  	for (tok = strtok_r(s, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
> -		unsigned int flags = get_type_flag(tok);
> +		bool found = false;
>  
> -		if (flags == -1U) {
> -			pr_err("Unknown lock flags: %s\n", tok);
> -			ret = -1;
> -			break;
> +		/* `tok` is `str` in `lock_type_table` if it contains ':'. */
> +		if (strchr(tok, ':')) {
> +			for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
> +				if (!strcmp(lock_type_table[i].str, tok) &&
> +				    add_lock_type(lock_type_table[i].flags)) {
> +					found = true;
> +					break;
> +				}
> +			}
> +
> +			if (!found) {
> +				pr_err("Unknown lock flags name: %s\n", tok);
> +				free(s);
> +				return -1;
> +			}
> +
> +			continue;
>  		}
>  
> -		if (!add_lock_type(flags)) {
> -			ret = -1;
> -			break;
> +		/*
> +		 * Otherwise `tok` is `name` in `lock_type_table`.
> +		 * Single lock name could contain multiple flags.
> +		 */
> +		for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
> +			if (!strcmp(lock_type_table[i].name, tok)) {
> +				if (add_lock_type(lock_type_table[i].flags)) {
> +					found = true;
> +				} else {
> +					free(s);
> +					return -1;
> +				}
> +			}
>  		}
> +
> +		if (!found) {
> +			pr_err("Unknown lock name: %s\n", tok);
> +			free(s);
> +			return -1;
> +		}
> +
>  	}
>  
>  	free(s);
> -	return ret;
> +	return 0;
>  }
>  
>  static bool add_lock_addr(unsigned long addr)
> -- 
> 2.47.1.545.g3c1d2e2a6a-goog
> 

Re: [PATCH v5 2/3] perf lock: Add percpu-rwsem for type filter

[Namhyung Kim]

On Tue, Dec 10, 2024 at 12:08:21PM -0800, Chun-Tse Shao wrote:
> percpu-rwsem was missing in man page. And for backward compatibility,
> replace `pcpu-sem` with `percpu-rwsem` before parsing lock name.
> Tested `./perf lock con -ab -Y pcpu-sem` and `./perf lock con -ab -Y
> percpu-rwsem`
> 
> Fixes: 4f701063bfa2 ("perf lock contention: Show lock type with address")
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
>  tools/perf/Documentation/perf-lock.txt | 4 ++--
>  tools/perf/builtin-lock.c              | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index 57a940399de0..d3793054f7d3 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -187,8 +187,8 @@ CONTENTION OPTIONS
>  	Show lock contention only for given lock types (comma separated list).
>  	Available values are:
>  	  semaphore, spinlock, rwlock, rwlock:R, rwlock:W, rwsem, rwsem:R, rwsem:W,
> -	  rtmutex, rwlock-rt, rwlock-rt:R, rwlock-rt:W, pcpu-sem, pcpu-sem:R, pcpu-sem:W,
> -	  mutex
> +	  rtmutex, rwlock-rt, rwlock-rt:R, rwlock-rt:W, percpu-rwmem, pcpu-sem,
> +	  pcpu-sem:R, pcpu-sem:W, mutex
>  
>  	Note that RW-variant of locks have :R and :W suffix.  Names without the
>  	suffix are shortcuts for the both variants.  Ex) rwsem = rwsem:R + rwsem:W.
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 7e36bbe3cb80..50630551adad 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -2365,7 +2365,10 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
>  		/*
>  		 * Otherwise `tok` is `name` in `lock_type_table`.
>  		 * Single lock name could contain multiple flags.
> +		 * Replace alias `pcpu-sem` with actual name `percpu-rwsem.
>  		 */
> +		if (!strcmp(tok, "pcpu-sem"))
> +			tok = (char *)"percpu-rwsem";
>  		for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
>  			if (!strcmp(lock_type_table[i].name, tok)) {
>  				if (add_lock_type(lock_type_table[i].flags)) {
> -- 
> 2.47.1.545.g3c1d2e2a6a-goog
> 

Re: [PATCH v5 3/3] perf lock: Rename fields in lock_type_table

[Namhyung Kim]

On Tue, Dec 10, 2024 at 12:08:22PM -0800, Chun-Tse Shao wrote:
> `lock_type_table` contains `name` and `str` which can be confusing.
> Rename them to `flags_name` and `lock_name` and add descriptions to
> enhance understanding.
> Tested by building perf for x86.
> 
> Signed-off-by: Chun-Tse Shao <ctshao@google.com>

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
>  tools/perf/builtin-lock.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 50630551adad..74085c8e32d3 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -1575,8 +1575,13 @@ static void sort_result(void)
>  
>  static const struct {
>  	unsigned int flags;
> -	const char *str;
> -	const char *name;
> +	/*
> +	 * Name of the lock flags (access), with delimeter ':'.
> +	 * For example, rwsem:R of rwsem:W.
> +	 */
> +	const char *flags_name;
> +	/* Name of the lock (type), for example, rwlock or rwsem. */
> +	const char *lock_name;
>  } lock_type_table[] = {
>  	{ 0,				"semaphore",	"semaphore" },
>  	{ LCB_F_SPIN,			"spinlock",	"spinlock" },
> @@ -1593,24 +1598,24 @@ static const struct {
>  	{ LCB_F_MUTEX | LCB_F_SPIN,	"mutex",	"mutex" },
>  };
>  
> -static const char *get_type_str(unsigned int flags)
> +static const char *get_type_flags_name(unsigned int flags)
>  {
>  	flags &= LCB_F_MAX_FLAGS - 1;
>  
>  	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
>  		if (lock_type_table[i].flags == flags)
> -			return lock_type_table[i].str;
> +			return lock_type_table[i].flags_name;
>  	}
>  	return "unknown";
>  }
>  
> -static const char *get_type_name(unsigned int flags)
> +static const char *get_type_lock_name(unsigned int flags)
>  {
>  	flags &= LCB_F_MAX_FLAGS - 1;
>  
>  	for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
>  		if (lock_type_table[i].flags == flags)
> -			return lock_type_table[i].name;
> +			return lock_type_table[i].lock_name;
>  	}
>  	return "unknown";
>  }
> @@ -1717,7 +1722,7 @@ static void print_lock_stat_stdio(struct lock_contention *con, struct lock_stat
>  
>  	switch (aggr_mode) {
>  	case LOCK_AGGR_CALLER:
> -		fprintf(lock_output, "  %10s   %s\n", get_type_str(st->flags), st->name);
> +		fprintf(lock_output, "  %10s   %s\n", get_type_flags_name(st->flags), st->name);
>  		break;
>  	case LOCK_AGGR_TASK:
>  		pid = st->addr;
> @@ -1727,7 +1732,7 @@ static void print_lock_stat_stdio(struct lock_contention *con, struct lock_stat
>  		break;
>  	case LOCK_AGGR_ADDR:
>  		fprintf(lock_output, "  %016llx   %s (%s)\n", (unsigned long long)st->addr,
> -			st->name, get_type_name(st->flags));
> +			st->name, get_type_lock_name(st->flags));
>  		break;
>  	case LOCK_AGGR_CGROUP:
>  		fprintf(lock_output, "  %s\n", st->name);
> @@ -1768,7 +1773,7 @@ static void print_lock_stat_csv(struct lock_contention *con, struct lock_stat *s
>  
>  	switch (aggr_mode) {
>  	case LOCK_AGGR_CALLER:
> -		fprintf(lock_output, "%s%s %s", get_type_str(st->flags), sep, st->name);
> +		fprintf(lock_output, "%s%s %s", get_type_flags_name(st->flags), sep, st->name);
>  		if (verbose <= 0)
>  			fprintf(lock_output, "\n");
>  		break;
> @@ -1780,7 +1785,7 @@ static void print_lock_stat_csv(struct lock_contention *con, struct lock_stat *s
>  		break;
>  	case LOCK_AGGR_ADDR:
>  		fprintf(lock_output, "%llx%s %s%s %s\n", (unsigned long long)st->addr, sep,
> -			st->name, sep, get_type_name(st->flags));
> +			st->name, sep, get_type_lock_name(st->flags));
>  		break;
>  	case LOCK_AGGR_CGROUP:
>  		fprintf(lock_output, "%s\n",st->name);
> @@ -2343,10 +2348,10 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
>  	for (tok = strtok_r(s, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
>  		bool found = false;
>  
> -		/* `tok` is `str` in `lock_type_table` if it contains ':'. */
> +		/* `tok` is a flags name if it contains ':'. */
>  		if (strchr(tok, ':')) {
>  			for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
> -				if (!strcmp(lock_type_table[i].str, tok) &&
> +				if (!strcmp(lock_type_table[i].flags_name, tok) &&
>  				    add_lock_type(lock_type_table[i].flags)) {
>  					found = true;
>  					break;
> @@ -2363,14 +2368,14 @@ static int parse_lock_type(const struct option *opt __maybe_unused, const char *
>  		}
>  
>  		/*
> -		 * Otherwise `tok` is `name` in `lock_type_table`.
> +		 * Otherwise `tok` is a lock name.
>  		 * Single lock name could contain multiple flags.
>  		 * Replace alias `pcpu-sem` with actual name `percpu-rwsem.
>  		 */
>  		if (!strcmp(tok, "pcpu-sem"))
>  			tok = (char *)"percpu-rwsem";
>  		for (unsigned int i = 0; i < ARRAY_SIZE(lock_type_table); i++) {
> -			if (!strcmp(lock_type_table[i].name, tok)) {
> +			if (!strcmp(lock_type_table[i].lock_name, tok)) {
>  				if (add_lock_type(lock_type_table[i].flags)) {
>  					found = true;
>  				} else {
> -- 
> 2.47.1.545.g3c1d2e2a6a-goog
> 

Re: [PATCH v5 2/3] perf lock: Add percpu-rwsem for type filter

[Arnaldo Carvalho de Melo]

On Wed, Dec 11, 2024 at 11:15:34AM -0800, Namhyung Kim wrote:
> On Tue, Dec 10, 2024 at 12:08:21PM -0800, Chun-Tse Shao wrote:
> > percpu-rwsem was missing in man page. And for backward compatibility,
> > replace `pcpu-sem` with `percpu-rwsem` before parsing lock name.
> > Tested `./perf lock con -ab -Y pcpu-sem` and `./perf lock con -ab -Y
> > percpu-rwsem`

> > Fixes: 4f701063bfa2 ("perf lock contention: Show lock type with address")
> > Signed-off-by: Chun-Tse Shao <ctshao@google.com>

> Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Here the reviewer can also add info about where this should go, i.e.
this is a fix, has a Fixes tag, but then there is extra work for
maintainers to do: Is this a regression introduced in this merge window?
Should this go to urgent or next? If the submitter adds this, it helps,
if the reviewer agrees, even better, otherwise emit an opinion where it
should go.

In this specific case:

⬢ [acme@toolbox perf-tools-next]$ git tag --contains 4f701063bfa2 | grep ^v[56] | grep -v -- -rc
v6.10
v6.11
v6.12
v6.4
v6.5
v6.6
v6.7
v6.8
v6.9
⬢ [acme@toolbox perf-tools-next]$

Looks something its there for quite a while, so probably can go to
perf-tools-next?

- Arnaldo

Re: [PATCH v5 2/3] perf lock: Add percpu-rwsem for type filter

[Namhyung Kim]

On Thu, Dec 12, 2024 at 04:00:40PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Dec 11, 2024 at 11:15:34AM -0800, Namhyung Kim wrote:
> > On Tue, Dec 10, 2024 at 12:08:21PM -0800, Chun-Tse Shao wrote:
> > > percpu-rwsem was missing in man page. And for backward compatibility,
> > > replace `pcpu-sem` with `percpu-rwsem` before parsing lock name.
> > > Tested `./perf lock con -ab -Y pcpu-sem` and `./perf lock con -ab -Y
> > > percpu-rwsem`
> 
> > > Fixes: 4f701063bfa2 ("perf lock contention: Show lock type with address")
> > > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
> 
> > Reviewed-by: Namhyung Kim <namhyung@kernel.org>
> 
> Here the reviewer can also add info about where this should go, i.e.
> this is a fix, has a Fixes tag, but then there is extra work for
> maintainers to do: Is this a regression introduced in this merge window?
> Should this go to urgent or next? If the submitter adds this, it helps,
> if the reviewer agrees, even better, otherwise emit an opinion where it
> should go.
> 
> In this specific case:
> 
> ⬢ [acme@toolbox perf-tools-next]$ git tag --contains 4f701063bfa2 | grep ^v[56] | grep -v -- -rc
> v6.10
> v6.11
> v6.12
> v6.4
> v6.5
> v6.6
> v6.7
> v6.8
> v6.9
> ⬢ [acme@toolbox perf-tools-next]$
> 
> Looks something its there for quite a while, so probably can go to
> perf-tools-next?

Yeah, I think it should be ok to go to perf-tools-next.

Thanks,
Namhyung

Re: [PATCH v5 3/3] perf lock: Rename fields in lock_type_table

[Namhyung Kim]

Hello,

On Wed, Dec 11, 2024 at 11:20 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Dec 10, 2024 at 12:08:22PM -0800, Chun-Tse Shao wrote:
> > `lock_type_table` contains `name` and `str` which can be confusing.
> > Rename them to `flags_name` and `lock_name` and add descriptions to
> > enhance understanding.
> > Tested by building perf for x86.
> >
> > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
>
> Reviewed-by: Namhyung Kim <namhyung@kernel.org>

I'm trying to process this patchset but it doesn't apply to perf-tools-next
anymore.  Can you please rebase?

Thanks,
Namhyung

Re: [PATCH v5 1/3] perf lock: Fix parse_lock_type which only retrieve one lock flag

[Namhyung Kim]

On Wed, Dec 11, 2024 at 11:17 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Dec 10, 2024 at 12:08:20PM -0800, Chun-Tse Shao wrote:
> > `parse_lock_type` can only add the first lock flag in `lock_type_table`
> > given input `str`. For example, for `Y rwlock`, it only adds `rwlock:R`
> > into this perf session. Another example is for `-Y mutex`, it only adds
> > the mutex without `LCB_F_SPIN` flag. The patch fixes this issue, makes
> > sure both `rwlock:R` and `rwlock:W` will be added with `-Y rwlock`, and
> > so on.
> >
> > Testing:
> >   $ ./perf lock con -ab -Y mutex,rwlock -- perf bench sched pipe
> >   # Running 'sched/pipe' benchmark:
> >   # Executed 1000000 pipe operations between two processes
> >
> >        Total time: 9.313 [sec]
> >
> >          9.313976 usecs/op
> >            107365 ops/sec
> >    contended   total wait     max wait     avg wait         type   caller
> >
> >          176      1.65 ms     19.43 us      9.38 us        mutex   pipe_read+0x57
> >           34    180.14 us     10.93 us      5.30 us        mutex   pipe_write+0x50
> >            7     77.48 us     16.09 us     11.07 us        mutex   do_epoll_wait+0x24d
> >            7     74.70 us     13.50 us     10.67 us        mutex   do_epoll_wait+0x24d
> >            3     35.97 us     14.44 us     11.99 us     rwlock:W   ep_done_scan+0x2d
> >            3     35.00 us     12.23 us     11.66 us     rwlock:W   do_epoll_wait+0x255
> >            2     15.88 us     11.96 us      7.94 us     rwlock:W   do_epoll_wait+0x47c
> >            1     15.23 us     15.23 us     15.23 us     rwlock:W   do_epoll_wait+0x4d0
> >            1     14.26 us     14.26 us     14.26 us     rwlock:W   ep_done_scan+0x2d
> >            2     14.00 us      7.99 us      7.00 us        mutex   pipe_read+0x282
> >            1     12.29 us     12.29 us     12.29 us     rwlock:R   ep_poll_callback+0x35
> >            1     12.02 us     12.02 us     12.02 us     rwlock:W   do_epoll_ctl+0xb65
> >            1     10.25 us     10.25 us     10.25 us     rwlock:R   ep_poll_callback+0x35
> >            1      7.86 us      7.86 us      7.86 us        mutex   do_epoll_ctl+0x6c1
> >            1      5.04 us      5.04 us      5.04 us        mutex   do_epoll_ctl+0x3d4
> >
> > Fixes: d783ea8f62c4 ("perf lock contention: Simplify parse_lock_type()")
> > Signed-off-by: Chun-Tse Shao <ctshao@google.com>
>
> Reviewed-by: Namhyung Kim <namhyung@kernel.org>
>
> Thanks,
> Namhyung
>
> > ---
> >  tools/perf/builtin-lock.c | 64 ++++++++++++++++++++++++---------------
> >  1 file changed, 39 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index 062e2b56a2ab..7e36bbe3cb80 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -1591,8 +1591,6 @@ static const struct {
> >       { LCB_F_PERCPU | LCB_F_WRITE,   "pcpu-sem:W",   "percpu-rwsem" },
> >       { LCB_F_MUTEX,                  "mutex",        "mutex" },
> >       { LCB_F_MUTEX | LCB_F_SPIN,     "mutex",        "mutex" },
> > -     /* alias for get_type_flag() */
> > -     { LCB_F_MUTEX | LCB_F_SPIN,     "mutex-spin",   "mutex" },

Hmm.. now I think we need this part to keep it compatible.
Let's have "mutex-spin" in the 'name' as well.

Thanks,
Namhyung