ixgbe: convert Rx to libeth and page_pool - #6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the ixgbe driver’s Rx path away from legacy page-sharing/recycling to the libeth + page_pool infrastructure, and updates the XDP integration accordingly (including multi-buffer/XDP helpers and pseudo header split support).
Changes:
- Switch Rx buffer management to
libethfill/completion queues backed bypage_pool(and integratelibethXDP helpers). - Rework XDP Tx queue handling/locking and add new ring state (
__IXGBE_TX_XDP_RING_PRIMED), plus MTU/XDP frags capability gating. - Remove the legacy-rx private flag and update various supporting paths (self-tests, ring setup, register programming).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/net/ethernet/intel/Kconfig | Selects LIBETH_XDP for ixgbe builds. |
| drivers/net/ethernet/intel/ixgbe/ixgbe.h | Adopts libeth types, updates ring layout/state for page_pool + libeth queues. |
| drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | Adjusts AF_XDP ZC path to new buffer structs and libeth XDP locking. |
| drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | Renames SRRCTL buffer-size conversion macro to ..._STEP. |
| drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | Core Rx conversion to libeth/page_pool + XDP helpers, plus ring/resource lifecycle updates. |
| drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | Updates loopback/self-test Rx buffer access to libeth_fqe/page_pool buffers. |
Comments suppressed due to low confidence (1)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:2316
- In the main Rx path, the skb protocol is never set (the
eth_type_trans()call was removed fromixgbe_process_skb_fields(), andixgbe_clean_rx_irq()doesn’t set it before passing the skb up). This can break upper stack parsing. Setskb->protocolviaeth_type_trans()before callingixgbe_process_skb_fields().
/* populate checksum, timestamp, VLAN, and protocol */
ixgbe_process_skb_fields(rx_ring, rx_desc, skb);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rx_desc->read.pkt_addr = cpu_to_le64(addr); | ||
|
|
||
| /* | ||
| * Refresh the desc even if buffer_addrs didn't change | ||
| * because each write-back erases this info. | ||
| */ | ||
| rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset); | ||
| if (hdr_fq.pp) { | ||
| addr = libeth_rx_alloc(&hdr_fq, ntu); | ||
| if (addr == DMA_MAPPING_ERROR) { |
| if (unlikely(!test_bit(__IXGBE_TX_XDP_RING_PRIMED, | ||
| xdp_ring->state))) { | ||
| struct ixgbe_adv_tx_context_desc *context_desc; | ||
|
|
||
| return rx_buffer; | ||
| } | ||
| set_bit(__IXGBE_TX_XDP_RING_PRIMED, xdp_ring->state); |
| clear_bit(__IXGBE_HANG_CHECK_ARMED, ring->state); | ||
| clear_bit(__IXGBE_TX_XDP_RING_PRIMED, ring->state); |
| __xdp_rxq_info_reg(&ring->xdp_rxq, ring->netdev, | ||
| ring->queue_index, | ||
| ixgbe_rx_napi_id(ring), 0); | ||
| WARN_ON(xdp_rxq_info_reg_mem_model(&ring->xdp_rxq, | ||
| MEM_TYPE_XSK_BUFF_POOL, | ||
| NULL)); | ||
| xsk_pool_set_rxq_info(ring->xsk_pool, &ring->xdp_rxq); |
3176c41 to
d9c60d3
Compare
walking-machine
left a comment
There was a problem hiding this comment.
Patch 1:
Aside from a silly '&' mistake, code looks OK, though I would make a few changes.
walking-machine
left a comment
There was a problem hiding this comment.
Patch 2
It wouldn't hurt to mention the transition from skb-saved-in-buffer to skb-recorded-on-ring model in the commit message.
Fixed XDP_DROP/ABORTED: reaches ixgbe_put_rx_buffer() but page
is never
Either you have fixed an existing problem in code, in which case you should submit a patch to a stable tree (net-queue in this case) prior to sending the -next series.
Or you have fixed an issue compared to the previous version of the series, in which case you have 2 options:
- changelog in the cover letter (PR comment in this case)
- changelog under '---' in the formatted patch (right under
Signed-off-by: Magdalena Pytel <magdalena.pytel@intel.com>)
Fix this by calling vlan_vids_del_by_dev(port->dev, master->dev) in
hsr_del_port() before unlinking slave A or slave B ports, matching the
propagation logic in hsr_ndo_vlan_rx_add_vid() / hsr_ndo_vlan_rx_kill_vid()
and the cleanup behavior in bonding and team drivers.
Fixes: 1a8a63a5305e ("net: hsr: Add VLAN CTAG filter support")
Reported-by: syzbot+456957213f32970c0762@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/6a4cb6ca.57639fcc.86d58.000b.GAE@google.com/T/#u
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Felix Maurer <fmaurer@redhat.com>
Cc: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: incorporated Sashiko's feedback
v1: https://lore.kernel.org/netdev/20260707082327.3238690-1-edumazet@google.com/
net/hsr/hsr_slave.c | 2 ++
1 file changed, 2 insertions(+)
You can leave the changelog inside of the commit message for the github review, but best distinguish it in a way that makes it immediately clear that this is not a real part of a commit message, smth like [INTERNAL ONLY!!!], so it is straightforward to delete later
There was a problem hiding this comment.
This does not compile :(
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c: In function ‘ixgbe_clean_rx_irq’:
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:2159:25: error: implicit declaration of function ‘ixgbe_put_rx_buffer’; did you mean ‘ixgbe_print_buffer’? [-Wimplicit-function-declaration]
2159 | ixgbe_put_rx_buffer(rx_ring, rx_buffer, true);
| ^~~~~~~~~~~~~~~~~~~
| ixgbe_print_buffer
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c: In function ‘ixgbe_setup_rx_resources’:
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6669:19: error: ‘struct libeth_fq’ has no member named ‘no_napi’
6669 | fq.no_napi = true;
| ^
To fix:
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index aa682cb23b2b..2d83bfb309db 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2156,7 +2156,6 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
/* exit if we failed to retrieve a buffer */
if (!xdp_res && !skb) {
rx_ring->rx_stats.alloc_rx_buff_failed++;
- ixgbe_put_rx_buffer(rx_ring, rx_buffer, true);
cleaned_count++;
break;
}
AND
cherry-pick commit "libeth: allow to create fill queues without NAPI" to the beginning of the series
|
|
||
| MODULE_IMPORT_NS("LIBIE_FWLOG"); | ||
| MODULE_DESCRIPTION("Intel(R) 10 Gigabit PCI Express Network Driver"); | ||
| MODULE_IMPORT_NS("LIBETH"); |
There was a problem hiding this comment.
ERROR: modpost: module ixgbe uses symbol libeth_xdp_buff_add_frag from namespace LIBETH_XDP, but does not import it.
Please do:
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index fa7bd4a666ae..3cef92c1a559 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -176,6 +176,7 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
MODULE_IMPORT_NS("LIBIE_FWLOG");
MODULE_DESCRIPTION("Intel(R) 10 Gigabit PCI Express Network Driver");
MODULE_IMPORT_NS("LIBETH");
+MODULE_IMPORT_NS("LIBETH_XDP");
MODULE_LICENSE("GPL v2");
DEFINE_STATIC_KEY_FALSE(ixgbe_xdp_locking_key);
| /* Setup Rx Descriptor ring and Rx buffers */ | ||
| rx_ring->count = IXGBE_DEFAULT_RXD; | ||
| rx_ring->queue_index = 0; | ||
| rx_ring->dev = &adapter->pdev->dev; |
There was a problem hiding this comment.
There is still ring->dev = ... for an Rx ring in ixgbe_alloc_q_vector(). This is a mistake that you have admittedly inherited from me ;)
| } else { | ||
| WARN_ON(xdp_rxq_info_reg_mem_model(&ring->xdp_rxq, | ||
| MEM_TYPE_PAGE_SHARED, NULL)); | ||
| xdp_rxq_info_attach_page_pool(&ring->xdp_rxq, ring->pp); |
There was a problem hiding this comment.
rxq_info is not registered at this point, and xdp_rxq_info_attach_page_pool() does not perform this itself
| srrctl |= IXGBE_RXBUFFER_3K >> IXGBE_SRRCTL_BSIZEPKT_SHIFT; | ||
| } | ||
| if (ring->xsk_pool) | ||
| srrctl |= DIV_ROUND_UP(xsk_pool_get_rx_frame_size(ring->xsk_pool), |
There was a problem hiding this comment.
I think you shouldn't have touched the xsk srrctl programming in this patch.
But if you want to. We cannot control xsk pool buffer size, unlike page_pool. User provides us with xsk buffers. Those are usually 2K or 4K in total, so HW-writable size is always less, because we have both headroom and tailroom, so without RLPML there will be overlap, if we do DIV_ROUND_UP.
What I would do here in case of ixgbe_mac_82599EB, is just do regular DIV(), so that e.g. 3400 frame size will become 3K.
There was a problem hiding this comment.
This is a little bit more complex, since in ixgbevf we could just not support this particular HW, because the implementation was new.
| ring->rx_buf_len = xsk_buf_len; | ||
| rxdctl &= ~(IXGBE_RXDCTL_RLPMLMASK | IXGBE_RXDCTL_RLPML_EN); | ||
| if (pkt_len <= IXGBE_RXDCTL_RLPMLMASK) | ||
| rxdctl |= pkt_len | IXGBE_RXDCTL_RLPML_EN; |
There was a problem hiding this comment.
Those are not correct rlpml calculations for an xsk ZC ring
| { | ||
| struct ixgbe_hw *hw = &adapter->hw; | ||
| struct net_device *netdev = adapter->netdev; | ||
| int max_frame = netdev->mtu + ETH_HLEN + ETH_FCS_LEN; |
There was a problem hiding this comment.
Why not READ_ONCE(adapter->netdev->mtu) + LIBETH_RX_LL_LEN;?
| int orig_node = dev_to_node(dev); | ||
| void *napi_dev; | ||
| int ring_node = NUMA_NO_NODE; | ||
| int size; |
There was a problem hiding this comment.
We generally try to follow so called Reverse Christmas Tree ordering of declarations
|
Patch 4 looks fine, but I advise to squash it into patch 3, so we can be sure XDP and AF_XDP will fit into 1 upstream PR (15 patches) |
|
Build errors in each patch, which I have not yet reviewed: patch 6 patch 7 patch 8 patch 9 patch 10 |
d9c60d3 to
0a7a647
Compare
e0b9442 to
4c01f57
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:10428
xdp_progis an__rcupointer instruct ixgbe_ring, but this update path usesWRITE_ONCE(). This should usercu_assign_pointer()so readers usingrcu_dereference(_bh)get the proper ordering guarantees.
for (int i = 0; i < adapter->num_rx_queues; i++) {
WRITE_ONCE(adapter->rx_ring[i]->xdp_prog,
adapter->xdp_prog);
}
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6621
ixgbe_setup_rx_resources()sizes the libeth FQ based onrx_ring->xdp_prog, butrx_ring->xdp_progis only assigned later in this function. If an XDP program is already attached (e.g., device is brought up while a prog is configured), the FQ can be created with the wrong headroom/buffer length, which can break XDP or force unexpected non-linear handling. This should key offadapter->xdp_prog(current device-wide program) when initializingstruct libeth_fq.
.type = LIBETH_FQE_MTU,
.xdp = !!rx_ring->xdp_prog,
.buf_len = IXGBE_RX_PAGE_LEN(rx_ring->xdp_prog ?
LIBETH_XDP_HEADROOM :
LIBETH_SKB_HEADROOM),
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:6786
ixgbe_free_rx_resources()returns early whenrx_ring->descis NULL, skipping cleanup of other allocations (e.g.,rx_xsk_buffer_infoand the libeth FQ/page_pool). Even if current call paths usually havedesc != NULL, this structure makes the cleanup fragile and can leak resources in partial-init / error paths. Prefer freeing the descriptor conditionally but always running the rest of the teardown.
/* if not set, then don't free */
if (!rx_ring->desc)
return;
| /* populate checksum, timestamp, VLAN, and protocol */ | ||
| ixgbe_process_skb_fields(rx_ring, rx_desc, skb); |
| if (ring->xsk_pool) | ||
| srrctl |= DIV_ROUND_UP(xsk_pool_get_rx_frame_size(ring->xsk_pool), | ||
| IXGBE_SRRCTL_BSIZEPKT_STEP); | ||
| else | ||
| srrctl |= DIV_ROUND_UP(ring->rx_buf_len, | ||
| IXGBE_SRRCTL_BSIZEPKT_STEP); |
| if (!xdp_prog) | ||
| goto xdp_out; | ||
|
|
||
| prefetchw(xdp->data_hard_start); /* xdp_frame write */ | ||
| prefetchw(xdp->base.data_hard_start); /* xdp_frame write */ |
Similarly, like for virtual function in ixgbevf, remove ixgbe_construct_skb(), the legacy-rx private flag, and all ring_uses_build_skb() conditionals. build_skb is now the only RX code path. This is a preparation for the conversion to libeth and page_pool based Rx buffer management. Signed-off-by: Magdalena Pytel <magdalena.pytel@intel.com>
Stop sharing pages between multiple Rx buffers. Convert ixgbe Rx path from page-sharing/recycling to a simple one-page-per-packet model. Remove logic that refers to page-sharing buffer management. This is a preparatory step before converting the Rx path to page_pool and libeth. Fixed XDP_DROP/ABORTED: reaches ixgbe_put_rx_buffer() but page is never freed. Signed-off-by: Magdalena Pytel <magdalena.pytel@intel.com>
Convert ixgbe Rx path from manual page-per-packet DMA management to libeth/page_pool infrastructure, and integrates libeth XDP helpers. Each packet gets its own buffer instead of sharing pages between packets. Signed-off-by: Magdalena Pytel <magdalena.pytel@intel.com>
* Added branch prediction hints to the ixgbe Rx path and clean up the packet accounting. * Moved total_rx_bytes and total_rx_packets accounting together. Signed-off-by: Magdalena Pytel <magdalena.pytel@intel.com>
Implement XDP multi-buffer support for Rx fragmented packets
using libeth_xdp.
* Added MODULE_IMPORT_NS("LIBETH_XDP")
and #include <net/libeth/types.h>
* Replaced struct sk_buff *skb in ixgbe_ring with
struct libeth_xdp_buff_stash
and ixgbe_clean_rx_ring skb cleanup with libeth_xdp_return_stash
* Removed ixgbe_add_rx_frag()
* Reorganized ixgbe_clean_rx_irq() and ixgbe_is_non_eop()
* Updated ixgbe_xdp_setup() to allow multi-buffer XDP programs
* Added NETDEV_XDP_ACT_RX_SG in xdp_features
Signed-off-by: Magdalena Pytel <magdalena.pytel@intel.com>
4c01f57 to
f38e292
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:3720
- ixgbe_configure_srrctl() programs SRRCTL.BSIZEPKT in 1KB units. Using DIV_ROUND_UP() for the AF_XDP ZC frame size can round the programmed DMA buffer size up beyond the actual UMEM frame size, risking DMA writes past the end of the UMEM chunk (memory corruption). This should round down / align down to the supported step, like the previous >>10 behavior.
if (ring->xsk_pool)
srrctl |= DIV_ROUND_UP(xsk_pool_get_rx_frame_size(ring->xsk_pool),
IXGBE_SRRCTL_BSIZEPKT_STEP);
else
srrctl |= DIV_ROUND_UP(ring->rx_buf_len,
IXGBE_SRRCTL_BSIZEPKT_STEP);
| union { | ||
| struct libeth_fqe *rx_fqes; | ||
| struct ixgbe_xsk_rx_buffer *rx_xsk_buffer_info; | ||
| struct ixgbe_tx_buffer *tx_buffer_info; | ||
| struct ixgbe_rx_buffer *rx_buffer_info; | ||
| }; |
| rx_ring->next_to_clean = 0; | ||
| rx_ring->next_to_use = 0; | ||
|
|
||
| /* XDP RX-queue info */ | ||
| if (xdp_rxq_info_reg(&rx_ring->xdp_rxq, adapter->netdev, | ||
| rx_ring->queue_index, ixgbe_rx_napi_id(rx_ring)) < 0) | ||
| goto err; | ||
|
|
||
| WRITE_ONCE(rx_ring->xdp_prog, adapter->xdp_prog); | ||
| ret = __xdp_rxq_info_reg(&rx_ring->xdp_rxq, adapter->netdev, | ||
| rx_ring->queue_index, 0, rx_ring->truesize); | ||
| if (ret < 0) | ||
| goto err_free_desc; |
Convert the ixgbe Rx path from the legacy page-sharing buffer
management to libeth and page_pool, following the same approach
used in the ixgbevf conversion.
Patch 1 removes the legacy-rx ethtool private flag and makes
build_skb the only Rx code path, simplifying the driver for the
conversion.
Patch 2 converts ixgbe Rx path from page-sharing/recycling
to a simple one-page-per-packet model. Remove logic that refers
to page-sharing buffer management. This is a preparatory step
before converting the Rx path to page_pool and libeth.
Patch 3 Convert ixgbe Rx path from manual page-per-packet DMA
management to libeth/page_pool infrastructure, and integrates
libeth XDP helpers. Each packet gets its own buffer instead of
sharing pages between packets.
Patch 4 adds branch prediction hints and minor cleanups to the
Rx path, improving performance after the libeth conversion.
Patch 5 adds XDP multi-buffer support on the Rx path, allowing
XDP programs with frags support to handle packets that span
multiple buffers.
Patch 6 adds XDP_TX support in multi-buffer mode through libeth,
enabling XDP programs to transmit multi-buffer packets back out
the same interface.
Patch 7 adds XDP_REDIRECT and .ndo_xdp_xmit support through
libeth, enabling redirection of packets between interfaces via
XDP.
Patch 8 introduces pseudo header split support, copying packet
headers into a separate buffer
Patch 9 adds error handling for page pool creation in
ixgbe_configure_rx_ring() and allows buffer reallocation when
the page pool is not present.
Patch 10 eases the MTU restriction when an XDP program is
attached. With libeth page pool and multi-buffer XDP support,
MTU changes are allowed as long as the XDP program supports
frags or the frame fits in a single buffer.
Patch 1 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m5a77d9a3d6495b21d80e1f90d49470914ed950dc
Patch 2 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m4956a9538e51e0e0486b5d577fa3a1899897ba28
Patch 3 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m67766454f9439e608779ad85291e43e9de4b568d
Patch 4 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#mb10a5ccd87dc79065e6f8bcc723101b9b4966d27
Patch 5 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m74bb0fc2103acefa7a222b94bcd00cc4dae61eb1
Patch 6 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#mcc688bbe93c754d0a5e36e18e706505037f7ac51
Patch 7 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m7f0a243d405642def7f22e97a9490a2a85851886
Patch 8 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m81aae544a7e63736f2d8897d3cbaf78005bc8bd3
Patch 9 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m172f6be60eec974111c5065e8c8c2fab6f0d855b
Patch 10 based on:
commit: https://lore.kernel.org/netdev/20260304160345.1340940-1-larysa.zaremba@intel.com/T/#m5e0629f3a8d72c85b1527ef8727c9ee07a7b1428