On Thu, Mar 19, 2020 at 08:49:22PM -0700, Yonghong Song wrote:
>
>
> On 3/19/20 4:50 PM, Martin KaFai Lau wrote:
> > This patch adds test to exercise the bpf_sk_storage_get()
> > and bpf_sk_storage_delete() helper from the bpf_dctcp.c.
> >
> > Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
> > ---
> > .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 28 +++++++++++++++++--
> > tools/testing/selftests/bpf/progs/bpf_dctcp.c | 16 +++++++++++
> > 2 files changed, 41 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> > index 8482bbc67eec..9aaecce0bc3c 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
> > @@ -11,6 +11,7 @@
> > static const unsigned int total_bytes = 10 * 1024 * 1024;
> > static const struct timeval timeo_sec = { .tv_sec = 10 };
> > static const size_t timeo_optlen = sizeof(timeo_sec);
> > +static int expected_stg = 0xeB9F;
> > static int stop, duration;
> > static int settimeo(int fd)
> > @@ -88,7 +89,7 @@ static void *server(void *arg)
> > return NULL;
> > }
> > -static void do_test(const char *tcp_ca)
> > +static void do_test(const char *tcp_ca, const struct bpf_map *sk_stg_map)
> > {
> > struct sockaddr_in6 sa6 = {};
> > ssize_t nr_recv = 0, bytes = 0;
> > @@ -110,6 +111,14 @@ static void do_test(const char *tcp_ca)
> > return;
> > }
> > + if (sk_stg_map) {
> > + err = bpf_map_update_elem(bpf_map__fd(sk_stg_map), &fd,
> > + &expected_stg, BPF_NOEXIST);
> > + if (CHECK(err, "bpf_map_update_elem(sk_stg_map)",
> > + "err:%d errno:%d\n", err, errno))
> > + goto done;
> > + }
> > +
> > if (settcpca(lfd, tcp_ca) || settcpca(fd, tcp_ca) ||
> > settimeo(lfd) || settimeo(fd))
> > goto done;
> > @@ -149,6 +158,16 @@ static void do_test(const char *tcp_ca)
> > CHECK(bytes != total_bytes, "recv", "%zd != %u nr_recv:%zd errno:%d\n",
> > bytes, total_bytes, nr_recv, errno);
>
> Should the control go to "wait_thread" here if failure?
Thanks for the review!
I did think about that. I did not bail on this because the
sk_stg_map check below does not depend on the about check.
Hence, I didn't bail here. I moved the sk_stg_map test
to the very bottom is for this reason also.
Since you asked, I think it makes sense to go back to my first
approach which is to do the below test immediately after
connect() and just bail there.
I will also take this chance to postpone the thread creation
after connect().
>
> > + if (sk_stg_map) {
> > + int tmp_stg;
> > +
> > + err = bpf_map_lookup_elem(bpf_map__fd(sk_stg_map), &fd,
> > + &tmp_stg);
> > + CHECK(!err || errno != ENOENT,
> > + "bpf_map_lookup_elem(sk_stg_map)",
> > + "err:%d errno:%d\n", err, errno);
> > + }
> > +
> > wait_thread:
> > WRITE_ONCE(stop, 1);
> > pthread_join(srv_thread, &thread_ret);
> > @@ -175,7 +194,7 @@ static void test_cubic(void)
> > return;
> > }
> > - do_test("bpf_cubic");
> > + do_test("bpf_cubic", NULL);
> > bpf_link__destroy(link);
> > bpf_cubic__destroy(cubic_skel);
> > @@ -197,7 +216,10 @@ static void test_dctcp(void)
> > return;
> > }
> > - do_test("bpf_dctcp");
> > + do_test("bpf_dctcp", dctcp_skel->maps.sk_stg_map);
> > + CHECK(dctcp_skel->bss->stg_result != expected_stg,
> > + "Unexpected stg_result", "stg_result (%x) != expected_stg (%x)\n",
> > + dctcp_skel->bss->stg_result, expected_stg);
> > bpf_link__destroy(link);
> > bpf_dctcp__destroy(dctcp_skel);
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_dctcp.c b/tools/testing/selftests/bpf/progs/bpf_dctcp.c
> > index 127ea762a062..5c1fc584f3ae 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_dctcp.c
> > +++ b/tools/testing/selftests/bpf/progs/bpf_dctcp.c
> > @@ -6,6 +6,7 @@
> > * the kernel BPF logic.
> > */
> > +#include <stddef.h>
> > #include <linux/bpf.h>
> > #include <linux/types.h>
> > #include <bpf/bpf_helpers.h>
> > @@ -14,6 +15,15 @@
> > char _license[] SEC("license") = "GPL";
> > +static volatile int stg_result;
>
> "int stg_result = 0;" should work too.
will use.
