On 02/23/2019 09:44 AM, Alexei Starovoitov wrote:
...
>
> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep();
> (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> +DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> +
> +#define BPF_PROG_RUN(prog, ctx) ({ \
> + u32 ret; \
> + cant_sleep(); \
> + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
> + struct bpf_prog_stats *stats; \
> + u64 start = sched_clock(); \
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + stats = this_cpu_ptr(prog->aux->stats); \
> + u64_stats_update_begin(&stats->syncp); \
> + stats->cnt++; \
> + stats->nsecs += sched_clock() - start; \
> + u64_stats_update_end(&stats->syncp); \
> + } else { \
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + } \
> + ret; })
>
It seems a cpu running there could still be interrupted (by an interrupt)
and re-enter this section ?
If yes, u64_stats_update_begin() and u64_stats_update_end() are unsafe (on
32bit arches)
u64_stats_update_{begin|end}() assume proper locking, since they use a simple
increment.
But then, even on 64bit arches, the stats->{cnt|nsecs} updates are unsafe ?
