[PATCH 0/2] Perf lock improvements

[PATCH 0/2] Perf lock improvements

[Nick Forrington]

Perf lock improvements, to re-introduce aggregation by caller to perf
lock report, and improve command line handling for perf lock info.

These changes are technically independent, but submitted together to
ensure they apply cleanly.

Nick Forrington (2):
  perf lock report: Restore aggregation by caller by default
  perf lock info: Enforce exactly one of --map and --thread

 tools/perf/Documentation/perf-lock.txt |  4 +++
 tools/perf/builtin-lock.c              | 49 ++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 3 deletions(-)

-- 
2.42.0

[PATCH 1/2] perf lock report: Restore aggregation by caller by default

[Nick Forrington]

This change restores the previous default behaviour for "perf lock
report", making the current aggregate-by-address behaviour available via
the new "--lock-addr" command line parameter.

This makes the behaviour consistent with "perf lock contention" (which
also aggregates by caller by default, or by address when "--lock-addr"
is specified).

Commit 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")
introduced aggregation modes for "perf lock contention" and (potentially
inadvertently) changed the behaviour of "perf lock report" from
aggregate-by-caller to aggregate-by-address (making the prior behaviour
inaccessible).

Example aggregate-by-address output:

$ perf lock report -F acquired
                Name   acquired

         event_mutex         34
                             21
                              1

Example aggregate-by-caller output:

$ perf lock report -F acquired
                Name   acquired

 perf_trace_init+...         34
 lock_mm_and_find...         20
 inherit_event.co...          1
    do_madvise+0x1f8          1

Cc: stable@kernel.org
Fixes: 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")
Signed-off-by: Nick Forrington <nick.forrington@arm.com>
---
 tools/perf/Documentation/perf-lock.txt |  4 ++++
 tools/perf/builtin-lock.c              | 24 +++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
index 503abcba1438..349333acbbfc 100644
--- a/tools/perf/Documentation/perf-lock.txt
+++ b/tools/perf/Documentation/perf-lock.txt
@@ -80,6 +80,10 @@ REPORT OPTIONS
 --combine-locks::
 	Merge lock instances in the same class (based on name).
 
+-l::
+--lock-addr::
+	Show lock contention stat by address
+
 -t::
 --threads::
     The -t option is to show per-thread lock stat like below:
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index fa7419978353..3aa8ba5ad928 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -78,7 +78,7 @@ struct callstack_filter {
 
 static struct lock_filter filters;
 
-static enum lock_aggr_mode aggr_mode = LOCK_AGGR_ADDR;
+static enum lock_aggr_mode aggr_mode = LOCK_AGGR_CALLER;
 
 static bool needs_callstack(void)
 {
@@ -1983,8 +1983,8 @@ static int __cmd_report(bool display_info)
 	if (select_key(false))
 		goto out_delete;
 
-	if (show_thread_stats)
-		aggr_mode = LOCK_AGGR_TASK;
+	aggr_mode = show_thread_stats ? LOCK_AGGR_TASK :
+		show_lock_addrs ? LOCK_AGGR_ADDR : LOCK_AGGR_CALLER;
 
 	err = perf_session__process_events(session);
 	if (err)
@@ -2008,6 +2008,19 @@ static void sighandler(int sig __maybe_unused)
 {
 }
 
+static int check_lock_report_options(const struct option *options,
+				     const char * const *usage)
+{
+	if (show_thread_stats && show_lock_addrs) {
+		pr_err("Cannot use thread and addr mode together\n");
+		parse_options_usage(usage, options, "threads", 0);
+		parse_options_usage(NULL, options, "lock-addr", 0);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int check_lock_contention_options(const struct option *options,
 					 const char * const *usage)
 
@@ -2589,6 +2602,7 @@ int cmd_lock(int argc, const char **argv)
 	/* TODO: type */
 	OPT_BOOLEAN('c', "combine-locks", &combine_locks,
 		    "combine locks in the same class"),
+	OPT_BOOLEAN('l', "lock-addr", &show_lock_addrs, "show lock stats by address"),
 	OPT_BOOLEAN('t', "threads", &show_thread_stats,
 		    "show per-thread lock stats"),
 	OPT_INTEGER('E', "entries", &print_nr_entries, "display this many functions"),
@@ -2680,6 +2694,10 @@ int cmd_lock(int argc, const char **argv)
 			if (argc)
 				usage_with_options(report_usage, report_options);
 		}
+
+		if (check_lock_report_options(report_options, report_usage) < 0)
+			return -1;
+
 		rc = __cmd_report(false);
 	} else if (!strcmp(argv[0], "script")) {
 		/* Aliased to 'perf script' */
-- 
2.42.0

[PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread

[Nick Forrington]

Improve error reporting for command line arguments.

Display error/usage if neither --map or --thread are specified (rather
than a non user-friendly error "Unknown type of information").

Display error/usage if both --map and --thread are specified (rather
than ignoring "--map" and displaying only thread information).

Signed-off-by: Nick Forrington <nick.forrington@arm.com>
---
 tools/perf/builtin-lock.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 3aa8ba5ad928..cf29f648d291 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -2021,6 +2021,27 @@ static int check_lock_report_options(const struct option *options,
 	return 0;
 }
 
+static int check_lock_info_options(const struct option *options,
+				   const char * const *usage)
+{
+	if (!info_map && !info_threads) {
+		pr_err("Requires one of --map or --threads\n");
+		parse_options_usage(usage, options, "map", 0);
+		parse_options_usage(NULL, options, "threads", 0);
+		return -1;
+
+	}
+
+	if (info_map && info_threads) {
+		pr_err("Cannot show map and threads together\n");
+		parse_options_usage(usage, options, "map", 0);
+		parse_options_usage(NULL, options, "threads", 0);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int check_lock_contention_options(const struct option *options,
 					 const char * const *usage)
 
@@ -2709,6 +2730,10 @@ int cmd_lock(int argc, const char **argv)
 			if (argc)
 				usage_with_options(info_usage, info_options);
 		}
+
+		if (check_lock_info_options(info_options, info_usage) < 0)
+			return -1;
+
 		/* recycling report_lock_ops */
 		trace_handler = &report_lock_ops;
 		rc = __cmd_report(true);
-- 
2.42.0

Re: [PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread

[Arnaldo Carvalho de Melo]

Em Tue, Oct 31, 2023 at 12:05:25PM +0000, Nick Forrington escreveu:
> Improve error reporting for command line arguments.
> 
> Display error/usage if neither --map or --thread are specified (rather
> than a non user-friendly error "Unknown type of information").
> 
> Display error/usage if both --map and --thread are specified (rather
> than ignoring "--map" and displaying only thread information).

Shouldn't one of them be the default so that we type less for the most
common usage?

- Arnaldo
 
> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
> ---
>  tools/perf/builtin-lock.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 3aa8ba5ad928..cf29f648d291 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -2021,6 +2021,27 @@ static int check_lock_report_options(const struct option *options,
>  	return 0;
>  }
>  
> +static int check_lock_info_options(const struct option *options,
> +				   const char * const *usage)
> +{
> +	if (!info_map && !info_threads) {
> +		pr_err("Requires one of --map or --threads\n");
> +		parse_options_usage(usage, options, "map", 0);
> +		parse_options_usage(NULL, options, "threads", 0);
> +		return -1;
> +
> +	}
> +
> +	if (info_map && info_threads) {
> +		pr_err("Cannot show map and threads together\n");
> +		parse_options_usage(usage, options, "map", 0);
> +		parse_options_usage(NULL, options, "threads", 0);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int check_lock_contention_options(const struct option *options,
>  					 const char * const *usage)
>  
> @@ -2709,6 +2730,10 @@ int cmd_lock(int argc, const char **argv)
>  			if (argc)
>  				usage_with_options(info_usage, info_options);
>  		}
> +
> +		if (check_lock_info_options(info_options, info_usage) < 0)
> +			return -1;
> +
>  		/* recycling report_lock_ops */
>  		trace_handler = &report_lock_ops;
>  		rc = __cmd_report(true);
> -- 
> 2.42.0
> 
> 

-- 

- Arnaldo

Re: [PATCH 1/2] perf lock report: Restore aggregation by caller by default

[James Clark]

On 31/10/2023 12:05, Nick Forrington wrote:
> This change restores the previous default behaviour for "perf lock
> report", making the current aggregate-by-address behaviour available via
> the new "--lock-addr" command line parameter.
> 
> This makes the behaviour consistent with "perf lock contention" (which
> also aggregates by caller by default, or by address when "--lock-addr"
> is specified).
> 
> Commit 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")
> introduced aggregation modes for "perf lock contention" and (potentially
> inadvertently) changed the behaviour of "perf lock report" from
> aggregate-by-caller to aggregate-by-address (making the prior behaviour
> inaccessible).
> 
> Example aggregate-by-address output:
> 
> $ perf lock report -F acquired
>                 Name   acquired
> 
>          event_mutex         34
>                              21
>                               1
> 
> Example aggregate-by-caller output:
> 
> $ perf lock report -F acquired
>                 Name   acquired
> 
>  perf_trace_init+...         34
>  lock_mm_and_find...         20
>  inherit_event.co...          1
>     do_madvise+0x1f8          1
> 
> Cc: stable@kernel.org
> Fixes: 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")
> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
> ---
>  tools/perf/Documentation/perf-lock.txt |  4 ++++
>  tools/perf/builtin-lock.c              | 24 +++++++++++++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 

Reviewed-by: James Clark <james.clark@arm.com>

> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index 503abcba1438..349333acbbfc 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -80,6 +80,10 @@ REPORT OPTIONS
>  --combine-locks::
>  	Merge lock instances in the same class (based on name).
>  
> +-l::
> +--lock-addr::
> +	Show lock contention stat by address
> +
>  -t::
>  --threads::
>      The -t option is to show per-thread lock stat like below:
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index fa7419978353..3aa8ba5ad928 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -78,7 +78,7 @@ struct callstack_filter {
>  
>  static struct lock_filter filters;
>  
> -static enum lock_aggr_mode aggr_mode = LOCK_AGGR_ADDR;
> +static enum lock_aggr_mode aggr_mode = LOCK_AGGR_CALLER;
>  
>  static bool needs_callstack(void)
>  {
> @@ -1983,8 +1983,8 @@ static int __cmd_report(bool display_info)
>  	if (select_key(false))
>  		goto out_delete;
>  
> -	if (show_thread_stats)
> -		aggr_mode = LOCK_AGGR_TASK;
> +	aggr_mode = show_thread_stats ? LOCK_AGGR_TASK :
> +		show_lock_addrs ? LOCK_AGGR_ADDR : LOCK_AGGR_CALLER;
>  
>  	err = perf_session__process_events(session);
>  	if (err)
> @@ -2008,6 +2008,19 @@ static void sighandler(int sig __maybe_unused)
>  {
>  }
>  
> +static int check_lock_report_options(const struct option *options,
> +				     const char * const *usage)
> +{
> +	if (show_thread_stats && show_lock_addrs) {
> +		pr_err("Cannot use thread and addr mode together\n");
> +		parse_options_usage(usage, options, "threads", 0);
> +		parse_options_usage(NULL, options, "lock-addr", 0);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int check_lock_contention_options(const struct option *options,
>  					 const char * const *usage)
>  
> @@ -2589,6 +2602,7 @@ int cmd_lock(int argc, const char **argv)
>  	/* TODO: type */
>  	OPT_BOOLEAN('c', "combine-locks", &combine_locks,
>  		    "combine locks in the same class"),
> +	OPT_BOOLEAN('l', "lock-addr", &show_lock_addrs, "show lock stats by address"),
>  	OPT_BOOLEAN('t', "threads", &show_thread_stats,
>  		    "show per-thread lock stats"),
>  	OPT_INTEGER('E', "entries", &print_nr_entries, "display this many functions"),
> @@ -2680,6 +2694,10 @@ int cmd_lock(int argc, const char **argv)
>  			if (argc)
>  				usage_with_options(report_usage, report_options);
>  		}
> +
> +		if (check_lock_report_options(report_options, report_usage) < 0)
> +			return -1;
> +
>  		rc = __cmd_report(false);
>  	} else if (!strcmp(argv[0], "script")) {
>  		/* Aliased to 'perf script' */

Re: [PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread

[Nick Forrington]

On 31/10/2023 15:38, Arnaldo Carvalho de Melo wrote:
> Em Tue, Oct 31, 2023 at 12:05:25PM +0000, Nick Forrington escreveu:
>> Improve error reporting for command line arguments.
>>
>> Display error/usage if neither --map or --thread are specified (rather
>> than a non user-friendly error "Unknown type of information").
>>
>> Display error/usage if both --map and --thread are specified (rather
>> than ignoring "--map" and displaying only thread information).
> Shouldn't one of them be the default so that we type less for the most
> common usage?
>
> - Arnaldo
>   

There isn't an obvious choice (to me) for which would be the default.

Both options display completely different data/outputs, so I think it 
makes sense to be explicit about which data is requested.


An alternative could be to use sub-commands e.g. "perf lock info 
threads" or just "perf lock threads", although changing the existing 
options would be more disruptive.


Cheers,
Nick

>> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
>> ---
>>   tools/perf/builtin-lock.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
>> index 3aa8ba5ad928..cf29f648d291 100644
>> --- a/tools/perf/builtin-lock.c
>> +++ b/tools/perf/builtin-lock.c
>> @@ -2021,6 +2021,27 @@ static int check_lock_report_options(const struct option *options,
>>   	return 0;
>>   }
>>   
>> +static int check_lock_info_options(const struct option *options,
>> +				   const char * const *usage)
>> +{
>> +	if (!info_map && !info_threads) {
>> +		pr_err("Requires one of --map or --threads\n");
>> +		parse_options_usage(usage, options, "map", 0);
>> +		parse_options_usage(NULL, options, "threads", 0);
>> +		return -1;
>> +
>> +	}
>> +
>> +	if (info_map && info_threads) {
>> +		pr_err("Cannot show map and threads together\n");
>> +		parse_options_usage(usage, options, "map", 0);
>> +		parse_options_usage(NULL, options, "threads", 0);
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int check_lock_contention_options(const struct option *options,
>>   					 const char * const *usage)
>>   
>> @@ -2709,6 +2730,10 @@ int cmd_lock(int argc, const char **argv)
>>   			if (argc)
>>   				usage_with_options(info_usage, info_options);
>>   		}
>> +
>> +		if (check_lock_info_options(info_options, info_usage) < 0)
>> +			return -1;
>> +
>>   		/* recycling report_lock_ops */
>>   		trace_handler = &report_lock_ops;
>>   		rc = __cmd_report(true);
>> -- 
>> 2.42.0
>>
>>

Re: [PATCH 1/2] perf lock report: Restore aggregation by caller by default

[Namhyung Kim]

Hello,

On Tue, Oct 31, 2023 at 5:05 AM Nick Forrington <nick.forrington@arm.com> wrote:
>
> This change restores the previous default behaviour for "perf lock
> report", making the current aggregate-by-address behaviour available via
> the new "--lock-addr" command line parameter.
>
> This makes the behaviour consistent with "perf lock contention" (which
> also aggregates by caller by default, or by address when "--lock-addr"
> is specified).

I understand your concern but actually there's a difference.
"perf lock contention" is a new command which works with new
contention tracepoints whereas "perf lock report" works with old
lockdep/lockstat tracepoints which are not available in the default
configuration.

I made "perf lock contention" compatible to "perf lock report" so
it mimics the old tracepoints behavior as much as possible using
new tracepoints.  But the important difference is that new contention
tracepoints don't have lock names.  The old perf lock report showed
lock names by default but contention output had to use the caller
instead.

>
> Commit 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")
> introduced aggregation modes for "perf lock contention" and (potentially
> inadvertently) changed the behaviour of "perf lock report" from
> aggregate-by-caller to aggregate-by-address (making the prior behaviour
> inaccessible).

So it doesn't change the behavior of perf lock report.
You're adding a new (default) feature for perf lock report
to sort the output by caller.  And please note that caller
info needs callstacks.  perf lock record adds it by default
when it finds there are only lock contention tracepoints.
But if it really has the old tracepoints, caller won't work
unless you enabled callstack collection manually (-g).

>
> Example aggregate-by-address output:
>
> $ perf lock report -F acquired

I guess you need -l option here.

>                 Name   acquired
>
>          event_mutex         34
>                              21
>                               1

This is because you used contention tracepoints
and they don't have lock names.

>
> Example aggregate-by-caller output:
>
> $ perf lock report -F acquired
>                 Name   acquired
>
>  perf_trace_init+...         34
>  lock_mm_and_find...         20
>  inherit_event.co...          1
>     do_madvise+0x1f8          1

Maybe it's ok to change the default behavior for contention
tracepoints.  But when lockdep tracepoints are available
it should use the existing addr (symbol) mode.

>
> Cc: stable@kernel.org
> Fixes: 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")

So, I don't think this is a fix.

Thanks,
Namhyung


> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
> ---
>  tools/perf/Documentation/perf-lock.txt |  4 ++++
>  tools/perf/builtin-lock.c              | 24 +++++++++++++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index 503abcba1438..349333acbbfc 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -80,6 +80,10 @@ REPORT OPTIONS
>  --combine-locks::
>         Merge lock instances in the same class (based on name).
>
> +-l::
> +--lock-addr::
> +       Show lock contention stat by address
> +
>  -t::
>  --threads::
>      The -t option is to show per-thread lock stat like below:
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index fa7419978353..3aa8ba5ad928 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -78,7 +78,7 @@ struct callstack_filter {
>
>  static struct lock_filter filters;
>
> -static enum lock_aggr_mode aggr_mode = LOCK_AGGR_ADDR;
> +static enum lock_aggr_mode aggr_mode = LOCK_AGGR_CALLER;
>
>  static bool needs_callstack(void)
>  {
> @@ -1983,8 +1983,8 @@ static int __cmd_report(bool display_info)
>         if (select_key(false))
>                 goto out_delete;
>
> -       if (show_thread_stats)
> -               aggr_mode = LOCK_AGGR_TASK;
> +       aggr_mode = show_thread_stats ? LOCK_AGGR_TASK :
> +               show_lock_addrs ? LOCK_AGGR_ADDR : LOCK_AGGR_CALLER;
>
>         err = perf_session__process_events(session);
>         if (err)
> @@ -2008,6 +2008,19 @@ static void sighandler(int sig __maybe_unused)
>  {
>  }
>
> +static int check_lock_report_options(const struct option *options,
> +                                    const char * const *usage)
> +{
> +       if (show_thread_stats && show_lock_addrs) {
> +               pr_err("Cannot use thread and addr mode together\n");
> +               parse_options_usage(usage, options, "threads", 0);
> +               parse_options_usage(NULL, options, "lock-addr", 0);
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
>  static int check_lock_contention_options(const struct option *options,
>                                          const char * const *usage)
>
> @@ -2589,6 +2602,7 @@ int cmd_lock(int argc, const char **argv)
>         /* TODO: type */
>         OPT_BOOLEAN('c', "combine-locks", &combine_locks,
>                     "combine locks in the same class"),
> +       OPT_BOOLEAN('l', "lock-addr", &show_lock_addrs, "show lock stats by address"),
>         OPT_BOOLEAN('t', "threads", &show_thread_stats,
>                     "show per-thread lock stats"),
>         OPT_INTEGER('E', "entries", &print_nr_entries, "display this many functions"),
> @@ -2680,6 +2694,10 @@ int cmd_lock(int argc, const char **argv)
>                         if (argc)
>                                 usage_with_options(report_usage, report_options);
>                 }
> +
> +               if (check_lock_report_options(report_options, report_usage) < 0)
> +                       return -1;
> +
>                 rc = __cmd_report(false);
>         } else if (!strcmp(argv[0], "script")) {
>                 /* Aliased to 'perf script' */
> --
> 2.42.0
>

Re: [PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread

[Namhyung Kim]

On Wed, Nov 1, 2023 at 7:35 AM Nick Forrington <nick.forrington@arm.com> wrote:
>
>
> On 31/10/2023 15:38, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Oct 31, 2023 at 12:05:25PM +0000, Nick Forrington escreveu:
> >> Improve error reporting for command line arguments.
> >>
> >> Display error/usage if neither --map or --thread are specified (rather
> >> than a non user-friendly error "Unknown type of information").
> >>
> >> Display error/usage if both --map and --thread are specified (rather
> >> than ignoring "--map" and displaying only thread information).
> > Shouldn't one of them be the default so that we type less for the most
> > common usage?
> >
> > - Arnaldo
> >
>
> There isn't an obvious choice (to me) for which would be the default.
>
> Both options display completely different data/outputs, so I think it
> makes sense to be explicit about which data is requested.

Maybe we can default to display both. :)

Thanks,
Namhyung

>
>
> An alternative could be to use sub-commands e.g. "perf lock info
> threads" or just "perf lock threads", although changing the existing
> options would be more disruptive.
>
>
> Cheers,
> Nick
>
> >> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
> >> ---
> >>   tools/perf/builtin-lock.c | 25 +++++++++++++++++++++++++
> >>   1 file changed, 25 insertions(+)
> >>
> >> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> >> index 3aa8ba5ad928..cf29f648d291 100644
> >> --- a/tools/perf/builtin-lock.c
> >> +++ b/tools/perf/builtin-lock.c
> >> @@ -2021,6 +2021,27 @@ static int check_lock_report_options(const struct option *options,
> >>      return 0;
> >>   }
> >>
> >> +static int check_lock_info_options(const struct option *options,
> >> +                               const char * const *usage)
> >> +{
> >> +    if (!info_map && !info_threads) {
> >> +            pr_err("Requires one of --map or --threads\n");
> >> +            parse_options_usage(usage, options, "map", 0);
> >> +            parse_options_usage(NULL, options, "threads", 0);
> >> +            return -1;
> >> +
> >> +    }
> >> +
> >> +    if (info_map && info_threads) {
> >> +            pr_err("Cannot show map and threads together\n");
> >> +            parse_options_usage(usage, options, "map", 0);
> >> +            parse_options_usage(NULL, options, "threads", 0);
> >> +            return -1;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>   static int check_lock_contention_options(const struct option *options,
> >>                                       const char * const *usage)
> >>
> >> @@ -2709,6 +2730,10 @@ int cmd_lock(int argc, const char **argv)
> >>                      if (argc)
> >>                              usage_with_options(info_usage, info_options);
> >>              }
> >> +
> >> +            if (check_lock_info_options(info_options, info_usage) < 0)
> >> +                    return -1;
> >> +
> >>              /* recycling report_lock_ops */
> >>              trace_handler = &report_lock_ops;
> >>              rc = __cmd_report(true);
> >> --
> >> 2.42.0
> >>
> >>

Re: [PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread

[Arnaldo Carvalho de Melo]

Em Wed, Nov 01, 2023 at 11:00:42PM -0700, Namhyung Kim escreveu:
> On Wed, Nov 1, 2023 at 7:35 AM Nick Forrington <nick.forrington@arm.com> wrote:
> >
> >
> > On 31/10/2023 15:38, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Oct 31, 2023 at 12:05:25PM +0000, Nick Forrington escreveu:
> > >> Improve error reporting for command line arguments.
> > >>
> > >> Display error/usage if neither --map or --thread are specified (rather
> > >> than a non user-friendly error "Unknown type of information").
> > >>
> > >> Display error/usage if both --map and --thread are specified (rather
> > >> than ignoring "--map" and displaying only thread information).
> > > Shouldn't one of them be the default so that we type less for the most
> > > common usage?
> > >
> > > - Arnaldo
> > >
> >
> > There isn't an obvious choice (to me) for which would be the default.
> >
> > Both options display completely different data/outputs, so I think it
> > makes sense to be explicit about which data is requested.
> 
> Maybe we can default to display both. :)

Yeah, that would be a better approach, I think.

- Arnaldo
 
> Thanks,
> Namhyung
> 
> >
> >
> > An alternative could be to use sub-commands e.g. "perf lock info
> > threads" or just "perf lock threads", although changing the existing
> > options would be more disruptive.
> >
> >
> > Cheers,
> > Nick
> >
> > >> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
> > >> ---
> > >>   tools/perf/builtin-lock.c | 25 +++++++++++++++++++++++++
> > >>   1 file changed, 25 insertions(+)
> > >>
> > >> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > >> index 3aa8ba5ad928..cf29f648d291 100644
> > >> --- a/tools/perf/builtin-lock.c
> > >> +++ b/tools/perf/builtin-lock.c
> > >> @@ -2021,6 +2021,27 @@ static int check_lock_report_options(const struct option *options,
> > >>      return 0;
> > >>   }
> > >>
> > >> +static int check_lock_info_options(const struct option *options,
> > >> +                               const char * const *usage)
> > >> +{
> > >> +    if (!info_map && !info_threads) {
> > >> +            pr_err("Requires one of --map or --threads\n");
> > >> +            parse_options_usage(usage, options, "map", 0);
> > >> +            parse_options_usage(NULL, options, "threads", 0);
> > >> +            return -1;
> > >> +
> > >> +    }
> > >> +
> > >> +    if (info_map && info_threads) {
> > >> +            pr_err("Cannot show map and threads together\n");
> > >> +            parse_options_usage(usage, options, "map", 0);
> > >> +            parse_options_usage(NULL, options, "threads", 0);
> > >> +            return -1;
> > >> +    }
> > >> +
> > >> +    return 0;
> > >> +}
> > >> +
> > >>   static int check_lock_contention_options(const struct option *options,
> > >>                                       const char * const *usage)
> > >>
> > >> @@ -2709,6 +2730,10 @@ int cmd_lock(int argc, const char **argv)
> > >>                      if (argc)
> > >>                              usage_with_options(info_usage, info_options);
> > >>              }
> > >> +
> > >> +            if (check_lock_info_options(info_options, info_usage) < 0)
> > >> +                    return -1;
> > >> +
> > >>              /* recycling report_lock_ops */
> > >>              trace_handler = &report_lock_ops;
> > >>              rc = __cmd_report(true);
> > >> --
> > >> 2.42.0
> > >>
> > >>

-- 

- Arnaldo

Re: [PATCH 1/2] perf lock report: Restore aggregation by caller by default

[Nick Forrington]

On 02/11/2023 05:55, Namhyung Kim wrote:
> Hello,
>
> On Tue, Oct 31, 2023 at 5:05 AM Nick Forrington <nick.forrington@arm.com> wrote:
>> This change restores the previous default behaviour for "perf lock
>> report", making the current aggregate-by-address behaviour available via
>> the new "--lock-addr" command line parameter.
>>
>> This makes the behaviour consistent with "perf lock contention" (which
>> also aggregates by caller by default, or by address when "--lock-addr"
>> is specified).
> I understand your concern but actually there's a difference.
> "perf lock contention" is a new command which works with new
> contention tracepoints whereas "perf lock report" works with old
> lockdep/lockstat tracepoints which are not available in the default
> configuration.
>
> I made "perf lock contention" compatible to "perf lock report" so
> it mimics the old tracepoints behavior as much as possible using
> new tracepoints.  But the important difference is that new contention
> tracepoints don't have lock names.  The old perf lock report showed
> lock names by default but contention output had to use the caller
> instead.

Thanks for the information.


It looks like "perf lock record" requests both tracepoint types.


However, on the system I used the lockdep tracepoints were not available 
- only lock:contention_begin/end.

>
>> Commit 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")
>> introduced aggregation modes for "perf lock contention" and (potentially
>> inadvertently) changed the behaviour of "perf lock report" from
>> aggregate-by-caller to aggregate-by-address (making the prior behaviour
>> inaccessible).
> So it doesn't change the behavior of perf lock report.
> You're adding a new (default) feature for perf lock report
> to sort the output by caller.  And please note that caller
> info needs callstacks.  perf lock record adds it by default
> when it finds there are only lock contention tracepoints.
> But if it really has the old tracepoints, caller won't work
> unless you enabled callstack collection manually (-g).

With the caveat that only lock:contention_begin/end were available in my 
test, there does appear to be a difference in output for perf lock report.


I arrived at 688d2e8de231 via git bisect. 688d2e8de231~ (eca949b2b4ad) 
shows the aggregate-by-caller behaviour (seen below, and "restored" by 
this patch).

>
>> Example aggregate-by-address output:
>>
>> $ perf lock report -F acquired
> I guess you need -l option here.

This was the default output before this patch (or output with the patch 
and "-l"), I can clarify this if the patch goes ahead.

>>                  Name   acquired
>>
>>           event_mutex         34
>>                               21
>>                                1
> This is because you used contention tracepoints
> and they don't have lock names.

Yes, the lockdep tracepoints weren't available for my test, although on 
the same machine, versions prior to 688d2e8de231 appear to show the 
below output rather than the above.

>
>> Example aggregate-by-caller output:
>>
>> $ perf lock report -F acquired
>>                  Name   acquired
>>
>>   perf_trace_init+...         34
>>   lock_mm_and_find...         20
>>   inherit_event.co...          1
>>      do_madvise+0x1f8          1
> Maybe it's ok to change the default behavior for contention
> tracepoints.  But when lockdep tracepoints are available
> it should use the existing addr (symbol) mode.
>
>> Cc: stable@kernel.org
>> Fixes: 688d2e8de231 ("perf lock contention: Add -l/--lock-addr option")
> So, I don't think this is a fix.
>
> Thanks,
> Namhyung


Thanks,
Nick

>
>
>> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
>> ---
>>   tools/perf/Documentation/perf-lock.txt |  4 ++++
>>   tools/perf/builtin-lock.c              | 24 +++++++++++++++++++++---
>>   2 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
>> index 503abcba1438..349333acbbfc 100644
>> --- a/tools/perf/Documentation/perf-lock.txt
>> +++ b/tools/perf/Documentation/perf-lock.txt
>> @@ -80,6 +80,10 @@ REPORT OPTIONS
>>   --combine-locks::
>>          Merge lock instances in the same class (based on name).
>>
>> +-l::
>> +--lock-addr::
>> +       Show lock contention stat by address
>> +
>>   -t::
>>   --threads::
>>       The -t option is to show per-thread lock stat like below:
>> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
>> index fa7419978353..3aa8ba5ad928 100644
>> --- a/tools/perf/builtin-lock.c
>> +++ b/tools/perf/builtin-lock.c
>> @@ -78,7 +78,7 @@ struct callstack_filter {
>>
>>   static struct lock_filter filters;
>>
>> -static enum lock_aggr_mode aggr_mode = LOCK_AGGR_ADDR;
>> +static enum lock_aggr_mode aggr_mode = LOCK_AGGR_CALLER;
>>
>>   static bool needs_callstack(void)
>>   {
>> @@ -1983,8 +1983,8 @@ static int __cmd_report(bool display_info)
>>          if (select_key(false))
>>                  goto out_delete;
>>
>> -       if (show_thread_stats)
>> -               aggr_mode = LOCK_AGGR_TASK;
>> +       aggr_mode = show_thread_stats ? LOCK_AGGR_TASK :
>> +               show_lock_addrs ? LOCK_AGGR_ADDR : LOCK_AGGR_CALLER;
>>
>>          err = perf_session__process_events(session);
>>          if (err)
>> @@ -2008,6 +2008,19 @@ static void sighandler(int sig __maybe_unused)
>>   {
>>   }
>>
>> +static int check_lock_report_options(const struct option *options,
>> +                                    const char * const *usage)
>> +{
>> +       if (show_thread_stats && show_lock_addrs) {
>> +               pr_err("Cannot use thread and addr mode together\n");
>> +               parse_options_usage(usage, options, "threads", 0);
>> +               parse_options_usage(NULL, options, "lock-addr", 0);
>> +               return -1;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int check_lock_contention_options(const struct option *options,
>>                                           const char * const *usage)
>>
>> @@ -2589,6 +2602,7 @@ int cmd_lock(int argc, const char **argv)
>>          /* TODO: type */
>>          OPT_BOOLEAN('c', "combine-locks", &combine_locks,
>>                      "combine locks in the same class"),
>> +       OPT_BOOLEAN('l', "lock-addr", &show_lock_addrs, "show lock stats by address"),
>>          OPT_BOOLEAN('t', "threads", &show_thread_stats,
>>                      "show per-thread lock stats"),
>>          OPT_INTEGER('E', "entries", &print_nr_entries, "display this many functions"),
>> @@ -2680,6 +2694,10 @@ int cmd_lock(int argc, const char **argv)
>>                          if (argc)
>>                                  usage_with_options(report_usage, report_options);
>>                  }
>> +
>> +               if (check_lock_report_options(report_options, report_usage) < 0)
>> +                       return -1;
>> +
>>                  rc = __cmd_report(false);
>>          } else if (!strcmp(argv[0], "script")) {
>>                  /* Aliased to 'perf script' */
>> --
>> 2.42.0
>>

Re: [PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread

[Nick Forrington]

On 08/11/2023 20:28, Arnaldo Carvalho de Melo wrote:
> Em Wed, Nov 01, 2023 at 11:00:42PM -0700, Namhyung Kim escreveu:
>> On Wed, Nov 1, 2023 at 7:35 AM Nick Forrington <nick.forrington@arm.com> wrote:
>>>
>>> On 31/10/2023 15:38, Arnaldo Carvalho de Melo wrote:
>>>> Em Tue, Oct 31, 2023 at 12:05:25PM +0000, Nick Forrington escreveu:
>>>>> Improve error reporting for command line arguments.
>>>>>
>>>>> Display error/usage if neither --map or --thread are specified (rather
>>>>> than a non user-friendly error "Unknown type of information").
>>>>>
>>>>> Display error/usage if both --map and --thread are specified (rather
>>>>> than ignoring "--map" and displaying only thread information).
>>>> Shouldn't one of them be the default so that we type less for the most
>>>> common usage?
>>>>
>>>> - Arnaldo
>>>>
>>> There isn't an obvious choice (to me) for which would be the default.
>>>
>>> Both options display completely different data/outputs, so I think it
>>> makes sense to be explicit about which data is requested.
>> Maybe we can default to display both. :)
> Yeah, that would be a better approach, I think.
>
> - Arnaldo
>   


I'll submit an updated series for this, with the next update to patch 1/2

Thanks,
Nick

>> Thanks,
>> Namhyung
>>
>>>
>>> An alternative could be to use sub-commands e.g. "perf lock info
>>> threads" or just "perf lock threads", although changing the existing
>>> options would be more disruptive.
>>>
>>>
>>> Cheers,
>>> Nick
>>>
>>>>> Signed-off-by: Nick Forrington <nick.forrington@arm.com>
>>>>> ---
>>>>>    tools/perf/builtin-lock.c | 25 +++++++++++++++++++++++++
>>>>>    1 file changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
>>>>> index 3aa8ba5ad928..cf29f648d291 100644
>>>>> --- a/tools/perf/builtin-lock.c
>>>>> +++ b/tools/perf/builtin-lock.c
>>>>> @@ -2021,6 +2021,27 @@ static int check_lock_report_options(const struct option *options,
>>>>>       return 0;
>>>>>    }
>>>>>
>>>>> +static int check_lock_info_options(const struct option *options,
>>>>> +                               const char * const *usage)
>>>>> +{
>>>>> +    if (!info_map && !info_threads) {
>>>>> +            pr_err("Requires one of --map or --threads\n");
>>>>> +            parse_options_usage(usage, options, "map", 0);
>>>>> +            parse_options_usage(NULL, options, "threads", 0);
>>>>> +            return -1;
>>>>> +
>>>>> +    }
>>>>> +
>>>>> +    if (info_map && info_threads) {
>>>>> +            pr_err("Cannot show map and threads together\n");
>>>>> +            parse_options_usage(usage, options, "map", 0);
>>>>> +            parse_options_usage(NULL, options, "threads", 0);
>>>>> +            return -1;
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>    static int check_lock_contention_options(const struct option *options,
>>>>>                                        const char * const *usage)
>>>>>
>>>>> @@ -2709,6 +2730,10 @@ int cmd_lock(int argc, const char **argv)
>>>>>                       if (argc)
>>>>>                               usage_with_options(info_usage, info_options);
>>>>>               }
>>>>> +
>>>>> +            if (check_lock_info_options(info_options, info_usage) < 0)
>>>>> +                    return -1;
>>>>> +
>>>>>               /* recycling report_lock_ops */
>>>>>               trace_handler = &report_lock_ops;
>>>>>               rc = __cmd_report(true);
>>>>> --
>>>>> 2.42.0
>>>>>
>>>>>

Re: [PATCH 2/2] perf lock info: Enforce exactly one of --map and --thread

[Arnaldo Carvalho de Melo]

Em Mon, Nov 13, 2023 at 11:50:16AM +0000, Nick Forrington escreveu:
> On 08/11/2023 20:28, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Nov 01, 2023 at 11:00:42PM -0700, Namhyung Kim escreveu:
> > > On Wed, Nov 1, 2023 at 7:35 AM Nick Forrington <nick.forrington@arm.com> wrote:
> > > > On 31/10/2023 15:38, Arnaldo Carvalho de Melo wrote:
> > > > > Em Tue, Oct 31, 2023 at 12:05:25PM +0000, Nick Forrington escreveu:
> > > > > > Improve error reporting for command line arguments.
> > > > > > Display error/usage if neither --map or --thread are specified (rather
> > > > > > than a non user-friendly error "Unknown type of information").
> > > > > > Display error/usage if both --map and --thread are specified (rather
> > > > > > than ignoring "--map" and displaying only thread information).
> > > > > Shouldn't one of them be the default so that we type less for the most
> > > > > common usage?
> > > > There isn't an obvious choice (to me) for which would be the default.
> > > > Both options display completely different data/outputs, so I think it
> > > > makes sense to be explicit about which data is requested.
> > > Maybe we can default to display both. :)
> > Yeah, that would be a better approach, I think.
 
> I'll submit an updated series for this, with the next update to patch 1/2
 
Thanks, tried using b4 but it din't find a v2, will wait then.

- Arnaldo