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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>>
>>
On Wed, Nov 1, 2023 at 7:35 AM Nick Forrington <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
> >>
> >>
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 <[email protected]> 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