| Message ID | 20210217160214.7869-1-ciara.loftus@intel.com |
|---|---|
| Headers | show |
| Series | selftests/bpf: xsk improvements and new stats tests | expand |
On Wed, 17 Feb 2021 at 17:33, Ciara Loftus <ciara.loftus@intel.com> wrote: > > Prior to this commit individual xsk tests were launched from the > shell script 'test_xsk.sh'. When adding a new test type, two new test > configurations had to be added to this file - one for each of the > supported XDP 'modes' (skb or drv). Should zero copy support be added to > the xsk selftest framework in the future, three new test configurations > would need to be added for each new test type. Each new test type also > typically requires new CLI arguments for the xdpxceiver program. > > This commit aims to reduce the overhead of adding new tests, by launching > the test configurations from within the xdpxceiver program itself, using > simple loops. Every test is run every time the C program is executed. Many > of the CLI arguments can be removed as a result. > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > --- > tools/testing/selftests/bpf/test_xsk.sh | 112 +----------- > tools/testing/selftests/bpf/xdpxceiver.c | 199 ++++++++++++--------- > tools/testing/selftests/bpf/xdpxceiver.h | 27 ++- > tools/testing/selftests/bpf/xsk_prereqs.sh | 24 +-- > 4 files changed, 139 insertions(+), 223 deletions(-) > I like where this is going! Nice! [...] > diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c > index 8af746c9a6b6..7cb4a13597d0 100644 > --- a/tools/testing/selftests/bpf/xdpxceiver.c > +++ b/tools/testing/selftests/bpf/xdpxceiver.c [...] > > +static int configure_skb(void) > +{ > + > + char cmd[80]; > + > + snprintf(cmd, sizeof(cmd), "ip link set dev %s xdpdrv off", ifdict[0]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface %s\n", > + ifdict[0]->ifname); > + return -1; > + } > + snprintf(cmd, sizeof(cmd), "ip netns exec %s ip link set dev %s xdpdrv off", > + ifdict[1]->nsname, ifdict[1]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface/ns %s\n", > + ifdict[1]->ifname, ifdict[1]->nsname); > + return -1; > + } > + > + cur_mode = TEST_MODE_SKB; > + > + return 0; > + > +} > + > +static int configure_drv(void) > +{ > + char cmd[80]; > + > + snprintf(cmd, sizeof(cmd), "ip link set dev %s xdpgeneric off", ifdict[0]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface %s\n", > + ifdict[0]->ifname); > + return -1; > + } > + snprintf(cmd, sizeof(cmd), "ip netns exec %s ip link set dev %s xdpgeneric off", > + ifdict[1]->nsname, ifdict[1]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface/ns %s\n", > + ifdict[1]->ifname, ifdict[1]->nsname); > + return -1; > + } > + > + cur_mode = TEST_MODE_DRV; > + > + return 0; > +} > + We're already using libbpf, so please use the libbpf APIs instead of calling iproute2. Björn [...]
On Wed, Feb 17, 2021 at 5:36 PM Ciara Loftus <ciara.loftus@intel.com> wrote: > > Launching xdpxceiver with -D enables debug mode. Make it possible Would be clearer if the option was the same both in the shell and in the xdpreceiver app, so please pick -d or -D and stick with it. And how about calling the mode "dump packets" instead of debug, because that is what it is doing right now? > to pass this flag to the app via the test_xsk.sh shell script like > so: > > ./test_xsk.sh -d > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > --- > tools/testing/selftests/bpf/test_xsk.sh | 7 ++++++- > tools/testing/selftests/bpf/xsk_prereqs.sh | 3 ++- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_xsk.sh b/tools/testing/selftests/bpf/test_xsk.sh > index 91127a5be90d..a72f8ed2932d 100755 > --- a/tools/testing/selftests/bpf/test_xsk.sh > +++ b/tools/testing/selftests/bpf/test_xsk.sh > @@ -74,11 +74,12 @@ > > . xsk_prereqs.sh > > -while getopts "cv" flag > +while getopts "cvd" flag > do > case "${flag}" in > c) colorconsole=1;; > v) verbose=1;; > + d) debug=1;; > esac > done > > @@ -135,6 +136,10 @@ if [[ $verbose -eq 1 ]]; then > VERBOSE_ARG="-v" > fi > > +if [[ $debug -eq 1 ]]; then > + DEBUG_ARG="-D" > +fi > + > test_status $retval "${TEST_NAME}" > > ## START TESTS > diff --git a/tools/testing/selftests/bpf/xsk_prereqs.sh b/tools/testing/selftests/bpf/xsk_prereqs.sh > index ef8c5b31f4b6..d95018051fcc 100755 > --- a/tools/testing/selftests/bpf/xsk_prereqs.sh > +++ b/tools/testing/selftests/bpf/xsk_prereqs.sh > @@ -128,5 +128,6 @@ execxdpxceiver() > copy[$index]=${!current} > done > > - ./${XSKOBJ} -i ${VETH0} -i ${VETH1},${NS1} ${copy[*]} -C ${NUMPKTS} ${VERBOSE_ARG} > + ./${XSKOBJ} -i ${VETH0} -i ${VETH1},${NS1} ${copy[*]} -C ${NUMPKTS} ${VERBOSE_ARG} \ > + ${DEBUG_ARG} > } > -- > 2.17.1 >
On Wed, Feb 17, 2021 at 04:02:13PM +0000, Ciara Loftus wrote: > Prior to this commit individual xsk tests were launched from the > shell script 'test_xsk.sh'. When adding a new test type, two new test > configurations had to be added to this file - one for each of the > supported XDP 'modes' (skb or drv). Should zero copy support be added to > the xsk selftest framework in the future, three new test configurations > would need to be added for each new test type. Each new test type also > typically requires new CLI arguments for the xdpxceiver program. > > This commit aims to reduce the overhead of adding new tests, by launching > the test configurations from within the xdpxceiver program itself, using > simple loops. Every test is run every time the C program is executed. Many > of the CLI arguments can be removed as a result. > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > --- > tools/testing/selftests/bpf/test_xsk.sh | 112 +----------- > tools/testing/selftests/bpf/xdpxceiver.c | 199 ++++++++++++--------- > tools/testing/selftests/bpf/xdpxceiver.h | 27 ++- > tools/testing/selftests/bpf/xsk_prereqs.sh | 24 +-- > 4 files changed, 139 insertions(+), 223 deletions(-) > Good cleanup! I have a series of fixes/cleanups as well and I need to introduce a new test over here, so your work makes it easier for me. One nit below and once you address Bjorn's request, then feel free to add my: Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> [...] > +static int configure_skb(void) > +{ > + > + char cmd[80]; > + > + snprintf(cmd, sizeof(cmd), "ip link set dev %s xdpdrv off", ifdict[0]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface %s\n", > + ifdict[0]->ifname); > + return -1; > + } > + snprintf(cmd, sizeof(cmd), "ip netns exec %s ip link set dev %s xdpdrv off", > + ifdict[1]->nsname, ifdict[1]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface/ns %s\n", > + ifdict[1]->ifname, ifdict[1]->nsname); > + return -1; > + } > + > + cur_mode = TEST_MODE_SKB; > + > + return 0; > + > +} > + > +static int configure_drv(void) > +{ > + char cmd[80]; > + > + snprintf(cmd, sizeof(cmd), "ip link set dev %s xdpgeneric off", ifdict[0]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface %s\n", > + ifdict[0]->ifname); > + return -1; > + } > + snprintf(cmd, sizeof(cmd), "ip netns exec %s ip link set dev %s xdpgeneric off", > + ifdict[1]->nsname, ifdict[1]->ifname); > + if (system(cmd)) { > + ksft_test_result_fail("Failed to configure native mode on iface/ns %s\n", > + ifdict[1]->ifname, ifdict[1]->nsname); > + return -1; > + } > + > + cur_mode = TEST_MODE_DRV; > + > + return 0; > +} > + > +static void run_pkt_test(int mode, int type) > +{ > + test_type = type; > + > + /* reset defaults after potential previous test */ > + xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST; > + pkt_counter = 0; > + switching_notify = 0; > + bidi_pass = 0; > + prev_pkt = -1; > + ifdict[0]->fv.vector = tx; > + ifdict[1]->fv.vector = rx; > + > + switch (mode) { > + case (TEST_MODE_SKB): > + if (cur_mode != TEST_MODE_SKB) > + configure_skb(); Should you check a return value over here? > + xdp_flags |= XDP_FLAGS_SKB_MODE; > + uut = TEST_MODE_SKB; > + break; > + case (TEST_MODE_DRV): > + if (cur_mode != TEST_MODE_DRV) > + configure_drv(); ditto > + xdp_flags |= XDP_FLAGS_DRV_MODE; > + uut = TEST_MODE_DRV; > + break; > + default: > + break; > + } > + > + pthread_init_mutex(); > + > + if ((test_type != TEST_TYPE_TEARDOWN) && (test_type != TEST_TYPE_BIDI)) > + testapp_validate(); > + else > + testapp_sockets(); > + > + pthread_destroy_mutex(); > +} > + > int main(int argc, char **argv) > { > struct rlimit _rlim = { RLIM_INFINITY, RLIM_INFINITY }; > @@ -1021,6 +1062,7 @@ int main(int argc, char **argv) > const char *IP2 = "192.168.100.161"; > u16 UDP_DST_PORT = 2020; > u16 UDP_SRC_PORT = 2121; > + int i, j; > > ifaceconfig = malloc(sizeof(struct ifaceconfigobj)); > memcpy(ifaceconfig->dst_mac, MAC1, ETH_ALEN); > @@ -1046,24 +1088,19 @@ int main(int argc, char **argv) > > init_iface_config(ifaceconfig); > > - pthread_init_mutex(); > + ksft_set_plan(TEST_MODE_MAX * TEST_TYPE_MAX); > > - ksft_set_plan(1); > + configure_skb(); > + cur_mode = TEST_MODE_SKB; > > - if (!opt_teardown && !opt_bidi) { > - testapp_validate(); > - } else if (opt_teardown && opt_bidi) { > - ksft_test_result_fail("ERROR: parameters -T and -B cannot be used together\n"); > - ksft_exit_xfail(); > - } else { > - testapp_sockets(); > + for (i = 0; i < TEST_MODE_MAX; i++) { > + for (j = 0; j < TEST_TYPE_MAX; j++) > + run_pkt_test(i, j); > } > > for (int i = 0; i < MAX_INTERFACES; i++) > free(ifdict[i]); > > - pthread_destroy_mutex(); > - > ksft_exit_pass(); > > return 0;
> > On Wed, Feb 17, 2021 at 5:36 PM Ciara Loftus <ciara.loftus@intel.com> > wrote: > > > > Launching xdpxceiver with -D enables debug mode. Make it possible > > Would be clearer if the option was the same both in the shell and in > the xdpreceiver app, so please pick -d or -D and stick with it. And > how about calling the mode "dump packets" instead of debug, because > that is what it is doing right now? +1. Will stick with -D to be consistent with the current interface. Will make the long opt '--dump_pkts'. Thanks, Ciara > > > to pass this flag to the app via the test_xsk.sh shell script like > > so: > > > > ./test_xsk.sh -d > > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > --- > > tools/testing/selftests/bpf/test_xsk.sh | 7 ++++++- > > tools/testing/selftests/bpf/xsk_prereqs.sh | 3 ++- > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/test_xsk.sh > b/tools/testing/selftests/bpf/test_xsk.sh > > index 91127a5be90d..a72f8ed2932d 100755 > > --- a/tools/testing/selftests/bpf/test_xsk.sh > > +++ b/tools/testing/selftests/bpf/test_xsk.sh > > @@ -74,11 +74,12 @@ > > > > . xsk_prereqs.sh > > > > -while getopts "cv" flag > > +while getopts "cvd" flag > > do > > case "${flag}" in > > c) colorconsole=1;; > > v) verbose=1;; > > + d) debug=1;; > > esac > > done > > > > @@ -135,6 +136,10 @@ if [[ $verbose -eq 1 ]]; then > > VERBOSE_ARG="-v" > > fi > > > > +if [[ $debug -eq 1 ]]; then > > + DEBUG_ARG="-D" > > +fi > > + > > test_status $retval "${TEST_NAME}" > > > > ## START TESTS > > diff --git a/tools/testing/selftests/bpf/xsk_prereqs.sh > b/tools/testing/selftests/bpf/xsk_prereqs.sh > > index ef8c5b31f4b6..d95018051fcc 100755 > > --- a/tools/testing/selftests/bpf/xsk_prereqs.sh > > +++ b/tools/testing/selftests/bpf/xsk_prereqs.sh > > @@ -128,5 +128,6 @@ execxdpxceiver() > > copy[$index]=${!current} > > done > > > > - ./${XSKOBJ} -i ${VETH0} -i ${VETH1},${NS1} ${copy[*]} -C ${NUMPKTS} > ${VERBOSE_ARG} > > + ./${XSKOBJ} -i ${VETH0} -i ${VETH1},${NS1} ${copy[*]} -C ${NUMPKTS} > ${VERBOSE_ARG} \ > > + ${DEBUG_ARG} > > } > > -- > > 2.17.1 > >
> > On Wed, Feb 17, 2021 at 04:02:13PM +0000, Ciara Loftus wrote: > > Prior to this commit individual xsk tests were launched from the > > shell script 'test_xsk.sh'. When adding a new test type, two new test > > configurations had to be added to this file - one for each of the > > supported XDP 'modes' (skb or drv). Should zero copy support be added to > > the xsk selftest framework in the future, three new test configurations > > would need to be added for each new test type. Each new test type also > > typically requires new CLI arguments for the xdpxceiver program. > > > > This commit aims to reduce the overhead of adding new tests, by launching > > the test configurations from within the xdpxceiver program itself, using > > simple loops. Every test is run every time the C program is executed. Many > > of the CLI arguments can be removed as a result. > > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > --- > > tools/testing/selftests/bpf/test_xsk.sh | 112 +----------- > > tools/testing/selftests/bpf/xdpxceiver.c | 199 ++++++++++++--------- > > tools/testing/selftests/bpf/xdpxceiver.h | 27 ++- > > tools/testing/selftests/bpf/xsk_prereqs.sh | 24 +-- > > 4 files changed, 139 insertions(+), 223 deletions(-) > > > > Good cleanup! I have a series of fixes/cleanups as well and I need to > introduce a new test over here, so your work makes it easier for me. > > One nit below and once you address Bjorn's request, then feel free to add > my: Thanks Björn and Maciej for the feedback. Will include your suggestions in the v2. I discovered some extra things to tweak for the v2 but hope to have it up shortly. Thanks, Ciara > > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > [...] > > > +static int configure_skb(void) > > +{ > > + > > + char cmd[80]; > > + > > + snprintf(cmd, sizeof(cmd), "ip link set dev %s xdpdrv off", ifdict[0]- > >ifname); > > + if (system(cmd)) { > > + ksft_test_result_fail("Failed to configure native mode on > iface %s\n", > > + ifdict[0]->ifname); > > + return -1; > > + } > > + snprintf(cmd, sizeof(cmd), "ip netns exec %s ip link set dev %s > xdpdrv off", > > + ifdict[1]->nsname, ifdict[1]->ifname); > > + if (system(cmd)) { > > + ksft_test_result_fail("Failed to configure native mode on > iface/ns %s\n", > > + ifdict[1]->ifname, ifdict[1]- > >nsname); > > + return -1; > > + } > > + > > + cur_mode = TEST_MODE_SKB; > > + > > + return 0; > > + > > +} > > + > > +static int configure_drv(void) > > +{ > > + char cmd[80]; > > + > > + snprintf(cmd, sizeof(cmd), "ip link set dev %s xdpgeneric off", > ifdict[0]->ifname); > > + if (system(cmd)) { > > + ksft_test_result_fail("Failed to configure native mode on > iface %s\n", > > + ifdict[0]->ifname); > > + return -1; > > + } > > + snprintf(cmd, sizeof(cmd), "ip netns exec %s ip link set dev %s > xdpgeneric off", > > + ifdict[1]->nsname, ifdict[1]->ifname); > > + if (system(cmd)) { > > + ksft_test_result_fail("Failed to configure native mode on > iface/ns %s\n", > > + ifdict[1]->ifname, ifdict[1]- > >nsname); > > + return -1; > > + } > > + > > + cur_mode = TEST_MODE_DRV; > > + > > + return 0; > > +} > > + > > +static void run_pkt_test(int mode, int type) > > +{ > > + test_type = type; > > + > > + /* reset defaults after potential previous test */ > > + xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST; > > + pkt_counter = 0; > > + switching_notify = 0; > > + bidi_pass = 0; > > + prev_pkt = -1; > > + ifdict[0]->fv.vector = tx; > > + ifdict[1]->fv.vector = rx; > > + > > + switch (mode) { > > + case (TEST_MODE_SKB): > > + if (cur_mode != TEST_MODE_SKB) > > + configure_skb(); > > Should you check a return value over here? > > > + xdp_flags |= XDP_FLAGS_SKB_MODE; > > + uut = TEST_MODE_SKB; > > + break; > > + case (TEST_MODE_DRV): > > + if (cur_mode != TEST_MODE_DRV) > > + configure_drv(); > > ditto > > > + xdp_flags |= XDP_FLAGS_DRV_MODE; > > + uut = TEST_MODE_DRV; > > + break; > > + default: > > + break; > > + } > > + > > + pthread_init_mutex(); > > + > > + if ((test_type != TEST_TYPE_TEARDOWN) && (test_type != > TEST_TYPE_BIDI)) > > + testapp_validate(); > > + else > > + testapp_sockets(); > > + > > + pthread_destroy_mutex(); > > +} > > + > > int main(int argc, char **argv) > > { > > struct rlimit _rlim = { RLIM_INFINITY, RLIM_INFINITY }; > > @@ -1021,6 +1062,7 @@ int main(int argc, char **argv) > > const char *IP2 = "192.168.100.161"; > > u16 UDP_DST_PORT = 2020; > > u16 UDP_SRC_PORT = 2121; > > + int i, j; > > > > ifaceconfig = malloc(sizeof(struct ifaceconfigobj)); > > memcpy(ifaceconfig->dst_mac, MAC1, ETH_ALEN); > > @@ -1046,24 +1088,19 @@ int main(int argc, char **argv) > > > > init_iface_config(ifaceconfig); > > > > - pthread_init_mutex(); > > + ksft_set_plan(TEST_MODE_MAX * TEST_TYPE_MAX); > > > > - ksft_set_plan(1); > > + configure_skb(); > > + cur_mode = TEST_MODE_SKB; > > > > - if (!opt_teardown && !opt_bidi) { > > - testapp_validate(); > > - } else if (opt_teardown && opt_bidi) { > > - ksft_test_result_fail("ERROR: parameters -T and -B cannot > be used together\n"); > > - ksft_exit_xfail(); > > - } else { > > - testapp_sockets(); > > + for (i = 0; i < TEST_MODE_MAX; i++) { > > + for (j = 0; j < TEST_TYPE_MAX; j++) > > + run_pkt_test(i, j); > > } > > > > for (int i = 0; i < MAX_INTERFACES; i++) > > free(ifdict[i]); > > > > - pthread_destroy_mutex(); > > - > > ksft_exit_pass(); > > > > return 0;