Re: [PATCH bpf-next v4 21/21] tools/bpf: selftests: add bpf_iter selftests

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Yonghong Song <yhs@fb.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>,
	Network Development <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next v4 21/21] tools/bpf: selftests: add bpf_iter selftests
Date: Sun, 10 May 2020 09:14:59 -0700	[thread overview]
Message-ID: <CAADnVQKtSV=yjV9UjHQ5SRmAx9LB+ma6AOJstyCuAFbU0j40QA@mail.gmail.com> (raw)
In-Reply-To: <3b2b2e40-4e80-2a5d-e479-fc12a95162f2@fb.com>

On Sat, May 9, 2020 at 10:01 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 5/9/20 5:34 PM, Alexei Starovoitov wrote:
> > On Sat, May 09, 2020 at 10:59:23AM -0700, Yonghong Song wrote:
> >> +static volatile const __u32 ret1;
> >> +
> >> +SEC("iter/bpf_map")
> >> +int dump_bpf_map(struct bpf_iter__bpf_map *ctx)
> >> +{
> >> +    struct seq_file *seq = ctx->meta->seq;
> >> +    struct bpf_map *map = ctx->map;
> >> +    __u64 seq_num;
> >> +    int i, ret = 0;
> >> +
> >> +    if (map == (void *)0)
> >> +            return 0;
> >> +
> >> +    /* only dump map1_id and map2_id */
> >> +    if (map->id != map1_id && map->id != map2_id)
> >> +            return 0;
> >> +
> >> +    seq_num = ctx->meta->seq_num;
> >> +    if (map->id == map1_id) {
> >> +            map1_seqnum = seq_num;
> >> +            map1_accessed++;
> >> +    }
> >> +
> >> +    if (map->id == map2_id) {
> >> +            if (map2_accessed == 0) {
> >> +                    map2_seqnum1 = seq_num;
> >> +                    if (ret1)
> >> +                            ret = 1;
> >> +            } else {
> >> +                    map2_seqnum2 = seq_num;
> >> +            }
> >> +            map2_accessed++;
> >> +    }
> >> +
> >> +    /* fill seq_file buffer */
> >> +    for (i = 0; i < print_len; i++)
> >> +            bpf_seq_write(seq, &seq_num, sizeof(seq_num));
> >> +
> >> +    return ret;
> >> +}
> >
> > I couldn't find where 'return 1' behavior is documented clearly.
>
> It is in the commit comments:
>
> commit 15d83c4d7cef5c067a8b075ce59e97df4f60706e
> Author: Yonghong Song <yhs@fb.com>
> Date:   Sat May 9 10:59:00 2020 -0700
>
>      bpf: Allow loading of a bpf_iter program
> ...
>      The program return value must be 0 or 1 for now.
>        0 : successful, except potential seq_file buffer overflow
>            which is handled by seq_file reader.
>        1 : request to restart the same object
>
> Internally, bpf program returning 1 will translate
> show() return -EAGAIN and this error code will
> return to user space.
>
> I will add some comments in the code to
> document this behavior.
>
> > I think it's a workaround for overflow.
>
> This can be used for overflow but overflow already been taken
> care of by bpf_seq_read(). This is mostly used for other use
> cases:
>     - currently under RT-linux, bpf_seq_printf() may return
>       -EBUSY. In this case, bpf program itself can request
>       retrying the same object.
>     - for other conditions where bpf program itself wants
>       to retry the same object. For example, hash table full,
>       the bpf progam can return 1, in which case, user space
>       read() will receive -EAGAIN and may check and make room
>       for hash table and then read() again.
>
> > When bpf prog detects overflow it can request replay of the element?
>
> It can. But it can return 0 too since bpf_seq_read() handles
> this transparently.
>
> > What if it keeps returning 1 ? read() will never finish?
>
> The read() will finish and return -EAGAIN to user space.
> It is up to user space to decide whether to call read()
> again or not.

Ahh. Got it. So that EAGAIN returned by bpf_iter_run_prog()
propagates by bpf_seq_read() all the way to read() syscall.
Now I see it. Thanks for explaining.