On Mon, 2022-06-13 at 10:17 -0700, Martin KaFai Lau wrote:
> On Thu, Jun 09, 2022 at 10:47:02PM +0200, Jörn-Thorben Hinz wrote:
> > Remove the check for required and optional functions in a struct
> > tcp_congestion_ops from bpf_tcp_ca.c. Rely on
> > tcp_register_congestion_control() to reject a BPF CC that does not
> > implement all required functions, as it will do for a non-BPF CC.
> >
> > When a CC implements tcp_congestion_ops.cong_control(), the
> > alternate
> > cong_avoid() is not in use in the TCP stack. Previously, a BPF CC
> > was
> > still forced to implement cong_avoid() as a no-op since it was
> > non-optional in bpf_tcp_ca.c.
> >
> > Signed-off-by: Jörn-Thorben Hinz <jthinz@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > net/ipv4/bpf_tcp_ca.c | 32 --------------------------------
> > 1 file changed, 32 deletions(-)
> >
> > diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> > index 1f5c53ede4e5..39b64f317499 100644
> > --- a/net/ipv4/bpf_tcp_ca.c
> > +++ b/net/ipv4/bpf_tcp_ca.c
> > @@ -14,18 +14,6 @@
> > /* "extern" is to avoid sparse warning. It is only used in
> > bpf_struct_ops.c. */
> > extern struct bpf_struct_ops bpf_tcp_congestion_ops;
> >
> > -static u32 optional_ops[] = {
> > - offsetof(struct tcp_congestion_ops, init),
> > - offsetof(struct tcp_congestion_ops, release),
> > - offsetof(struct tcp_congestion_ops, set_state),
> > - offsetof(struct tcp_congestion_ops, cwnd_event),
> > - offsetof(struct tcp_congestion_ops, in_ack_event),
> > - offsetof(struct tcp_congestion_ops, pkts_acked),
> > - offsetof(struct tcp_congestion_ops, min_tso_segs),
> > - offsetof(struct tcp_congestion_ops, sndbuf_expand),
> > - offsetof(struct tcp_congestion_ops, cong_control),
> > -};
> > -
> > static u32 unsupported_ops[] = {
> > offsetof(struct tcp_congestion_ops, get_info),
> > };
> > @@ -51,18 +39,6 @@ static int bpf_tcp_ca_init(struct btf *btf)
> > return 0;
> > }
> >
> > -static bool is_optional(u32 member_offset)
> > -{
> > - unsigned int i;
> > -
> > - for (i = 0; i < ARRAY_SIZE(optional_ops); i++) {
> > - if (member_offset == optional_ops[i])
> > - return true;
> > - }
> > -
> > - return false;
> > -}
> > -
> > static bool is_unsupported(u32 member_offset)
> > {
> > unsigned int i;
> > @@ -268,14 +244,6 @@ static int bpf_tcp_ca_init_member(const struct
> > btf_type *t,
> > return 1;
> > }
> >
> > - if (!btf_type_resolve_func_ptr(btf_vmlinux, member->type,
> > NULL))
> > - return 0;
> > -
> > - /* Ensure bpf_prog is provided for compulsory func ptr */
> > - prog_fd = (int)(*(unsigned long *)(udata + moff));
> > - if (!prog_fd && !is_optional(moff) &&
> > !is_unsupported(moff))
> !is_unsupported() is still needed.
I don’t think it is necessary here.
There is also bpf_tcp_ca_check_member(), which also and only tests for
is_unsupported(). And check_member() will be called even earlier than
init_member() when loading a struct_ops. This deleted is_unsupported()
call should have been there only to test for missing but required &&
supported functions.
I added a test in v3 to make sure the check for the unsupported
get_info() (still) works.
Please correct me, if I misread/misfollowed the code paths!
>
> and remove 'int prog_fd' as reported by the test bot.
Of course, sorry about that … Oversight on my side.
>
> Test is still needed. You can copy the simpler "bpf_dctcp"
> to another tcp_congestion_ops. Write+read the sk_packing
> and also use .cong_control instead of .cong_avoid. I think rs-
> >acked_sacked
> is the 'delivered' and the 'ack' is not used.
Added tests in v3 of the patch series. I’m open for further feedback.
