Re: [PATCH v2 12/16] perf lock: Move common lock contention code to new file

[Namhyung Kim <namhyung@kernel.org>]

On Tue, Oct 15, 2024 at 09:24:11PM -0700, Ian Rogers wrote:
> Avoid references from util code to builtin-lock that require python
> stubs. Move the functions and related variables to
> util/lock-contention.c. Add max_stack_depth parameter to
> match_callstack_filter to avoid sharing a global variable.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-lock.c             | 137 +--------------------
>  tools/perf/util/Build                 |   1 +
>  tools/perf/util/bpf_lock_contention.c |   2 +-
>  tools/perf/util/lock-contention.c     | 170 ++++++++++++++++++++++++++
>  tools/perf/util/lock-contention.h     |  37 ++----
>  tools/perf/util/python.c              |  17 ---
>  6 files changed, 185 insertions(+), 179 deletions(-)
>  create mode 100644 tools/perf/util/lock-contention.c
> 
[SNIP]
> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 2a2f7780e595..ea2d9eced92e 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -121,6 +121,7 @@ perf-util-y += topdown.o
>  perf-util-y += iostat.o
>  perf-util-y += stream.o
>  perf-util-y += kvm-stat.o
> +perf-util-y += lock-contention.o
>  perf-util-$(CONFIG_AUXTRACE) += auxtrace.o
>  perf-util-$(CONFIG_AUXTRACE) += intel-pt-decoder/
>  perf-util-$(CONFIG_AUXTRACE) += intel-pt.o
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index 41a1ad087895..37e17c56f106 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -458,7 +458,7 @@ int lock_contention_read(struct lock_contention *con)
>  		if (con->save_callstack) {
>  			bpf_map_lookup_elem(stack, &key.stack_id, stack_trace);
>  
> -			if (!match_callstack_filter(machine, stack_trace)) {
> +			if (!match_callstack_filter(machine, stack_trace, con->max_stack)) {
>  				con->nr_filtered += data.count;
>  				goto next;
>  			}
> diff --git a/tools/perf/util/lock-contention.c b/tools/perf/util/lock-contention.c
> new file mode 100644
> index 000000000000..841bb18b1f06
> --- /dev/null
> +++ b/tools/perf/util/lock-contention.c
[SNIP]
> +#ifndef HAVE_BPF_SKEL
> +int lock_contention_prepare(struct lock_contention *con __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +int lock_contention_start(void)
> +{
> +	return 0;
> +}
> +
> +int lock_contention_stop(void)
> +{
> +	return 0;
> +}
> +
> +int lock_contention_finish(struct lock_contention *con __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +int lock_contention_read(struct lock_contention *con __maybe_unused)
> +{
> +	return 0;
> +}
> +#endif  /* !HAVE_BPF_SKEL */

Do we really need this part?  Why not leave it in the header file?

Thanks,
Namhyung


> diff --git a/tools/perf/util/lock-contention.h b/tools/perf/util/lock-contention.h
> index 1a7248ff3889..bfa5c7db0a5d 100644
> --- a/tools/perf/util/lock-contention.h
> +++ b/tools/perf/util/lock-contention.h
> @@ -67,10 +67,11 @@ struct lock_stat {
>   */
>  #define MAX_LOCK_DEPTH 48
>  
> -struct lock_stat *lock_stat_find(u64 addr);
> -struct lock_stat *lock_stat_findnew(u64 addr, const char *name, int flags);
> +/* based on kernel/lockdep.c */
> +#define LOCKHASH_BITS		12
> +#define LOCKHASH_SIZE		(1UL << LOCKHASH_BITS)
>  
> -bool match_callstack_filter(struct machine *machine, u64 *callstack);
> +extern struct hlist_head *lockhash_table;
>  
>  /*
>   * struct lock_seq_stat:
> @@ -148,7 +149,14 @@ struct lock_contention {
>  	bool save_callstack;
>  };
>  
> -#ifdef HAVE_BPF_SKEL
> +struct option;
> +int parse_call_stack(const struct option *opt, const char *str, int unset);
> +bool needs_callstack(void);
> +
> +struct lock_stat *lock_stat_find(u64 addr);
> +struct lock_stat *lock_stat_findnew(u64 addr, const char *name, int flags);
> +
> +bool match_callstack_filter(struct machine *machine, u64 *callstack, int max_stack_depth);
>  
>  int lock_contention_prepare(struct lock_contention *con);
>  int lock_contention_start(void);
> @@ -156,25 +164,4 @@ int lock_contention_stop(void);
>  int lock_contention_read(struct lock_contention *con);
>  int lock_contention_finish(struct lock_contention *con);
>  
> -#else  /* !HAVE_BPF_SKEL */
> -
> -static inline int lock_contention_prepare(struct lock_contention *con __maybe_unused)
> -{
> -	return 0;
> -}
> -
> -static inline int lock_contention_start(void) { return 0; }
> -static inline int lock_contention_stop(void) { return 0; }
> -static inline int lock_contention_finish(struct lock_contention *con __maybe_unused)
> -{
> -	return 0;
> -}
> -
> -static inline int lock_contention_read(struct lock_contention *con __maybe_unused)
> -{
> -	return 0;
> -}
> -
> -#endif  /* HAVE_BPF_SKEL */
> -
>  #endif  /* PERF_LOCK_CONTENTION_H */
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 35d84a96dbec..91fd444615cd 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -18,7 +18,6 @@
>  #include "mmap.h"
>  #include "util/kwork.h"
>  #include "util/sample.h"
> -#include "util/lock-contention.h"
>  #include <internal/lib.h>
>  #include "../builtin.h"
>  
> @@ -1311,22 +1310,6 @@ struct kwork_work *perf_kwork_add_work(struct perf_kwork *kwork __maybe_unused,
>  	return NULL;
>  }
>  
> -bool match_callstack_filter(struct machine *machine __maybe_unused, u64 *callstack __maybe_unused)
> -{
> -	return false;
> -}
> -
> -struct lock_stat *lock_stat_find(u64 addr __maybe_unused)
> -{
> -	return NULL;
> -}
> -
> -struct lock_stat *lock_stat_findnew(u64 addr __maybe_unused, const char *name __maybe_unused,
> -				int flags __maybe_unused)
> -{
> -	return NULL;
> -}
> -
>  int cmd_inject(int argc __maybe_unused, const char *argv[] __maybe_unused)
>  {
>  	return -1;
> -- 
> 2.47.0.rc1.288.g06298d1525-goog
>