[bpf-next v1] bpf: introduce stats update helper

[bpf-next v1] bpf: introduce stats update helper

[tong]

From: Tonghao Zhang <tong@infragraf.org>

This patch introduce a stats update helper to simplify codes.

Signed-off-by: Tonghao Zhang <tong@infragraf.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Song Liu <song@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Hou Tao <houtao1@huawei.com>
---
 include/linux/filter.h  | 22 +++++++++++++++-------
 kernel/bpf/trampoline.c | 10 +---------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 1727898f1641..582dfe1188e8 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -579,6 +579,20 @@ typedef unsigned int (*bpf_dispatcher_fn)(const void *ctx,
 					  unsigned int (*bpf_func)(const void *,
 								   const struct bpf_insn *));
 
+static inline void bpf_prog_update_stats(const struct bpf_prog *prog, u64 start)
+{
+	struct bpf_prog_stats *stats;
+	unsigned long flags;
+
+	stats = this_cpu_ptr(prog->stats);
+	flags = u64_stats_update_begin_irqsave(&stats->syncp);
+
+	u64_stats_inc(&stats->cnt);
+	u64_stats_add(&stats->nsecs, sched_clock() - start);
+
+	u64_stats_update_end_irqrestore(&stats->syncp, flags);
+}
+
 static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
 					  const void *ctx,
 					  bpf_dispatcher_fn dfunc)
@@ -587,16 +601,10 @@ static __always_inline u32 __bpf_prog_run(const struct bpf_prog *prog,
 
 	cant_migrate();
 	if (static_branch_unlikely(&bpf_stats_enabled_key)) {
-		struct bpf_prog_stats *stats;
 		u64 start = sched_clock();
-		unsigned long flags;
 
 		ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
-		stats = this_cpu_ptr(prog->stats);
-		flags = u64_stats_update_begin_irqsave(&stats->syncp);
-		u64_stats_inc(&stats->cnt);
-		u64_stats_add(&stats->nsecs, sched_clock() - start);
-		u64_stats_update_end_irqrestore(&stats->syncp, flags);
+		bpf_prog_update_stats(prog, start);
 	} else {
 		ret = dfunc(ctx, prog->insnsi, prog->bpf_func);
 	}
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index d0ed7d6f5eec..07bc7c9a18d5 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -885,8 +885,6 @@ static u64 notrace __bpf_prog_enter_recur(struct bpf_prog *prog, struct bpf_tram
 static void notrace update_prog_stats(struct bpf_prog *prog,
 				      u64 start)
 {
-	struct bpf_prog_stats *stats;
-
 	if (static_branch_unlikely(&bpf_stats_enabled_key) &&
 	    /* static_key could be enabled in __bpf_prog_enter*
 	     * and disabled in __bpf_prog_exit*.
@@ -894,13 +892,7 @@ static void notrace update_prog_stats(struct bpf_prog *prog,
 	     * Hence check that 'start' is valid.
 	     */
 	    start > NO_START_TIME) {
-		unsigned long flags;
-
-		stats = this_cpu_ptr(prog->stats);
-		flags = u64_stats_update_begin_irqsave(&stats->syncp);
-		u64_stats_inc(&stats->cnt);
-		u64_stats_add(&stats->nsecs, sched_clock() - start);
-		u64_stats_update_end_irqrestore(&stats->syncp, flags);
+		bpf_prog_update_stats(prog, start);
 	}
 }
 
-- 
2.27.0

RE: [bpf-next v1] bpf: introduce stats update helper

[John Fastabend]

tong@ wrote:
> From: Tonghao Zhang <tong@infragraf.org>
> 
> This patch introduce a stats update helper to simplify codes.
> 
> Signed-off-by: Tonghao Zhang <tong@infragraf.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Martin KaFai Lau <martin.lau@linux.dev>
> Cc: Song Liu <song@kernel.org>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Stanislav Fomichev <sdf@google.com>
> Cc: Hao Luo <haoluo@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Hou Tao <houtao1@huawei.com>
> ---
>  include/linux/filter.h  | 22 +++++++++++++++-------
>  kernel/bpf/trampoline.c | 10 +---------
>  2 files changed, 16 insertions(+), 16 deletions(-)

Seems fine but I'm not sure it makes much difference. I guess see if
Daniel, Andrii or Alexei want to take it or just drop it as random
code churn.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [bpf-next v1] bpf: introduce stats update helper

[Martin KaFai Lau]

On 2/3/23 7:47 PM, John Fastabend wrote:
> tong@ wrote:
>> From: Tonghao Zhang <tong@infragraf.org>
>>
>> This patch introduce a stats update helper to simplify codes.
>>
>> Signed-off-by: Tonghao Zhang <tong@infragraf.org>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Andrii Nakryiko <andrii@kernel.org>
>> Cc: Martin KaFai Lau <martin.lau@linux.dev>
>> Cc: Song Liu <song@kernel.org>
>> Cc: Yonghong Song <yhs@fb.com>
>> Cc: John Fastabend <john.fastabend@gmail.com>
>> Cc: KP Singh <kpsingh@kernel.org>
>> Cc: Stanislav Fomichev <sdf@google.com>
>> Cc: Hao Luo <haoluo@google.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Hou Tao <houtao1@huawei.com>
>> ---
>>   include/linux/filter.h  | 22 +++++++++++++++-------
>>   kernel/bpf/trampoline.c | 10 +---------
>>   2 files changed, 16 insertions(+), 16 deletions(-)
> 
> Seems fine but I'm not sure it makes much difference.

I also don't think it is needed. There are only two places. Also, it is not 
encouraged to collect more stats. Refactoring it is not going to help in the future.

Re: [bpf-next v1] bpf: introduce stats update helper

[Tonghao Zhang]

> On Feb 4, 2023, at 2:06 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote:
> 
> On 2/3/23 7:47 PM, John Fastabend wrote:
>> tong@ wrote:
>>> From: Tonghao Zhang <tong@infragraf.org>
>>> 
>>> This patch introduce a stats update helper to simplify codes.
>>> 
>>> Signed-off-by: Tonghao Zhang <tong@infragraf.org>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: Andrii Nakryiko <andrii@kernel.org>
>>> Cc: Martin KaFai Lau <martin.lau@linux.dev>
>>> Cc: Song Liu <song@kernel.org>
>>> Cc: Yonghong Song <yhs@fb.com>
>>> Cc: John Fastabend <john.fastabend@gmail.com>
>>> Cc: KP Singh <kpsingh@kernel.org>
>>> Cc: Stanislav Fomichev <sdf@google.com>
>>> Cc: Hao Luo <haoluo@google.com>
>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>> Cc: Hou Tao <houtao1@huawei.com>
>>> ---
>>>  include/linux/filter.h  | 22 +++++++++++++++-------
>>>  kernel/bpf/trampoline.c | 10 +---------
>>>  2 files changed, 16 insertions(+), 16 deletions(-)
>> Seems fine but I'm not sure it makes much difference.
> 
> I also don't think it is needed. There are only two places. Also, it is not encouraged to collect more stats. Refactoring it is not going to help in the future.
It only simply the codes, make the code more readable.
----
Best Regards, Tonghao <tong@infragraf.org>