| Message ID | 20230307032117.3461008-1-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | perf lock contention: Fix builtin detection | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hi Ian, On Mon, Mar 6, 2023 at 7:21 PM Ian Rogers <irogers@google.com> wrote: > > __has_builtin was passed the macro rather than the actual builtin > feature. Oh, I missed it's a macro define in tools/lib/bpf/bpf_core_read.h file. Looking at some BPF test codes, it seems you also need to check the clang compiler version. Please take a look at the file below: tools/testing/selftests/bpf/progs/test_core_reloc_type_base.c Thanks, Namhyung > > Fixes: 1bece1351c65 ("perf lock contention: Support old rw_semaphore type") > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/bpf_skel/lock_contention.bpf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c > index e6007eaeda1a..e422eee0f942 100644 > --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c > +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c > @@ -182,7 +182,7 @@ static inline struct task_struct *get_lock_owner(__u64 lock, __u32 flags) > struct mutex *mutex = (void *)lock; > owner = BPF_CORE_READ(mutex, owner.counter); > } else if (flags == LCB_F_READ || flags == LCB_F_WRITE) { > -#if __has_builtin(bpf_core_type_matches) > +#if __has_builtin(__builtin_preserve_type_info) > if (bpf_core_type_matches(struct rw_semaphore___old)) { > struct rw_semaphore___old *rwsem = (void *)lock; > owner = (unsigned long)BPF_CORE_READ(rwsem, owner); > -- > 2.40.0.rc0.216.gc4246ad0f0-goog >
On Mon, Mar 6, 2023 at 8:43 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Hi Ian, > > On Mon, Mar 6, 2023 at 7:21 PM Ian Rogers <irogers@google.com> wrote: > > > > __has_builtin was passed the macro rather than the actual builtin > > feature. > > Oh, I missed it's a macro define in tools/lib/bpf/bpf_core_read.h file. > > Looking at some BPF test codes, it seems you also need to check the > clang compiler version. Please take a look at the file below: > > tools/testing/selftests/bpf/progs/test_core_reloc_type_base.c > > Thanks, > Namhyung Hmm.. tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c: ... /* Support for the BPF_TYPE_MATCHES argument to the * __builtin_preserve_type_info builtin was added at some point during * development of clang 15 and it's what we require for this test. */ #if __has_builtin(__builtin_preserve_type_info) && __clang_major__ >= 15 ... I'm not sure we need to worry about development clang builds and just the __has_builtin is cleaner. Perhaps the BPF folks can chime in. Thanks, Ian > > > > Fixes: 1bece1351c65 ("perf lock contention: Support old rw_semaphore type") > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/bpf_skel/lock_contention.bpf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c > > index e6007eaeda1a..e422eee0f942 100644 > > --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c > > +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c > > @@ -182,7 +182,7 @@ static inline struct task_struct *get_lock_owner(__u64 lock, __u32 flags) > > struct mutex *mutex = (void *)lock; > > owner = BPF_CORE_READ(mutex, owner.counter); > > } else if (flags == LCB_F_READ || flags == LCB_F_WRITE) { > > -#if __has_builtin(bpf_core_type_matches) > > +#if __has_builtin(__builtin_preserve_type_info) > > if (bpf_core_type_matches(struct rw_semaphore___old)) { > > struct rw_semaphore___old *rwsem = (void *)lock; > > owner = (unsigned long)BPF_CORE_READ(rwsem, owner); > > -- > > 2.40.0.rc0.216.gc4246ad0f0-goog > >
On Mon, Mar 6, 2023 at 9:56 PM Ian Rogers <irogers@google.com> wrote: > > On Mon, Mar 6, 2023 at 8:43 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > Hi Ian, > > > > On Mon, Mar 6, 2023 at 7:21 PM Ian Rogers <irogers@google.com> wrote: > > > > > > __has_builtin was passed the macro rather than the actual builtin > > > feature. > > > > Oh, I missed it's a macro define in tools/lib/bpf/bpf_core_read.h file. > > > > Looking at some BPF test codes, it seems you also need to check the > > clang compiler version. Please take a look at the file below: > > > > tools/testing/selftests/bpf/progs/test_core_reloc_type_base.c > > > > Thanks, > > Namhyung > > Hmm.. > > tools/testing/selftests/bpf/progs/test_core_reloc_kernel.c: > ... > /* Support for the BPF_TYPE_MATCHES argument to the > * __builtin_preserve_type_info builtin was added at some point during > * development of clang 15 and it's what we require for this test. > */ > #if __has_builtin(__builtin_preserve_type_info) && __clang_major__ >= 15 > ... > > I'm not sure we need to worry about development clang builds and just > the __has_builtin is cleaner. Perhaps the BPF folks can chime in. My understanding is that __builtin_preserve_type_info is supported before. But clang 15 added BPF_TYPE_MATCHES which is used for bpf_core_type_matches(). So just checking the builtin would not work. Thanks, Namhyung
diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c index e6007eaeda1a..e422eee0f942 100644 --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c @@ -182,7 +182,7 @@ static inline struct task_struct *get_lock_owner(__u64 lock, __u32 flags) struct mutex *mutex = (void *)lock; owner = BPF_CORE_READ(mutex, owner.counter); } else if (flags == LCB_F_READ || flags == LCB_F_WRITE) { -#if __has_builtin(bpf_core_type_matches) +#if __has_builtin(__builtin_preserve_type_info) if (bpf_core_type_matches(struct rw_semaphore___old)) { struct rw_semaphore___old *rwsem = (void *)lock; owner = (unsigned long)BPF_CORE_READ(rwsem, owner);
__has_builtin was passed the macro rather than the actual builtin feature. Fixes: 1bece1351c65 ("perf lock contention: Support old rw_semaphore type") Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/bpf_skel/lock_contention.bpf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)