Re: [PATCH bpf-next 1/2] selftests/bpf: add loop test 4

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Sun, Aug 04, 2019 at 05:29:42AM +0000, Yonghong Song wrote:
> 
> 
> On 8/2/19 4:33 PM, Alexei Starovoitov wrote:
> > Add a test that returns a 'random' number between [0, 2^20)
> > If state pruning is not working correctly for loop body the number of
> > processed insns will be 2^20 * num_of_insns_in_loop_body and the program
> > will be rejected.
> 
> The maximum processed insns will be 2^20 or 2^20 * 
> num_of_insns_in_loop_body? 

the later.

> I thought the verifier will
> stop processing once processed insns reach 1M?

right.

> Could you elaborate which potential issues in verifier
> you try to cover with this test case? Extra tests are
> always welcome. We already have scale/loop tests and some
> (e.g., strobemeta tests) are more complex than this one.
> Maybe you have something in mind for this particular
> test? Putting in the commit message may help people understand
> the concerns.

it's not testing any new functionality.
It's more targeted test that pruning happens in loop body due to precision tracking.
When precision tracking is off the pruning won't be happening and 1m will be hit.
Since it's a small test comparing to others it's easier to analyze.

2^20 is to make sure 1m will be hit even if loop body is 1 insn.
For the given llmv the loop body is 16 insn, but it may vary.

> > 
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> >   .../bpf/prog_tests/bpf_verif_scale.c          |  1 +
> >   tools/testing/selftests/bpf/progs/loop4.c     | 23 +++++++++++++++++++
> >   2 files changed, 24 insertions(+)
> >   create mode 100644 tools/testing/selftests/bpf/progs/loop4.c
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> > index b4be96162ff4..757e39540eda 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
> > @@ -71,6 +71,7 @@ void test_bpf_verif_scale(void)
> >   
> >   		{ "loop1.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
> >   		{ "loop2.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
> > +		{ "loop4.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
> 
> The program is more like a BPF_PROG_TYPE_SCHED_CLS type than
> a BPF_PROG_TYPE_RAW_TRACEPOINT?

right. that's more accurate.

> >   
> >   		/* partial unroll. 19k insn in a loop.
> >   		 * Total program size 20.8k insn.
> > diff --git a/tools/testing/selftests/bpf/progs/loop4.c b/tools/testing/selftests/bpf/progs/loop4.c
> > new file mode 100644
> > index 000000000000..3e7ee14fddbd
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/loop4.c
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019 Facebook
> > +#include <linux/sched.h>
> > +#include <linux/ptrace.h>
> 
> Since the program is a networking type,
> the above two headers are probably unneeded.

yeah. just a copy paste. will remove

> > +#include <stdint.h>
> > +#include <stddef.h>
> > +#include <stdbool.h>
> > +#include <linux/bpf.h>
> > +#include "bpf_helpers.h"
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("socket")
> > +int combinations(volatile struct __sk_buff* skb)
> > +{
> > +	int ret = 0, i;
> > +
> > +#pragma nounroll
> > +	for (i = 0; i < 20; i++)
> > +		if (skb->len)
> > +			ret |= 1 << i;
> > +	return ret;
> > +}
> >