On Sun, Jul 26, 2026 at 7:27 PM Stephen Hemminger <[email protected]> wrote: > > On Mon, 20 Jul 2026 14:06:56 +0200 > Maxime Leroy <[email protected]> wrote: > > > The tap PMD registers each queue file descriptor for Rx interrupts but the > > feature is not usable through the generic API. This series fixes two > > distinct issues against the original Rx interrupt support, plus a small > > type cleanup. > > > > Patch 1 adds the missing rx_queue_intr_enable/disable ops. Without them > > rte_eth_dev_rx_intr_enable() returns -ENOTSUP, so applications that arm > > queues before sleeping (l3fwd-power and similar) treat tap as having no > > interrupt support and fall back to polling. > > > > Patch 2 fixes a traffic stall specific to interrupt mode: the Rx burst > > skips reading the queue fd until the SIGIO trigger advances, but in > > interrupt mode the application wakes through epoll, a distinct signal, so > > the burst can return 0 right after a wakeup and the fd stays readable with > > no further edge. The fd is now drained unconditionally in interrupt mode > > and the SIGIO trigger is not armed on the data queue fds. > > > > Patch 3 converts the driver's int-as-boolean fields to bool. > > > > Patches 1 and 2 are candidates for stable. > > > > v2: > > - use bool for the new interrupt-mode fields instead of int (Stephen); > > both fit existing padding, so neither struct grows > > - add patch 3 converting the existing int booleans to bool > > > > The coding_style.rst note that discouraged bool in structures no longer > > matches DPDK practice; it is dropped by a separate patch. > > > > Maxime Leroy (3): > > net/tap: support Rx queue interrupt enable/disable > > net/tap: drain queue FD in Rx interrupt mode > > net/tap: use bool for boolean flags > > > > drivers/net/tap/rte_eth_tap.c | 25 +++++++++++++++++++- > > drivers/net/tap/rte_eth_tap.h | 11 ++++++--- > > drivers/net/tap/tap_flow.c | 4 ++-- > > drivers/net/tap/tap_intr.c | 44 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 78 insertions(+), 6 deletions(-) > > > > Like this but detailed AI review found some things. > > Review of [PATCH v2 0/3] net/tap: Rx interrupt fixes > > Applied clean to 38f72e5. All three commits build independently under > -Dwerror=true, so the series is bisect-safe. No compiler diagnostics. > > Verified against a running tap port (/dev/net/tun, kernel-side traffic), > not just by reading the diffs. > > > Patch 1/3: net/tap: support Rx queue interrupt enable/disable > ------------------------------------------------------------ > > Confirmed working. rte_eth_dev_rx_intr_enable()/disable() return 0 in > interrupt mode where they previously returned -ENOTSUP, and an > epoll-driven receive loop delivers 100% of offered packets. > > The -ENOTSUP branch is not dead code: rte_eth_dev_rx_intr_enable() > validates only the queue id and the op pointer, never intr_conf.rxq, so > the driver is the sole guard. > > tap.ini already advertises "Rx interrupt = Y", so no features-matrix > change is needed -- that overclaim is precisely what this patch fixes. > > Info: pmd->intr_mode duplicates dev->data->dev_conf.intr_conf.rxq, which > tap_rx_intr_vec_install() already reads directly. The rx_queue copy earns > its place on the fast path; the pmd_internals copy is redundant state > that can drift. Not blocking. > It is not a copy of the current value but the one locked by the first configure. rte_eth_dev_configure() has already written the new dev_conf by the time tap_dev_configure() runs, so comparing against dev->data->dev_conf.intr_conf.rxq would compare the new value with itself and the mode change could never be detected.
> > Error (pre-existing, but patch 2 makes it reachable) > --------------------------------------------------- > > tap_tx_queue_release() dereferences dev->data->rx_queues after ethdev has > already freed it: > > /* lib/ethdev/rte_ethdev.c */ > reset_queues: > eth_dev_rx_queue_config(dev, 0); /* rte_free(rx_queues); = NULL */ > eth_dev_tx_queue_config(dev, 0); /* -> tap_tx_queue_release() */ > > /* drivers/net/tap/rte_eth_tap.c */ > if (dev->data->rx_queues[qid] == NULL) /* rx_queues is NULL here */ > > Confirmed by three independent reproducers, all segfaulting in > tap_tx_queue_release(): > > 1. reconfigure that trips patch 2's new intr_mode check > 2. reconfigure that trips the pre-existing nb_rx != nb_tx check > (intr_conf.rxq never set -- so this is NOT introduced by the series) > 3. rte_eth_dev_internal_reset() on a normally configured port, with no > dev_configure failure at all > > Path 3 matters independently of this series: bonding's member_remove() > calls rte_eth_dev_internal_reset() unconditionally, so removing a tap > port from a bonding device crashes on current main. Agreed. I will fix it in the next version. > > Both ethdev call sites free rx before tx, so tap_rx_queue_release()'s > symmetric read of dev->data->tx_queues[qid] is always safe. Only the tx > side needs the guard, though guarding both is cheap. > > This is not your bug. But patch 2 adds a second failure return to > tap_dev_configure() that fires on an ordinary reconfigure, and the commit > message points users straight at it -- an application trying to toggle > interrupt mode crashes instead of receiving -ENOTSUP. Suggest a > preceding patch in the series fixing the deref, with its own Fixes: and > Cc: [email protected]. > > > Patch 2/3: net/tap: drain queue FD in Rx interrupt mode > ------------------------------------------------------- > > The mechanism works as designed. Verified at the kernel fd-flag level > (/proc/PID/fdinfo) that in interrupt mode the data queue fd is created > without O_ASYNC while the keep-alive fd retains it: > > patch 1 only : ka fd O_ASYNC=yes, data fd O_ASYNC=yes > full series : ka fd O_ASYNC=yes, data fd O_ASYNC=no > > The early return in tun_alloc() sits after the O_NONBLOCK fcntl, so > nothing required is skipped and there is no fd leak. Reading dev_private > is safe: rte_eth_dev_release_port() frees it on close, so "close and > reopen the port to change it" is accurate. > > Warning: I could not reproduce the stall this patch fixes. With patch 1 > applied alone -- SIGIO still armed on the data fd -- three traffic > patterns all delivered every packet with zero empty bursts after a > wakeup: > > - low-rate broadcast ARP : 117/117 rx, 0 empty after wake > - flood ping, static neighbour : 437/437 rx, 0 empty after wake > - strict l3fwd-power-style loop > (one burst per wakeup, no > re-poll) with bursty traffic : 100/100 rx, 0 empty after wake > > The full series produced identical numbers in each case. Single queue, > single core, kernel-side tap traffic, so this does not disprove the race > -- it may need multiple queues, higher rates than the tap path gives, or > signal-queue pressure. But the justification is not demonstrated by any > reproducer I could build, and the patch has real cost: a permanent > restriction that interrupt mode cannot be toggled after configure, a new > error path that exposes the crash above, a fast-path branch, and a > readv() syscall on every idle burst (exactly what the SIGIO trigger > exists to avoid). > > Could you share a reproducer, or describe the configuration where the > stall appears? Note that the Rx interrupt epoll registration is > edge-triggered (EPOLLIN|EPOLLPRI|EPOLLET in rte_intr_rx_ctl), which is > what would turn a single missed read into a permanent stall rather than > a spin -- that detail seems worth stating explicitly in the commit > message, since it is the part that makes the failure non-self-healing. The drain patch, a reproducer https://github.com/DPDK/grout/actions/runs/30259855532/job/89956997416 grout has an opt-in mode where an idle dataplane worker stops busy polling and blocks on its Rx queue interrupts through rte_eth_dev_rx_intr_enable() and rte_epoll_wait(). Its smoke suite runs on net_tap ports, so that mode exercises this path in CI. At sha b2f775810a6a the vendored DPDK carries the intr enable/disable patch but not the drain one, which isolates it, and build-and-tests (x86_64, gcc-14, DPDK 25.11, FRR 10.5, adaptive-irq) fails on smoke/bgp_frr_test.sh: timeout: icmp_seq=1 [tx p0] ICMP echo request id=28908 seq=2 [rx p0] ICMP echo reply id=28908 seq=1 [rx p0] ICMP echo reply id=28908 seq=2 reply from 16.0.0.1: icmp_seq=2 ttl=64 time=0.083 ms error: command failed: Connection timed out (ETIMEDOUT) The reply to seq=1 sits readable in the queue fd. The burst that ran after the epoll wakeup returned 0, and being edge triggered there is no second wakeup for data already queued, so it comes out only when the request for seq=2 creates a new edge: both replies at once, and the first ping times out. The previous revision that morning, 08f1ccd941bb, carries both patches and the same job passes. What the trace establishes is that pmd_rx_burst() ran and returned 0 with the fd readable, so tap_trigger had not advanced at that point. It can lag because the trigger is process wide and the fd owner is the process: fcntl(fd, F_SETOWN, getpid()); The signal is therefore handled by some thread of the kernel's choosing, with no relation to the lcore that epoll woke for that queue, while the wakeup goes precisely to that lcore. A busy polling loop does not care, which is why the trigger has been fine since 2018: it re-reads the counter on the next iteration, so a late signal costs only latency. Interrupt mode bursts once per edge, so it is never caught up. On the cost: the mode restriction is there because tap_setup_queue() reuses queue fds across a reconfigure, so without it a port could mix fds with and without O_ASYNC. And the crash that made the new error path dangerous is fixed by the queue release patch, independently of this one. > > For what it is worth, the EAL side is safe: tap sets > RTE_INTR_HANDLE_EXT, so eal_intr_proc_rxtx_intr() returns early and never > consumes packet bytes from the queue fd. > > > Patch 3/3: net/tap: use bool for boolean flags > ---------------------------------------------- > > Warning: the conversion is incomplete on its own terms. > > - tap_flow.c:1247 still reads "if (pmd->flow_init == 0 && ...)"; > on a bool this should be "!pmd->flow_init" > - the three converted fields keep "/* 1 if ... */" comments > - the local "int persist" in rte_pmd_tap_probe() and the "int persist" > parameter of pmd_init_internals() were not converted > > Info: rte_eth_tap.h uses bool without including <stdbool.h>, relying on a > transitive include via ethdev_driver.h. It compiles; a direct include > would make the header self-contained. > > I initially flagged the (set ^ pmd->flow_isolate) idiom in > tap_flow_isolate() as losing idempotency for non-0/1 values under the > bool conversion. There is a "if (set) set = 1;" normalization immediately > above it, so the XOR is correct. Dropped. Agreed. I will fix it in the next version. > > > Summary > ------- > > Patch 1 is good and independently verified. Patch 3 needs the leftovers > cleaned up. Patch 2 is mechanically sound but its justification is > unreproduced here, and it should not go in ahead of a fix for the > tap_tx_queue_release() NULL dereference it makes easy to hit. ------------------------------- Maxime Leroy [email protected]
