On Fri, Mar 01, 2019 at 02:17:40PM -0800, Eric Dumazet wrote:
>
>
> On 03/01/2019 02:03 PM, Guenter Roeck wrote:
> > Hi,
> >
> > On Mon, Feb 25, 2019 at 02:28:39PM -0800, Alexei Starovoitov wrote:
> >> JITed BPF programs are indistinguishable from kernel functions, but unlike
> >> kernel code BPF code can be changed often.
> >> Typical approach of "perf record" + "perf report" profiling and tuning of
> >> kernel code works just as well for BPF programs, but kernel code doesn't
> >> need to be monitored whereas BPF programs do.
> >> Users load and run large amount of BPF programs.
> >> These BPF stats allow tools monitor the usage of BPF on the server.
> >> The monitoring tools will turn sysctl kernel.bpf_stats_enabled
> >> on and off for few seconds to sample average cost of the programs.
> >> Aggregated data over hours and days will provide an insight into cost of BPF
> >> and alarms can trigger in case given program suddenly gets more expensive.
> >>
> >> The cost of two sched_clock() per program invocation adds ~20 nsec.
> >> Fast BPF progs (like selftests/bpf/progs/test_pkt_access.c) will slow down
> >> from ~10 nsec to ~30 nsec.
> >> static_key minimizes the cost of the stats collection.
> >> There is no measurable difference before/after this patch
> >> with kernel.bpf_stats_enabled=0
> >>
> >
> > This patch causes my qemu tests for 'parisc' to crash. Reverting this patch
> > as well as "bpf: expose program stats via bpf_prog_info" fixes the problem.
> >
> > Crash log and bisect results are attached. Bisect ends with the merge;
> > I identified the two patches manually.
> >
> > I suspect that
> > prog->aux->stats = alloc_percpu_gfp(struct bpf_prog_stats, gfp_flags);
> > ...
> > u64_stats_init(&prog->aux->stats->syncp);
> > may be wrong. At the very least it looks odd, and I don't find a similar use
> > of u64_stats_init() anywhere else in the kernel.
>
> Yes, a loop is needed there.
>
> Something like :
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 1c14c347f3cfe1f7c0cf8a7eccff8135b16df81f..3f08c257858e1570339cd64a6351824bcc332ee3 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -109,6 +109,7 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
> {
> gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags;
> struct bpf_prog *prog;
> + int cpu;
>
> prog = bpf_prog_alloc_no_stats(size, gfp_extra_flags);
> if (!prog)
> @@ -121,7 +122,12 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
> return NULL;
> }
>
> - u64_stats_init(&prog->aux->stats->syncp);
> + for_each_possible_cpu(cpu) {
> + struct bpf_prog_stats *pstats;
> +
> + pstats = per_cpu_ptr(prog->aux->stats, cpu);
> + u64_stats_init(&pstats->syncp);
> + }
> return prog;
> }
> EXPORT_SYMBOL_GPL(bpf_prog_alloc);
>
Yes, that works, or at least my test no longer crashes after applying the
above patch. Feel free to add
Tested-by: Guenter Roeck <linux@xxxxxxxxxxxx>
Thanks,
Guenter
