linux-kernel - Re: [PATCH 2/3] perf lock: Add -m/--map-length option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YuliN2UDbls5uW3u@kernel.org>
Date:   Tue, 2 Aug 2022 14:43:19 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ian Rogers <irogers@...gle.com>,
        linux-perf-users@...r.kernel.org, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Song Liu <songliubraving@...com>,
        Blake Jones <blakejones@...gle.com>
Subject: Re: [PATCH 2/3] perf lock: Add -m/--map-length option

Em Tue, Aug 02, 2022 at 12:35:10AM -0700, Namhyung Kim escreveu:
> The -m/--map-length option is to control number of max entries in the
> perf lock contention BPF maps.
> 
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  tools/perf/Documentation/perf-lock.txt |  4 ++++
>  tools/perf/builtin-lock.c              | 23 ++++++++++++++++++++++-
>  tools/perf/util/bpf_lock_contention.c  |  3 +++
>  tools/perf/util/lock-contention.h      |  1 +
>  4 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index 7949d2e6891b..2101644785e0 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -145,6 +145,10 @@ CONTENTION OPTIONS
>  --tid=::
>          Record events on existing thread ID (comma separated list).
>  
> +-m::
> +--map-length::
> +	Maximum number of BPF map entries (default: 10240).

--map-nr-entries?

I think we use this jargon "nr-entries" for arrays, lists, etc, better
try to stick to it.

Also what do you think about not using single letter options for things
that are not that used?

The map size has a default, one that seems generous, so changing it
should be something uncommon, and then, if it becomes common that more
entries are needed by default, we can change the default in the tool.

- Arnaldo

> +
>  
>  SEE ALSO
>  --------
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index eef778b7d33d..e32fdcd497e0 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -55,6 +55,7 @@ static struct rb_root		thread_stats;
>  static bool combine_locks;
>  static bool show_thread_stats;
>  static bool use_bpf;
> +static unsigned long bpf_map_len = 10240;
>  
>  static enum {
>  	LOCK_AGGR_ADDR,
> @@ -1597,6 +1598,7 @@ static int __cmd_contention(int argc, const char **argv)
>  	struct lock_contention con = {
>  		.target = &target,
>  		.result = &lockhash_table[0],
> +		.map_len = bpf_map_len,
>  	};
>  
>  	session = perf_session__new(use_bpf ? NULL : &data, &eops);
> @@ -1786,6 +1788,24 @@ static int __cmd_record(int argc, const char **argv)
>  	return ret;
>  }
>  
> +static int parse_map_length(const struct option *opt, const char *str,
> +			    int unset __maybe_unused)
> +{
> +	unsigned long *len = (unsigned long *)opt->value;
> +	unsigned long val;
> +	char *endptr;
> +
> +	errno = 0;
> +	val = strtoul(str, &endptr, 0);
> +	if (*endptr != '\0' || errno != 0) {
> +		pr_err("invalid BPF map length: %s\n", str);
> +		return -1;
> +	}
> +
> +	*len = val;
> +	return 0;
> +}
> +
>  int cmd_lock(int argc, const char **argv)
>  {
>  	const struct option lock_options[] = {
> @@ -1835,9 +1855,10 @@ int cmd_lock(int argc, const char **argv)
>  		    "List of cpus to monitor"),
>  	OPT_STRING('p', "pid", &target.pid, "pid",
>  		   "Trace on existing process id"),
> -	/* TODO: Add short option -t after -t/--tracer can be removed. */
>  	OPT_STRING(0, "tid", &target.tid, "tid",
>  		   "Trace on existing thread id (exclusive to --pid)"),
> +	OPT_CALLBACK('m', "map-length", &bpf_map_len, "len",
> +		     "Max number of BPF map entries", parse_map_length),
>  	OPT_PARENT(lock_options)
>  	};
>  
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index f5e2b4f19a72..26128e5bb659 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -40,6 +40,9 @@ int lock_contention_prepare(struct lock_contention *con)
>  		return -1;
>  	}
>  
> +	bpf_map__set_max_entries(skel->maps.stacks, con->map_len);
> +	bpf_map__set_max_entries(skel->maps.lock_stat, con->map_len);
> +
>  	if (target__has_cpu(target))
>  		ncpus = perf_cpu_map__nr(evlist->core.user_requested_cpus);
>  	if (target__has_task(target))
> diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
> index a0df5308cca4..b09fd6eb978a 100644
> --- a/tools/perf/util/lock-contention.h
> +++ b/tools/perf/util/lock-contention.h
> @@ -112,6 +112,7 @@ struct lock_contention {
>  	struct target *target;
>  	struct machine *machine;
>  	struct hlist_head *result;
> +	unsigned long map_len;
>  };
>  
>  #ifdef HAVE_BPF_SKEL
> -- 
> 2.37.1.455.g008518b4e5-goog

-- 

- Arnaldo