-----Original Message----- From: Alexei Starovoitov alexei.starovoitov@gmail.com
On Fri, Mar 11, 2022 at 8:36 AM Maxim Mikityanskiy maximmi@nvidia.com wrote:
-----Original Message----- From: Alexei Starovoitov alexei.starovoitov@gmail.com 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.