On Wed, Feb 8, 2023 at 1:47 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> On Tue, Jan 31, 2023 at 9:54 AM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> >
> > On Mon, Jan 30, 2023 at 9:36 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> 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@xxxxxxxxx> 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 ?
