[PATCH 1/3] perf lock: Add --synth=no option for record

[PATCH 1/3] perf lock: Add --synth=no option for record

[Namhyung Kim]

The perf lock command has nothing to symbolize and lock names come
from the tracepoint.  Moreover, kernel symbols are available even the
--synth=no option is given.

This will reduce the startup time by avoiding unnecessary synthesis.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-lock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 57b9ebd7118a..1ebff88bc5ba 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -990,7 +990,7 @@ static int __cmd_report(bool display_info)
 static int __cmd_record(int argc, const char **argv)
 {
 	const char *record_args[] = {
-		"record", "-R", "-m", "1024", "-c", "1",
+		"record", "-R", "-m", "1024", "-c", "1", "--synth", "no",
 	};
 	unsigned int rec_argc, i, j, ret;
 	const char **rec_argv;
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 2/3] perf lock: Extend struct lock_key to have print function

[Namhyung Kim]

And use it to print output for each key field.  No functional change
intended and the output should be identical.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-lock.c | 91 ++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 26 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 1ebff88bc5ba..c2ecb6acb87d 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -237,9 +237,43 @@ struct lock_key {
 	 * e.g. nr_acquired -> acquired, wait_time_total -> wait_total
 	 */
 	const char		*name;
+	/* header: the string printed on the header line */
+	const char		*header;
+	/* len: the printing width of the field */
+	int			len;
+	/* key: a pointer to function to compare two lock stats for sorting */
 	int			(*key)(struct lock_stat*, struct lock_stat*);
+	/* print: a pointer to function to print a given lock stats */
+	void			(*print)(struct lock_key*, struct lock_stat*);
+	/* list: list entry to link this */
+	struct list_head	list;
 };
 
+#define PRINT_KEY(member)						\
+static void lock_stat_key_print_ ## member(struct lock_key *key,	\
+					   struct lock_stat *ls)	\
+{									\
+	pr_info("%*llu", key->len, (unsigned long long)ls->member);	\
+}
+
+PRINT_KEY(nr_acquired)
+PRINT_KEY(nr_contended)
+PRINT_KEY(avg_wait_time)
+PRINT_KEY(wait_time_total)
+PRINT_KEY(wait_time_max)
+
+static void lock_stat_key_print_wait_time_min(struct lock_key *key,
+					      struct lock_stat *ls)
+{
+	u64 wait_time = ls->wait_time_min;
+
+	if (wait_time == ULLONG_MAX)
+		wait_time = 0;
+
+	pr_info("%*"PRIu64, key->len, wait_time);
+}
+
+
 static const char		*sort_key = "acquired";
 
 static int			(*compare)(struct lock_stat *, struct lock_stat *);
@@ -247,19 +281,20 @@ static int			(*compare)(struct lock_stat *, struct lock_stat *);
 static struct rb_root		sorted; /* place to store intermediate data */
 static struct rb_root		result;	/* place to store sorted data */
 
-#define DEF_KEY_LOCK(name, fn_suffix)	\
-	{ #name, lock_stat_key_ ## fn_suffix }
+static LIST_HEAD(lock_keys);
+
+#define DEF_KEY_LOCK(name, header, fn_suffix, len)			\
+	{ #name, header, len, lock_stat_key_ ## fn_suffix, lock_stat_key_print_ ## fn_suffix, {} }
 struct lock_key keys[] = {
-	DEF_KEY_LOCK(acquired, nr_acquired),
-	DEF_KEY_LOCK(contended, nr_contended),
-	DEF_KEY_LOCK(avg_wait, avg_wait_time),
-	DEF_KEY_LOCK(wait_total, wait_time_total),
-	DEF_KEY_LOCK(wait_min, wait_time_min),
-	DEF_KEY_LOCK(wait_max, wait_time_max),
+	DEF_KEY_LOCK(acquired, "acquired", nr_acquired, 10),
+	DEF_KEY_LOCK(contended, "contended", nr_contended, 10),
+	DEF_KEY_LOCK(avg_wait, "avg wait (ns)", avg_wait_time, 15),
+	DEF_KEY_LOCK(wait_total, "total wait (ns)", wait_time_total, 15),
+	DEF_KEY_LOCK(wait_max, "max wait (ns)", wait_time_max, 15),
+	DEF_KEY_LOCK(wait_min, "min wait (ns)", wait_time_min, 15),
 
 	/* extra comparisons much complicated should be here */
-
-	{ NULL, NULL }
+	{ }
 };
 
 static int select_key(void)
@@ -278,6 +313,16 @@ static int select_key(void)
 	return -1;
 }
 
+static int setup_output_field(void)
+{
+	int i;
+
+	for (i = 0; keys[i].name; i++)
+		list_add_tail(&keys[i].list, &lock_keys);
+
+	return 0;
+}
+
 static void combine_lock_stats(struct lock_stat *st)
 {
 	struct rb_node **rb = &sorted.rb_node;
@@ -753,18 +798,13 @@ static void print_bad_events(int bad, int total)
 static void print_result(void)
 {
 	struct lock_stat *st;
+	struct lock_key *key;
 	char cut_name[20];
 	int bad, total;
 
 	pr_info("%20s ", "Name");
-	pr_info("%10s ", "acquired");
-	pr_info("%10s ", "contended");
-
-	pr_info("%15s ", "avg wait (ns)");
-	pr_info("%15s ", "total wait (ns)");
-	pr_info("%15s ", "max wait (ns)");
-	pr_info("%15s ", "min wait (ns)");
-
+	list_for_each_entry(key, &lock_keys, list)
+		pr_info("%*s ", key->len, key->header);
 	pr_info("\n\n");
 
 	bad = total = 0;
@@ -789,14 +829,10 @@ static void print_result(void)
 			pr_info("%20s ", cut_name);
 		}
 
-		pr_info("%10u ", st->nr_acquired);
-		pr_info("%10u ", st->nr_contended);
-
-		pr_info("%15" PRIu64 " ", st->avg_wait_time);
-		pr_info("%15" PRIu64 " ", st->wait_time_total);
-		pr_info("%15" PRIu64 " ", st->wait_time_max);
-		pr_info("%15" PRIu64 " ", st->wait_time_min == ULLONG_MAX ?
-		       0 : st->wait_time_min);
+		list_for_each_entry(key, &lock_keys, list) {
+			key->print(key, st);
+			pr_info(" ");
+		}
 		pr_info("\n");
 	}
 
@@ -966,6 +1002,9 @@ static int __cmd_report(bool display_info)
 		goto out_delete;
 	}
 
+	if (setup_output_field())
+		goto out_delete;
+
 	if (select_key())
 		goto out_delete;
 
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 3/3] perf lock: Add -F/--field option to control output

[Namhyung Kim]

The -F/--field option is to customize output field.

  $ perf lock report -F contended,wait_max -k avg_wait
                  Name  contended   max wait (ns)   avg wait (ns)

        slock-AF_INET6          1           23543           23543
     &lruvec->lru_lock          5           18317           11254
        slock-AF_INET6          1           10379           10379
            rcu_node_1          1            2104            2104
   &dentry->d_lockr...          1            1844            1844
   &dentry->d_lockr...          1            1672            1672
      &newf->file_lock         15            2279            1025
   &dentry->d_lockr...          1             792             792

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-lock.txt |  6 +++
 tools/perf/builtin-lock.c              | 55 +++++++++++++++++++++++---
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
index f5eb95788969..b43222229807 100644
--- a/tools/perf/Documentation/perf-lock.txt
+++ b/tools/perf/Documentation/perf-lock.txt
@@ -54,6 +54,12 @@ REPORT OPTIONS
         Sorting key. Possible values: acquired (default), contended,
 	avg_wait, wait_total, wait_max, wait_min.
 
+-F::
+--field=<value>::
+        Output fields. By default it shows all the fields but users can
+	customize that using this.  Possible values: acquired, contended,
+	avg_wait, wait_total, wait_max, wait_min.
+
 -c::
 --combine-locks::
 	Merge lock instances in the same class (based on name).
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index c2ecb6acb87d..a7089760a7de 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -282,6 +282,7 @@ static struct rb_root		sorted; /* place to store intermediate data */
 static struct rb_root		result;	/* place to store sorted data */
 
 static LIST_HEAD(lock_keys);
+static const char		*output_fields;
 
 #define DEF_KEY_LOCK(name, header, fn_suffix, len)			\
 	{ #name, header, len, lock_stat_key_ ## fn_suffix, lock_stat_key_print_ ## fn_suffix, {} }
@@ -304,23 +305,65 @@ static int select_key(void)
 	for (i = 0; keys[i].name; i++) {
 		if (!strcmp(keys[i].name, sort_key)) {
 			compare = keys[i].key;
+
+			/* selected key should be in the output fields */
+			if (list_empty(&keys[i].list))
+				list_add_tail(&keys[i].list, &lock_keys);
+
 			return 0;
 		}
 	}
 
 	pr_err("Unknown compare key: %s\n", sort_key);
-
 	return -1;
 }
 
-static int setup_output_field(void)
+static int add_output_field(struct list_head *head, char *name)
 {
 	int i;
 
+	for (i = 0; keys[i].name; i++) {
+		if (strcmp(keys[i].name, name))
+			continue;
+
+		/* prevent double link */
+		if (list_empty(&keys[i].list))
+			list_add_tail(&keys[i].list, head);
+
+		return 0;
+	}
+
+	pr_err("Unknown output field: %s\n", name);
+	return -1;
+}
+
+static int setup_output_field(const char *str)
+{
+	char *tok, *tmp, *orig;
+	int i, ret = 0;
+
+	/* no output field given: use all of them */
+	if (str == NULL) {
+		for (i = 0; keys[i].name; i++)
+			list_add_tail(&keys[i].list, &lock_keys);
+		return 0;
+	}
+
 	for (i = 0; keys[i].name; i++)
-		list_add_tail(&keys[i].list, &lock_keys);
+		INIT_LIST_HEAD(&keys[i].list);
 
-	return 0;
+	orig = tmp = strdup(str);
+	if (orig == NULL)
+		return -ENOMEM;
+
+	while ((tok = strsep(&tmp, ",")) != NULL){
+		ret = add_output_field(&lock_keys, tok);
+		if (ret < 0)
+			break;
+	}
+	free(orig);
+
+	return ret;
 }
 
 static void combine_lock_stats(struct lock_stat *st)
@@ -1002,7 +1045,7 @@ static int __cmd_report(bool display_info)
 		goto out_delete;
 	}
 
-	if (setup_output_field())
+	if (setup_output_field(output_fields))
 		goto out_delete;
 
 	if (select_key())
@@ -1090,6 +1133,8 @@ int cmd_lock(int argc, const char **argv)
 	const struct option report_options[] = {
 	OPT_STRING('k', "key", &sort_key, "acquired",
 		    "key for sorting (acquired / contended / avg_wait / wait_total / wait_max / wait_min)"),
+	OPT_STRING('F', "field", &output_fields, NULL,
+		    "output fields (acquired / contended / avg_wait / wait_total / wait_max / wait_min)"),
 	/* TODO: type */
 	OPT_BOOLEAN('c', "combine-locks", &combine_locks,
 		    "combine locks in the same class"),
-- 
2.35.1.894.gb6a874cedc-goog

Re: [PATCH 1/3] perf lock: Add --synth=no option for record

[Arnaldo Carvalho de Melo]

Em Wed, Mar 23, 2022 at 04:02:57PM -0700, Namhyung Kim escreveu:
> The perf lock command has nothing to symbolize and lock names come
> from the tracepoint.  Moreover, kernel symbols are available even the
> --synth=no option is given.
> 
> This will reduce the startup time by avoiding unnecessary synthesis.

Thanks, applied the series.

- Arnaldo

 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-lock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 57b9ebd7118a..1ebff88bc5ba 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -990,7 +990,7 @@ static int __cmd_report(bool display_info)
>  static int __cmd_record(int argc, const char **argv)
>  {
>  	const char *record_args[] = {
> -		"record", "-R", "-m", "1024", "-c", "1",
> +		"record", "-R", "-m", "1024", "-c", "1", "--synth", "no",
>  	};
>  	unsigned int rec_argc, i, j, ret;
>  	const char **rec_argv;
> -- 
> 2.35.1.894.gb6a874cedc-goog

-- 

- Arnaldo