[PATCH bpf-next] selftests: xsk: fix bpf_res cleanup test

[PATCH bpf-next] selftests: xsk: fix bpf_res cleanup test

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

After commit 710ad98c363a ("veth: Do not record rx queue hint in
veth_xmit"), veth no longer receives traffic on the same queue as it
was sent on. This breaks the bpf_res test for the AF_XDP selftests as
the socket tied to queue 1 will not receive traffic anymore. Modify
the test so that two sockets are tied to queue id 0 using a shared
umem instead. When killing the first socket enter the second socket
into the xskmap so that traffic will flow to it. This will still test
that the resources are not cleaned up until after the second socket
dies, without having to rely on veth supporting rx_queue hints.

Reported-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/testing/selftests/bpf/xdpxceiver.c | 80 +++++++++++++++---------
 tools/testing/selftests/bpf/xdpxceiver.h |  2 +-
 2 files changed, 50 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
index ffa5502ad95e..5f8296d29e77 100644
--- a/tools/testing/selftests/bpf/xdpxceiver.c
+++ b/tools/testing/selftests/bpf/xdpxceiver.c
@@ -266,22 +266,24 @@ static int xsk_configure_umem(struct xsk_umem_info *umem, void *buffer, u64 size
 }
 
 static int xsk_configure_socket(struct xsk_socket_info *xsk, struct xsk_umem_info *umem,
-				struct ifobject *ifobject, u32 qid)
+				struct ifobject *ifobject, bool shared)
 {
-	struct xsk_socket_config cfg;
+	struct xsk_socket_config cfg = {};
 	struct xsk_ring_cons *rxr;
 	struct xsk_ring_prod *txr;
 
 	xsk->umem = umem;
 	cfg.rx_size = xsk->rxqsize;
 	cfg.tx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS;
-	cfg.libbpf_flags = 0;
+	cfg.libbpf_flags = XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD;
 	cfg.xdp_flags = ifobject->xdp_flags;
 	cfg.bind_flags = ifobject->bind_flags;
+	if (shared)
+		cfg.bind_flags |= XDP_SHARED_UMEM;
 
 	txr = ifobject->tx_on ? &xsk->tx : NULL;
 	rxr = ifobject->rx_on ? &xsk->rx : NULL;
-	return xsk_socket__create(&xsk->xsk, ifobject->ifname, qid, umem->umem, rxr, txr, &cfg);
+	return xsk_socket__create(&xsk->xsk, ifobject->ifname, 0, umem->umem, rxr, txr, &cfg);
 }
 
 static struct option long_options[] = {
@@ -387,7 +389,6 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
 	for (i = 0; i < MAX_INTERFACES; i++) {
 		struct ifobject *ifobj = i ? ifobj_rx : ifobj_tx;
 
-		ifobj->umem = &ifobj->umem_arr[0];
 		ifobj->xsk = &ifobj->xsk_arr[0];
 		ifobj->use_poll = false;
 		ifobj->pacing_on = true;
@@ -401,11 +402,12 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
 			ifobj->tx_on = false;
 		}
 
+		memset(ifobj->umem, 0, sizeof(*ifobj->umem));
+		ifobj->umem->num_frames = DEFAULT_UMEM_BUFFERS;
+		ifobj->umem->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
+
 		for (j = 0; j < MAX_SOCKETS; j++) {
-			memset(&ifobj->umem_arr[j], 0, sizeof(ifobj->umem_arr[j]));
 			memset(&ifobj->xsk_arr[j], 0, sizeof(ifobj->xsk_arr[j]));
-			ifobj->umem_arr[j].num_frames = DEFAULT_UMEM_BUFFERS;
-			ifobj->umem_arr[j].frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
 			ifobj->xsk_arr[j].rxqsize = XSK_RING_CONS__DEFAULT_NUM_DESCS;
 		}
 	}
@@ -950,7 +952,10 @@ static void tx_stats_validate(struct ifobject *ifobject)
 
 static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
 {
+	u64 umem_sz = ifobject->umem->num_frames * ifobject->umem->frame_size;
 	int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE;
+	int ret, ifindex;
+	void *bufs;
 	u32 i;
 
 	ifobject->ns_fd = switch_namespace(ifobject->nsname);
@@ -958,23 +963,20 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
 	if (ifobject->umem->unaligned_mode)
 		mmap_flags |= MAP_HUGETLB;
 
-	for (i = 0; i < test->nb_sockets; i++) {
-		u64 umem_sz = ifobject->umem->num_frames * ifobject->umem->frame_size;
-		u32 ctr = 0;
-		void *bufs;
-		int ret;
+	bufs = mmap(NULL, umem_sz, PROT_READ | PROT_WRITE, mmap_flags, -1, 0);
+	if (bufs == MAP_FAILED)
+		exit_with_error(errno);
 
-		bufs = mmap(NULL, umem_sz, PROT_READ | PROT_WRITE, mmap_flags, -1, 0);
-		if (bufs == MAP_FAILED)
-			exit_with_error(errno);
+	ret = xsk_configure_umem(ifobject->umem, bufs, umem_sz);
+	if (ret)
+		exit_with_error(-ret);
 
-		ret = xsk_configure_umem(&ifobject->umem_arr[i], bufs, umem_sz);
-		if (ret)
-			exit_with_error(-ret);
+	for (i = 0; i < test->nb_sockets; i++) {
+		u32 ctr = 0;
 
 		while (ctr++ < SOCK_RECONF_CTR) {
-			ret = xsk_configure_socket(&ifobject->xsk_arr[i], &ifobject->umem_arr[i],
-						   ifobject, i);
+			ret = xsk_configure_socket(&ifobject->xsk_arr[i], ifobject->umem,
+						   ifobject, !!i);
 			if (!ret)
 				break;
 
@@ -985,8 +987,22 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
 		}
 	}
 
-	ifobject->umem = &ifobject->umem_arr[0];
 	ifobject->xsk = &ifobject->xsk_arr[0];
+
+	if (!ifobject->rx_on)
+		return;
+
+	ifindex = if_nametoindex(ifobject->ifname);
+	if (!ifindex)
+		exit_with_error(errno);
+
+	ret = xsk_setup_xdp_prog(ifindex, &ifobject->xsk_map_fd);
+	if (ret)
+		exit_with_error(-ret);
+
+	ret = xsk_socket__update_xskmap(ifobject->xsk->xsk, ifobject->xsk_map_fd);
+	if (ret)
+		exit_with_error(-ret);
 }
 
 static void testapp_cleanup_xsk_res(struct ifobject *ifobj)
@@ -1142,14 +1158,16 @@ static void testapp_bidi(struct test_spec *test)
 
 static void swap_xsk_resources(struct ifobject *ifobj_tx, struct ifobject *ifobj_rx)
 {
+	int ret;
+
 	xsk_socket__delete(ifobj_tx->xsk->xsk);
-	xsk_umem__delete(ifobj_tx->umem->umem);
 	xsk_socket__delete(ifobj_rx->xsk->xsk);
-	xsk_umem__delete(ifobj_rx->umem->umem);
-	ifobj_tx->umem = &ifobj_tx->umem_arr[1];
 	ifobj_tx->xsk = &ifobj_tx->xsk_arr[1];
-	ifobj_rx->umem = &ifobj_rx->umem_arr[1];
 	ifobj_rx->xsk = &ifobj_rx->xsk_arr[1];
+
+	ret = xsk_socket__update_xskmap(ifobj_rx->xsk->xsk, ifobj_rx->xsk_map_fd);
+	if (ret)
+		exit_with_error(-ret);
 }
 
 static void testapp_bpf_res(struct test_spec *test)
@@ -1408,13 +1426,13 @@ static struct ifobject *ifobject_create(void)
 	if (!ifobj->xsk_arr)
 		goto out_xsk_arr;
 
-	ifobj->umem_arr = calloc(MAX_SOCKETS, sizeof(*ifobj->umem_arr));
-	if (!ifobj->umem_arr)
-		goto out_umem_arr;
+	ifobj->umem = calloc(1, sizeof(*ifobj->umem));
+	if (!ifobj->umem)
+		goto out_umem;
 
 	return ifobj;
 
-out_umem_arr:
+out_umem:
 	free(ifobj->xsk_arr);
 out_xsk_arr:
 	free(ifobj);
@@ -1423,7 +1441,7 @@ static struct ifobject *ifobject_create(void)
 
 static void ifobject_delete(struct ifobject *ifobj)
 {
-	free(ifobj->umem_arr);
+	free(ifobj->umem);
 	free(ifobj->xsk_arr);
 	free(ifobj);
 }
diff --git a/tools/testing/selftests/bpf/xdpxceiver.h b/tools/testing/selftests/bpf/xdpxceiver.h
index 2f705f44b748..62a3e6388632 100644
--- a/tools/testing/selftests/bpf/xdpxceiver.h
+++ b/tools/testing/selftests/bpf/xdpxceiver.h
@@ -125,10 +125,10 @@ struct ifobject {
 	struct xsk_socket_info *xsk;
 	struct xsk_socket_info *xsk_arr;
 	struct xsk_umem_info *umem;
-	struct xsk_umem_info *umem_arr;
 	thread_func_t func_ptr;
 	struct pkt_stream *pkt_stream;
 	int ns_fd;
+	int xsk_map_fd;
 	u32 dst_ip;
 	u32 src_ip;
 	u32 xdp_flags;

base-commit: 74bb0f0c299cdc9c68cb3bc8f452e5812aa9eab0
-- 
2.34.1

RE: [PATCH bpf-next] selftests: xsk: fix bpf_res cleanup test

[John Fastabend]

Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> After commit 710ad98c363a ("veth: Do not record rx queue hint in
> veth_xmit"), veth no longer receives traffic on the same queue as it
> was sent on. This breaks the bpf_res test for the AF_XDP selftests as
> the socket tied to queue 1 will not receive traffic anymore. Modify
> the test so that two sockets are tied to queue id 0 using a shared
> umem instead. When killing the first socket enter the second socket
> into the xskmap so that traffic will flow to it. This will still test
> that the resources are not cleaned up until after the second socket
> dies, without having to rely on veth supporting rx_queue hints.
> 
> Reported-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next] selftests: xsk: fix bpf_res cleanup test

[Maciej Fijalkowski]

On Tue, Jan 25, 2022 at 09:29:45AM +0100, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> After commit 710ad98c363a ("veth: Do not record rx queue hint in
> veth_xmit"), veth no longer receives traffic on the same queue as it
> was sent on. This breaks the bpf_res test for the AF_XDP selftests as
> the socket tied to queue 1 will not receive traffic anymore. Modify
> the test so that two sockets are tied to queue id 0 using a shared
> umem instead. When killing the first socket enter the second socket
> into the xskmap so that traffic will flow to it. This will still test
> that the resources are not cleaned up until after the second socket
> dies, without having to rely on veth supporting rx_queue hints.
> 
> Reported-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>

Thanks, makes sense as we discussed this internally. I didn't have a
chance to apologize to the mess I introduced with the stuff that
710ad98c363a fixed, so sorry everyone, I didn't mean that :<

FWIW I think now we can drop the numtxqueues numrxqueues args from
test_xsk.sh that are used when creating veth pair.

Besides that:
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  tools/testing/selftests/bpf/xdpxceiver.c | 80 +++++++++++++++---------
>  tools/testing/selftests/bpf/xdpxceiver.h |  2 +-
>  2 files changed, 50 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
> index ffa5502ad95e..5f8296d29e77 100644
> --- a/tools/testing/selftests/bpf/xdpxceiver.c
> +++ b/tools/testing/selftests/bpf/xdpxceiver.c
> @@ -266,22 +266,24 @@ static int xsk_configure_umem(struct xsk_umem_info *umem, void *buffer, u64 size
>  }
>  
>  static int xsk_configure_socket(struct xsk_socket_info *xsk, struct xsk_umem_info *umem,
> -				struct ifobject *ifobject, u32 qid)
> +				struct ifobject *ifobject, bool shared)
>  {
> -	struct xsk_socket_config cfg;
> +	struct xsk_socket_config cfg = {};
>  	struct xsk_ring_cons *rxr;
>  	struct xsk_ring_prod *txr;
>  
>  	xsk->umem = umem;
>  	cfg.rx_size = xsk->rxqsize;
>  	cfg.tx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS;
> -	cfg.libbpf_flags = 0;
> +	cfg.libbpf_flags = XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD;
>  	cfg.xdp_flags = ifobject->xdp_flags;
>  	cfg.bind_flags = ifobject->bind_flags;
> +	if (shared)
> +		cfg.bind_flags |= XDP_SHARED_UMEM;
>  
>  	txr = ifobject->tx_on ? &xsk->tx : NULL;
>  	rxr = ifobject->rx_on ? &xsk->rx : NULL;
> -	return xsk_socket__create(&xsk->xsk, ifobject->ifname, qid, umem->umem, rxr, txr, &cfg);
> +	return xsk_socket__create(&xsk->xsk, ifobject->ifname, 0, umem->umem, rxr, txr, &cfg);
>  }
>  
>  static struct option long_options[] = {
> @@ -387,7 +389,6 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  	for (i = 0; i < MAX_INTERFACES; i++) {
>  		struct ifobject *ifobj = i ? ifobj_rx : ifobj_tx;
>  
> -		ifobj->umem = &ifobj->umem_arr[0];
>  		ifobj->xsk = &ifobj->xsk_arr[0];
>  		ifobj->use_poll = false;
>  		ifobj->pacing_on = true;
> @@ -401,11 +402,12 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  			ifobj->tx_on = false;
>  		}
>  
> +		memset(ifobj->umem, 0, sizeof(*ifobj->umem));
> +		ifobj->umem->num_frames = DEFAULT_UMEM_BUFFERS;
> +		ifobj->umem->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
> +
>  		for (j = 0; j < MAX_SOCKETS; j++) {
> -			memset(&ifobj->umem_arr[j], 0, sizeof(ifobj->umem_arr[j]));
>  			memset(&ifobj->xsk_arr[j], 0, sizeof(ifobj->xsk_arr[j]));
> -			ifobj->umem_arr[j].num_frames = DEFAULT_UMEM_BUFFERS;
> -			ifobj->umem_arr[j].frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
>  			ifobj->xsk_arr[j].rxqsize = XSK_RING_CONS__DEFAULT_NUM_DESCS;
>  		}
>  	}
> @@ -950,7 +952,10 @@ static void tx_stats_validate(struct ifobject *ifobject)
>  
>  static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
>  {
> +	u64 umem_sz = ifobject->umem->num_frames * ifobject->umem->frame_size;
>  	int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE;
> +	int ret, ifindex;
> +	void *bufs;
>  	u32 i;
>  
>  	ifobject->ns_fd = switch_namespace(ifobject->nsname);
> @@ -958,23 +963,20 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
>  	if (ifobject->umem->unaligned_mode)
>  		mmap_flags |= MAP_HUGETLB;
>  
> -	for (i = 0; i < test->nb_sockets; i++) {
> -		u64 umem_sz = ifobject->umem->num_frames * ifobject->umem->frame_size;
> -		u32 ctr = 0;
> -		void *bufs;
> -		int ret;
> +	bufs = mmap(NULL, umem_sz, PROT_READ | PROT_WRITE, mmap_flags, -1, 0);
> +	if (bufs == MAP_FAILED)
> +		exit_with_error(errno);
>  
> -		bufs = mmap(NULL, umem_sz, PROT_READ | PROT_WRITE, mmap_flags, -1, 0);
> -		if (bufs == MAP_FAILED)
> -			exit_with_error(errno);
> +	ret = xsk_configure_umem(ifobject->umem, bufs, umem_sz);
> +	if (ret)
> +		exit_with_error(-ret);
>  
> -		ret = xsk_configure_umem(&ifobject->umem_arr[i], bufs, umem_sz);
> -		if (ret)
> -			exit_with_error(-ret);
> +	for (i = 0; i < test->nb_sockets; i++) {
> +		u32 ctr = 0;
>  
>  		while (ctr++ < SOCK_RECONF_CTR) {
> -			ret = xsk_configure_socket(&ifobject->xsk_arr[i], &ifobject->umem_arr[i],
> -						   ifobject, i);
> +			ret = xsk_configure_socket(&ifobject->xsk_arr[i], ifobject->umem,
> +						   ifobject, !!i);
>  			if (!ret)
>  				break;
>  
> @@ -985,8 +987,22 @@ static void thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
>  		}
>  	}
>  
> -	ifobject->umem = &ifobject->umem_arr[0];
>  	ifobject->xsk = &ifobject->xsk_arr[0];
> +
> +	if (!ifobject->rx_on)
> +		return;
> +
> +	ifindex = if_nametoindex(ifobject->ifname);
> +	if (!ifindex)
> +		exit_with_error(errno);
> +
> +	ret = xsk_setup_xdp_prog(ifindex, &ifobject->xsk_map_fd);
> +	if (ret)
> +		exit_with_error(-ret);
> +
> +	ret = xsk_socket__update_xskmap(ifobject->xsk->xsk, ifobject->xsk_map_fd);
> +	if (ret)
> +		exit_with_error(-ret);
>  }
>  
>  static void testapp_cleanup_xsk_res(struct ifobject *ifobj)
> @@ -1142,14 +1158,16 @@ static void testapp_bidi(struct test_spec *test)
>  
>  static void swap_xsk_resources(struct ifobject *ifobj_tx, struct ifobject *ifobj_rx)
>  {
> +	int ret;
> +
>  	xsk_socket__delete(ifobj_tx->xsk->xsk);
> -	xsk_umem__delete(ifobj_tx->umem->umem);
>  	xsk_socket__delete(ifobj_rx->xsk->xsk);
> -	xsk_umem__delete(ifobj_rx->umem->umem);
> -	ifobj_tx->umem = &ifobj_tx->umem_arr[1];
>  	ifobj_tx->xsk = &ifobj_tx->xsk_arr[1];
> -	ifobj_rx->umem = &ifobj_rx->umem_arr[1];
>  	ifobj_rx->xsk = &ifobj_rx->xsk_arr[1];
> +
> +	ret = xsk_socket__update_xskmap(ifobj_rx->xsk->xsk, ifobj_rx->xsk_map_fd);
> +	if (ret)
> +		exit_with_error(-ret);
>  }
>  
>  static void testapp_bpf_res(struct test_spec *test)
> @@ -1408,13 +1426,13 @@ static struct ifobject *ifobject_create(void)
>  	if (!ifobj->xsk_arr)
>  		goto out_xsk_arr;
>  
> -	ifobj->umem_arr = calloc(MAX_SOCKETS, sizeof(*ifobj->umem_arr));
> -	if (!ifobj->umem_arr)
> -		goto out_umem_arr;
> +	ifobj->umem = calloc(1, sizeof(*ifobj->umem));
> +	if (!ifobj->umem)
> +		goto out_umem;
>  
>  	return ifobj;
>  
> -out_umem_arr:
> +out_umem:
>  	free(ifobj->xsk_arr);
>  out_xsk_arr:
>  	free(ifobj);
> @@ -1423,7 +1441,7 @@ static struct ifobject *ifobject_create(void)
>  
>  static void ifobject_delete(struct ifobject *ifobj)
>  {
> -	free(ifobj->umem_arr);
> +	free(ifobj->umem);
>  	free(ifobj->xsk_arr);
>  	free(ifobj);
>  }
> diff --git a/tools/testing/selftests/bpf/xdpxceiver.h b/tools/testing/selftests/bpf/xdpxceiver.h
> index 2f705f44b748..62a3e6388632 100644
> --- a/tools/testing/selftests/bpf/xdpxceiver.h
> +++ b/tools/testing/selftests/bpf/xdpxceiver.h
> @@ -125,10 +125,10 @@ struct ifobject {
>  	struct xsk_socket_info *xsk;
>  	struct xsk_socket_info *xsk_arr;
>  	struct xsk_umem_info *umem;
> -	struct xsk_umem_info *umem_arr;
>  	thread_func_t func_ptr;
>  	struct pkt_stream *pkt_stream;
>  	int ns_fd;
> +	int xsk_map_fd;
>  	u32 dst_ip;
>  	u32 src_ip;
>  	u32 xdp_flags;
> 
> base-commit: 74bb0f0c299cdc9c68cb3bc8f452e5812aa9eab0
> -- 
> 2.34.1
> 

Re: [PATCH bpf-next] selftests: xsk: fix bpf_res cleanup test

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Tue, 25 Jan 2022 09:29:45 +0100 you wrote:
> From: Magnus Karlsson <magnus.karlsson@intel.com>
> 
> After commit 710ad98c363a ("veth: Do not record rx queue hint in
> veth_xmit"), veth no longer receives traffic on the same queue as it
> was sent on. This breaks the bpf_res test for the AF_XDP selftests as
> the socket tied to queue 1 will not receive traffic anymore. Modify
> the test so that two sockets are tied to queue id 0 using a shared
> umem instead. When killing the first socket enter the second socket
> into the xskmap so that traffic will flow to it. This will still test
> that the resources are not cleaned up until after the second socket
> dies, without having to rely on veth supporting rx_queue hints.
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests: xsk: fix bpf_res cleanup test
    https://git.kernel.org/bpf/bpf-next/c/3b22523bca02

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html