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

[Namhyung Kim <namhyung@kernel.org>]

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
>