|SUCCESS| [v6,3/3] test: add test for af_packet

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# Review of DPDK AF_PACKET PMD Patch Series

git sha: Not available in patch headers
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v6 1/3] maintainers: nominate myself for af_packet

This patch series updates the AF_PACKET maintainer, fixes indentation
issues, and adds comprehensive unit tests for the AF_PACKET PMD.

## Patch 1/3: MAINTAINERS update

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0f5539f851..723e26dec3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -671,7 +671,7 @@ F: app/test/test_link_bonding*
>  F: examples/bond/
>  
>  Linux AF_PACKET
> -M: John W. Linville <linville at tuxdriver.com>
> +M: Stephen Hemminger <stephen at networkplumber.org>
>  F: drivers/net/af_packet/
>  F: doc/guides/nics/features/afpacket.ini

No issues found.

## Patch 2/3: net/af_packet: fix indentation

> 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
> @@ -79,7 +79,7 @@ struct __rte_cache_aligned pkt_tx_queue {
>  };
>  
>  struct pmd_internals {
> -	unsigned nb_queues;
> +	unsigned int nb_queues;

Does this change from "unsigned" to "unsigned int" preserve consistency
with the rest of the structure? Looking at the broader context, this
appears correct per DPDK coding standards.

[ ... ]

> @@ -602,7 +602,7 @@ eth_dev_close(struct rte_eth_dev *dev)
>  
>  static int
>  eth_link_update(struct rte_eth_dev *dev,
> -                int wait_to_complete __rte_unused)
> +		int wait_to_complete __rte_unused)

The indentation fixes correctly replace spaces with tabs throughout the
file. No issues found.

## Patch 3/3: test: add test for af_packet

> 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

[ ... ]

> +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 leak the socket if the ioctl calls fail? The function
returns -1 after close(sock) only in the success path.

> +	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;
> +}

If sock >= 0 but the SIOCGIFFLAGS ioctl fails, does this still close the
socket properly? Looking at the code, close(sock) is always called if
sock >= 0, so this appears correct.

[ ... ]

> +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);
> +			return 0;
> +		}

Does this code correctly free mbufs when rte_pktmbuf_append() fails for
mbuf i > 0? The code calls rte_pktmbuf_free_bulk(bufs, count), but some
bufs[0..i-1] may have had data appended while bufs[i..count-1] may still
be empty. Is this still safe?

Looking at the DPDK documentation, rte_pktmbuf_free_bulk() should handle
this correctly as it frees all valid mbufs in the array.

[ ... ]

> +static int
> +test_af_packet_loopback(void)
> +{
> +	struct rte_mbuf *tx_bufs[BURST_SIZE];
> +	struct rte_mbuf *rx_bufs[BURST_SIZE];
> +	uint16_t nb_tx, total_rx = 0;
> +	uint64_t elapsed = 0;
> +	unsigned int allocated;
> +	char tap_buf[2048];
> +
> +	if (!tap_created || port_id >= RTE_MAX_ETHPORTS) {
> +		printf("SKIPPED: Port not available (need root)\n");
> +		return TEST_SKIPPED;
> +	}
> +
> +	/* Set TAP fd to non-blocking for reading */
> +	int flags = fcntl(tap_fd, F_GETFL, 0);
> +	fcntl(tap_fd, F_SETFL, flags | O_NONBLOCK);

Does this code check the return value of the first fcntl() call? If
fcntl(tap_fd, F_GETFL, 0) fails, flags will contain -1, and the second
fcntl() call will attempt to set invalid flags.

> +
> +	/* Allocate and send packets */
> +	allocated = alloc_tx_mbufs(tx_bufs, BURST_SIZE);
> +	if (allocated == 0) {
> +		printf("SKIPPED: Could not allocate mbufs\n");
> +		return TEST_SKIPPED;
> +	}

Does this restore the TAP fd flags before returning on allocation
failure?

> +
> +	nb_tx = do_tx_burst(port_id, 0, tx_bufs, allocated);
> +	if (nb_tx == 0) {
> +		printf("SKIPPED: No packets transmitted\n");
> +		return TEST_SKIPPED;
> +	}

Does this restore the TAP fd flags before returning when no packets are
transmitted?

[ ... ]

> +	/* Restore TAP fd flags */
> +	fcntl(tap_fd, F_SETFL, flags);

The function only restores flags on the success path. Does this leak the
modified fd flags on early returns?

[ ... ]

> +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;

Does this code properly synchronize ctx->rx_count updates? The rx_count
field is uint32_t but not declared as atomic. Is there a race condition
between the reader thread updating rx_count and the main thread reading
it?

> +			rte_pktmbuf_free_bulk(bufs, nb_rx);
> +		}
> +
> +		rte_delay_us_block(50);
> +	}
> +
> +	return NULL;
> +}

[ ... ]

> +static int
> +test_af_packet_multithread(void)
> +{
> +	struct mt_test_ctx ctx = {0};
> +	struct rte_mbuf *bufs[BURST_SIZE];
> +	pthread_t reader_tid;
> +	struct timespec start, now;
> +	uint32_t tx_count = 0;
> +	unsigned int allocated;
> +	uint16_t nb_tx;
> +	int ret;
> +
> +	if (!tap_created || port_id >= RTE_MAX_ETHPORTS) {
> +		printf("SKIPPED: Port not available (need root)\n");
> +		return TEST_SKIPPED;
> +	}
> +
> +	ctx.port_id = port_id;
> +
> +	/* Start reader thread */
> +	ret = pthread_create(&reader_tid, NULL, mt_reader_thread, &ctx);
> +	if (ret != 0) {
> +		printf("SKIPPED: Failed to create reader thread: %d\n", ret);
> +		return TEST_SKIPPED;
> +	}
> +
> +	/* Main thread sends packets for the configured duration */
> +	clock_gettime(CLOCK_MONOTONIC, &start);
> +	do {
> +		allocated = alloc_tx_mbufs(bufs, BURST_SIZE / 4);
> +		if (allocated == 0)
> +			break;
> +
> +		nb_tx = do_tx_burst(port_id, 0, bufs, allocated);
> +		tx_count += nb_tx;
> +
> +		rte_delay_us_block(100);
> +		clock_gettime(CLOCK_MONOTONIC, &now);
> +	} while (tx_count < MT_NUM_PACKETS &&
> +		(now.tv_sec - start.tv_sec) < MT_TEST_DURATION_SEC);
> +
> +	/* Signal reader to stop */
> +	rte_atomic_store_explicit(&ctx.stop, 1, rte_memory_order_relaxed);
> +	pthread_join(reader_tid, NULL);
> +
> +	printf("Multi-threaded test: TX=%u, RX=%u (duration=%ds)\n",
> +	       tx_count, ctx.rx_count, MT_TEST_DURATION_SEC);

Does reading ctx.rx_count here properly synchronize with the reader
thread's updates? Should there be a memory barrier or should rx_count be
declared as RTE_ATOMIC(uint32_t)?