Re: [PATCH v2 0/3] net/tap: fix Rx queue interrupt support

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 20 Jul 2026 14:06:56 +0200
Maxime Leroy <maxime@leroys.fr> 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.


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.

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: stable@dpdk.org.


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.

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.


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.