diff mbox series

[bpf-next,v2,1/5] bpf: make MAX_BPF_FUNC_ARGS 14

Message ID 20230602065958.2869555-2-imagedong@tencent.com
State New
Headers show
Series [bpf-next,v2,1/5] bpf: make MAX_BPF_FUNC_ARGS 14 | expand

Commit Message

Menglong Dong June 2, 2023, 6:59 a.m. UTC
From: Menglong Dong <imagedong@tencent.com>

According to the current kernel version, below is a statistics of the
function arguments count:

argument count | FUNC_PROTO count
7              | 367
8              | 196
9              | 71
10             | 43
11             | 22
12             | 10
13             | 15
14             | 4
15             | 0
16             | 1

It's hard to statisics the function count, so I use FUNC_PROTO in the btf
of vmlinux instead. The function with 16 arguments is ZSTD_buildCTable(),
which I think can be ignored.

Therefore, let's make the maximum of function arguments count 14. It used
to be 12, but it seems that there is no harm to make it big enough.

Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/bpf.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Simon Horman June 3, 2023, 2:13 p.m. UTC | #1
On Fri, Jun 02, 2023 at 02:59:54PM +0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> According to the current kernel version, below is a statistics of the
> function arguments count:
> 
> argument count | FUNC_PROTO count
> 7              | 367
> 8              | 196
> 9              | 71
> 10             | 43
> 11             | 22
> 12             | 10
> 13             | 15
> 14             | 4
> 15             | 0
> 16             | 1
> 
> It's hard to statisics the function count, so I use FUNC_PROTO in the btf
> of vmlinux instead. The function with 16 arguments is ZSTD_buildCTable(),
> which I think can be ignored.
> 
> Therefore, let's make the maximum of function arguments count 14. It used
> to be 12, but it seems that there is no harm to make it big enough.
> 
> Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  include/linux/bpf.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index f58895830ada..8b997779faf7 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -961,10 +961,10 @@ enum bpf_cgroup_storage_type {
>  
>  #define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
>  
> -/* The longest tracepoint has 12 args.
> - * See include/trace/bpf_probe.h
> +/* The maximun number of the kernel function arguments.

Hi Menglong Dong,

as it looks like there will be a v3 anyway, please
consider correcting the spelling of maximum.

...
Menglong Dong June 5, 2023, 2:49 a.m. UTC | #2
On Sat, Jun 3, 2023 at 2:17 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jun 2, 2023 at 12:01 AM <menglong8.dong@gmail.com> wrote:
> >
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > According to the current kernel version, below is a statistics of the
> > function arguments count:
> >
> > argument count | FUNC_PROTO count
> > 7              | 367
> > 8              | 196
> > 9              | 71
> > 10             | 43
> > 11             | 22
> > 12             | 10
> > 13             | 15
> > 14             | 4
> > 15             | 0
> > 16             | 1
> >
> > It's hard to statisics the function count, so I use FUNC_PROTO in the btf
> > of vmlinux instead. The function with 16 arguments is ZSTD_buildCTable(),
> > which I think can be ignored.
> >
> > Therefore, let's make the maximum of function arguments count 14. It used
> > to be 12, but it seems that there is no harm to make it big enough.
>
> I think we're just fine at 12.
> People need to fix their code. ZSTD_buildCTable should be first in line.
> Passing arguments on the stack is not efficient from performance pov.

But we still need to keep this part:

@@ -2273,7 +2273,8 @@ bool btf_ctx_access(int off, int size, enum
bpf_access_type type,
 static inline bool bpf_tracing_ctx_access(int off, int size,
                                          enum bpf_access_type type)
 {
-       if (off < 0 || off >= sizeof(__u64) * MAX_BPF_FUNC_ARGS)
+       /* "+1" here is for FEXIT return value. */
+       if (off < 0 || off >= sizeof(__u64) * (MAX_BPF_FUNC_ARGS + 1))
                return false;
        if (type != BPF_READ)
                return false;

Isn't it? Otherwise, it will make that the maximum arguments
is 12 for FENTRY, but 11 for FEXIT, as FEXIT needs to store
the return value in ctx.

How about that we change bpf_tracing_ctx_access() into:

static inline bool bpf_tracing_ctx_access(int off, int size,
                      enum bpf_access_type type,
                      int max_args)

And the caller can pass MAX_BPF_FUNC_ARGS to
it on no-FEXIT, and 'MAX_BPF_FUNC_ARGS + 1'
on FEXIT.

What do you think?

Thanks!
Menglong Dong
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f58895830ada..8b997779faf7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -961,10 +961,10 @@  enum bpf_cgroup_storage_type {
 
 #define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
 
-/* The longest tracepoint has 12 args.
- * See include/trace/bpf_probe.h
+/* The maximun number of the kernel function arguments.
+ * Let's make it 14, for now.
  */
-#define MAX_BPF_FUNC_ARGS 12
+#define MAX_BPF_FUNC_ARGS 14
 
 /* The maximum number of arguments passed through registers
  * a single function may have.
@@ -2273,7 +2273,8 @@  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 static inline bool bpf_tracing_ctx_access(int off, int size,
 					  enum bpf_access_type type)
 {
-	if (off < 0 || off >= sizeof(__u64) * MAX_BPF_FUNC_ARGS)
+	/* "+1" here is for FEXIT return value. */
+	if (off < 0 || off >= sizeof(__u64) * (MAX_BPF_FUNC_ARGS + 1))
 		return false;
 	if (type != BPF_READ)
 		return false;