Date:   Mon, 20 Jun 2022 11:08:08 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Jörn-Thorben Hinz <jthinz@...lbox.tu-berlin.de>,
        Yonghong Song <yhs@...com>
Cc:     bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 3/5] selftests/bpf: Test a BPF CC writing
 sk_pacing_*

On Mon, Jun 20, 2022 at 09:06:13AM -0700, Yonghong Song wrote:
[ ... ]
> > > > a/tools/testing/selftests/bpf/progs/tcp_ca_write_sk_pacing.c
> > > > b/tools/testing/selftests/bpf/progs/tcp_ca_write_sk_pacing.c
> > > > new file mode 100644
> > > > index 000000000000..43447704cf0e
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/bpf/progs/tcp_ca_write_sk_pacing.c
> > > > @@ -0,0 +1,60 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +#include "vmlinux.h"
> > > > +
> > > > +#include <bpf/bpf_helpers.h>
> > > > +#include <bpf/bpf_tracing.h>
> > > > +
> > > > +char _license[] SEC("license") = "GPL";
> > > > +
> > > > +#define USEC_PER_SEC 1000000UL
> > > > +
> > > > +#define min(a, b) ((a) < (b) ? (a) : (b))
> > > > +
> > > > +static inline struct tcp_sock *tcp_sk(const struct sock *sk)
> > > > +{
> > > This helper is already available in bpf_tcp_helpers.h.
> > > Is there a reason not to use that one and redefine
> > > it in both patch 3 and 4?  The mss_cache and srtt_us can be added
> > > to bpf_tcp_helpers.h.  It will need another effort to move
> > > all selftest's bpf-cc to vmlinux.h.
> > I fully agree it’s not elegant to redefine tcp_sk() twice more.
> > 
> > It was between either using bpf_tcp_helpers.h and adding and
> > maintaining additional struct members there. Or using the (as I
> > understood it) more “modern” approach with vmlinux.h and redefining the
> > trivial tcp_sk(). I chose the later. Didn’t see a reason not to slowly
> > introduce vmlinux.h into the CA tests.
> > 
> > I had the same dilemma for the algorithm I’m implementing: Reuse
> > bpf_tcp_helpers.h from the kernel tree and extend it. Or use vmlinux.h
> > and copy only some of the (mostly trivial) helper functions. Also chose
> > the later here.
> > 
> > While doing the above, I also considered extracting the type
> > declarations from bpf_tcp_helpers.h into an, e.g.,
> > bpf_tcp_types_helper.h, keeping only the functions in
> > bpf_tcp_helpers.h. bpf_tcp_helpers.h could have been a base helper for
> > any BPF CA implementation then and used with either vmlinux.h or the
> > “old-school” includes. Similar to the way bpf_helpers.h is used. But at
> > that point, a bpf_tcp_types_helper.h could have probably just been
> > dropped for good and in favor of vmlinux.h. So I didn’t continue with
> > that.
I think a trimmed down bpf_tcp_helpers.h + vmlinux.h is good.  Basically
what Yonghong has suggested.  Not sure what you meant by 'old-school' includes.
I don't think it needs a new bpf_tcp_types_helper.h also. 

I think it makes sense to remove everything from bpf_tcp_helpers.h
that is already available from vmlinux.h.  bpf_tcp_helpers.h
should only have some macros and helpers left.  Then move
bpf_dctcp.c, bpf_cubic.c, and a few others under progs/ to
use vmlinux.h.  I haven't tried but it should be doable
from a quick look at bpf_cubic.c and bpf_dctcp.c.

I agree it is better to directly use the struct tcp_sock, inet_connection_sock,
inet_sock... from the vmlinux.h.  However, bpf_tcp_helpers.h does not
only have the struct and enum defined in the kernel.  It has some
helpers and macros (e.g. TCP_CONG_NEEDS_ECN, TCP_ECN_*) that are missing
from vmlinux.h.  These are actually used by the realistic bpf-tcp-cc like
bpf_cubic.c and bpf_dctcp.c.

The simple test in this patch is not a fully implemented bpf-tcp-cc and
it only needs to duplicate the tcp_sk() helper which looks ok at the beginning.
Later, this simple test will be copied-and-pasted to create another test.
These new tests may then need to duplicate more helpers and macros.
It was what I meant it needs separate patches to migrate all bpf-tcp-cc
tests to vmlinux.h.  Otherwise, when they are migrated to vmlinux.h later,
we have another pattern of tests that can be cleared up to remove
these helpers/macros duplication.

I don't mind to keep a duplicate tcp_sk() in this set for now
if you can do a follow up to move all bpf-tcp-cc tests
to this path (vmlinux.h + a trimmed down bpf_tcp_helpers.h) and then
remove the tcp_sk() duplication here.  This will be very useful.

> > 
> > Do you insist to use bpf_tcp_helpers.h instead of vmlinux.h? Or could
> > the described split into two headers make sense after all?
> 
> I prefer to use vmlinux.h. Eventually we would like to use vmlinux.h
> for progs which include bpf_tcp_healpers.h. Basically remove the struct
> definitions in bpf_tcp_helpers.h and replacing "bpf.h, stddef.h, tcp.h ..."
> with vmlinux.h. We may not be there yet, but that is the goal.
> 
> > 
> > (Will wait for your reply here before sending a v4.)

Powered by blists - more mailing lists