[bpf-next,v3,1/3] libbpf: Refactor libbpf_probe_bpf_helper

Commit Message

Tao Chen Jan. 24, 2025, 2:44 p.m. UTC

Extract the common part as probe_func_comm, which will be used in
both libbpf_probe_bpf_{helper, kfunc}

Signed-off-by: Tao Chen <[e-mail]>
---
 tools/lib/bpf/libbpf_probes.c | 38 ++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 12 deletions(-)

Comments

Jiri Olsa Jan. 24, 2025, 4:26 p.m. UTC | #1

On Fri, Jan 24, 2025 at 10:44:09PM +0800, Tao Chen wrote:
> Extract the common part as probe_func_comm, which will be used in
> both libbpf_probe_bpf_{helper, kfunc}
> 
> Signed-off-by: Tao Chen <[e-mail]>
> ---
>  tools/lib/bpf/libbpf_probes.c | 38 ++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> index 9dfbe7750f56..b73345977b4e 100644
> --- a/tools/lib/bpf/libbpf_probes.c
> +++ b/tools/lib/bpf/libbpf_probes.c
> @@ -413,22 +413,20 @@ int libbpf_probe_bpf_map_type(enum bpf_map_type map_type, const void *opts)
>  	return libbpf_err(ret);
>  }
>  
> -int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
> -			    const void *opts)
> +static int probe_func_comm(enum bpf_prog_type prog_type, struct bpf_insn insn,
> +			   char *accepted_msgs, size_t msgs_size)
>  {
>  	struct bpf_insn insns[] = {
> -		BPF_EMIT_CALL((__u32)helper_id),
> +		BPF_EXIT_INSN(),
>  		BPF_EXIT_INSN(),

I'd just keep above in libbpf_probe_bpf_helper and pass insns to probe_func_comm,
seems easier

jirka

>  	};
>  	const size_t insn_cnt = ARRAY_SIZE(insns);
> -	char buf[4096];
> -	int ret;
> +	int err;
>  
> -	if (opts)
> -		return libbpf_err(-EINVAL);
> +	insns[0] = insn;
>  
>  	/* we can't successfully load all prog types to check for BPF helper
> -	 * support, so bail out with -EOPNOTSUPP error
> +	 * and kfunc support, so bail out with -EOPNOTSUPP error
>  	 */
>  	switch (prog_type) {
>  	case BPF_PROG_TYPE_TRACING:
> @@ -440,10 +438,26 @@ int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helpe
>  		break;
>  	}
>  
> -	buf[0] = '\0';
> -	ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf));
> -	if (ret < 0)
> -		return libbpf_err(ret);
> +	accepted_msgs[0] = '\0';
> +	err = probe_prog_load(prog_type, insns, insn_cnt, accepted_msgs, msgs_size);
> +	if (err < 0)
> +		return libbpf_err(err);
> +
> +	return 0;
> +}
> +
> +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
> +			    const void *opts)
> +{
> +	char buf[4096];
> +	int ret;
> +
> +	if (opts)
> +		return libbpf_err(-EINVAL);
> +
> +	ret = probe_func_comm(prog_type, BPF_EMIT_CALL((__u32)helper_id), buf, sizeof(buf));
> +	if (ret)
> +		return ret;
>  
>  	/* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
>  	 * at all, it will emit something like "invalid func unknown#181".
> -- 
> 2.43.0
>

Andrii Nakryiko Jan. 24, 2025, 6:44 p.m. UTC | #2

On Fri, Jan 24, 2025 at 6:44 AM Tao Chen <[e-mail]> wrote:
>
> Extract the common part as probe_func_comm, which will be used in
> both libbpf_probe_bpf_{helper, kfunc}
>
> Signed-off-by: Tao Chen <[e-mail]>
> ---
>  tools/lib/bpf/libbpf_probes.c | 38 ++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> index 9dfbe7750f56..b73345977b4e 100644
> --- a/tools/lib/bpf/libbpf_probes.c
> +++ b/tools/lib/bpf/libbpf_probes.c
> @@ -413,22 +413,20 @@ int libbpf_probe_bpf_map_type(enum bpf_map_type map_type, const void *opts)
>         return libbpf_err(ret);
>  }
>
> -int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
> -                           const void *opts)
> +static int probe_func_comm(enum bpf_prog_type prog_type, struct bpf_insn insn,
> +                          char *accepted_msgs, size_t msgs_size)
>  {
>         struct bpf_insn insns[] = {
> -               BPF_EMIT_CALL((__u32)helper_id),
> +               BPF_EXIT_INSN(),
>                 BPF_EXIT_INSN(),
>         };
>         const size_t insn_cnt = ARRAY_SIZE(insns);
> -       char buf[4096];
> -       int ret;
> +       int err;
>
> -       if (opts)
> -               return libbpf_err(-EINVAL);
> +       insns[0] = insn;
>
>         /* we can't successfully load all prog types to check for BPF helper
> -        * support, so bail out with -EOPNOTSUPP error
> +        * and kfunc support, so bail out with -EOPNOTSUPP error
>          */
>         switch (prog_type) {
>         case BPF_PROG_TYPE_TRACING:

there isn't much logic that you will extract here besides this check
whether program type can even be successfully loaded, so I wouldn't
extract probe_func_comm(), but rather extract just the check:

static bool can_probe_prog_type(enum bpf_prog_type prog_type)
{
        /* we can't successfully load all prog types to check for BPF
helper/kfunc
         * support, so check this early and bail
         */
        switch (prog_type) {
            ...: return false
        default:
            return true;
}


And just check that can_probe_prog_type() inside
libbpf_probe_bpf_helper and libbpf_probe_bpf_kfunc

pw-bot: cr

> @@ -440,10 +438,26 @@ int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helpe
>                 break;
>         }
>
> -       buf[0] = '\0';
> -       ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf));
> -       if (ret < 0)
> -               return libbpf_err(ret);
> +       accepted_msgs[0] = '\0';
> +       err = probe_prog_load(prog_type, insns, insn_cnt, accepted_msgs, msgs_size);
> +       if (err < 0)
> +               return libbpf_err(err);
> +
> +       return 0;
> +}
> +
> +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
> +                           const void *opts)
> +{
> +       char buf[4096];
> +       int ret;
> +
> +       if (opts)
> +               return libbpf_err(-EINVAL);
> +
> +       ret = probe_func_comm(prog_type, BPF_EMIT_CALL((__u32)helper_id), buf, sizeof(buf));
> +       if (ret)
> +               return ret;
>
>         /* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
>          * at all, it will emit something like "invalid func unknown#181".
> --
> 2.43.0
>

Tao Chen Jan. 25, 2025, 2:52 p.m. UTC | #3

在 2025/1/25 00:26, Jiri Olsa 写道:
> On Fri, Jan 24, 2025 at 10:44:09PM +0800, Tao Chen wrote:
>> Extract the common part as probe_func_comm, which will be used in
>> both libbpf_probe_bpf_{helper, kfunc}
>>
>> Signed-off-by: Tao Chen <[e-mail]>
>> ---
>>   tools/lib/bpf/libbpf_probes.c | 38 ++++++++++++++++++++++++-----------
>>   1 file changed, 26 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
>> index 9dfbe7750f56..b73345977b4e 100644
>> --- a/tools/lib/bpf/libbpf_probes.c
>> +++ b/tools/lib/bpf/libbpf_probes.c
>> @@ -413,22 +413,20 @@ int libbpf_probe_bpf_map_type(enum bpf_map_type map_type, const void *opts)
>>   	return libbpf_err(ret);
>>   }
>>   
>> -int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
>> -			    const void *opts)
>> +static int probe_func_comm(enum bpf_prog_type prog_type, struct bpf_insn insn,
>> +			   char *accepted_msgs, size_t msgs_size)
>>   {
>>   	struct bpf_insn insns[] = {
>> -		BPF_EMIT_CALL((__u32)helper_id),
>> +		BPF_EXIT_INSN(),
>>   		BPF_EXIT_INSN(),
> 
> I'd just keep above in libbpf_probe_bpf_helper and pass insns to probe_func_comm,
> seems easier
> 
> jirka
> 

Hi jiri,
Thank you for your review, your suggestion seems better, i will
send it in v4.

>>   	};
>>   	const size_t insn_cnt = ARRAY_SIZE(insns);
>> -	char buf[4096];
>> -	int ret;
>> +	int err;
>>   
>> -	if (opts)
>> -		return libbpf_err(-EINVAL);
>> +	insns[0] = insn;
>>   
>>   	/* we can't successfully load all prog types to check for BPF helper
>> -	 * support, so bail out with -EOPNOTSUPP error
>> +	 * and kfunc support, so bail out with -EOPNOTSUPP error
>>   	 */
>>   	switch (prog_type) {
>>   	case BPF_PROG_TYPE_TRACING:
>> @@ -440,10 +438,26 @@ int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helpe
>>   		break;
>>   	}
>>   
>> -	buf[0] = '\0';
>> -	ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf));
>> -	if (ret < 0)
>> -		return libbpf_err(ret);
>> +	accepted_msgs[0] = '\0';
>> +	err = probe_prog_load(prog_type, insns, insn_cnt, accepted_msgs, msgs_size);
>> +	if (err < 0)
>> +		return libbpf_err(err);
>> +
>> +	return 0;
>> +}
>> +
>> +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
>> +			    const void *opts)
>> +{
>> +	char buf[4096];
>> +	int ret;
>> +
>> +	if (opts)
>> +		return libbpf_err(-EINVAL);
>> +
>> +	ret = probe_func_comm(prog_type, BPF_EMIT_CALL((__u32)helper_id), buf, sizeof(buf));
>> +	if (ret)
>> +		return ret;
>>   
>>   	/* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
>>   	 * at all, it will emit something like "invalid func unknown#181".
>> -- 
>> 2.43.0
>>

Tao Chen Jan. 25, 2025, 3:05 p.m. UTC | #4

在 2025/1/25 02:44, Andrii Nakryiko 写道:
> On Fri, Jan 24, 2025 at 6:44 AM Tao Chen <[e-mail]> wrote:
>>
>> Extract the common part as probe_func_comm, which will be used in
>> both libbpf_probe_bpf_{helper, kfunc}
>>
>> Signed-off-by: Tao Chen <[e-mail]>
>> ---
>>   tools/lib/bpf/libbpf_probes.c | 38 ++++++++++++++++++++++++-----------
>>   1 file changed, 26 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
>> index 9dfbe7750f56..b73345977b4e 100644
>> --- a/tools/lib/bpf/libbpf_probes.c
>> +++ b/tools/lib/bpf/libbpf_probes.c
>> @@ -413,22 +413,20 @@ int libbpf_probe_bpf_map_type(enum bpf_map_type map_type, const void *opts)
>>          return libbpf_err(ret);
>>   }
>>
>> -int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
>> -                           const void *opts)
>> +static int probe_func_comm(enum bpf_prog_type prog_type, struct bpf_insn insn,
>> +                          char *accepted_msgs, size_t msgs_size)
>>   {
>>          struct bpf_insn insns[] = {
>> -               BPF_EMIT_CALL((__u32)helper_id),
>> +               BPF_EXIT_INSN(),
>>                  BPF_EXIT_INSN(),
>>          };
>>          const size_t insn_cnt = ARRAY_SIZE(insns);
>> -       char buf[4096];
>> -       int ret;
>> +       int err;
>>
>> -       if (opts)
>> -               return libbpf_err(-EINVAL);
>> +       insns[0] = insn;
>>
>>          /* we can't successfully load all prog types to check for BPF helper
>> -        * support, so bail out with -EOPNOTSUPP error
>> +        * and kfunc support, so bail out with -EOPNOTSUPP error
>>           */
>>          switch (prog_type) {
>>          case BPF_PROG_TYPE_TRACING:
> 
> there isn't much logic that you will extract here besides this check
> whether program type can even be successfully loaded, so I wouldn't
> extract probe_func_comm(), but rather extract just the check:
> 
> static bool can_probe_prog_type(enum bpf_prog_type prog_type)
> {
>          /* we can't successfully load all prog types to check for BPF
> helper/kfunc
>           * support, so check this early and bail
>           */
>          switch (prog_type) {
>              ...: return false
>          default:
>              return true;
> }
> 
> 
> And just check that can_probe_prog_type() inside
> libbpf_probe_bpf_helper and libbpf_probe_bpf_kfunc
> 
> pw-bot: cr
> 

Hi Andrii,
Thank you for your review, jiri also suggested putting the insn part 
back into libbpf_bpf_probe_{helper, kfunc}, so I'll make the 
modifications as you suggested in v4.

>> @@ -440,10 +438,26 @@ int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helpe
>>                  break;
>>          }
>>
>> -       buf[0] = '\0';
>> -       ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf));
>> -       if (ret < 0)
>> -               return libbpf_err(ret);
>> +       accepted_msgs[0] = '\0';
>> +       err = probe_prog_load(prog_type, insns, insn_cnt, accepted_msgs, msgs_size);
>> +       if (err < 0)
>> +               return libbpf_err(err);
>> +
>> +       return 0;
>> +}
>> +
>> +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
>> +                           const void *opts)
>> +{
>> +       char buf[4096];
>> +       int ret;
>> +
>> +       if (opts)
>> +               return libbpf_err(-EINVAL);
>> +
>> +       ret = probe_func_comm(prog_type, BPF_EMIT_CALL((__u32)helper_id), buf, sizeof(buf));
>> +       if (ret)
>> +               return ret;
>>
>>          /* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
>>           * at all, it will emit something like "invalid func unknown#181".
>> --
>> 2.43.0
>>

Patch

diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index 9dfbe7750f56..b73345977b4e 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -413,22 +413,20 @@  int libbpf_probe_bpf_map_type(enum bpf_map_type map_type, const void *opts)
 	return libbpf_err(ret);
 }
 
-int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
-			    const void *opts)
+static int probe_func_comm(enum bpf_prog_type prog_type, struct bpf_insn insn,
+			   char *accepted_msgs, size_t msgs_size)
 {
 	struct bpf_insn insns[] = {
-		BPF_EMIT_CALL((__u32)helper_id),
+		BPF_EXIT_INSN(),
 		BPF_EXIT_INSN(),
 	};
 	const size_t insn_cnt = ARRAY_SIZE(insns);
-	char buf[4096];
-	int ret;
+	int err;
 
-	if (opts)
-		return libbpf_err(-EINVAL);
+	insns[0] = insn;
 
 	/* we can't successfully load all prog types to check for BPF helper
-	 * support, so bail out with -EOPNOTSUPP error
+	 * and kfunc support, so bail out with -EOPNOTSUPP error
 	 */
 	switch (prog_type) {
 	case BPF_PROG_TYPE_TRACING:
@@ -440,10 +438,26 @@  int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helpe
 		break;
 	}
 
-	buf[0] = '\0';
-	ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf));
-	if (ret < 0)
-		return libbpf_err(ret);
+	accepted_msgs[0] = '\0';
+	err = probe_prog_load(prog_type, insns, insn_cnt, accepted_msgs, msgs_size);
+	if (err < 0)
+		return libbpf_err(err);
+
+	return 0;
+}
+
+int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id,
+			    const void *opts)
+{
+	char buf[4096];
+	int ret;
+
+	if (opts)
+		return libbpf_err(-EINVAL);
+
+	ret = probe_func_comm(prog_type, BPF_EMIT_CALL((__u32)helper_id), buf, sizeof(buf));
+	if (ret)
+		return ret;
 
 	/* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id)
 	 * at all, it will emit something like "invalid func unknown#181".