[PATCH 2/2] bpf: fix have_attach_type() detection

[PATCH 2/2] bpf: fix have_attach_type() detection

[Kris Van Hees]

There are kernel versions that support the BPF_TRACE_FENTRY attach type
at program load, but do not support opening the attachment point (a
kernel symbol by BTF id) as a raw tracepoint.  The cause is that the
support for fentry as a raw tracepoint was initially only implemented
on x86_64.

We now test both program load *and* opening the raw tracepoint to know
if BPF_TRACE_FENTRY as attach type is supported.

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 libdtrace/dt_bpf.c      | 29 ++++++++++++++++++++++-------
 libdtrace/dt_prov_fbt.c |  2 ++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
index e50bb536..9ee32e8b 100644
--- a/libdtrace/dt_bpf.c
+++ b/libdtrace/dt_bpf.c
@@ -486,19 +486,34 @@ have_attach_type(enum bpf_prog_type ptype, enum bpf_attach_type atype,
 				BPF_RETURN()
 			};
 	dtrace_difo_t	dp;
-	int		fd;
+	int		pfd, tfd = -1;
 
 	dp.dtdo_buf = insns;
 	dp.dtdo_len = ARRAY_SIZE(insns);
 
-	fd = dt_bpf_prog_attach(ptype, atype, 0, btf_id, &dp, 0, NULL, 0);
-	/* If the program loads, we can use the attach type. */
-	if (fd > 0) {
-		close(fd);
-		return 1;
-	}
+	pfd = dt_bpf_prog_attach(ptype, atype, 0, btf_id, &dp, 0, NULL, 0);
+	/* If the program load fails, we cannot iuse the attach type. */
+	if (pfd < 0)
+		goto fail;
 
+	/*
+	 * If the program loads, we still need to verify that probe can be
+	 * opened as a raw tracepoint.  Some kernels allow the program load
+	 * but return -ENOTSUPP when you try to open the raw tracepoint.
+	 */
+	tfd = dt_bpf_raw_tracepoint_open(NULL, pfd);
+	if (tfd < 0)
+		goto fail;
+
+	close(tfd);
+	close(pfd);
+	return 1;
+
+fail:
 	/* Failed -> attach type not available to us */
+	if (pfd >= 0)
+		close(pfd);
+
 	return 0;
 }
 
diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index 1489275a..00f9174c 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -74,6 +74,8 @@ dt_provimpl_t			dt_rawfbt;
 static int populate(dtrace_hdl_t *dtp)
 {
 	dt_fbt = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? dt_fbt_fprobe : dt_fbt_kprobe;
+	dt_dprintf("fbt: Using %s implementation\n",
+		   BPF_HAS(dtp, BPF_FEAT_FENTRY) ? "fentry/fexit" : "kprobe");
 
 	if (dt_provider_create(dtp, dt_fbt.name, &dt_fbt, &pattr,
 			       NULL) == NULL ||
-- 
2.43.5

Re: [PATCH 2/2] bpf: fix have_attach_type() detection

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>

I'll try to run tests tonight.

In what order will patches be applied?  I vote for these two ahead of 
the 8-patch perf series.

dt_prov_fbt.c needs updated copyright year

Would it be possible/practical in the commit message to mention kernel 
commits or version numbers perhaps?  Not a big deal, perhaps.

On 3/17/25 14:41, Kris Van Hees wrote:
> There are kernel versions that support the BPF_TRACE_FENTRY attach type
> at program load, but do not support opening the attachment point (a
> kernel symbol by BTF id) as a raw tracepoint.  The cause is that the
> support for fentry as a raw tracepoint was initially only implemented
> on x86_64.
>
> We now test both program load *and* opening the raw tracepoint to know
> if BPF_TRACE_FENTRY as attach type is supported.
>
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
>   libdtrace/dt_bpf.c      | 29 ++++++++++++++++++++++-------
>   libdtrace/dt_prov_fbt.c |  2 ++
>   2 files changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index e50bb536..9ee32e8b 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -486,19 +486,34 @@ have_attach_type(enum bpf_prog_type ptype, enum bpf_attach_type atype,
>   				BPF_RETURN()
>   			};
>   	dtrace_difo_t	dp;
> -	int		fd;
> +	int		pfd, tfd = -1;
>   
>   	dp.dtdo_buf = insns;
>   	dp.dtdo_len = ARRAY_SIZE(insns);
>   
> -	fd = dt_bpf_prog_attach(ptype, atype, 0, btf_id, &dp, 0, NULL, 0);
> -	/* If the program loads, we can use the attach type. */
> -	if (fd > 0) {
> -		close(fd);
> -		return 1;
> -	}
> +	pfd = dt_bpf_prog_attach(ptype, atype, 0, btf_id, &dp, 0, NULL, 0);
> +	/* If the program load fails, we cannot iuse the attach type. */
> +	if (pfd < 0)
> +		goto fail;
>   
> +	/*
> +	 * If the program loads, we still need to verify that probe can be
> +	 * opened as a raw tracepoint.  Some kernels allow the program load
> +	 * but return -ENOTSUPP when you try to open the raw tracepoint.
> +	 */
> +	tfd = dt_bpf_raw_tracepoint_open(NULL, pfd);
> +	if (tfd < 0)
> +		goto fail;
> +
> +	close(tfd);
> +	close(pfd);
> +	return 1;
> +
> +fail:
>   	/* Failed -> attach type not available to us */
> +	if (pfd >= 0)
> +		close(pfd);
> +
>   	return 0;
>   }
>   
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> index 1489275a..00f9174c 100644
> --- a/libdtrace/dt_prov_fbt.c
> +++ b/libdtrace/dt_prov_fbt.c
> @@ -74,6 +74,8 @@ dt_provimpl_t			dt_rawfbt;
>   static int populate(dtrace_hdl_t *dtp)
>   {
>   	dt_fbt = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? dt_fbt_fprobe : dt_fbt_kprobe;
> +	dt_dprintf("fbt: Using %s implementation\n",
> +		   BPF_HAS(dtp, BPF_FEAT_FENTRY) ? "fentry/fexit" : "kprobe");
>   
>   	if (dt_provider_create(dtp, dt_fbt.name, &dt_fbt, &pattr,
>   			       NULL) == NULL ||

Re: [PATCH 2/2] bpf: fix have_attach_type() detection

[Kris Van Hees]

On Mon, Mar 17, 2025 at 04:16:01PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>

Thanks.

> I'll try to run tests tonight.
> 
> In what order will patches be applied?  I vote for these two ahead of the
> 8-patch perf series.

I really prefer the opposite order, mainly because the performance improvements
really help with speeding up the testsuite runs.

> dt_prov_fbt.c needs updated copyright year

The perf series handles that.

> Would it be possible/practical in the commit message to mention kernel
> commits or version numbers perhaps?  Not a big deal, perhaps.

Yes, because it is a bit of work to hunt it down, and not really relevant.

> On 3/17/25 14:41, Kris Van Hees wrote:
> > There are kernel versions that support the BPF_TRACE_FENTRY attach type
> > at program load, but do not support opening the attachment point (a
> > kernel symbol by BTF id) as a raw tracepoint.  The cause is that the
> > support for fentry as a raw tracepoint was initially only implemented
> > on x86_64.
> > 
> > We now test both program load *and* opening the raw tracepoint to know
> > if BPF_TRACE_FENTRY as attach type is supported.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> >   libdtrace/dt_bpf.c      | 29 ++++++++++++++++++++++-------
> >   libdtrace/dt_prov_fbt.c |  2 ++
> >   2 files changed, 24 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index e50bb536..9ee32e8b 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -486,19 +486,34 @@ have_attach_type(enum bpf_prog_type ptype, enum bpf_attach_type atype,
> >   				BPF_RETURN()
> >   			};
> >   	dtrace_difo_t	dp;
> > -	int		fd;
> > +	int		pfd, tfd = -1;
> >   	dp.dtdo_buf = insns;
> >   	dp.dtdo_len = ARRAY_SIZE(insns);
> > -	fd = dt_bpf_prog_attach(ptype, atype, 0, btf_id, &dp, 0, NULL, 0);
> > -	/* If the program loads, we can use the attach type. */
> > -	if (fd > 0) {
> > -		close(fd);
> > -		return 1;
> > -	}
> > +	pfd = dt_bpf_prog_attach(ptype, atype, 0, btf_id, &dp, 0, NULL, 0);
> > +	/* If the program load fails, we cannot iuse the attach type. */
> > +	if (pfd < 0)
> > +		goto fail;
> > +	/*
> > +	 * If the program loads, we still need to verify that probe can be
> > +	 * opened as a raw tracepoint.  Some kernels allow the program load
> > +	 * but return -ENOTSUPP when you try to open the raw tracepoint.
> > +	 */
> > +	tfd = dt_bpf_raw_tracepoint_open(NULL, pfd);
> > +	if (tfd < 0)
> > +		goto fail;
> > +
> > +	close(tfd);
> > +	close(pfd);
> > +	return 1;
> > +
> > +fail:
> >   	/* Failed -> attach type not available to us */
> > +	if (pfd >= 0)
> > +		close(pfd);
> > +
> >   	return 0;
> >   }
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > index 1489275a..00f9174c 100644
> > --- a/libdtrace/dt_prov_fbt.c
> > +++ b/libdtrace/dt_prov_fbt.c
> > @@ -74,6 +74,8 @@ dt_provimpl_t			dt_rawfbt;
> >   static int populate(dtrace_hdl_t *dtp)
> >   {
> >   	dt_fbt = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? dt_fbt_fprobe : dt_fbt_kprobe;
> > +	dt_dprintf("fbt: Using %s implementation\n",
> > +		   BPF_HAS(dtp, BPF_FEAT_FENTRY) ? "fentry/fexit" : "kprobe");
> >   	if (dt_provider_create(dtp, dt_fbt.name, &dt_fbt, &pattr,
> >   			       NULL) == NULL ||

Re: [PATCH 2/2] bpf: fix have_attach_type() detection

[Eugene Loh]

On 3/17/25 16:20, Kris Van Hees wrote:

> On Mon, Mar 17, 2025 at 04:16:01PM -0400, Eugene Loh wrote:
>
>> In what order will patches be applied?  I vote for these two ahead of the
>> 8-patch perf series.
> I really prefer the opposite order, mainly because the performance improvements
> really help with speeding up the testsuite runs.

Maybe it won't matter, but the question is what one is looking for when 
one patch set is in and the other is not.  One choice is that one gets 
bad tests results... but at least they'll be faster.  The other choice 
is that the test results will still be as slow as before, but at least 
they'll be meaningful.  I vote for the latter.  (I guess the other 
question is what platform one is talking about...)  Anyhow, I vote for 
correctness before speed, but maybe we'll only test all patches at once.

Re: [PATCH 2/2] bpf: fix have_attach_type() detection

[Kris Van Hees]

On Mon, Mar 17, 2025 at 09:36:56PM -0400, Eugene Loh wrote:
> On 3/17/25 16:20, Kris Van Hees wrote:
> 
> > On Mon, Mar 17, 2025 at 04:16:01PM -0400, Eugene Loh wrote:
> > 
> > > In what order will patches be applied?  I vote for these two ahead of the
> > > 8-patch perf series.
> > I really prefer the opposite order, mainly because the performance improvements
> > really help with speeding up the testsuite runs.
> 
> Maybe it won't matter, but the question is what one is looking for when one
> patch set is in and the other is not.  One choice is that one gets bad tests
> results... but at least they'll be faster.  The other choice is that the
> test results will still be as slow as before, but at least they'll be
> meaningful.  I vote for the latter.  (I guess the other question is what
> platform one is talking about...)  Anyhow, I vote for correctness before
> speed, but maybe we'll only test all patches at once.

There is not really an option here of having one patch set in and not the
other, so it doesn't really matter.  The perf series does not introduce any
new regressions as far as I can see, so there is no downside to applying it
first yet there is a significant benefit in terms of speeding up testing.  The
attach-type patches work around a problem with a range of kernels, which is a
problem that preceeds the perf-series already, and that fix will definitely be
in the tree right after the perf-series so the end result is good.

If easier, feel free to consider them altogether a single series rather than
two series.