|SUCCESS| [v5,2/2] test: add test for af_packet

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163317

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-28

Reviewing the patch series for DPDK coding standards and guidelines compliance.

commit sha: (from patch headers)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v5 1/2] net/af_packet: fix indentation

This patch fixes indentation issues by converting spaces to tabs and
changes 'unsigned' to 'unsigned int' in the af_packet driver.

> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index 0ee94e71ea..8303ff5ca9 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c

[ ... ]

> @@ -854,17 +854,17 @@ get_fanout(const char *fanout_mode, int if_index)
>  
>  static int
>  rte_pmd_init_internals(struct rte_vdev_device *dev,
> -                       const int sockfd,
> -                       const unsigned nb_queues,
> -                       unsigned int blocksize,
> -                       unsigned int blockcnt,
> -                       unsigned int framesize,
> -                       unsigned int framecnt,
> +		       const int sockfd,
> +		       const unsigned int nb_queues,
> +		       unsigned int blocksize,
> +		       unsigned int blockcnt,
> +		       unsigned int framesize,
> +		       unsigned int framecnt,
>  		       unsigned int qdisc_bypass,
>  		       const char *fanout_mode,
> -                       struct pmd_internals **internals,
> -                       struct rte_eth_dev **eth_dev,
> -                       struct rte_kvargs *kvlist)
> +		       struct pmd_internals **internals,
> +		       struct rte_eth_dev **eth_dev,
> +		       struct rte_kvargs *kvlist)

Does this code change qdisc_bypass from 'const unsigned int' to
'unsigned int'? The parameter qdisc_bypass appears to lose its const
qualifier compared to the other parameters, while nb_queues gains one.
Is this intentional or does qdisc_bypass need const added for
consistency?

[ ... ]

---

commit sha: (from patch headers)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v5 2/2] test: add test for af_packet

This patch adds comprehensive unit tests for the af_packet PMD.

> diff --git a/app/test/test_pmd_af_packet.c b/app/test/test_pmd_af_packet.c
> new file mode 100644
> index 0000000000..8b36c3b539
> --- /dev/null
> +++ b/app/test/test_pmd_af_packet.c

[ ... ]

> +/*
> + * Create a TAP interface for testing.
> + * Returns fd on success, -1 on failure.
> + */
> +static int
> +create_tap_interface(const char *name)
> +{
> +	struct ifreq ifr;
> +	int fd, ret;
> +
> +	fd = open("/dev/net/tun", O_RDWR);
> +	if (fd < 0) {
> +		printf("Cannot open /dev/net/tun: %s\n", strerror(errno));
> +		printf("(Are you running as root?)\n");
> +		return -1;
> +	}
> +
> +	memset(&ifr, 0, sizeof(ifr));
> +	ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
> +	snprintf(ifr.ifr_name, IFNAMSIZ, "%s", name);
> +
> +	ret = ioctl(fd, TUNSETIFF, &ifr);
> +	if (ret < 0) {
> +		printf("Cannot create TAP interface '%s': %s\n",
> +		       name, strerror(errno));
> +		close(fd);
> +		return -1;
> +	}
> +
> +	/* Bring the interface up */
> +	int sock = socket(AF_INET, SOCK_DGRAM, 0);

Does this code have a resource leak if socket() succeeds but subsequent
operations fail? The socket needs to be closed on all paths.

> +	if (sock >= 0) {
> +		memset(&ifr, 0, sizeof(ifr));
> +		snprintf(ifr.ifr_name, IFNAMSIZ, "%s", name);
> +
> +		/* Get current flags */
> +		if (ioctl(sock, SIOCGIFFLAGS, &ifr) == 0) {
> +			ifr.ifr_flags |= IFF_UP;
> +			ioctl(sock, SIOCSIFFLAGS, &ifr);
> +		}
> +		close(sock);
> +	}
> +
> +	printf("Created TAP interface '%s'\n", name);
> +	return fd;
> +}

[ ... ]

> +/*
> + * Helper: Allocate and prepare a burst of TX mbufs with minimal Ethernet frames.
> + * Returns the number of successfully allocated mbufs.
> + */
> +static unsigned int
> +alloc_tx_mbufs(struct rte_mbuf **bufs, unsigned int count)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	ret = rte_pktmbuf_alloc_bulk(mp, bufs, count);
> +	if (ret != 0)
> +		return 0;
> +
> +	for (i = 0; i < count; i++) {
> +		struct rte_ether_hdr *eth_hdr;
> +		eth_hdr = (struct rte_ether_hdr *)rte_pktmbuf_append(bufs[i],
> +				sizeof(struct rte_ether_hdr) + 46);
> +		if (eth_hdr == NULL) {
> +			/* Free all allocated mbufs on failure */
> +			rte_pktmbuf_free_bulk(bufs, count);

Does this code potentially free uninitialized pointers? When
rte_pktmbuf_append() fails for bufs[i], the function calls
rte_pktmbuf_free_bulk(bufs, count) which will attempt to free bufs[i+1]
through bufs[count-1] that were never initialized by
rte_pktmbuf_alloc_bulk(). Should this be
rte_pktmbuf_free_bulk(bufs, i + 1) instead?

> +			return 0;
> +		}
> +
> +		/* Set broadcast destination and zero source MAC */
> +		memset(&eth_hdr->dst_addr, 0xFF, RTE_ETHER_ADDR_LEN);
> +		memset(&eth_hdr->src_addr, 0x00, RTE_ETHER_ADDR_LEN);
> +		eth_hdr->ether_type = rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV4);
> +	}
> +
> +	return count;
> +}

[ ... ]

> +static int
> +test_af_packet_setup(void)
> +{
> +	char devargs[256];
> +	int ret;
> +
> +	/* Create mempool for mbufs */
> +	mp = rte_pktmbuf_pool_create("af_packet_test_pool", NUM_MBUFS,
> +				     MBUF_CACHE_SIZE, 0,
> +				     RTE_MBUF_DEFAULT_BUF_SIZE,
> +				     rte_socket_id());
> +	if (mp == NULL) {
> +		printf("Failed to create mempool\n");
> +		return -1;
> +	}
> +
> +	/* Create TAP interface for testing */
> +	tap_fd = create_tap_interface(TAP_DEV_NAME);
> +	if (tap_fd >= 0)
> +		tap_created = true;
> +	else {
> +		printf("TAP interface creation failed - tests will be skipped\n");
> +		return 0; /* Return success to allow skipped tests */
> +	}
> +
> +	/* Create and configure af_packet port */
> +	snprintf(devargs, sizeof(devargs), "iface=%s", TAP_DEV_NAME);
> +	ret = create_af_packet_port(AF_PACKET_DEV_NAME, devargs, &port_id);
> +	if (ret != 0) {
> +		printf("Failed to create af_packet port\n");
> +		return -1;
> +	}
> +	port_created = true;
> +
> +	ret = configure_af_packet_port(port_id, 1, 1);
> +	if (ret != 0) {
> +		printf("Failed to configure af_packet port\n");
> +		return -1;
> +	}

Does this code leak resources on error? When
configure_af_packet_port() fails, the function returns -1 without
calling test_af_packet_teardown(), leaving tap_fd, port_id, and mp
allocated. Should there be cleanup here or does the test framework
call teardown on setup failure?

[ ... ]

> +/*
> + * Multi-threaded test context
> + */
> +struct mt_test_ctx {
> +	uint16_t port_id;
> +	RTE_ATOMIC(uint32_t) stop;
> +	uint32_t rx_count;
> +};

Does this code have a potential race condition? The rx_count field is
accessed from multiple threads (incremented in mt_reader_thread() and
read in test_af_packet_multithread()) but is not declared as atomic.
Should rx_count be RTE_ATOMIC(uint32_t) or should the access be
protected?

> +
> +/*
> + * Reader thread function for multi-threaded test
> + */
> +static void *
> +mt_reader_thread(void *arg)
> +{
> +	struct mt_test_ctx *ctx = arg;
> +	struct rte_mbuf *bufs[BURST_SIZE];
> +	uint16_t nb_rx;
> +
> +	while (!rte_atomic_load_explicit(&ctx->stop, rte_memory_order_relaxed)) {
> +		nb_rx = rte_eth_rx_burst(ctx->port_id, 0, bufs, BURST_SIZE);
> +		if (nb_rx > 0) {
> +			ctx->rx_count += nb_rx;
> +			rte_pktmbuf_free_bulk(bufs, nb_rx);
> +		}
> +
> +		rte_delay_us_block(50);
> +	}
> +
> +	return NULL;
> +}