Re: [PATCH v9 bpf-next 3/5] bpf: Add skb dynptrs

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <martin.lau@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v9 bpf-next 3/5] bpf: Add skb dynptrs
Date: Wed, 8 Feb 2023 15:22:50 -0800	[thread overview]
Message-ID: <CAADnVQJYOR7YMEFV7c1e4p8hvrEmoa3VA2wp0oJSgmuAjSF+EA@mail.gmail.com> (raw)
In-Reply-To: <CAJnrk1bxm3_QQFK_aqiApiu5vYC+z++jRj9HF2jO6a+WWkswpQ@mail.gmail.com>

On Wed, Feb 8, 2023 at 1:47 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Tue, Jan 31, 2023 at 9:54 AM Joanne Koong <joannelkoong@gmail.com> wrote:
> >
> > On Mon, Jan 30, 2023 at 9:36 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Jan 30, 2023 at 04:44:12PM -0800, Joanne Koong wrote:
> > > > On Sun, Jan 29, 2023 at 3:39 PM Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jan 27, 2023 at 11:17:01AM -0800, Joanne Koong wrote:
> [...]
> > > > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > > > index 6da78b3d381e..ddb47126071a 100644
> > > > > > --- a/net/core/filter.c
> > > > > > +++ b/net/core/filter.c
> > > > > > @@ -1684,8 +1684,8 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
> > > > > >               skb_postpull_rcsum(skb, skb_mac_header(skb), skb->mac_len);
> > > > > >  }
> > > > > >
> > > > > > -BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
> > > > > > -        const void *, from, u32, len, u64, flags)
> > > > > > +int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
> > > > > > +                       u32 len, u64 flags)
> > > > >
> > > > > This change is just to be able to call __bpf_skb_store_bytes() ?
> > > > > If so, it's unnecessary.
> > > > > See:
> > > > > BPF_CALL_4(sk_reuseport_load_bytes,
> > > > >            const struct sk_reuseport_kern *, reuse_kern, u32, offset,
> > > > >            void *, to, u32, len)
> > > > > {
> > > > >         return ____bpf_skb_load_bytes(reuse_kern->skb, offset, to, len);
> > > > > }
> > > > >
> > > >
> > > > There was prior feedback [0] that using four underscores to call a
> > > > helper function is confusing and makes it ungreppable
> > >
> > > There are plenty of ungreppable funcs in the kernel.
> > > Try finding where folio_test_dirty() is defined.
> > > mm subsystem is full of such 'features'.
> > > Not friendly for casual kernel code reader, but useful.
> > >
> > > Since quadruple underscore is already used in the code base
> > > I see no reason to sacrifice bpf_skb_load_bytes performance with extra call.
> >
> > I don't have a preference either way, I'll change it to use the
> > quadruple underscore in the next version
>
> I think we still need these extra __bpf_skb_store/load_bytes()
> functions, because BPF_CALL_x static inlines the
> bpf_skb_store/load_bytes helpers in net/core/filter.c, and we need to
> call these bpf_skb_store/load_bytes helpers from another file
> (kernel/bpf/helpers.c). I think the only other alternative is moving
> the BPF_CALL_x declaration of bpf_skb_store/load bytes to
> include/linux/filter.h, but I think having the extra
> __bpf_skb_store/load_bytes() is cleaner.

bpf_skb_load_bytes() is a performance critical function.
I'm worried about the cost of the extra call.
Will compiler be smart enough to inline __bpf_skb_load_bytes()
in both cases? Probably not if they're in different .c files.
Not sure how to solve it. Make it a static inline in skbuff.h ?