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
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 062e2b56a2ab..f66948b1fbed 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -46,15 +46,6 @@
static struct perf_session *session;
static struct target target;
-/* based on kernel/lockdep.c */
-#define LOCKHASH_BITS 12
-#define LOCKHASH_SIZE (1UL << LOCKHASH_BITS)
-
-static struct hlist_head *lockhash_table;
-
-#define __lockhashfn(key) hash_long((unsigned long)key, LOCKHASH_BITS)
-#define lockhashentry(key) (lockhash_table + __lockhashfn((key)))
-
static struct rb_root thread_stats;
static bool combine_locks;
@@ -67,24 +58,13 @@ static unsigned long bpf_map_entries = MAX_ENTRIES;
static int max_stack_depth = CONTENTION_STACK_DEPTH;
static int stack_skip = CONTENTION_STACK_SKIP;
static int print_nr_entries = INT_MAX / 2;
-static LIST_HEAD(callstack_filters);
static const char *output_name = NULL;
static FILE *lock_output;
-struct callstack_filter {
- struct list_head list;
- char name[];
-};
-
static struct lock_filter filters;
static enum lock_aggr_mode aggr_mode = LOCK_AGGR_ADDR;
-static bool needs_callstack(void)
-{
- return !list_empty(&callstack_filters);
-}
-
static struct thread_stat *thread_stat_find(u32 tid)
{
struct rb_node *node;
@@ -477,93 +457,6 @@ static struct lock_stat *pop_from_result(void)
return container_of(node, struct lock_stat, rb);
}
-struct lock_stat *lock_stat_find(u64 addr)
-{
- struct hlist_head *entry = lockhashentry(addr);
- struct lock_stat *ret;
-
- hlist_for_each_entry(ret, entry, hash_entry) {
- if (ret->addr == addr)
- return ret;
- }
- return NULL;
-}
-
-struct lock_stat *lock_stat_findnew(u64 addr, const char *name, int flags)
-{
- struct hlist_head *entry = lockhashentry(addr);
- struct lock_stat *ret, *new;
-
- hlist_for_each_entry(ret, entry, hash_entry) {
- if (ret->addr == addr)
- return ret;
- }
-
- new = zalloc(sizeof(struct lock_stat));
- if (!new)
- goto alloc_failed;
-
- new->addr = addr;
- new->name = strdup(name);
- if (!new->name) {
- free(new);
- goto alloc_failed;
- }
-
- new->flags = flags;
- new->wait_time_min = ULLONG_MAX;
-
- hlist_add_head(&new->hash_entry, entry);
- return new;
-
-alloc_failed:
- pr_err("memory allocation failed\n");
- return NULL;
-}
-
-bool match_callstack_filter(struct machine *machine, u64 *callstack)
-{
- struct map *kmap;
- struct symbol *sym;
- u64 ip;
- const char *arch = perf_env__arch(machine->env);
-
- if (list_empty(&callstack_filters))
- return true;
-
- for (int i = 0; i < max_stack_depth; i++) {
- struct callstack_filter *filter;
-
- /*
- * In powerpc, the callchain saved by kernel always includes
- * first three entries as the NIP (next instruction pointer),
- * LR (link register), and the contents of LR save area in the
- * second stack frame. In certain scenarios its possible to have
- * invalid kernel instruction addresses in either LR or the second
- * stack frame's LR. In that case, kernel will store that address as
- * zero.
- *
- * The below check will continue to look into callstack,
- * incase first or second callstack index entry has 0
- * address for powerpc.
- */
- if (!callstack || (!callstack[i] && (strcmp(arch, "powerpc") ||
- (i != 1 && i != 2))))
- break;
-
- ip = callstack[i];
- sym = machine__find_kernel_symbol(machine, ip, &kmap);
- if (sym == NULL)
- continue;
-
- list_for_each_entry(filter, &callstack_filters, list) {
- if (strstr(sym->name, filter->name))
- return true;
- }
- }
- return false;
-}
-
struct trace_lock_handler {
/* it's used on CONFIG_LOCKDEP */
int (*acquire_event)(struct evsel *evsel,
@@ -1165,7 +1058,7 @@ static int report_lock_contention_begin_event(struct evsel *evsel,
if (callstack == NULL)
return -ENOMEM;
- if (!match_callstack_filter(machine, callstack)) {
+ if (!match_callstack_filter(machine, callstack, max_stack_depth)) {
free(callstack);
return 0;
}
@@ -2449,34 +2342,6 @@ static int parse_lock_addr(const struct option *opt __maybe_unused, const char *
return ret;
}
-static int parse_call_stack(const struct option *opt __maybe_unused, const char *str,
- int unset __maybe_unused)
-{
- char *s, *tmp, *tok;
- int ret = 0;
-
- s = strdup(str);
- if (s == NULL)
- return -1;
-
- for (tok = strtok_r(s, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
- struct callstack_filter *entry;
-
- entry = malloc(sizeof(*entry) + strlen(tok) + 1);
- if (entry == NULL) {
- pr_err("Memory allocation failure\n");
- free(s);
- return -1;
- }
-
- strcpy(entry->name, tok);
- list_add_tail(&entry->list, &callstack_filters);
- }
-
- free(s);
- return ret;
-}
-
static int parse_output(const struct option *opt __maybe_unused, const char *str,
int unset __maybe_unused)
{
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
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "debug.h"
+#include "env.h"
+#include "lock-contention.h"
+#include "machine.h"
+#include "symbol.h"
+
+#include <limits.h>
+#include <string.h>
+
+#include <linux/hash.h>
+#include <linux/zalloc.h>
+
+#define __lockhashfn(key) hash_long((unsigned long)key, LOCKHASH_BITS)
+#define lockhashentry(key) (lockhash_table + __lockhashfn((key)))
+
+struct callstack_filter {
+ struct list_head list;
+ char name[];
+};
+
+static LIST_HEAD(callstack_filters);
+struct hlist_head *lockhash_table;
+
+int parse_call_stack(const struct option *opt __maybe_unused, const char *str,
+ int unset __maybe_unused)
+{
+ char *s, *tmp, *tok;
+ int ret = 0;
+
+ s = strdup(str);
+ if (s == NULL)
+ return -1;
+
+ for (tok = strtok_r(s, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
+ struct callstack_filter *entry;
+
+ entry = malloc(sizeof(*entry) + strlen(tok) + 1);
+ if (entry == NULL) {
+ pr_err("Memory allocation failure\n");
+ free(s);
+ return -1;
+ }
+
+ strcpy(entry->name, tok);
+ list_add_tail(&entry->list, &callstack_filters);
+ }
+
+ free(s);
+ return ret;
+}
+
+bool needs_callstack(void)
+{
+ return !list_empty(&callstack_filters);
+}
+
+struct lock_stat *lock_stat_find(u64 addr)
+{
+ struct hlist_head *entry = lockhashentry(addr);
+ struct lock_stat *ret;
+
+ hlist_for_each_entry(ret, entry, hash_entry) {
+ if (ret->addr == addr)
+ return ret;
+ }
+ return NULL;
+}
+
+struct lock_stat *lock_stat_findnew(u64 addr, const char *name, int flags)
+{
+ struct hlist_head *entry = lockhashentry(addr);
+ struct lock_stat *ret, *new;
+
+ hlist_for_each_entry(ret, entry, hash_entry) {
+ if (ret->addr == addr)
+ return ret;
+ }
+
+ new = zalloc(sizeof(struct lock_stat));
+ if (!new)
+ goto alloc_failed;
+
+ new->addr = addr;
+ new->name = strdup(name);
+ if (!new->name) {
+ free(new);
+ goto alloc_failed;
+ }
+
+ new->flags = flags;
+ new->wait_time_min = ULLONG_MAX;
+
+ hlist_add_head(&new->hash_entry, entry);
+ return new;
+
+alloc_failed:
+ pr_err("memory allocation failed\n");
+ return NULL;
+}
+
+bool match_callstack_filter(struct machine *machine, u64 *callstack, int max_stack_depth)
+{
+ struct map *kmap;
+ struct symbol *sym;
+ u64 ip;
+ const char *arch = perf_env__arch(machine->env);
+
+ if (list_empty(&callstack_filters))
+ return true;
+
+ for (int i = 0; i < max_stack_depth; i++) {
+ struct callstack_filter *filter;
+
+ /*
+ * In powerpc, the callchain saved by kernel always includes
+ * first three entries as the NIP (next instruction pointer),
+ * LR (link register), and the contents of LR save area in the
+ * second stack frame. In certain scenarios its possible to have
+ * invalid kernel instruction addresses in either LR or the second
+ * stack frame's LR. In that case, kernel will store that address as
+ * zero.
+ *
+ * The below check will continue to look into callstack,
+ * incase first or second callstack index entry has 0
+ * address for powerpc.
+ */
+ if (!callstack || (!callstack[i] && (strcmp(arch, "powerpc") ||
+ (i != 1 && i != 2))))
+ break;
+
+ ip = callstack[i];
+ sym = machine__find_kernel_symbol(machine, ip, &kmap);
+ if (sym == NULL)
+ continue;
+
+ list_for_each_entry(filter, &callstack_filters, list) {
+ if (strstr(sym->name, filter->name))
+ return true;
+ }
+ }
+ return false;
+}
+
+#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 */
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-googOn 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
>
On Mon, Oct 21, 2024 at 11:36 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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?
I agree it could be in the header but I think it is better not to have
it in the header. The change is trying to move things into a new
util/lock-contention.c. We didn't have the C file in the past so
putting things in the header was okay. In general `static inline` is
something of a code smell, the static says make it local to every C
file leading to duplication, the inline doesn't actually mean inline
the code it means the symbol definition should be hidden. There's
generally plenty of confusion over static inline that it's best to
avoid its use, we don't want code in header files and let LTO resolve
performance issues - there are different arguments in particular
performance crucial code that relies on inlining, but this isn't it.
It also helps the debugger to place the code into the C file. There is
also a lessening of cognitive load when you read the header file which
is smaller and without #ifdefs. I was also hoping to reduce/remove the
scope of these functions if possible.
So yes, strictly this tidying isn't necessary but given I was creating
the C file and tidying lock_contention.h it made sense to me to clean
up these.
Thanks,
Ian
© 2016 - 2024 Red Hat, Inc.