RE: [PATCH bpf-next v3 4/5] bpf: Add helpers to issue and check SYN cookies in XDP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
> 
> On Fri, Mar 11, 2022 at 8:36 AM Maxim Mikityanskiy <maximmi@xxxxxxxxxx>
> wrote:
> >
> > > -----Original Message-----
> > > From: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
> > > Sent: 27 February, 2022 05:25
> > >
> > > On Thu, Feb 24, 2022 at 05:11:44PM +0200, Maxim Mikityanskiy wrote:
> > > > @@ -7798,6 +7916,14 @@ xdp_func_proto(enum bpf_func_id func_id, const
> > > struct bpf_prog *prog)
> > > >             return &bpf_tcp_check_syncookie_proto;
> > > >     case BPF_FUNC_tcp_gen_syncookie:
> > > >             return &bpf_tcp_gen_syncookie_proto;
> > > > +   case BPF_FUNC_tcp_raw_gen_syncookie_ipv4:
> > > > +           return &bpf_tcp_raw_gen_syncookie_ipv4_proto;
> > > > +   case BPF_FUNC_tcp_raw_gen_syncookie_ipv6:
> > > > +           return &bpf_tcp_raw_gen_syncookie_ipv6_proto;
> > > > +   case BPF_FUNC_tcp_raw_check_syncookie_ipv4:
> > > > +           return &bpf_tcp_raw_check_syncookie_ipv4_proto;
> > > > +   case BPF_FUNC_tcp_raw_check_syncookie_ipv6:
> > > > +           return &bpf_tcp_raw_check_syncookie_ipv6_proto;
> > > >  #endif
> > >
> > > I understand that the main use case for new helpers is XDP specific,
> > > but why limit them to XDP?
> > > The feature looks generic and applicable to skb too.
> >
> > That sounds like an extra feature, rather than a limitation. That's out
> > of scope of what I planned to do.
> >
> > Besides, it sounds kind of useless to me, because the intention of the
> > new helpers is to accelerate synproxy, and I doubt BPF over SKBs will
> > accelerate anything. Maybe someone else has another use case for these
> > helpers and SKBs - in that case I leave the opportunity to add this
> > feature up to them.
> 
> This patchset will not be accepted until the feature is generalized
> to both xdp and skb and tested for both.
> "I dont have a use case for it" is not an excuse to narrow down the scope.

I'm not narrowing down the scope. It was defined from the very
beginning: accelerating synproxy with XDP. Asking me to add SKB support
is extending the scope and making me develop extra things.

It's a well-known fact that features aren't added to the kernel if there
is no user. Sometimes, features without users (or with few users) are
even removed. Could you elaborate why you require me to develop a
feature that no one is going to use?

In my opinion, the whole review process of this series has many
unhealthy points:

1. I'm constantly requested to increase the scope.

1.1. Split the helpers not related to the feature.

1.2. Add new functionality to the verifier to accept memory regions of
fixed size.

1.3. Add support for SKB.

2. I'm requested to rewrite the same code multiple times, every time in
a new way.

2.1. I had to rewrite the sample into a selftest, then into a different
kind of a selftest. It's not obvious that standalone selftests are
deprecated (am I blind, or is it not documented anywhere?), there are a
lot of such tests already existing. Instead of saying it from the
beginning that the test must use the test_progs runner, you waited until
I implement a standalone one.

3. I receive comments on old code that passed a few iterations of the
review.

3.1. SKB support was never part of the feature, and the series passed
two iterations, but not the third.

3.2. Standalone selftest was OK in v2, but suddenly got unacceptable in
v3.





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux