Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Geliang Tang <geliang@kernel.org>]

On Tue, 2024-09-03 at 11:16 +0200, Matthieu Baerts wrote:
> On 03/09/2024 11:11, Matthieu Baerts wrote:
> > On 03/09/2024 10:04, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > This patch adds a "cgroup/getsockopt" way to inspect the subflows
> > > of a
> > > mptcp socket.
> > > 
> > > mptcp_for_each_stubflow() and other helpers related to
> > > list_dentry are
> > > added into progs/mptcp_bpf.h.
> > > 
> > > Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list
> > > and use
> > > bpf_core_cast to cast a pointer to tcp_sock for readonly. It will
> > > allow
> > > to inspect all the fields in a tcp_sock.
> > 
> > (...)
> > 
> > > diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > index 2e28f4a215b5..1053a795eb43 100644
> > > --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > @@ -4,6 +4,7 @@
> > >  
> > >  /* vmlinux.h, bpf_helpers.h and other 'define' */
> > >  #include "bpf_tracing_net.h"
> > > +#include "mptcp_bpf.h"
> > >  
> > >  char _license[] SEC("license") = "GPL";
> > >  
> > > @@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
> > >  
> > >  	return 1;
> > >  }
> > > +
> > > +static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
> > > +{
> > > +	struct mptcp_subflow_context *subflow;
> > > +	struct mptcp_sock *msk;
> > > +	int i = 0;
> > > +
> > > +	if (sk->protocol != IPPROTO_MPTCP) {
> > > +		bpf_printk("MPTCP Subflow: unexpected protocol
> > > %u", sk->protocol);
> > > +		return -1;
> > > +	}
> > 
> > Out of curiosity, why did you move this code here and in
> > _check_getsockopt_subflow_cc()?
> > 
> > I'm just surprised because recently, you tried to reduce the amount
> > of
> > duplicated code :)
> 
> While at it: it sounds better to have unique error messages, to know
> where was the error. Here, a few messages are the same, e.g.
> protocol,
> number of subflows, sk is null. Do not hesitate to add something to
> differentiate them: level & optname, a string prefix, the line
> number, etc.

No, I think we should drop all these "bpf_printk" messages. I checked
all other BPF selftests programs, no one use these error messages to
debug like this. Just "return 1" is enough:

        if (!sk || sk->protocol != IPPROTO_MPTCP)
                return 1;

If a BPF program fail, its own load log will show:

libbpf: prog '_getsockopt_subflow': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; int _getsockopt_subflow(struct bpf_sockopt *ctx) @
mptcp_subflow.c:136
0: (bf) r9 = r1                       ; R1=ctx() R9_w=ctx()
; struct bpf_sock *sk = ctx->sk; @ mptcp_subflow.c:138
1: (79) r7 = *(u64 *)(r9 +0)          ; R7_w=sock() R9_w=ctx()
; if (bpf_get_current_pid_tgid() >> 32 != pid) @ mptcp_subflow.c:141
2: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()

No need to add any line number by ourselves.

> 
> Cheers,
> Matt