Re: [bpf-next PATCH 2/4] selftests/bpf: Drop python client/server in favor of threads

On Thu, Oct 29, 2020 at 11:13 AM Martin KaFai Lau [off-list ref] wrote:

On Thu, Oct 29, 2020 at 09:58:15AM -0700, Alexander Duyck wrote:
[ ... ]

[...]

quoted

quoted

Also, when looking closer at BPF_SOCK_OPS_STATE_CB in test_tcpbpf_kern.c,
it seems the map value "gp" is slapped together across multiple
TCP_CLOSE events which may be not easy to understand.

How about it checks different states: TCP_CLOSE, TCP_LAST_ACK,
and BPF_TCP_FIN_WAIT2.  Each of this state will update its own
values under "gp".  Something like this (only compiler tested on
top of patch 4):

diff --git i/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c w/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
index 7e92c37976ac..65b247b03dfc 100644
--- i/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
+++ w/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c

@@ -90,15 +90,14 @@ static void verify_result(int map_fd, int sock_map_fd)
              result.event_map, expected_events);

        ASSERT_EQ(result.bytes_received, 501, "bytes_received");
-       ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
+       ASSERT_EQ(result.bytes_acked, 1001, "bytes_acked");
        ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
        ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
        ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
        ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
-       ASSERT_EQ(result.num_listen, 1, "num_listen");
-
-       /* 3 comes from one listening socket + both ends of the connection */
-       ASSERT_EQ(result.num_close_events, 3, "num_close_events");
+       ASSERT_EQ(result.num_listen_close, 1, "num_listen");
+       ASSERT_EQ(result.num_last_ack, 1, "num_last_ack");
+       ASSERT_EQ(result.num_fin_wait2, 1, "num_fin_wait2");

        /* check setsockopt for SAVE_SYN */
        key = 0;

diff --git i/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c w/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c
index 3e6912e4df3d..2c5ffb50d6e0 100644
--- i/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c
+++ w/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c

@@ -55,9 +55,11 @@ int bpf_testcb(struct bpf_sock_ops *skops)
 {
        char header[sizeof(struct ipv6hdr) + sizeof(struct tcphdr)];
        struct bpf_sock_ops *reuse = skops;
+       struct tcpbpf_globals *gp;
        struct tcphdr *thdr;
        int good_call_rv = 0;
        int bad_call_rv = 0;
+       __u32 key_zero = 0;
        int save_syn = 1;
        int rv = -1;
        int v = 0;

@@ -155,26 +157,21 @@ int bpf_testcb(struct bpf_sock_ops *skops)
        case BPF_SOCK_OPS_RETRANS_CB:
                break;
        case BPF_SOCK_OPS_STATE_CB:
-               if (skops->args[1] == BPF_TCP_CLOSE) {
-                       __u32 key = 0;
-                       struct tcpbpf_globals g, *gp;
-
-                       gp = bpf_map_lookup_elem(&global_map, &key);
-                       if (!gp)
-                               break;
-                       g = *gp;
-                       if (skops->args[0] == BPF_TCP_LISTEN) {
-                               g.num_listen++;
-                       } else {
-                               g.total_retrans = skops->total_retrans;
-                               g.data_segs_in = skops->data_segs_in;
-                               g.data_segs_out = skops->data_segs_out;
-                               g.bytes_received = skops->bytes_received;
-                               g.bytes_acked = skops->bytes_acked;
-                       }
-                       g.num_close_events++;
-                       bpf_map_update_elem(&global_map, &key, &g,
-                                           BPF_ANY);
+               gp = bpf_map_lookup_elem(&global_map, &key_zero);
+               if (!gp)
+                       break;
+               if (skops->args[1] == BPF_TCP_CLOSE &&
+                   skops->args[0] == BPF_TCP_LISTEN) {
+                       gp->num_listen_close++;
+               } else if (skops->args[1] == BPF_TCP_LAST_ACK) {
+                       gp->total_retrans = skops->total_retrans;
+                       gp->data_segs_in = skops->data_segs_in;
+                       gp->data_segs_out = skops->data_segs_out;
+                       gp->bytes_received = skops->bytes_received;
+                       gp->bytes_acked = skops->bytes_acked;
+                       gp->num_last_ack++;
+               } else if (skops->args[1] == BPF_TCP_FIN_WAIT2) {
+                       gp->num_fin_wait2++;

I meant with the above change in "case BPF_SOCK_OPS_STATE_CB".
The retry-and-wait in tcpbpf_user.c can be avoided.

What may still be needed in tcpbpf_user.c is to use shutdown and
read-zero to ensure the sk has gone through those states before
calling verify_result().  Something like this [ uncompiled code again :) ]:

        /* Always send FIN from accept_fd first to
         * ensure it will go through FIN_WAIT_2.
         */
        shutdown(accept_fd, SHUT_WR);
        /* Ensure client_fd gets the FIN */
        err = read(client_fd, buf, sizeof(buf));
        if (CHECK(err != 0, "read-after-shutdown(client_fd):",
                  "err:%d errno:%d\n", err, errno))
                goto close_accept_fd;

        /* FIN sends from client_fd and it must be in LAST_ACK now */
        shutdown(client_fd, SHUT_WR);
        /* Ensure accept_fd gets the FIN-ACK.
         * accept_fd must have passed the FIN_WAIT2.
         */
        err = read(accept_fd, buf, sizeof(buf));
        if (CHECK(err != 0, "read-after-shutdown(accept_fd):",
                  "err:%d errno:%d\n", err, errno))
                goto close_accept_fd;

        close(server_fd);
        close(accept_fd);
        close(client_fd);

        /* All sk has gone through the states being tested.
         * check the results now.
         */
        verify_result(map_fd, sock_map_fd);

Okay. I think I see how that works then. Basically shutdown the write
on one end and read on the other expecting to hold until it forces us
out with a read of length 0 on the other end. Although I might just
use recv since that was the call being used to pull data from the
socket rather than read. I just need to make sure I perform it
starting with the shutdown on the accept end first so that it will
close first to avoid causing the received/acked to be swapped.

Thanks.

- Alex