[PATCH 2/4] fbt: clean up fprobe/kprobe support

[PATCH 2/4] fbt: clean up fprobe/kprobe support

[Kris Van Hees]

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 libdtrace/dt_prov_fbt.c | 14 +++++++++++---
 libdtrace/dt_provider.c |  2 +-
 libdtrace/dt_provider.h |  3 +--
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index da288401..a60a2e14 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -57,6 +57,9 @@ static const dtrace_pattr_t	pattr = {
 { DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_ISA },
 };
 
+dt_provimpl_t			dt_fbt_fprobe;
+dt_provimpl_t			dt_fbt_kprobe;
+
 /*
  * Scan the PROBE_LIST file and add entry and return probes for every function
  * that is listed.
@@ -64,7 +67,6 @@ static const dtrace_pattr_t	pattr = {
 static int populate(dtrace_hdl_t *dtp)
 {
 	dt_provider_t		*prv;
-	dt_provimpl_t		*impl;
 	FILE			*f;
 	char			*buf = NULL;
 	char			*p;
@@ -73,9 +75,10 @@ static int populate(dtrace_hdl_t *dtp)
 	dtrace_syminfo_t	sip;
 	dtrace_probedesc_t	pd;
 
-	impl = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? &dt_fbt_fprobe : &dt_fbt_kprobe;
+	if (!BPF_HAS(dtp, BPF_FEAT_FENTRY))
+		dt_fbt = dt_fbt_kprobe;
 
-	prv = dt_provider_create(dtp, prvname, impl, &pattr, NULL);
+	prv = dt_provider_create(dtp, prvname, &dt_fbt, &pattr, NULL);
 	if (prv == NULL)
 		return -1;			/* errno already set */
 
@@ -463,3 +466,8 @@ dt_provimpl_t	dt_fbt_kprobe = {
 	.detach		= &kprobe_detach,
 	.probe_destroy	= &dt_tp_probe_destroy,
 };
+
+dt_provimpl_t	dt_fbt = {
+	.name		= prvname,
+	.populate	= &populate,
+};
diff --git a/libdtrace/dt_provider.c b/libdtrace/dt_provider.c
index ca8b53df..1e2e844e 100644
--- a/libdtrace/dt_provider.c
+++ b/libdtrace/dt_provider.c
@@ -30,7 +30,7 @@
 const dt_provimpl_t *dt_providers[] = {
 	&dt_dtrace,		/* list dt_dtrace first */
 	&dt_cpc,
-	&dt_fbt_fprobe,
+	&dt_fbt,
 	&dt_io,
 	&dt_ip,
 	&dt_lockstat,
diff --git a/libdtrace/dt_provider.h b/libdtrace/dt_provider.h
index 8f143dce..f62137de 100644
--- a/libdtrace/dt_provider.h
+++ b/libdtrace/dt_provider.h
@@ -76,8 +76,7 @@ typedef struct dt_provimpl {
 /* list dt_dtrace first */
 extern dt_provimpl_t dt_dtrace;
 extern dt_provimpl_t dt_cpc;
-extern dt_provimpl_t dt_fbt_fprobe;
-extern dt_provimpl_t dt_fbt_kprobe;
+extern dt_provimpl_t dt_fbt;
 extern dt_provimpl_t dt_io;
 extern dt_provimpl_t dt_ip;
 extern dt_provimpl_t dt_lockstat;
-- 
2.45.2

Re: [DTrace-devel] [PATCH 2/4] fbt: clean up fprobe/kprobe support

[Eugene Loh]

I wouldn't mind a few commit msg words on what "clean up" means.

Mostly, how/where is dt_fbt_fprobe used?  Put another way...

On 12/5/24 13:53, Kris Van Hees via DTrace-devel wrote:
> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> @@ -73,9 +75,10 @@ static int populate(dtrace_hdl_t *dtp)
>   	dtrace_syminfo_t	sip;
>   	dtrace_probedesc_t	pd;
>   
> -	impl = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? &dt_fbt_fprobe : &dt_fbt_kprobe;
> +	if (!BPF_HAS(dtp, BPF_FEAT_FENTRY))
> +		dt_fbt = dt_fbt_kprobe;

If the BPF_HAS() test passes, shouldn't we have basically 
dt_fbt=dt_fbt_fprobe?  In which case, one might arguably go back to the 
ternary op?

Re: [DTrace-devel] [PATCH 2/4] fbt: clean up fprobe/kprobe support

[Kris Van Hees]

On Thu, Dec 05, 2024 at 05:05:15PM -0500, Eugene Loh wrote:
> I wouldn't mind a few commit msg words on what "clean up" means.
> 
> Mostly, how/where is dt_fbt_fprobe used?  Put another way...
> 
> On 12/5/24 13:53, Kris Van Hees via DTrace-devel wrote:
> > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > @@ -73,9 +75,10 @@ static int populate(dtrace_hdl_t *dtp)
> >   	dtrace_syminfo_t	sip;
> >   	dtrace_probedesc_t	pd;
> > -	impl = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? &dt_fbt_fprobe : &dt_fbt_kprobe;
> > +	if (!BPF_HAS(dtp, BPF_FEAT_FENTRY))
> > +		dt_fbt = dt_fbt_kprobe;
> 
> If the BPF_HAS() test passes, shouldn't we have basically
> dt_fbt=dt_fbt_fprobe?  In which case, one might arguably go back to the
> ternary op?

Since dt_fbt is initialized as dt_fbt_fprobe (the default), this conditional
sets it to dt_fbt_kprobe if fprobes are not available.  I figured that was
obvious from the code.

Re: [DTrace-devel] [PATCH 2/4] fbt: clean up fprobe/kprobe support

[Eugene Loh]

On 12/5/24 17:17, Kris Van Hees wrote:

> On Thu, Dec 05, 2024 at 05:05:15PM -0500, Eugene Loh wrote:
>> I wouldn't mind a few commit msg words on what "clean up" means.
>>
>> Mostly, how/where is dt_fbt_fprobe used?  Put another way...
>>
>> On 12/5/24 13:53, Kris Van Hees via DTrace-devel wrote:
>>> diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
>>> @@ -73,9 +75,10 @@ static int populate(dtrace_hdl_t *dtp)
>>>    	dtrace_syminfo_t	sip;
>>>    	dtrace_probedesc_t	pd;
>>> -	impl = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? &dt_fbt_fprobe : &dt_fbt_kprobe;
>>> +	if (!BPF_HAS(dtp, BPF_FEAT_FENTRY))
>>> +		dt_fbt = dt_fbt_kprobe;
>> If the BPF_HAS() test passes, shouldn't we have basically
>> dt_fbt=dt_fbt_fprobe?  In which case, one might arguably go back to the
>> ternary op?
> Since dt_fbt is initialized as dt_fbt_fprobe (the default), this conditional
> sets it to dt_fbt_kprobe if fprobes are not available.  I figured that was
> obvious from the code.

Sorry, where should I look to see that dt_fbt is initialized to 
dt_fbt_fprobe?

Re: [DTrace-devel] [PATCH 2/4] fbt: clean up fprobe/kprobe support

[Kris Van Hees]

On Thu, Dec 05, 2024 at 05:58:04PM -0500, Eugene Loh wrote:
> On 12/5/24 17:17, Kris Van Hees wrote:
> 
> > On Thu, Dec 05, 2024 at 05:05:15PM -0500, Eugene Loh wrote:
> > > I wouldn't mind a few commit msg words on what "clean up" means.
> > > 
> > > Mostly, how/where is dt_fbt_fprobe used?  Put another way...
> > > 
> > > On 12/5/24 13:53, Kris Van Hees via DTrace-devel wrote:
> > > > diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
> > > > @@ -73,9 +75,10 @@ static int populate(dtrace_hdl_t *dtp)
> > > >    	dtrace_syminfo_t	sip;
> > > >    	dtrace_probedesc_t	pd;
> > > > -	impl = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? &dt_fbt_fprobe : &dt_fbt_kprobe;
> > > > +	if (!BPF_HAS(dtp, BPF_FEAT_FENTRY))
> > > > +		dt_fbt = dt_fbt_kprobe;
> > > If the BPF_HAS() test passes, shouldn't we have basically
> > > dt_fbt=dt_fbt_fprobe?  In which case, one might arguably go back to the
> > > ternary op?
> > Since dt_fbt is initialized as dt_fbt_fprobe (the default), this conditional
> > sets it to dt_fbt_kprobe if fprobes are not available.  I figured that was
> > obvious from the code.
> 
> Sorry, where should I look to see that dt_fbt is initialized to
> dt_fbt_fprobe?

Oh right - that was in the previous version.  But on OL7, gcc complained about
that assignment.  But I didn't complete the change in the patch I posted.

Let me post the corrct one (and yes, the ternary comes back).