From: Magnus Karlsson <magnus.karlsson@gmail.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: "Karlsson, Magnus" <magnus.karlsson@intel.com>,
"Björn Töpel" <bjorn@kernel.org>,
"Alexei Starovoitov" <ast@kernel.org>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Network Development" <netdev@vger.kernel.org>,
"Jonathan Lemon" <jonathan.lemon@gmail.com>,
"Ciara Loftus" <ciara.loftus@intel.com>,
bpf <bpf@vger.kernel.org>, "Yonghong Song" <yhs@fb.com>,
"Andrii Nakryiko" <andrii@kernel.org>
Subject: Re: [PATCH bpf-next 16/20] selftests: xsk: introduce replacing the default packet stream
Date: Mon, 6 Sep 2021 10:00:09 +0200 [thread overview]
Message-ID: <CAJ8uoz3i6hnh+Nu8UB6QJWWJDTrH30_Te4jWQnrtqL83D_A23Q@mail.gmail.com> (raw)
In-Reply-To: <YTI4Ucn+6/uWLezP@localhost.localdomain>
On Fri, Sep 3, 2021 at 3:04 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Wed, Sep 01, 2021 at 12:47:28PM +0200, Magnus Karlsson wrote:
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> >
> > Introduce the concept of a default packet stream that is the set of
> > packets sent by most tests. Then add the ability to replace it for a
> > test that would like to send or receive something else through the use
> > of the function pkt_stream_replace() and then restored with
> > pkt_stream_restore_default(). These are then used to convert the
> > STAT_TX_INVALID_TEST to use these new APIs.
>
> s/STAT_TX_INVALID_TEST/STAT_TEST_TX_INVALID
Will fix.
> >
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ---
> > tools/testing/selftests/bpf/xdpxceiver.c | 67 +++++++++++++++++-------
> > tools/testing/selftests/bpf/xdpxceiver.h | 1 +
> > 2 files changed, 50 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
> > index 09d2854c10e6..d4aad4833754 100644
> > --- a/tools/testing/selftests/bpf/xdpxceiver.c
> > +++ b/tools/testing/selftests/bpf/xdpxceiver.c
> > @@ -390,6 +390,7 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
> > ifobj->umem = &ifobj->umem_arr[0];
> > ifobj->xsk = &ifobj->xsk_arr[0];
> > ifobj->use_poll = false;
> > + ifobj->pkt_stream = test->pkt_stream_default;
> >
> > if (i == 0) {
> > ifobj->rx_on = false;
> > @@ -418,9 +419,12 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
> > static void test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
> > struct ifobject *ifobj_rx, enum test_mode mode)
> > {
> > + struct pkt_stream *pkt_stream;
> > u32 i;
> >
> > + pkt_stream = test->pkt_stream_default;
> > memset(test, 0, sizeof(*test));
> > + test->pkt_stream_default = pkt_stream;
> >
> > for (i = 0; i < MAX_INTERFACES; i++) {
> > struct ifobject *ifobj = i ? ifobj_rx : ifobj_tx;
> > @@ -455,6 +459,19 @@ static struct pkt *pkt_stream_get_pkt(struct pkt_stream *pkt_stream, u32 pkt_nb)
> > return &pkt_stream->pkts[pkt_nb];
> > }
> >
> > +static void pkt_stream_delete(struct pkt_stream *pkt_stream)
> > +{
> > + free(pkt_stream->pkts);
> > + free(pkt_stream);
> > +}
> > +
> > +static void pkt_stream_restore_default(struct test_spec *test)
> > +{
> > + pkt_stream_delete(test->ifobj_tx->pkt_stream);
>
> I suppose that streams are the same for both tx and rx ifobjs hence it's
> enough to call the delete op a single time.
At this point in time, yes. But this has to change as we develop more tests.
> > + test->ifobj_tx->pkt_stream = test->pkt_stream_default;
> > + test->ifobj_rx->pkt_stream = test->pkt_stream_default;
> > +}
> > +
> > static struct pkt_stream *pkt_stream_generate(struct xsk_umem_info *umem, u32 nb_pkts, u32 pkt_len)
> > {
> > struct pkt_stream *pkt_stream;
> > @@ -483,6 +500,17 @@ static struct pkt_stream *pkt_stream_generate(struct xsk_umem_info *umem, u32 nb
> > return pkt_stream;
> > }
> >
> > +static void pkt_stream_replace(struct test_spec *test, u32 nb_pkts, u32 pkt_len)
> > +{
> > + struct pkt_stream *pkt_stream;
> > +
> > + pkt_stream = pkt_stream_generate(test->ifobj_tx->umem, nb_pkts, pkt_len);
> > + test->ifobj_tx->pkt_stream = pkt_stream;
> > + test->ifobj_rx->pkt_stream = pkt_stream;
> > +
> > + pkt_stream_delete(pkt_stream);
>
> Shouldn't this be deleting the stream that got replaced? You're assigning
> pkt_stream to ifobjs and then immediately free it.
>
> I'd say that we should drop this call to pkt_stream_delete() in here
> unless I'm missing something, pkt_stream_restore_default() will free the
> currently assigned pkt stream and bring back the default one to the
> ifobjs.
That is indeed a bug and should be removed.
> > +}
> > +
> > static struct pkt *pkt_generate(struct ifobject *ifobject, u32 pkt_nb)
> > {
> > struct pkt *pkt = pkt_stream_get_pkt(ifobject->pkt_stream, pkt_nb);
> > @@ -557,7 +585,7 @@ static bool is_pkt_valid(struct pkt *pkt, void *buffer, const struct xdp_desc *d
> > if (iphdr->version == IP_PKT_VER && iphdr->tos == IP_PKT_TOS) {
> > u32 seqnum = ntohl(*((u32 *)(data + PKT_HDR_SIZE)));
> >
> > - if (opt_pkt_dump && test_type != TEST_TYPE_STATS)
> > + if (opt_pkt_dump)
> > pkt_dump(data, PKT_SIZE);
> >
> > if (pkt->len != desc->len) {
> > @@ -598,9 +626,6 @@ static void complete_pkts(struct xsk_socket_info *xsk, int batch_size)
> > unsigned int rcvd;
> > u32 idx;
> >
> > - if (!xsk->outstanding_tx)
> > - return;
> > -
> > if (xsk_ring_prod__needs_wakeup(&xsk->tx))
> > kick_tx(xsk);
> >
> > @@ -831,6 +856,7 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
> >
> > static void testapp_cleanup_xsk_res(struct ifobject *ifobj)
> > {
> > + print_verbose("Destroying socket\n");
> > xsk_socket__delete(ifobj->xsk->xsk);
> > xsk_umem__delete(ifobj->umem->umem);
> > }
> > @@ -878,9 +904,6 @@ static void *worker_testapp_validate_rx(void *arg)
> > else
> > receive_pkts(ifobject->pkt_stream, ifobject->xsk, &fds);
> >
> > - if (test_type == TEST_TYPE_TEARDOWN)
> > - print_verbose("Destroying socket\n");
> > -
> > if (test->total_steps == test->current_step)
> > testapp_cleanup_xsk_res(ifobject);
> > pthread_exit(NULL);
> > @@ -890,19 +913,11 @@ static void testapp_validate_traffic(struct test_spec *test)
> > {
> > struct ifobject *ifobj_tx = test->ifobj_tx;
> > struct ifobject *ifobj_rx = test->ifobj_rx;
> > - struct pkt_stream *pkt_stream;
> > pthread_t t0, t1;
> >
> > if (pthread_barrier_init(&barr, NULL, 2))
> > exit_with_error(errno);
> >
> > - if (stat_test_type == STAT_TEST_TX_INVALID)
> > - pkt_stream = pkt_stream_generate(test->ifobj_tx->umem, DEFAULT_PKT_CNT,
> > - XSK_UMEM__INVALID_FRAME_SIZE);
> > - else
> > - pkt_stream = pkt_stream_generate(test->ifobj_tx->umem, DEFAULT_PKT_CNT, PKT_SIZE);
> > - ifobj_tx->pkt_stream = pkt_stream;
> > - ifobj_rx->pkt_stream = pkt_stream;
> > test->current_step++;
> >
> > /*Spawn RX thread */
> > @@ -982,7 +997,9 @@ static void testapp_bpf_res(struct test_spec *test)
> >
> > static void testapp_stats(struct test_spec *test)
> > {
> > - for (int i = 0; i < STAT_TEST_TYPE_MAX; i++) {
> > + int i;
> > +
> > + for (i = 0; i < STAT_TEST_TYPE_MAX; i++) {
> > test_spec_reset(test);
> > stat_test_type = i;
> >
> > @@ -991,21 +1008,27 @@ static void testapp_stats(struct test_spec *test)
> > test_spec_set_name(test, "STAT_RX_DROPPED");
> > test->ifobj_rx->umem->frame_headroom = test->ifobj_rx->umem->frame_size -
> > XDP_PACKET_HEADROOM - 1;
> > + testapp_validate_traffic(test);
> > break;
> > case STAT_TEST_RX_FULL:
> > test_spec_set_name(test, "STAT_RX_FULL");
> > test->ifobj_rx->xsk->rxqsize = RX_FULL_RXQSIZE;
> > + testapp_validate_traffic(test);
> > break;
> > case STAT_TEST_TX_INVALID:
> > test_spec_set_name(test, "STAT_TX_INVALID");
> > - continue;
> > + pkt_stream_replace(test, DEFAULT_PKT_CNT, XSK_UMEM__INVALID_FRAME_SIZE);
> > + testapp_validate_traffic(test);
> > +
> > + pkt_stream_restore_default(test);
> > + break;
> > case STAT_TEST_RX_FILL_EMPTY:
> > test_spec_set_name(test, "STAT_RX_FILL_EMPTY");
> > + testapp_validate_traffic(test);
> > break;
> > default:
> > break;
> > }
> > - testapp_validate_traffic(test);
> > }
> >
> > /* To only see the whole stat set being completed unless an individual test fails. */
> > @@ -1106,6 +1129,7 @@ int main(int argc, char **argv)
> > {
> > struct rlimit _rlim = { RLIM_INFINITY, RLIM_INFINITY };
> > struct ifobject *ifobj_tx, *ifobj_rx;
> > + struct pkt_stream *pkt_stream_default;
>
> rct broken by a little?
>
> > struct test_spec test;
> > u32 i, j;
> >
> > @@ -1133,6 +1157,12 @@ int main(int argc, char **argv)
> > init_iface(ifobj_rx, MAC2, MAC1, IP2, IP1, UDP_PORT2, UDP_PORT1,
> > worker_testapp_validate_rx);
> >
> > + test_spec_init(&test, ifobj_tx, ifobj_rx, 0);
> > + pkt_stream_default = pkt_stream_generate(ifobj_tx->umem, DEFAULT_PKT_CNT, PKT_SIZE);
> > + if (!pkt_stream_default)
> > + exit_with_error(ENOMEM);
>
> I missed this probably while reviewing previous set, but to be consistent
> with 083be682d976 ("selftests: xsk: Return correct error codes") this
> probably should have -ENOMEM as an arg?
Have eliminated exit_with_error from most of the functions I have been
touching, but not from the main function. Want the error to be printed
out before exiting the program, so kept it at this level.
> > + test.pkt_stream_default = pkt_stream_default;
> > +
> > ksft_set_plan(TEST_MODE_MAX * TEST_TYPE_MAX);
> >
> > for (i = 0; i < TEST_MODE_MAX; i++)
> > @@ -1142,6 +1172,7 @@ int main(int argc, char **argv)
> > usleep(USLEEP_MAX);
> > }
> >
> > + pkt_stream_delete(pkt_stream_default);
> > ifobject_delete(ifobj_tx);
> > ifobject_delete(ifobj_rx);
> >
> > diff --git a/tools/testing/selftests/bpf/xdpxceiver.h b/tools/testing/selftests/bpf/xdpxceiver.h
> > index c5baa7c5f560..e27fe348ae50 100644
> > --- a/tools/testing/selftests/bpf/xdpxceiver.h
> > +++ b/tools/testing/selftests/bpf/xdpxceiver.h
> > @@ -132,6 +132,7 @@ struct ifobject {
> > struct test_spec {
> > struct ifobject *ifobj_tx;
> > struct ifobject *ifobj_rx;
> > + struct pkt_stream *pkt_stream_default;
> > u16 total_steps;
> > u16 current_step;
> > u16 nb_sockets;
> > --
> > 2.29.0
> >
next prev parent reply other threads:[~2021-09-06 8:00 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-01 10:47 [PATCH bpf-next 00/20] selftests: xsk: facilitate adding tests Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 01/20] selftests: xsk: simplify xsk and umem arrays Magnus Karlsson
2021-09-03 17:53 ` John Fastabend
2021-09-01 10:47 ` [PATCH bpf-next 02/20] selftests: xsk: introduce type for thread function Magnus Karlsson
2021-09-03 17:56 ` John Fastabend
2021-09-01 10:47 ` [PATCH bpf-next 03/20] selftests: xsk: introduce test specifications Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 04/20] selftests: xsk: move num_frames and frame_headroom to xsk_umem_info Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 05/20] selftests: xsk: move rxqsize into xsk_socket_info Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 06/20] selftests: xsk: make frame_size configurable Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 07/20] selftests: xsx: introduce test name in test spec Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 08/20] selftests: xsk: add use_poll to ifobject Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 09/20] selftests: xsk: introduce rx_on and tx_on in ifobject Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 10/20] selftests: xsk: replace second_step global variable Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 11/20] selftests: xsk: specify number of sockets to create Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 12/20] selftests: xsk: make xdp_flags and bind_flags local Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 13/20] selftests: xsx: make pthreads local scope Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 14/20] selftests: xsk: eliminate MAX_SOCKS define Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 15/20] selftests: xsk: allow for invalid packets Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 16/20] selftests: xsk: introduce replacing the default packet stream Magnus Karlsson
2021-09-03 15:04 ` Maciej Fijalkowski
2021-09-06 8:00 ` Magnus Karlsson [this message]
2021-09-01 10:47 ` [PATCH bpf-next 17/20] selftests: xsk: add test for unaligned mode Magnus Karlsson
2021-09-03 15:38 ` Maciej Fijalkowski
2021-09-06 8:02 ` Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 18/20] selftests: xsk: eliminate test specific if-statement in test runner Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 19/20] selftests: xsk: add tests for invalid xsk descriptors Magnus Karlsson
2021-09-03 18:58 ` Maciej Fijalkowski
2021-09-06 8:06 ` Magnus Karlsson
2021-09-01 10:47 ` [PATCH bpf-next 20/20] selftests: xsk: add tests for 2K frame size Magnus Karlsson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJ8uoz3i6hnh+Nu8UB6QJWWJDTrH30_Te4jWQnrtqL83D_A23Q@mail.gmail.com \
--to=magnus.karlsson@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bjorn@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=ciara.loftus@intel.com \
--cc=daniel@iogearbox.net \
--cc=jonathan.lemon@gmail.com \
--cc=maciej.fijalkowski@intel.com \
--cc=magnus.karlsson@intel.com \
--cc=netdev@vger.kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.