| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJ8uoz1QtRzeo8DxrpujjHGoPWd8FJASUWjfNrROuaJOCw+ZGA@mail.gmail.com>
Date: Thu, 21 Sep 2023 09:01:22 +0200
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Tushar Vyavahare <tushar.vyavahare@...el.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, bjorn@...nel.org,
magnus.karlsson@...el.com, maciej.fijalkowski@...el.com,
jonathan.lemon@...il.com, davem@...emloft.net, kuba@...nel.org,
pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net,
tirthendu.sarkar@...el.com
Subject: Re: [PATCH bpf-next 6/8] selftests/xsk: iterate over all the sockets
in the send pkts function
On Mon, 18 Sept 2023 at 11:15, Tushar Vyavahare
<tushar.vyavahare@...el.com> wrote:
>
> Update send_pkts() to handle multiple sockets for sending packets.
> Multiple TX sockets are utilized alternately based on the batch size for
> improve packet transmission.
I do not know if it is "improved" ;-), but it is good to test sending
from multiple sockets. Please make that clearer.
> Signed-off-by: Tushar Vyavahare <tushar.vyavahare@...el.com>
> ---
> tools/testing/selftests/bpf/xskxceiver.c | 64 +++++++++++++++++-------
> 1 file changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index e67032f04a74..0ef0575c095c 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -1204,13 +1204,13 @@ static int receive_pkts(struct test_spec *test)
> return TEST_PASS;
> }
>
> -static int __send_pkts(struct ifobject *ifobject, struct pollfd *fds, bool timeout)
> +static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk, bool timeout)
> {
> u32 i, idx = 0, valid_pkts = 0, valid_frags = 0, buffer_len;
> - struct pkt_stream *pkt_stream = ifobject->xsk->pkt_stream;
> - struct xsk_socket_info *xsk = ifobject->xsk;
> + struct pkt_stream *pkt_stream = xsk->pkt_stream;
> struct xsk_umem_info *umem = ifobject->umem;
> bool use_poll = ifobject->use_poll;
> + struct pollfd fds = { };
> int ret;
>
> buffer_len = pkt_get_buffer_len(umem, pkt_stream->max_pkt_len);
> @@ -1222,9 +1222,12 @@ static int __send_pkts(struct ifobject *ifobject, struct pollfd *fds, bool timeo
> return TEST_CONTINUE;
> }
>
> + fds.fd = xsk_socket__fd(xsk->xsk);
> + fds.events = POLLOUT;
> +
> while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) < BATCH_SIZE) {
> if (use_poll) {
> - ret = poll(fds, 1, POLL_TMOUT);
> + ret = poll(&fds, 1, POLL_TMOUT);
> if (timeout) {
> if (ret < 0) {
> ksft_print_msg("ERROR: [%s] Poll error %d\n",
> @@ -1303,7 +1306,7 @@ static int __send_pkts(struct ifobject *ifobject, struct pollfd *fds, bool timeo
> xsk->outstanding_tx += valid_frags;
>
> if (use_poll) {
> - ret = poll(fds, 1, POLL_TMOUT);
> + ret = poll(&fds, 1, POLL_TMOUT);
> if (ret <= 0) {
> if (ret == 0 && timeout)
> return TEST_PASS;
> @@ -1349,27 +1352,50 @@ static int wait_for_tx_completion(struct xsk_socket_info *xsk)
> return TEST_PASS;
> }
>
> +bool all_packets_sent(struct test_spec *test, unsigned long *bitmap)
> +{
> + if (test_bit(test->nb_sockets, bitmap))
> + return true;
This does not seem to be correct. You are testing one bit here, but
are you not supposed to test that all bits have been set?
> +
> + return false;
> +}
> +
> static int send_pkts(struct test_spec *test, struct ifobject *ifobject)
> {
> - struct pkt_stream *pkt_stream = ifobject->xsk->pkt_stream;
> bool timeout = !is_umem_valid(test->ifobj_rx);
> - struct pollfd fds = { };
> - u32 ret;
> + u32 i, ret;
>
> - fds.fd = xsk_socket__fd(ifobject->xsk->xsk);
> - fds.events = POLLOUT;
> + DECLARE_BITMAP(bitmap, MAX_SOCKETS);
Should be with the declarations in RCT order.
>
> - while (pkt_stream->current_pkt_nb < pkt_stream->nb_pkts) {
> - ret = __send_pkts(ifobject, &fds, timeout);
> - if (ret == TEST_CONTINUE && !test->fail)
> - continue;
> - if ((ret || test->fail) && !timeout)
> - return TEST_FAILURE;
> - if (ret == TEST_PASS && timeout)
> - return ret;
> + while (!(all_packets_sent(test, bitmap))) {
> + for (i = 0; i < test->nb_sockets; i++) {
> + struct pkt_stream *pkt_stream;
> +
> + pkt_stream = ifobject->xsk_arr[i].pkt_stream;
> + if (!pkt_stream || pkt_stream->current_pkt_nb >= pkt_stream->nb_pkts) {
Can pkt_stream be NULL?
> + __test_and_set_bit((1 << i), bitmap);
test_and_set? You are not testing anything here so it is enough to just set it.
> + continue;
> + }
> + ret = __send_pkts(ifobject, &ifobject->xsk_arr[i], timeout);
> + if (ret == TEST_CONTINUE && !test->fail)
> + continue;
> +
> + if ((ret || test->fail) && !timeout)
> + return TEST_FAILURE;
> +
> + if (ret == TEST_PASS && timeout)
> + return ret;
> +
> + ret = wait_for_tx_completion(&ifobject->xsk_arr[i]);
> + if ((ret || test->fail) && !timeout)
> + return TEST_FAILURE;
> +
> + if (ret == TEST_PASS && timeout)
> + return ret;
Why testing the same things before and after wait_for_tx_completion?
Should it not be fine to just do it in one place?
> + }
> }
>
> - return wait_for_tx_completion(ifobject->xsk);
> + return TEST_PASS;
> }
>
> static int get_xsk_stats(struct xsk_socket *xsk, struct xdp_statistics *stats)
> --
> 2.34.1
>
>
Powered by blists - more mailing lists