On Tue, Dec 06, 2022 at 10:08:26AM +0100, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
>
> Implement automatic switching of XDP programs and execution modes if
> needed by a test. This makes it much simpler to write a test as it
> only has to say what XDP program it needs if it is not the default
> one. This also makes it possible to remove the initial explicit
> attachment of the XDP program as well as the explicit mode switch in
> the code. These are now handled by the same code that just checks if a
> switch is necessary, so no special cases are needed.
>
> The default XDP program for all tests is one that sends all packets to
> the AF_XDP socket. If you need another one, please use the new
> function test_spec_set_xdp_prog() to specify what XDP programs and
> maps to use for this test.
>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
> ---
> tools/testing/selftests/bpf/xsk.c | 14 +++
> tools/testing/selftests/bpf/xsk.h | 1 +
> tools/testing/selftests/bpf/xskxceiver.c | 150 ++++++++++++-----------
> tools/testing/selftests/bpf/xskxceiver.h | 7 +-
> 4 files changed, 102 insertions(+), 70 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/xsk.c b/tools/testing/selftests/bpf/xsk.c
> index dc6b47280ec4..d9d44a29c7cc 100644
> --- a/tools/testing/selftests/bpf/xsk.c
> +++ b/tools/testing/selftests/bpf/xsk.c
> @@ -267,6 +267,20 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area,
> return err;
> }
>
> +bool xsk_is_in_drv_mode(u32 ifindex)
any reason why this can't live in xskxceiver currently?
> +{
> + LIBBPF_OPTS(bpf_xdp_query_opts, opts);
> + int ret;
> +
> + ret = bpf_xdp_query(ifindex, XDP_FLAGS_DRV_MODE, &opts);
> + if (ret) {
> + printf("DRV mode query returned error %s\n", strerror(errno));
> + return false;
> + }
> +
> + return opts.attach_mode == XDP_ATTACHED_DRV;
> +}
> +
> int xsk_attach_xdp_program(struct bpf_program *prog, int ifindex, u32 xdp_flags)
> {
> int prog_fd;
> diff --git a/tools/testing/selftests/bpf/xsk.h b/tools/testing/selftests/bpf/xsk.h
> index 5624d31b8db7..3cb9d69589b8 100644
> --- a/tools/testing/selftests/bpf/xsk.h
> +++ b/tools/testing/selftests/bpf/xsk.h
> @@ -201,6 +201,7 @@ int xsk_attach_xdp_program(struct bpf_program *prog, int ifindex, u32 xdp_flags)
> void xsk_detach_xdp_program(int ifindex, u32 xdp_flags);
> int xsk_update_xskmap(struct bpf_map *map, struct xsk_socket *xsk);
> void xsk_clear_xskmap(struct bpf_map *map);
> +bool xsk_is_in_drv_mode(u32 ifindex);
>
> struct xsk_socket_config {
> __u32 rx_size;
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 26cd64d4209f..ae9370f7145e 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -96,6 +96,9 @@
> #include <time.h>
> #include <unistd.h>
> #include <stdatomic.h>
> +
> +#include "xsk_def_prog.skel.h"
> +#include "xsk_xdp_drop.skel.h"
> #include "xsk.h"
> #include "xskxceiver.h"
> #include <bpf/bpf.h>
> @@ -362,7 +365,6 @@ static bool ifobj_zc_avail(struct ifobject *ifobject)
> xsk = calloc(1, sizeof(struct xsk_socket_info));
> if (!xsk)
> goto out;
> - ifobject->xdp_flags = XDP_FLAGS_DRV_MODE;
> ifobject->bind_flags = XDP_USE_NEED_WAKEUP | XDP_ZEROCOPY;
> ifobject->rx_on = true;
> xsk->rxqsize = XSK_RING_CONS__DEFAULT_NUM_DESCS;
> @@ -501,6 +503,10 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
> test->total_steps = 1;
> test->nb_sockets = 1;
> test->fail = false;
> + test->xdp_prog_rx = ifobj_rx->def_prog->progs.xsk_def_prog;
> + test->xskmap_rx = ifobj_rx->def_prog->maps.xsk;
> + test->xdp_prog_tx = ifobj_tx->def_prog->progs.xsk_def_prog;
> + test->xskmap_tx = ifobj_tx->def_prog->maps.xsk;
> }
>
> static void test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
> @@ -540,6 +546,16 @@ static void test_spec_set_name(struct test_spec *test, const char *name)
> strncpy(test->name, name, MAX_TEST_NAME_SIZE);
> }
>
> +static void test_spec_set_xdp_prog(struct test_spec *test, struct bpf_program *xdp_prog_rx,
> + struct bpf_program *xdp_prog_tx, struct bpf_map *xskmap_rx,
> + struct bpf_map *xskmap_tx)
> +{
> + test->xdp_prog_rx = xdp_prog_rx;
> + test->xdp_prog_tx = xdp_prog_tx;
> + test->xskmap_rx = xskmap_rx;
> + test->xskmap_tx = xskmap_tx;
> +}
> +
> static void pkt_stream_reset(struct pkt_stream *pkt_stream)
> {
> if (pkt_stream)
> @@ -1364,6 +1380,57 @@ static void handler(int signum)
> pthread_exit(NULL);
> }
>
> +static bool xdp_prog_changed(struct test_spec *test, struct ifobject *ifobj)
> +{
> + return ifobj->xdp_prog != test->xdp_prog_rx || ifobj->mode != test->mode;
> +}
> +
> +static void xsk_attach_xdp_progs(struct test_spec *test, struct ifobject *ifobj_rx,
> + struct ifobject *ifobj_tx)
> +{
> + int err;
> +
> + if (xdp_prog_changed(test, ifobj_rx)) {
> + xsk_detach_xdp_program(ifobj_rx->ifindex, mode_to_xdp_flags(ifobj_rx->mode));
> + err = xsk_attach_xdp_program(test->xdp_prog_rx, ifobj_rx->ifindex,
> + mode_to_xdp_flags(test->mode));
> + if (err) {
> + printf("Error attaching XDP program\n");
> + exit_with_error(-err);
> + }
> +
> + if (ifobj_rx->mode != test->mode && test->mode == TEST_MODE_DRV)
This strictly implies that TEST_MODE_SKB is followed by TEST_MODE_DRV,
correct? What if I would swap the order of the tests?
IOW why is it specific to TEST_MODE_DRV? I know that current code also
implies that, but maybe making it generic would make it easier to for
example run a standalone test later on?
> + if (!xsk_is_in_drv_mode(ifobj_rx->ifindex)) {
> + ksft_print_msg("ERROR: XDP prog not in DRV mode\n");
> + exit_with_error(EINVAL);
> + }
> +
> + ifobj_rx->xdp_prog = test->xdp_prog_rx;
> + ifobj_rx->xskmap = test->xskmap_rx;
> + ifobj_rx->mode = test->mode;
> + }
> +
> + if (ifobj_tx && !ifobj_tx->shared_umem && xdp_prog_changed(test, ifobj_tx)) {
> + xsk_detach_xdp_program(ifobj_tx->ifindex, mode_to_xdp_flags(ifobj_tx->mode));
> + err = xsk_attach_xdp_program(test->xdp_prog_tx, ifobj_tx->ifindex,
> + mode_to_xdp_flags(test->mode));
> + if (err) {
> + printf("Error attaching XDP program\n");
> + exit_with_error(-err);
> + }
> +
> + if (ifobj_rx->mode != test->mode && test->mode == TEST_MODE_DRV)
> + if (!xsk_is_in_drv_mode(ifobj_tx->ifindex)) {
> + ksft_print_msg("ERROR: XDP prog not in DRV mode\n");
> + exit_with_error(EINVAL);
> + }
> +
> + ifobj_tx->xdp_prog = test->xdp_prog_tx;
> + ifobj_tx->xskmap = test->xskmap_tx;
> + ifobj_tx->mode = test->mode;
> + }
All of the code above is repeated for ifobj_rx. So what if we do this:
/* FIXME: proper args and naming */
static void xsk_reattach_prog(int xyz, int abc)
{
xsk_detach_xdp_program(ifobj_tx->ifindex, mode_to_xdp_flags(ifobj_tx->mode));
err = xsk_attach_xdp_program(test->xdp_prog_tx, ifobj_tx->ifindex,
mode_to_xdp_flags(test->mode));
if (err) {
printf("Error attaching XDP program\n");
exit_with_error(-err);
}
if (ifobj_rx->mode != test->mode && test->mode == TEST_MODE_DRV)
if (!xsk_is_in_drv_mode(ifobj_tx->ifindex)) {
ksft_print_msg("ERROR: XDP prog not in DRV mode\n");
exit_with_error(EINVAL);
}
ifobj_tx->xdp_prog = test->xdp_prog_tx;
ifobj_tx->xskmap = test->xskmap_tx;
ifobj_tx->mode = test->mode;
}
Then in xsk_attach_xdp_progs:
if (xdp_prog_changed(test, ifobj_rx)) {
xsk_reattach_prog(xyz, abc);
if (!ifobj_tx || ifobj_tx->shared_umem)
return;
if (xdp_prog_changed(test, ifobj_tx))
xsk_reattach_prog(xyz, abc);
?
> +}
> +
> static int __testapp_validate_traffic(struct test_spec *test, struct ifobject *ifobj_rx,
> struct ifobject *ifobj_tx)
> {
> @@ -1411,7 +1478,11 @@ static int __testapp_validate_traffic(struct test_spec *test, struct ifobject *i
>
> static int testapp_validate_traffic(struct test_spec *test)
> {
> - return __testapp_validate_traffic(test, test->ifobj_rx, test->ifobj_tx);
> + struct ifobject *ifobj_rx = test->ifobj_rx;
> + struct ifobject *ifobj_tx = test->ifobj_tx;
> +
> + xsk_attach_xdp_progs(test, ifobj_rx, ifobj_tx);
> + return __testapp_validate_traffic(test, ifobj_rx, ifobj_tx);
> }
>
> static int testapp_validate_traffic_single_thread(struct test_spec *test, struct ifobject *ifobj)
> @@ -1454,7 +1525,7 @@ static void testapp_bidi(struct test_spec *test)
>
> print_verbose("Switching Tx/Rx vectors\n");
> swap_directions(&test->ifobj_rx, &test->ifobj_tx);
> - testapp_validate_traffic(test);
> + __testapp_validate_traffic(test, test->ifobj_rx, test->ifobj_tx);
>
> swap_directions(&test->ifobj_rx, &test->ifobj_tx);
> }
> @@ -1623,31 +1694,15 @@ static void testapp_invalid_desc(struct test_spec *test)
>
> static void testapp_xdp_drop(struct test_spec *test)
> {
> - struct ifobject *ifobj = test->ifobj_rx;
> - int err;
> + struct xsk_xdp_drop *skel_rx = test->ifobj_rx->xdp_drop;
> + struct xsk_xdp_drop *skel_tx = test->ifobj_tx->xdp_drop;
>
> test_spec_set_name(test, "XDP_CONSUMES_SOME_PACKETS");
> - xsk_detach_xdp_program(ifobj->ifindex, ifobj->xdp_flags);
> - err = xsk_attach_xdp_program(ifobj->xdp_drop->progs.xsk_xdp_drop, ifobj->ifindex,
> - ifobj->xdp_flags);
> - if (err) {
> - printf("Error attaching XDP_DROP program\n");
> - test->fail = true;
> - return;
> - }
> - ifobj->xskmap = ifobj->xdp_drop->maps.xsk;
> + test_spec_set_xdp_prog(test, skel_rx->progs.xsk_xdp_drop, skel_tx->progs.xsk_xdp_drop,
> + skel_rx->maps.xsk, skel_tx->maps.xsk);
>
> pkt_stream_receive_half(test);
> testapp_validate_traffic(test);
> -
> - xsk_detach_xdp_program(ifobj->ifindex, ifobj->xdp_flags);
> - err = xsk_attach_xdp_program(ifobj->def_prog->progs.xsk_def_prog, ifobj->ifindex,
> - ifobj->xdp_flags);
> - if (err) {
> - printf("Error restoring default XDP program\n");
> - exit_with_error(-err);
> - }
> - ifobj->xskmap = ifobj->def_prog->maps.xsk;
> }
>
> static void testapp_poll_txq_tmout(struct test_spec *test)
> @@ -1689,7 +1744,7 @@ static void xsk_unload_xdp_programs(struct ifobject *ifobj)
>
> static void init_iface(struct ifobject *ifobj, const char *dst_mac, const char *src_mac,
> const char *dst_ip, const char *src_ip, const u16 dst_port,
> - const u16 src_port, thread_func_t func_ptr, bool load_xdp)
> + const u16 src_port, thread_func_t func_ptr)
> {
> struct in_addr ip;
> int err;
> @@ -1708,23 +1763,11 @@ static void init_iface(struct ifobject *ifobj, const char *dst_mac, const char *
>
> ifobj->func_ptr = func_ptr;
>
> - if (!load_xdp)
> - return;
> -
> err = xsk_load_xdp_programs(ifobj);
> if (err) {
> printf("Error loading XDP program\n");
> exit_with_error(err);
> }
> -
> - ifobj->xdp_flags = mode_to_xdp_flags(TEST_MODE_SKB);
> - err = xsk_attach_xdp_program(ifobj->def_prog->progs.xsk_def_prog, ifobj->ifindex,
> - ifobj->xdp_flags);
> - if (err) {
> - printf("Error attaching XDP program\n");
> - exit_with_error(-err);
> - }
> - ifobj->xskmap = ifobj->def_prog->maps.xsk;
> }
>
> static void run_pkt_test(struct test_spec *test, enum test_mode mode, enum test_type type)
> @@ -1886,31 +1929,6 @@ static bool is_xdp_supported(int ifindex)
> return true;
> }
>
> -static void change_to_drv_mode(struct ifobject *ifobj)
> -{
> - LIBBPF_OPTS(bpf_xdp_query_opts, opts);
> - int ret;
> -
> - xsk_detach_xdp_program(ifobj->ifindex, ifobj->xdp_flags);
> - ifobj->xdp_flags = XDP_FLAGS_DRV_MODE;
> - ret = xsk_attach_xdp_program(ifobj->def_prog->progs.xsk_def_prog, ifobj->ifindex,
> - ifobj->xdp_flags);
> - if (ret) {
> - printf("Error attaching XDP program\n");
> - exit_with_error(-ret);
> - }
> - ifobj->xskmap = ifobj->def_prog->maps.xsk;
> -
> - ret = bpf_xdp_query(ifobj->ifindex, XDP_FLAGS_DRV_MODE, &opts);
> - if (ret)
> - exit_with_error(errno);
> -
> - if (opts.attach_mode != XDP_ATTACHED_DRV) {
> - ksft_print_msg("ERROR: [%s] XDP prog not in DRV mode\n");
> - exit_with_error(EINVAL);
> - }
> -}
> -
> int main(int argc, char **argv)
> {
> struct pkt_stream *rx_pkt_stream_default;
> @@ -1951,9 +1969,9 @@ int main(int argc, char **argv)
> }
>
> init_iface(ifobj_rx, MAC1, MAC2, IP1, IP2, UDP_PORT1, UDP_PORT2,
> - worker_testapp_validate_rx, true);
> + worker_testapp_validate_rx);
> init_iface(ifobj_tx, MAC2, MAC1, IP2, IP1, UDP_PORT2, UDP_PORT1,
> - worker_testapp_validate_tx, !shared_netdev);
> + worker_testapp_validate_tx);
>
> test_spec_init(&test, ifobj_tx, ifobj_rx, 0);
> tx_pkt_stream_default = pkt_stream_generate(ifobj_tx->umem, DEFAULT_PKT_CNT, PKT_SIZE);
> @@ -1966,12 +1984,6 @@ int main(int argc, char **argv)
> ksft_set_plan(modes * TEST_TYPE_MAX);
>
> for (i = 0; i < modes; i++) {
> - if (i == TEST_MODE_DRV) {
> - change_to_drv_mode(ifobj_rx);
> - if (!shared_netdev)
> - change_to_drv_mode(ifobj_tx);
> - }
> -
> for (j = 0; j < TEST_TYPE_MAX; j++) {
> test_spec_init(&test, ifobj_tx, ifobj_rx, i);
> run_pkt_test(&test, i, j);
> diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
> index 3483ac240b2e..5c66908577ef 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.h
> +++ b/tools/testing/selftests/bpf/xskxceiver.h
> @@ -145,10 +145,11 @@ struct ifobject {
> struct xsk_def_prog *def_prog;
> struct xsk_xdp_drop *xdp_drop;
> struct bpf_map *xskmap;
> + struct bpf_program *xdp_prog;
> + enum test_mode mode;
> int ifindex;
> u32 dst_ip;
> u32 src_ip;
> - u32 xdp_flags;
> u32 bind_flags;
> u16 src_port;
> u16 dst_port;
> @@ -168,6 +169,10 @@ struct test_spec {
> struct ifobject *ifobj_rx;
> struct pkt_stream *tx_pkt_stream_default;
> struct pkt_stream *rx_pkt_stream_default;
> + struct bpf_program *xdp_prog_rx;
> + struct bpf_program *xdp_prog_tx;
> + struct bpf_map *xskmap_rx;
> + struct bpf_map *xskmap_tx;
> u16 total_steps;
> u16 current_step;
> u16 nb_sockets;
> --
> 2.34.1
>
