On 2025/7/25 18:11, Edward Cree wrote:
On 7/24/25 10:57, Paolo Abeni wrote:
On 7/23/25 2:32 AM, Chenyuan Yang wrote:
The xdp_convert_buff_to_frame() function can return NULL when there is
insufficient headroom in the buffer to store the xdp_frame structure
or when the driver didn't reserve enough tailroom for skb_shared_info.
AFAIC the sfc driver reserves both enough headroom and tailroom, but
this is after ebpf run, which in turn could consume enough headroom to
cause a failure, so I think this makes sense.
Your reasoning seems plausible to me.
However, I think the error path ought to more closely follow the existing
error cases in logging a ratelimited message and calling the tracepoint.
I think the cleanest way to do this would be:
if (unlikely(!xdpf))
err = -ENOBUFS;
else
err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);
so that it can make use of the existing failure path.
Adding the check to efx_xdp_tx_buffers() is also an option.
-ed
Hi Chenyuan,
THX for addressing this edge case. I concur with Edward's suggestion to
integrate this with the existing error handling flow. This will ensure:
Consistent observability (ratelimited logs + tracepoints)
Centralized resource cleanup
Clear error type differentiation via -ENOBUFS
Proposed refinement:
diff
case XDP_TX:
/* Buffer ownership passes to tx on success. */
xdpf = xdp_convert_buff_to_frame(&xdp);
+ if (unlikely(!xdpf)) {
+ err = -ENOBUFS;
+ } else {
+ err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
+ }
- err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
if (unlikely(err != 1)) {
efx_siena_free_rx_buffers(rx_queue, rx_buf, 1);
if (net_ratelimit())
netif_err(efx, rx_err, efx->net_dev,
- "XDP TX failed (%d)\n", err);
+ "XDP TX failed (%d)%s\n", err,
+ err == -ENOBUFS ? " [frame conversion]" : "");
channel->n_rx_xdp_bad_drops++;
- trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
+ if (err != -ENOBUFS)
+ trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
} else {
channel->n_rx_xdp_tx++;
}
break;
-- Thanks, TAO. --- “Life finds a way.”
