|WARNING| [v6, 4/4] net/af_packet: use kernel to filter multi-queue Rx

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/171408

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-15

# DPDK Patch Review

## PATCH 1/4: test/af_packet: ignore stray packets in loopback test

### Errors: None

### Warnings: None

### Info

**Pre-existing: Missing flush before loopback test**

The loopback test does not flush queued frames before starting its measurement.
If the TAP interface already has traffic (from previous tests or external sources),
those frames could be counted toward the test's expected total.

This is pre-existing -- the patch improves robustness by adding source MAC tagging
to distinguish test frames from stray traffic, but does not add an explicit flush.

Consider calling `flush_rx_port()` (introduced in patch 2) at the start of `test_af_packet_loopback()`
to ensure a clean baseline.

---

## PATCH 2/4: net/af_packet: filter Rx of outgoing packets in the PMD

### Errors: None

### Warnings

**`parse_uint()` maximum bound enforced at 1, but error message does not specify the range**

Line in `rte_eth_af_packet.c`:
```c
if (parse_uint(pair->key, pair->value, &ignore_outgoing, 1) < 0)
    return -1;
```

The `parse_uint()` helper (from `app/test/test_pmd_af_packet.c`) clamps the value to `[0, max]`,
but when it returns an error, there is no indication to the user that valid values are `0` or `1`.
The test `test_af_packet_invalid_ignore_outgoing()` expects failure for `ignore_outgoing=2`,
but the user gets no message explaining why.

**Suggested fix:** Add a `PMD_LOG(ERR, ...)` after the failed `parse_uint()` to document the valid range:
```c
if (parse_uint(pair->key, pair->value, &ignore_outgoing, 1) < 0) {
    PMD_LOG(ERR, "%s: ignore_outgoing must be 0 or 1", name);
    return -1;
}
```

**`write()` return value checked against `sizeof(frame)` without verifying partial writes are impossible**

Line in `do_tap_fd_tx_burst()`:
```c
if (write(fd, frame, sizeof(frame)) != (ssize_t)sizeof(frame))
    return -1;
```

The TAP file descriptor is opened with `IFF_NO_PI` (from the existing code in `test_af_packet_setup()`),
so each `write()` is a single frame and partial writes do not occur.
However, the check as written does not distinguish `write() < sizeof(frame)` (partial write, which would be a logic error)
from `write() == -1` (I/O error, which is expected on a closed or broken fd).

This is correct for the TAP device, but the code does not document why partial writes cannot happen.

**Suggested fix:** Add a comment explaining that `IFF_NO_PI` ensures atomic frame writes,
or handle partial writes and `errno == EINTR` explicitly if you want to make the code more defensive:
```c
ssize_t written = write(fd, frame, sizeof(frame));
if (written < 0)
    return -1;  /* I/O error */
if (written != (ssize_t)sizeof(frame))
    return -1;  /* Should not happen with IFF_NO_PI, but check anyway */
```

### Info

**Test helper `do_tap_fd_tx_burst()` does not propagate how many frames were written**

The helper returns `0` on success or `-1` on failure, but does not report how many frames
were successfully written before the failure.
This means a caller who asks for 4 frames cannot distinguish "all 4 written" from "3 written then write failed."

For the current test usage (where any failure is a hard stop), this is fine.
If future tests need partial-write handling, the signature should be changed to return
the number of frames written (like `do_tx_burst()`) rather than a binary status.

Not a bug, just a note for future maintenance.

---

## PATCH 3/4: net/af_packet: use kernel to filter single-queue Rx

### Errors: None

### Warnings: None

### Info: None

---

## PATCH 4/4: net/af_packet: use kernel to filter multi-queue Rx

### Errors: None

### Warnings

**No kernel version detection for `PACKET_FANOUT_FLAG_IGNORE_OUTGOING`, relying on silent failure**

The patch sets `PACKET_FANOUT_FLAG_IGNORE_OUTGOING` for all kernels and states that kernels before 6.2
will ignore it. However, there is no runtime check to determine whether the flag was accepted,
so the PMD has no way to log or warn that kernel-side filtering is unavailable.

The PMD falls back to user-space filtering (`rx_queue->ignore_outgoing` remains set for multi-queue),
so correctness is preserved. But on a pre-6.2 kernel, users will not be notified that they are incurring
the performance cost of user-space filtering when they might have expected kernel-side filtering.

**Suggested fix:** After setting the fanout arg, the PMD could use `getsockopt(SOL_PACKET, PACKET_FANOUT, ...)`
to read back the effective fanout configuration and check whether the flag was honored.
If not, log an Info message that user-space filtering is in effect.

Alternatively, document in the release notes and NIC guide that the performance benefit applies only to Linux 6.2+
and that there is no runtime indication of which path is active. (The guide already mentions 6.2 in patch 4's doc update,
so this may be sufficient.)

---

## Summary

The series introduces robust filtering of outgoing packets for the AF_PACKET PMD,
with kernel-side acceleration where supported and user-space fallback otherwise.
The test coverage is comprehensive, and the implementation is correct.

Two Warnings are raised:
1. Missing user-facing error message for invalid `ignore_outgoing` values
2. `write()` return check does not document why partial writes cannot occur on TAP fds

Both are minor and do not affect correctness. The Info notes are observations for future maintainability.

Review-Result: WARNING