Date:   Fri, 27 Jan 2023 18:26:24 +0100
From:   Lorenzo Bianconi <lorenzo@...nel.org>
To:     sdf@...gle.com
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org, davem@...emloft.net,
        kuba@...nel.org, hawk@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, toke@...hat.com, memxor@...il.com,
        alardam@...il.com, saeedm@...dia.com, anthony.l.nguyen@...el.com,
        gospo@...adcom.com, vladimir.oltean@....com, nbd@....name,
        john@...ozen.org, leon@...nel.org, simon.horman@...igine.com,
        aelior@...vell.com, christophe.jaillet@...adoo.fr,
        ecree.xilinx@...il.com, mst@...hat.com, bjorn@...nel.org,
        magnus.karlsson@...el.com, maciej.fijalkowski@...el.com,
        intel-wired-lan@...ts.osuosl.org, lorenzo.bianconi@...hat.com,
        martin.lau@...ux.dev
Subject: Re: [PATCH v3 bpf-next 8/8] selftests/bpf: introduce XDP compliance
 test tool

> On 01/26, Lorenzo Bianconi wrote:

[...]

> 
> Why do we need the namespaces? Why not have two veth peers in the
> current namespace?

I think we can use just a veth pair here, we do not need two, I will fix it.

> 
> (not sure it matters, just wondering)
> 
> > +ret=1
> > +
> > +setup() {
> > +	{
> > +		ip netns add ${NS0}
> > +		ip netns add ${NS1}
> > +
> > +		ip link add v01 index 111 type veth peer name v00 netns ${NS0}
> > +		ip link add v10 index 222 type veth peer name v11 netns ${NS1}
> > +
> > +		ip link set v01 up
> > +		ip addr add 10.10.0.1/24 dev v01
> > +		ip link set v01 address 00:11:22:33:44:55
> > +		ip -n ${NS0} link set dev v00 up
> > +		ip -n ${NS0} addr add 10.10.0.11/24 dev v00
> > +		ip -n ${NS0} route add default via 10.10.0.1
> > +		ip -n ${NS0} link set v00 address 00:12:22:33:44:55
> > +
> > +		ip link set v10 up
> > +		ip addr add 10.10.1.1/24 dev v10
> > +		ip link set v10 address 00:13:22:33:44:55
> > +		ip -n ${NS1} link set dev v11 up
> > +		ip -n ${NS1} addr add 10.10.1.11/24 dev v11
> > +		ip -n ${NS1} route add default via 10.10.1.1
> > +		ip -n ${NS1} link set v11 address 00:14:22:33:44:55
> > +
> > +		sysctl -w net.ipv4.ip_forward=1
> > +		# Enable XDP mode
> > +		ethtool -K v01 gro on
> > +		ethtool -K v01 tx-checksumming off
> > +		ip netns exec ${NS0} ethtool -K v00 gro on
> > +		ip netns exec ${NS0} ethtool -K v00 tx-checksumming off
> > +		ethtool -K v10 gro on
> > +		ethtool -K v10 tx-checksumming off
> > +		ip netns exec ${NS1} ethtool -K v11 gro on
> > +		ip netns exec ${NS1} ethtool -K v11 tx-checksumming off
> > +	} > /dev/null 2>&1
> > +}

[...]
> 
> IIRC, Martin mentioned IPv6 support in the previous version. Should we
> also make the userspace v6 aware by at least using AF_INET6 dualstack
> sockets? I feel like listening on inaddr_any with AF_INET6 should
> get us there without too much pain..

ack, I will fix it.

> 
> > +
> > +	/* start echo channel */
> > +	*echo_sockfd = sockfd;
> > +	err = pthread_create(t, NULL, dut_echo_thread, echo_sockfd);
> > +	if (err) {
> > +		fprintf(stderr, "Failed creating dut_echo thread: %s\n",
> > +			strerror(-err));
> > +		close(sockfd);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dut_attach_xdp_prog(struct xdp_features *skel, int feature,
> > +			       int flags)
> > +{
> > +	struct bpf_program *prog;
> > +	unsigned int key = 0;
> > +	int err, fd = 0;
> > +
> > +	switch (feature) {
> > +	case XDP_FEATURE_TX:
> > +		prog = skel->progs.xdp_do_tx;
> > +		break;
> > +	case XDP_FEATURE_DROP:
> > +	case XDP_FEATURE_ABORTED:
> > +		prog = skel->progs.xdp_do_drop;
> > +		break;
> > +	case XDP_FEATURE_PASS:
> > +		prog = skel->progs.xdp_do_pass;
> > +		break;
> > +	case XDP_FEATURE_NDO_XMIT: {
> > +		struct bpf_devmap_val entry = {
> > +			.ifindex = env.ifindex,
> > +		};
> > +
> > +		err = bpf_map__update_elem(skel->maps.dev_map,
> > +					   &key, sizeof(key),
> > +					   &entry, sizeof(entry), 0);
> > +		if (err < 0)
> > +			return err;
> > +
> > +		fd = bpf_program__fd(skel->progs.xdp_do_redirect_cpumap);
> > +	}
> > +	case XDP_FEATURE_REDIRECT: {
> > +		struct bpf_cpumap_val entry = {
> > +			.qsize = 2048,
> > +			.bpf_prog.fd = fd,
> > +		};
> > +
> > +		err = bpf_map__update_elem(skel->maps.cpu_map,
> > +					   &key, sizeof(key),
> > +					   &entry, sizeof(entry), 0);
> > +		if (err < 0)
> > +			return err;
> > +
> > +		prog = skel->progs.xdp_do_redirect;
> > +		break;
> > +	}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = bpf_xdp_attach(env.ifindex, bpf_program__fd(prog), flags, NULL);
> > +	if (err)
> > +		fprintf(stderr,
> > +			"Failed to attach XDP program to ifindex %d\n",
> > +			env.ifindex);
> > +	return err;
> > +}
> > +
> > +static int __recv_msg(int sockfd, void *buf, size_t bufsize,
> > +		      unsigned int *val, unsigned int val_size)
> > +{
> > +	struct tlv_hdr *tlv = (struct tlv_hdr *)buf;
> > +	int len, n = sizeof(*tlv), i = 0;
> > +
> > +	len = recv(sockfd, buf, bufsize, 0);
> > +	if (len != ntohs(tlv->len))
> > +		return -EINVAL;
> > +
> > +	while (n < len && i < val_size) {
> > +		val[i] = ntohl(tlv->data[i]);
> > +		n += sizeof(tlv->data[0]);
> > +		i++;
> > +	}
> > +
> > +	return i;
> > +}
> > +
> > +static int recv_msg(int sockfd, void *buf, size_t bufsize)
> > +{
> > +	return __recv_msg(sockfd, buf, bufsize, NULL, 0);
> > +}
> > +
> > +static int dut_run(struct xdp_features *skel)
> > +{
> > +	int flags = XDP_FLAGS_UPDATE_IF_NOEXIST | XDP_FLAGS_DRV_MODE;
> > +	int state, err, sockfd, ctrl_sockfd, echo_sockfd, optval = 1;
> > +	struct sockaddr_in ctrl_addr, addr = {
> > +		.sin_family = AF_INET,
> > +		.sin_addr.s_addr = htonl(INADDR_ANY),
> > +		.sin_port = htons(DUT_CTRL_PORT),
> > +	};
> > +	unsigned int len = sizeof(ctrl_addr);
> > +	pthread_t dut_thread;
> > +
> 
> [..]
> 
> > +	sockfd = socket(AF_INET, SOCK_STREAM, 0);
> > +	if (sockfd < 0) {
> > +		fprintf(stderr, "Failed to create DUT socket\n");
> > +		return -errno;
> > +	}
> > +
> > +	err = setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &optval,
> > +			 sizeof(optval));
> > +	if (err < 0) {
> > +		fprintf(stderr, "Failed sockopt on DUT socket\n");
> > +		return -errno;
> > +	}
> > +
> > +	err = bind(sockfd, (struct sockaddr *)&addr, sizeof(addr));
> > +	if (err < 0) {
> > +		fprintf(stderr, "Failed to bind DUT socket\n");
> > +		return -errno;
> > +	}
> > +
> > +	err = listen(sockfd, 5);
> > +	if (err) {
> > +		fprintf(stderr, "Failed to listen DUT socket\n");
> > +		return -errno;
> > +	}
> 
> Should we use start_server from network_helpers.h here?

ack, I will use it.

> 
> > +
> > +	ctrl_sockfd = accept(sockfd, (struct sockaddr *)&ctrl_addr, &len);
> > +	if (ctrl_sockfd < 0) {
> > +		fprintf(stderr, "Failed to accept connection on DUT socket\n");
> > +		close(sockfd);
> > +		return -errno;
> > +	}
> > +

[...]

> 
> There is also connect_to_fd, maybe we can use that? It should take
> care of the timeouts.. (requires plumbing server_fd, not sure whether
> it's a problem or not)

please correct me if I am wrong, but in order to have server_fd it is mandatory
both tester and DUT are running on the same process, right? Here, I guess 99% of
the times DUT and tester will run on two separated devices. Agree?

Regards,
Lorenzo

> 
> > +
> > +	if (i == 10) {
> > +		fprintf(stderr, "Failed to connect to the DUT\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	err = __send_and_recv_msg(sockfd, CMD_GET_XDP_CAP, val,
> > ARRAY_SIZE(val));
> > +	if (err < 0) {
> > +		close(sockfd);
> > +		return err;
> > +	}
> > +
> > +	advertised_cap = tester_collect_advertised_cap(val[0]);
> > +
> > +	err = bpf_xdp_attach(env.ifindex,
> > +			     bpf_program__fd(skel->progs.xdp_tester),
> > +			     flags, NULL);
> > +	if (err) {
> > +		fprintf(stderr, "Failed to attach XDP program to ifindex %d\n",
> > +			env.ifindex);
> > +		goto out;
> > +	}
> > +
> > +	err = send_and_recv_msg(sockfd, CMD_START);
> > +	if (err)
> > +		goto out;
> > +
> > +	for (i = 0; i < 10 && !exiting; i++) {
> > +		err = send_echo_msg();
> > +		if (err < 0)
> > +			goto out;
> > +
> > +		sleep(1);
> > +	}
> > +
> > +	err = __send_and_recv_msg(sockfd, CMD_GET_STATS, val, ARRAY_SIZE(val));
> > +	if (err)
> > +		goto out;
> > +
> > +	/* stop the test */
> > +	err = send_and_recv_msg(sockfd, CMD_STOP);
> > +	/* send a new echo message to wake echo thread of the dut */
> > +	send_echo_msg();
> > +
> > +	detected_cap = tester_collect_detected_cap(skel, val[0]);
> > +
> > +	fprintf(stdout, "Feature %s: [%s][%s]\n",
> > get_xdp_feature_str(env.feature),
> > +		detected_cap ? GREEN("DETECTED") : RED("NOT DETECTED"),
> > +		advertised_cap ? GREEN("ADVERTISED") : RED("NOT ADVERTISED"));
> > +out:
> > +	bpf_xdp_detach(env.ifindex, flags, NULL);
> > +	close(sockfd);
> > +	return err < 0 ? err : 0;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	struct xdp_features *skel;
> > +	int err;
> > +
> > +	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
> > +	libbpf_set_print(libbpf_print_fn);
> > +
> > +	signal(SIGINT, sig_handler);
> > +	signal(SIGTERM, sig_handler);
> > +
> > +	set_env_defaul();
> > +
> > +	/* Parse command line arguments */
> > +	err = argp_parse(&argp, argc, argv, 0, NULL, NULL);
> > +	if (err)
> > +		return err;
> > +
> > +	if (env.ifindex < 0) {
> > +		fprintf(stderr, "Invalid ifindex\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* Load and verify BPF application */
> > +	skel = xdp_features__open();
> > +	if (!skel) {
> > +		fprintf(stderr, "Failed to open and load BPF skeleton\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	skel->rodata->expected_feature = env.feature;
> > +	skel->rodata->dut_ip = env.dut_ip;
> > +	skel->rodata->tester_ip = env.tester_ip;
> > +
> > +	/* Load & verify BPF programs */
> > +	err = xdp_features__load(skel);
> > +	if (err) {
> > +		fprintf(stderr, "Failed to load and verify BPF skeleton\n");
> > +		goto cleanup;
> > +	}
> > +
> > +	err = xdp_features__attach(skel);
> > +	if (err) {
> > +		fprintf(stderr, "Failed to attach BPF skeleton\n");
> > +		goto cleanup;
> > +	}
> > +
> > +	if (env.tester) {
> > +		/* Tester */
> > +		fprintf(stdout, "Starting tester on device %d\n", env.ifindex);
> > +		err = tester_run(skel);
> > +	} else {
> > +		/* DUT */
> > +		fprintf(stdout, "Starting DUT on device %d\n", env.ifindex);
> > +		err = dut_run(skel);
> > +	}
> > +
> > +cleanup:
> > +	xdp_features__destroy(skel);
> > +
> > +	return err < 0 ? -err : 0;
> > +}
> > diff --git a/tools/testing/selftests/bpf/xdp_features.h
> > b/tools/testing/selftests/bpf/xdp_features.h
> > new file mode 100644
> > index 000000000000..28d7614c4f02
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/xdp_features.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/* test commands */
> > +enum test_commands {
> > +	CMD_STOP,		/* CMD */
> > +	CMD_START,		/* CMD + xdp feature */
> > +	CMD_ECHO,		/* CMD */
> > +	CMD_ACK,		/* CMD + data */
> > +	CMD_GET_XDP_CAP,	/* CMD */
> > +	CMD_GET_STATS,		/* CMD */
> > +};
> > +
> > +#define DUT_CTRL_PORT	12345
> > +#define DUT_ECHO_PORT	12346
> > +
> > +struct tlv_hdr {
> > +	__be16 type;
> > +	__be16 len;
> > +	__be32 data[];
> > +};
> > +
> > +enum {
> > +	XDP_FEATURE_ABORTED,
> > +	XDP_FEATURE_DROP,
> > +	XDP_FEATURE_PASS,
> > +	XDP_FEATURE_TX,
> > +	XDP_FEATURE_REDIRECT,
> > +	XDP_FEATURE_NDO_XMIT,
> > +	XDP_FEATURE_XSK_ZEROCOPY,
> > +	XDP_FEATURE_HW_OFFLOAD,
> > +	XDP_FEATURE_RX_SG,
> > +	XDP_FEATURE_NDO_XMIT_SG,
> > +};
> > --
> > 2.39.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists