Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Johan Almbladh <johan.almbladh@anyfinetworks.com>
To: Yonghong Song <yhs@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Tony Ambardar <Tony.Ambardar@gmail.com>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	kernel test robot <lkp@intel.com>
Subject: Re: [PATCH 14/14] bpf/tests: Add tail call test suite
Date: Thu, 29 Jul 2021 22:44:14 +0200	[thread overview]
Message-ID: <CAM1=_QT_5A=WBk9gzZCxtsL52DnLbG=W-5EphzikzvYhV59iwQ@mail.gmail.com> (raw)
In-Reply-To: <1483fad6-709a-50f5-4b8e-358ad2848dfe@fb.com>

On Thu, Jul 29, 2021 at 4:56 AM Yonghong Song <yhs@fb.com> wrote:
> > +static __init int prepare_tail_call_tests(struct bpf_array **pprogs)
> > +{
> > +     struct bpf_array *progs;
> > +     int ntests = ARRAY_SIZE(tail_call_tests);
> > +     int which, err;
>
> reverse christmas tree?

Will do.

> > +
> > +     /* Allocate the table of programs to be used for tall calls */
> > +     progs = kzalloc(sizeof(*progs) + (ntests + 1) * sizeof(progs->ptrs[0]),
> > +                     GFP_KERNEL);
> > +     if (!progs)
> > +             goto out_nomem;
> > +
> > +     /* Create all eBPF programs and populate the table */
> > +     for (which = 0; which < ntests; which++) {
> > +             struct tail_call_test *test = &tail_call_tests[which];
> > +             struct bpf_prog *fp;
> > +             int len, i;
> > +
> > +             /* Compute the number of program instructions */
> > +             for (len = 0; len < MAX_INSNS; len++) {
> > +                     struct bpf_insn *insn = &test->insns[len];
> > +
> > +                     if (len < MAX_INSNS - 1 &&
> > +                         insn->code == (BPF_LD | BPF_DW | BPF_IMM))
> > +                             len++;
> > +                     if (insn->code == 0)
> > +                             break;
> > +             }
> > +
> > +             /* Allocate and initialize the program */
> > +             fp = bpf_prog_alloc(bpf_prog_size(len), 0);
> > +             if (!fp)
> > +                     goto out_nomem;
> > +
> > +             fp->len = len;
> > +             fp->type = BPF_PROG_TYPE_SOCKET_FILTER;
> > +             fp->aux->stack_depth = test->stack_depth;
> > +             memcpy(fp->insnsi, test->insns, len * sizeof(struct bpf_insn));
> > +
> > +             /* Relocate runtime tail call offsets and addresses */
> > +             for (i = 0; i < len; i++) {
> > +                     struct bpf_insn *insn = &fp->insnsi[i];
> > +                     int target;
> > +
> > +                     if (insn->imm != TAIL_CALL_MARKER)
> > +                             continue;
> > +
> > +                     switch (insn->code) {
> > +                     case BPF_LD | BPF_DW | BPF_IMM:
> > +                             if (insn->dst_reg == R2) {
>
> Looks like the above condition is not needed. It is always true.
>
> > +                                     insn[0].imm = (u32)(long)progs;
> > +                                     insn[1].imm = ((u64)(long)progs) >> 32;
> > +                             }
> > +                             break;
> > +
> > +                     case BPF_ALU | BPF_MOV | BPF_K:
> > +                     case BPF_ALU64 | BPF_MOV | BPF_K:
>
> case BPF_ALU64 | BPF_MOV | BPF_K is not needed.
>
> > +                             if (insn->off == TAIL_CALL_NULL)
> > +                                     target = ntests;
> > +                             else
> > +                                     target = which + insn->off;
> > +                             if (insn->dst_reg == R3)
>
> the same here, insn->dst_reg == R3 is not needed. It is always true.

I added the register checks to further restrict the cases when
rewriting is done, but it might be more clear if the instruction is
always rewritten whenever the tail call marker is set. I can remove
the unnecessary conditions.

> I suggest to set insn->off = 0. Otherwise, it is an illegal insn.
> We won't issue here because we didn't invoke verifier. It is still
> good to make the insn legel.

I agree. Fixing it.

      reply	other threads:[~2021-07-29 20:44 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-28 17:04 [PATCH 00/14] bpf/tests: Extend the eBPF " Johan Almbladh
2021-07-28 17:04 ` [PATCH 01/14] bpf/tests: Add BPF_JMP32 test cases Johan Almbladh
2021-07-28 22:31   ` Yonghong Song
2021-07-29 21:30     ` Johan Almbladh
2021-07-28 17:04 ` [PATCH 02/14] bpf/tests: Add BPF_MOV tests for zero and sign extension Johan Almbladh
2021-07-28 22:36   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 03/14] bpf/tests: Fix typos in test case descriptions Johan Almbladh
2021-07-28 22:43   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 04/14] bpf/tests: Add more tests of ALU32 and ALU64 bitwise operations Johan Almbladh
2021-07-28 22:53   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 05/14] bpf/tests: Add more ALU32 tests for BPF_LSH/RSH/ARSH Johan Almbladh
2021-07-28 22:57   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 06/14] bpf/tests: Add more BPF_LSH/RSH/ARSH tests for ALU64 Johan Almbladh
2021-07-28 23:30   ` Yonghong Song
2021-07-29 12:34     ` Johan Almbladh
2021-07-29 15:39       ` Yonghong Song
2021-07-28 17:04 ` [PATCH 07/14] bpf/tests: Add more ALU64 BPF_MUL tests Johan Almbladh
2021-07-28 23:32   ` Yonghong Song
2021-07-29 21:21     ` Johan Almbladh
2021-07-28 17:04 ` [PATCH 08/14] bpf/tests: Add tests for ALU operations implemented with function calls Johan Almbladh
2021-07-28 23:52   ` Yonghong Song
2021-07-29 21:17     ` Johan Almbladh
2021-07-29 22:54       ` Yonghong Song
2021-07-28 17:04 ` [PATCH 09/14] bpf/tests: Add word-order tests for load/store of double words Johan Almbladh
2021-07-28 23:54   ` Yonghong Song
2021-07-28 17:04 ` [PATCH 10/14] bpf/tests: Add branch conversion JIT test Johan Almbladh
2021-07-28 23:58   ` Yonghong Song
2021-07-29 12:45     ` Johan Almbladh
2021-07-29 15:46       ` Yonghong Song
2021-07-29  0:55   ` Yonghong Song
2021-07-29 13:24     ` Johan Almbladh
2021-07-29 15:50       ` Yonghong Song
2021-07-28 17:04 ` [PATCH 11/14] bpf/tests: Add test for 32-bit context pointer argument passing Johan Almbladh
2021-07-29  0:09   ` Yonghong Song
2021-07-29 13:29     ` Johan Almbladh
2021-07-29 15:50       ` Yonghong Song
2021-07-28 17:05 ` [PATCH 12/14] bpf/tests: Add tests for atomic operations Johan Almbladh
2021-07-29  0:36   ` Yonghong Song
2021-07-28 17:05 ` [PATCH 13/14] bpf/tests: Add tests for BPF_CMPXCHG Johan Almbladh
2021-07-29  0:45   ` Yonghong Song
2021-07-28 17:05 ` [PATCH 14/14] bpf/tests: Add tail call test suite Johan Almbladh
2021-07-29  2:56   ` Yonghong Song
2021-07-29 20:44     ` Johan Almbladh [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAM1=_QT_5A=WBk9gzZCxtsL52DnLbG=W-5EphzikzvYhV59iwQ@mail.gmail.com' \
    --to=johan.almbladh@anyfinetworks.com \
    --cc=Tony.Ambardar@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=lkp@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH 14/14] bpf/tests: Add tail call test suite' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox