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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 29, 2020 at 11:13 AM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Thu, Oct 29, 2020 at 09:58:15AM -0700, Alexander Duyck wrote:
> [ ... ]
>

[...]

> >
> > > 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



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux