Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   KP Singh <kpsingh@chromium.org>
Date:   Wed, 4 Mar 2020 00:08:26 +0100
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     open list <linux-kernel@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Paul Turner <pjt@google.com>,
        Florent Revest <revest@chromium.org>,
        Brendan Jackman <jackmanb@chromium.org>
Subject: Re: [PATCH bpf-next 1/7] bpf: Refactor trampoline update code
Message-ID: <20200303230826.GA17103@chromium.org>
References: <20200303140950.6355-1-kpsingh@chromium.org>
 <20200303140950.6355-2-kpsingh@chromium.org>
 <CAEf4BzZj1+G7D2eZ9Enp_FtmmNPEkX7f6BDj2q=iZ1D8ZxxTMQ@mail.gmail.com>
 <20200303222433.GA3272@chromium.org>
 <CAEf4BzZey65RjDtAWojvtnakQgNiids4x8R-Hak6pZW1BqUfaQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEf4BzZey65RjDtAWojvtnakQgNiids4x8R-Hak6pZW1BqUfaQ@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 03-M?r 15:03, Andrii Nakryiko wrote:
> On Tue, Mar 3, 2020 at 2:24 PM KP Singh <kpsingh@chromium.org> wrote:
> >
> > On 03-M?r 14:12, Andrii Nakryiko wrote:
> > > On Tue, Mar 3, 2020 at 6:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > > >
> > > > From: KP Singh <kpsingh@google.com>
> > > >
> > > > As we need to introduce a third type of attachment for trampolines, the
> > > > flattened signature of arch_prepare_bpf_trampoline gets even more
> > > > complicated.
> > > >
> > > > Refactor the prog and count argument to arch_prepare_bpf_trampoline to
> > > > use bpf_tramp_progs to simplify the addition and accounting for new
> > > > attachment types.
> > > >
> > > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > > ---
> > > >  arch/x86/net/bpf_jit_comp.c | 31 +++++++++---------
> > > >  include/linux/bpf.h         | 13 ++++++--
> > > >  kernel/bpf/bpf_struct_ops.c | 13 +++++++-
> > > >  kernel/bpf/trampoline.c     | 63 +++++++++++++++++++++----------------
> > > >  4 files changed, 75 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > > index 9ba08e9abc09..15c7d28bc05c 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -1362,12 +1362,12 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
> > > >  }
> > > >
> > > >  static int invoke_bpf(const struct btf_func_model *m, u8 **pprog,
> > > > -                     struct bpf_prog **progs, int prog_cnt, int stack_size)
> > > > +                     struct bpf_tramp_progs *tp, int stack_size)
> > >
> > > nit: it's `tp` here, but `tprogs` in arch_prepare_bpf_trampoline. It's
> > > minor, but would be nice to stick to consistent naming.
> >
> > I did this to ~distinguish~ that rather than being an array of
> > tprogs it's a pointer to one of its members e.g.
> > &tprogs[BPF_TRAMP_FEXIT]).
> >
> > I change it if you feel this is not a valuable disntinction.
> 
> I think it's important distinction, but naming doesn't really help
> with it... Not sure how you can make it more clear, though.

I would prefer to keep the naming distinction. Hope that's okay with
you.

> >

[...]


> > >
> > count. Am I missing something :)
> 
> Ok, so it's setting entry 0 in bpf_tramp_progs->progs array, right?
> Wouldn't it be less mind-bending and confusing written this way:
> 
> tprogs[BPF_TRAMP_FENTRY].progs[0] = prog;

Definitely much cleaner/less mind bending :) Updated. Thanks!

- KP

> 
> ?
> 
> Syntax you used treats fixed-length progs array as a pointer, which is
> valid C, but not the best C either.
> 

[...]

> >
> > > [...]