Re: [PATCH bpf-next v2 2/2] bpf: Require only one of cong_avoid() and cong_control() from a TCP CC

["Jörn-Thorben Hinz" <jthinz@mailbox.tu-berlin.de>]

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@mailbox.tu-berlin.de>
> > ---
> >  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.