* [PATCH net-next v3] net: atlantic: convert RX path to page_pool
@ 2026-08-07 8:56 Yangyu Chen
2026-08-08 8:56 ` sashiko-bot
2026-08-17 16:59 ` Jakub Kicinski
0 siblings, 2 replies; 3+ messages in thread
From: Yangyu Chen @ 2026-08-07 8:56 UTC (permalink / raw)
To: Sukhdeep Singh, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Mina Almasry, Jesper Dangaard Brouer, Richard Cochran, netdev,
bpf, linux-kernel, Yangyu Chen
The driver currently allocates RX buffers with dev_alloc_pages(), maps
them with dma_map_page(), and uses a hand-rolled page-flip scheme to
subdivide high-order pages. Behind an IOMMU, the map/unmap churn is a
major RX cost: on a Thunderbolt-attached QNAP QNA-T310G1S, iperf3 -R
over IPv6 tops out at about 2.2 Gbit/s over MTU 1500.
Convert RX buffers to page_pool fragments. Pages are DMA-mapped once
when entering the pool and recycled through the stack or XDP via the
MEM_TYPE_PAGE_POOL memory model. This removes the custom page-flip
accounting, lets page_pool handle fragment reuse, and ensures every RX
path either keeps the ring's fragment reference for reposting or hands
it to the skb/xdp_buff for later recycling.
Register the PTP RX ring's xdp_rxq as well, since it shares the RX
clean paths. Drop the ethtool PageFlips/PageReuses/PageFrees counters
which only described the old scheme.
On the QNA-T310G1S, MTU 1500, TCP over IPv6, iperf3 -R improves from
2.24 Gbit/s to 9.14 Gbit/s. The module was also smoke-tested with native
XDP PASS, DROP, and ABORTED actions; carrier recovered after each
attach/detach cycle and dmesg showed no page_pool/DMA warnings.
Reviewed-by: Sukhdeep Singh <sukhdeeps@marvell.com>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
Notes:
v2 -> v3:
- rebase on net-next; patches 1 and 2 of v2 (the ring teardown leak
fixes) went in via net, so only the conversion is left
v2: https://lore.kernel.org/lkml/tencent_1F173E0FC1606D2AC704DC9C98AF10984607@qq.com/
v1: https://lore.kernel.org/lkml/tencent_7DB01BE7F8FA056BB5F11D3570CF636C4309@qq.com/
drivers/net/ethernet/aquantia/Kconfig | 1 +
.../ethernet/aquantia/atlantic/aq_ethtool.c | 3 -
.../net/ethernet/aquantia/atlantic/aq_ptp.c | 18 +-
.../net/ethernet/aquantia/atlantic/aq_ring.c | 210 +++++++-----------
.../net/ethernet/aquantia/atlantic/aq_ring.h | 6 +-
.../net/ethernet/aquantia/atlantic/aq_vec.c | 21 +-
6 files changed, 120 insertions(+), 139 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/Kconfig b/drivers/net/ethernet/aquantia/Kconfig
index cec2018c84a9..c8fb7b33e5b7 100644
--- a/drivers/net/ethernet/aquantia/Kconfig
+++ b/drivers/net/ethernet/aquantia/Kconfig
@@ -20,6 +20,7 @@ config AQTION
tristate "aQuantia AQtion(tm) Support"
depends on PCI
depends on MACSEC || MACSEC=n
+ select PAGE_POOL
help
This enables the support for the aQuantia AQtion(tm) Ethernet card.
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
index 420af958d486..0f5125bd2315 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
@@ -100,9 +100,6 @@ static const char * const aq_ethtool_queue_rx_stat_names[] = {
"%sQueue[%d] AllocFails",
"%sQueue[%d] SkbAllocFails",
"%sQueue[%d] Polls",
- "%sQueue[%d] PageFlips",
- "%sQueue[%d] PageReuses",
- "%sQueue[%d] PageFrees",
"%sQueue[%d] XdpAbort",
"%sQueue[%d] XdpDrop",
"%sQueue[%d] XdpPass",
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
index 558ac9237f75..3a40d986cd67 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
@@ -13,6 +13,7 @@
#include <linux/ptp_classify.h>
#include <linux/interrupt.h>
#include <linux/clocksource.h>
+#include <net/xdp.h>
#include "aq_nic.h"
#include "aq_ptp.h"
@@ -1192,12 +1193,23 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
if (err)
goto err_exit_ptp_tx;
+ err = xdp_rxq_info_reg(&aq_ptp->ptp_rx.xdp_rxq, aq_nic->ndev,
+ rx_ring_idx, aq_ptp->napi.napi_id);
+ if (err < 0)
+ goto err_exit_ptp_rx;
+
+ err = xdp_rxq_info_reg_mem_model(&aq_ptp->ptp_rx.xdp_rxq,
+ MEM_TYPE_PAGE_POOL,
+ aq_ptp->ptp_rx.pg_pool);
+ if (err < 0)
+ goto err_exit_xdp_rxq;
+
if (aq_ptp->a1_ptp) {
err = aq_ring_hwts_rx_alloc(&aq_ptp->hwts_rx, aq_nic, PTP_HWST_RING_IDX,
aq_nic->aq_nic_cfg.rxds,
aq_nic->aq_nic_cfg.aq_hw_caps->rxd_size);
if (err)
- goto err_exit_ptp_rx;
+ goto err_exit_xdp_rxq;
}
err = aq_ptp_skb_ring_init(&aq_ptp->skb_ring, aq_nic->aq_nic_cfg.rxds);
@@ -1217,6 +1229,8 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
err_exit_hwts_rx:
if (aq_ptp->a1_ptp)
aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
+err_exit_xdp_rxq:
+ xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
err_exit_ptp_rx:
aq_ring_free(&aq_ptp->ptp_rx);
err_exit_ptp_tx:
@@ -1233,6 +1247,8 @@ void aq_ptp_ring_free(struct aq_nic_s *aq_nic)
return;
aq_ring_free(&aq_ptp->ptp_tx);
+ if (xdp_rxq_info_is_reg(&aq_ptp->ptp_rx.xdp_rxq))
+ xdp_rxq_info_unreg(&aq_ptp->ptp_rx.xdp_rxq);
aq_ring_free(&aq_ptp->ptp_rx);
if (aq_ptp->a1_ptp)
aq_ring_hwts_rx_free(&aq_ptp->hwts_rx);
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index e1193c6719d9..9dd881710594 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -14,120 +14,37 @@
#include "aq_vec.h"
#include "aq_main.h"
+#include <net/page_pool/helpers.h>
#include <net/xdp.h>
#include <linux/filter.h>
#include <linux/bpf_trace.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
-static void aq_get_rxpages_xdp(struct aq_ring_buff_s *buff,
- struct xdp_buff *xdp)
-{
- struct skb_shared_info *sinfo;
- int i;
-
- if (xdp_buff_has_frags(xdp)) {
- sinfo = xdp_get_shared_info_from_buff(xdp);
-
- for (i = 0; i < sinfo->nr_frags; i++) {
- skb_frag_t *frag = &sinfo->frags[i];
-
- page_ref_inc(skb_frag_page(frag));
- }
- }
- page_ref_inc(buff->rxdata.page);
-}
-
-static inline void aq_free_rxpage(struct aq_rxpage *rxpage, struct device *dev)
-{
- unsigned int len = PAGE_SIZE << rxpage->order;
-
- dma_unmap_page(dev, rxpage->daddr, len, DMA_FROM_DEVICE);
-
- /* Drop the ref for being in the ring. */
- __free_pages(rxpage->page, rxpage->order);
- rxpage->page = NULL;
-}
-
-static int aq_alloc_rxpages(struct aq_rxpage *rxpage, struct aq_ring_s *rx_ring)
-{
- struct device *dev = aq_nic_get_dev(rx_ring->aq_nic);
- unsigned int order = rx_ring->page_order;
- struct page *page;
- int ret = -ENOMEM;
- dma_addr_t daddr;
-
- page = dev_alloc_pages(order);
- if (unlikely(!page))
- goto err_exit;
-
- daddr = dma_map_page(dev, page, 0, PAGE_SIZE << order,
- DMA_FROM_DEVICE);
-
- if (unlikely(dma_mapping_error(dev, daddr)))
- goto free_page;
-
- rxpage->page = page;
- rxpage->daddr = daddr;
- rxpage->order = order;
- rxpage->pg_off = rx_ring->page_offset;
-
- return 0;
-
-free_page:
- __free_pages(page, order);
-
-err_exit:
- return ret;
-}
-
static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
{
- unsigned int order = self->page_order;
- u16 page_offset = self->page_offset;
- u16 frame_max = self->frame_max;
- u16 tail_size = self->tail_size;
- int ret;
+ unsigned int size = self->page_offset + self->frame_max +
+ self->tail_size;
+ unsigned int pg_off;
+ struct page *page;
- if (rxbuf->rxdata.page) {
- /* One means ring is the only user and can reuse */
- if (page_ref_count(rxbuf->rxdata.page) > 1) {
- /* Try reuse buffer */
- rxbuf->rxdata.pg_off += frame_max + page_offset +
- tail_size;
- if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
- (PAGE_SIZE << order)) {
- u64_stats_update_begin(&self->stats.rx.syncp);
- self->stats.rx.pg_flips++;
- u64_stats_update_end(&self->stats.rx.syncp);
+ /* Buffers whose page was not passed up the stack are reposted
+ * with the data they already carry discarded.
+ */
+ if (rxbuf->rxdata.page)
+ return 0;
- } else {
- /* Buffer exhausted. We have other users and
- * should release this page and realloc
- */
- aq_free_rxpage(&rxbuf->rxdata,
- aq_nic_get_dev(self->aq_nic));
- u64_stats_update_begin(&self->stats.rx.syncp);
- self->stats.rx.pg_losts++;
- u64_stats_update_end(&self->stats.rx.syncp);
- }
- } else {
- rxbuf->rxdata.pg_off = page_offset;
- u64_stats_update_begin(&self->stats.rx.syncp);
- self->stats.rx.pg_reuses++;
- u64_stats_update_end(&self->stats.rx.syncp);
- }
+ page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
+ if (unlikely(!page)) {
+ u64_stats_update_begin(&self->stats.rx.syncp);
+ self->stats.rx.alloc_fails++;
+ u64_stats_update_end(&self->stats.rx.syncp);
+ return -ENOMEM;
}
- if (!rxbuf->rxdata.page) {
- ret = aq_alloc_rxpages(&rxbuf->rxdata, self);
- if (ret) {
- u64_stats_update_begin(&self->stats.rx.syncp);
- self->stats.rx.alloc_fails++;
- u64_stats_update_end(&self->stats.rx.syncp);
- }
- return ret;
- }
+ rxbuf->rxdata.page = page;
+ rxbuf->rxdata.daddr = page_pool_get_dma_addr(page);
+ rxbuf->rxdata.pg_off = pg_off + self->page_offset;
return 0;
}
@@ -179,6 +96,15 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
unsigned int idx,
struct aq_nic_cfg_s *aq_nic_cfg)
{
+ struct page_pool_params pp_params = {
+ .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+ .pool_size = aq_nic_cfg->rxds,
+ .nid = NUMA_NO_NODE,
+ .dev = aq_nic_get_dev(aq_nic),
+ .dma_dir = DMA_FROM_DEVICE,
+ };
+ struct page_pool *pool;
+
self->aq_nic = aq_nic;
self->idx = idx;
self->size = aq_nic_cfg->rxds;
@@ -200,6 +126,18 @@ int aq_ring_rx_alloc(struct aq_ring_s *self,
self->tail_size = 0;
}
+ pp_params.order = self->page_order;
+ pp_params.max_len = PAGE_SIZE << self->page_order;
+
+ pool = page_pool_create(&pp_params);
+ if (IS_ERR(pool))
+ return PTR_ERR(pool);
+
+ self->pg_pool = pool;
+
+ /* On failure aq_ring_alloc() calls aq_ring_free(), which also
+ * destroys the page pool.
+ */
return aq_ring_alloc(self, aq_nic);
}
@@ -346,7 +284,11 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
++self->stats.tx.packets;
self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
u64_stats_update_end(&self->stats.tx.syncp);
- xdp_return_frame_rx_napi(buff->xdpf);
+ /* Frames queued via ndo_xdp_xmit() may come from a
+ * page pool owned by another NAPI context: no direct
+ * recycling.
+ */
+ xdp_return_frame(buff->xdpf);
}
out:
@@ -437,22 +379,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
}
static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,
- struct net_device *dev,
- struct aq_ring_buff_s *buff)
+ struct net_device *dev)
{
struct xdp_frame *xdpf;
- struct sk_buff *skb;
xdpf = xdp_convert_buff_to_frame(xdp);
if (unlikely(!xdpf))
return NULL;
- skb = xdp_build_skb_from_frame(xdpf, dev);
- if (!skb)
- return NULL;
-
- aq_get_rxpages_xdp(buff, xdp);
- return skb;
+ return xdp_build_skb_from_frame(xdpf, dev);
}
static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
@@ -473,8 +408,16 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
u64_stats_update_end(&rx_ring->stats.rx.syncp);
prog = READ_ONCE(rx_ring->xdp_prog);
- if (!prog)
- return aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
+ if (!prog) {
+ skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
+ /* The ring has already handed its page pool reference to the
+ * xdp_buff, so if the skb could not be built the buffer must
+ * be returned to the pool here or its fragments would leak.
+ */
+ if (!skb)
+ xdp_return_buff(xdp);
+ return skb;
+ }
prefetchw(xdp->data_hard_start); /* xdp_frame write */
@@ -485,7 +428,7 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
act = bpf_prog_run_xdp(prog, xdp);
switch (act) {
case XDP_PASS:
- skb = aq_xdp_build_skb(xdp, aq_nic->ndev, buff);
+ skb = aq_xdp_build_skb(xdp, aq_nic->ndev);
if (!skb)
goto out_aborted;
u64_stats_update_begin(&rx_ring->stats.rx.syncp);
@@ -503,7 +446,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
u64_stats_update_begin(&rx_ring->stats.rx.syncp);
++rx_ring->stats.rx.xdp_tx;
u64_stats_update_end(&rx_ring->stats.rx.syncp);
- aq_get_rxpages_xdp(buff, xdp);
break;
case XDP_REDIRECT:
if (xdp_do_redirect(aq_nic->ndev, xdp, prog) < 0)
@@ -512,7 +454,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
u64_stats_update_begin(&rx_ring->stats.rx.syncp);
++rx_ring->stats.rx.xdp_redirect;
u64_stats_update_end(&rx_ring->stats.rx.syncp);
- aq_get_rxpages_xdp(buff, xdp);
break;
default:
fallthrough;
@@ -523,11 +464,13 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
u64_stats_update_end(&rx_ring->stats.rx.syncp);
trace_xdp_exception(aq_nic->ndev, prog, act);
bpf_warn_invalid_xdp_action(aq_nic->ndev, prog, act);
+ xdp_return_buff(xdp);
break;
case XDP_DROP:
u64_stats_update_begin(&rx_ring->stats.rx.syncp);
++rx_ring->stats.rx.xdp_drop;
u64_stats_update_end(&rx_ring->stats.rx.syncp);
+ xdp_return_buff(xdp);
break;
}
@@ -546,8 +489,11 @@ static bool aq_add_rx_fragment(struct device *dev,
do {
skb_frag_t *frag;
- if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
+ if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
+ /* Attached frags must reach xdp_return_buff() */
+ xdp_buff_set_frags_flag(xdp);
return true;
+ }
frag = &sinfo->frags[sinfo->nr_frags++];
buff_ = &ring->buff_ring[buff_->next];
@@ -571,6 +517,11 @@ static bool aq_add_rx_fragment(struct device *dev,
if (page_is_pfmemalloc(buff_->rxdata.page))
xdp_buff_set_frag_pfmemalloc(xdp);
+ /* The frag's page pool reference is owned by the xdp_buff
+ * from now on.
+ */
+ buff_->rxdata.page = NULL;
+
} while (!buff_->is_eop);
xdp_buff_set_frags_flag(xdp);
@@ -674,6 +625,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
err = -ENOMEM;
goto err_exit;
}
+ skb_mark_for_recycle(skb);
if (is_ptp_ring)
buff->len -=
aq_ptp_extract_ts(self->aq_nic, skb_hwtstamps(skb),
@@ -694,7 +646,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
buff->rxdata.pg_off + hdr_len,
buff->len - hdr_len,
self->frame_max);
- page_ref_inc(buff->rxdata.page);
+ buff->rxdata.page = NULL;
}
if (!buff->is_eop) {
@@ -713,7 +665,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
buff_->rxdata.pg_off,
buff_->len,
self->frame_max);
- page_ref_inc(buff_->rxdata.page);
+ buff_->rxdata.page = NULL;
buff_->is_cleaned = 1;
buff->is_ip_cso &= buff_->is_ip_cso;
@@ -851,6 +803,11 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
xdp_init_buff(&xdp, frame_sz, &rx_ring->xdp_rxq);
xdp_prepare_buff(&xdp, hard_start, rx_ring->page_offset,
buff->len, false);
+ /* The xdp_buff owns the buffer's page pool reference from
+ * here on; it comes back through the MEM_TYPE_PAGE_POOL
+ * memory model on every XDP verdict.
+ */
+ buff->rxdata.page = NULL;
if (!buff->is_eop) {
if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
u64_stats_update_begin(&rx_ring->stats.rx.syncp);
@@ -858,6 +815,7 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
rx_ring->stats.rx.bytes += xdp_get_buff_len(&xdp);
++rx_ring->stats.rx.xdp_aborted;
u64_stats_update_end(&rx_ring->stats.rx.syncp);
+ xdp_return_buff(&xdp);
continue;
}
}
@@ -969,7 +927,9 @@ void aq_ring_rx_deinit(struct aq_ring_s *self)
if (!buff->rxdata.page)
continue;
- aq_free_rxpage(&buff->rxdata, aq_nic_get_dev(self->aq_nic));
+ page_pool_put_full_page(self->pg_pool, buff->rxdata.page,
+ false);
+ buff->rxdata.page = NULL;
}
self->sw_head = self->sw_tail;
@@ -983,6 +943,11 @@ void aq_ring_free(struct aq_ring_s *self)
kfree(self->buff_ring);
self->buff_ring = NULL;
+ if (self->pg_pool) {
+ page_pool_destroy(self->pg_pool);
+ self->pg_pool = NULL;
+ }
+
if (self->dx_ring) {
dma_free_coherent(aq_nic_get_dev(self->aq_nic),
self->size * self->dx_size, self->dx_ring,
@@ -1021,9 +986,6 @@ unsigned int aq_ring_fill_stats_data(struct aq_ring_s *self, u64 *data)
data[++count] = self->stats.rx.alloc_fails;
data[++count] = self->stats.rx.skb_alloc_fails;
data[++count] = self->stats.rx.polls;
- data[++count] = self->stats.rx.pg_flips;
- data[++count] = self->stats.rx.pg_reuses;
- data[++count] = self->stats.rx.pg_losts;
data[++count] = self->stats.rx.xdp_aborted;
data[++count] = self->stats.rx.xdp_drop;
data[++count] = self->stats.rx.xdp_pass;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
index 6431cc62962f..58bcadb3e3cc 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.h
@@ -17,12 +17,12 @@
#define AQ_XDP_TAILROOM SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
struct page;
+struct page_pool;
struct aq_nic_cfg_s;
struct aq_rxpage {
struct page *page;
dma_addr_t daddr;
- unsigned int order;
unsigned int pg_off;
};
@@ -105,9 +105,6 @@ struct aq_ring_stats_rx_s {
u64 alloc_fails;
u64 skb_alloc_fails;
u64 polls;
- u64 pg_losts;
- u64 pg_flips;
- u64 pg_reuses;
u64 xdp_aborted;
u64 xdp_drop;
u64 xdp_pass;
@@ -151,6 +148,7 @@ struct aq_ring_s {
u16 tail_size;
union aq_ring_stats_s stats;
dma_addr_t dx_ring_pa;
+ struct page_pool *pg_pool;
struct bpf_prog *xdp_prog;
enum atl_ring_type ring_type;
struct xdp_rxq_info xdp_rxq;
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
index 05814fea0f5f..8e15405d4941 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
@@ -146,25 +146,32 @@ int aq_vec_ring_alloc(struct aq_vec_s *self, struct aq_nic_s *aq_nic,
aq_nic_set_tx_ring(aq_nic, idx_ring, ring);
ring = &self->ring[i][AQ_VEC_RX_ID];
+ /* Registering the MEM_TYPE_PAGE_POOL memory model below needs
+ * the page pool created by aq_ring_rx_alloc(), so the ring is
+ * allocated first. If a registration fails, the ring has to be
+ * freed explicitly: rx_rings is not incremented yet, so the
+ * unwind through aq_vec_ring_free() would not cover it.
+ */
+ err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
+ if (err)
+ goto err_exit;
+
if (xdp_rxq_info_reg(&ring->xdp_rxq,
aq_nic->ndev, idx,
self->napi.napi_id) < 0) {
+ aq_ring_free(ring);
err = -ENOMEM;
goto err_exit;
}
if (xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
- MEM_TYPE_PAGE_SHARED, NULL) < 0) {
+ MEM_TYPE_PAGE_POOL,
+ ring->pg_pool) < 0) {
xdp_rxq_info_unreg(&ring->xdp_rxq);
+ aq_ring_free(ring);
err = -ENOMEM;
goto err_exit;
}
- err = aq_ring_rx_alloc(ring, aq_nic, idx_ring, aq_nic_cfg);
- if (err) {
- xdp_rxq_info_unreg(&ring->xdp_rxq);
- goto err_exit;
- }
-
++self->rx_rings;
}
base-commit: 4fa4977a0d900f936bcae5cd2c510be5554e8dd6
--
2.47.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v3] net: atlantic: convert RX path to page_pool
2026-08-07 8:56 [PATCH net-next v3] net: atlantic: convert RX path to page_pool Yangyu Chen
@ 2026-08-08 8:56 ` sashiko-bot
2026-08-17 16:59 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-08 8:56 UTC (permalink / raw)
To: Yangyu Chen; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [High] Missing `dma_sync_single_for_device` on locally recycled RX buffers.
Pre-existing issues:
- [Critical] Kernel crash when `XDP_TX` is returned for a packet on the PTP RX ring due to a missing PTP TX ring mapping.
- [Critical] Garbage packet injection and state machine desynchronization when an XDP packet exceeds the MAX_SKB_FRAGS limit.
- [High] Infinite NAPI polling loop when receiving an oversized packet.
- [High] Concurrent execution of `u64_stats_update_begin` on `tx.syncp` corrupts seqcount.
- [High] Lockless concurrent access to TX ring descriptors in `aq_xdp_xmit`.
--
--- Patch [1]: [PATCH net-next v3] net: atlantic: convert RX path to page_pool ---
commit 6897c00e6a396ede4252845d65592374630adc51
Author: Yangyu Chen <cyy@cyyself.name>
net: atlantic: convert RX path to page_pool
The driver is converted to use page_pool fragments for RX buffers instead of
dev_alloc_pages() and a hand-rolled page-flip scheme. The PTP RX ring's
xdp_rxq is also registered to use the MEM_TYPE_PAGE_POOL memory model.
Link: https://lore.kernel.org/lkml/tencent_1F173E0FC1606D2AC704DC9C98AF10984607@qq.com/
Link: https://lore.kernel.org/lkml/tencent_7DB01BE7F8FA056BB5F11D3570CF636C4309@qq.com/
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> index 558ac9237f759..3a40d986cd678 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> @@ -1192,12 +1193,23 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
> if (err)
> goto err_exit_ptp_tx;
>
[Severity: Critical]
This is a pre-existing issue, but could this cause a kernel panic from a
NULL pointer dereference? In aq_ptp_ring_alloc(), the PTP TX ring is
allocated but not registered in the global aq_nic->aq_ring_tx array:
aq_ptp_ring_alloc()
err = aq_ring_tx_alloc(&aq_ptp->ptp_tx, aq_nic, ...);
If an XDP program is attached and returns XDP_TX for a packet received on
the PTP RX ring, the tx_ring lookup evaluates to NULL:
aq_xdp_run_prog()
tx_ring = aq_nic->aq_ring_tx[rx_ring->idx];
result = aq_nic_xmit_xdpf(aq_nic, tx_ring, xdpf);
> + err = xdp_rxq_info_reg(&aq_ptp->ptp_rx.xdp_rxq, aq_nic->ndev,
> + rx_ring_idx, aq_ptp->napi.napi_id);
[ ... ]
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e1193c6719d9d..9dd8817105942 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -14,120 +14,37 @@
> -static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
> +static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
> {
> - unsigned int order = self->page_order;
[ ... ]
> + unsigned int size = self->page_offset + self->frame_max +
> + self->tail_size;
> + unsigned int pg_off;
> struct page *page;
>
> - if (rxbuf->rxdata.page) {
[ ... ]
> + /* Buffers whose page was not passed up the stack are reposted
> + * with the data they already carry discarded.
> + */
> + if (rxbuf->rxdata.page)
> + return 0;
[Severity: High]
Are we missing a dma_sync_single_for_device() call for the locally
recycled buffer?
When a packet is smaller than the header copy threshold, its payload is
copied to a newly allocated SKB and its page is recycled directly. The CPU
reads the data after a dma_sync_single_range_for_cpu() call, but when the
page is passed back to the hardware, the lack of
dma_sync_single_for_device() might allow the CPU cache to overwrite DMA
memory while the NIC writes the next packet.
> + page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
[ ... ]
> @@ -346,7 +284,11 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
> ++self->stats.tx.packets;
> self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
> u64_stats_update_end(&self->stats.tx.syncp);
> - xdp_return_frame_rx_napi(buff->xdpf);
> + /* Frames queued via ndo_xdp_xmit() may come from a
[Severity: High]
This is a pre-existing issue, but might this lead to a corrupted seqcount
and infinite loops for readers on 32-bit systems?
Writers to a u64_stats_sync block must be mutually exclusive and disable
preemption. However, this code updates tx.syncp locklessly from the NAPI
poll context in aq_ring_tx_clean():
aq_ring_tx_clean()
if (buff->skb) {
u64_stats_update_begin(&self->stats.tx.syncp);
While aq_ptp_xmit() updates the exact same syncp block holding a spinlock
in process or softirq context:
aq_ptp_xmit()
spin_lock_irqsave(&aq_nic->aq_ptp->ptp_ring_lock, irq_flags);
...
u64_stats_update_begin(&ring->stats.tx.syncp);
> + * page pool owned by another NAPI context: no direct
> + * recycling.
> + */
> + xdp_return_frame(buff->xdpf);
> }
>
> out:
[ ... ]
> @@ -437,22 +379,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
> }
[Severity: High]
This is a pre-existing issue, but could this cause lockless concurrent
access to TX ring descriptors?
If the number of online CPUs exceeds the number of hardware queues, multiple
CPUs will map to the same ring and invoke aq_nic_xmit_xdpf() simultaneously
without any locks:
aq_xdp_xmit()
vec = cpu % aq_cfg->vecs;
ring = aq_nic->aq_ring_tx[AQ_NIC_CFG_TCVEC2RING(aq_cfg, 0, vec)];
for (i = 0; i < num_frames; i++) {
...
if (aq_nic_xmit_xdpf(aq_nic, ring, xdpf) == NETDEV_TX_BUSY)
>
> static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,
> - struct net_device *dev,
> - struct aq_ring_buff_s *buff)
[ ... ]
> @@ -546,8 +489,11 @@ static bool aq_add_rx_fragment(struct device *dev,
> do {
> skb_frag_t *frag;
>
> - if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
> + if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
> + /* Attached frags must reach xdp_return_buff() */
> + xdp_buff_set_frags_flag(xdp);
> return true;
> + }
>
> frag = &sinfo->frags[sinfo->nr_frags++];
> buff_ = &ring->buff_ring[buff_->next];
[Severity: Critical]
This is a pre-existing issue, but does this abandon trailing fragments
without marking them as cleaned?
When a packet exceeds MAX_SKB_FRAGS, aq_add_rx_fragment() returns true
early:
aq_add_rx_fragment()
if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
...
return true;
}
Then __aq_ring_xdp_clean() skips further processing of those fragments:
__aq_ring_xdp_clean()
if (!buff->is_eop) {
if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
...
xdp_return_buff(&xdp);
continue;
}
}
The uncleaned hardware descriptors are left behind. On the next loop
iteration, the driver will treat these uncleaned payload fragments as the
start of a completely new packet, potentially injecting garbage or bypassing
header checks.
> @@ -674,6 +625,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
> err = -ENOMEM;
> goto err_exit;
> }
> + skb_mark_for_recycle(skb);
[Severity: High]
This is a pre-existing issue, but can this create an infinite NAPI polling
loop when receiving an oversized packet?
In __aq_ring_rx_clean(), when a packet spanning more than MAX_SKB_FRAGS is
received:
__aq_ring_rx_clean()
if (unlikely(!is_rsc_completed) ||
frag_cnt > MAX_SKB_FRAGS) {
err = 0;
goto err_exit;
}
The error handler jumps to err_exit without advancing self->sw_head. This
would trap the driver into processing the exact same failing packet
continuously on every poll iteration, leading to a denial of service for
the affected RX queue.
> if (is_ptp_ring)
> buff->len -=
> aq_ptp_extract_ts(self->aq_nic, skb_hwtstamps(skb),
--
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_F771B81731B921458D6F7CBF82ECDA888E08@qq.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v3] net: atlantic: convert RX path to page_pool
2026-08-07 8:56 [PATCH net-next v3] net: atlantic: convert RX path to page_pool Yangyu Chen
2026-08-08 8:56 ` sashiko-bot
@ 2026-08-17 16:59 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2026-08-17 16:59 UTC (permalink / raw)
To: cyy
Cc: Jakub Kicinski, sukhdeeps, andrew+netdev, davem, edumazet, pabeni,
almasrymina, hawk, richardcochran, netdev, bpf, linux-kernel
This is an AI-generated review of your patch. The human sending this
email says "Comment about PTP queue index looks legit, I haven't
investigated the rest."
---
net: atlantic: convert RX path to page_pool
This converts the atlantic RX buffers from hand-rolled high-order page
allocation plus a page-flip reuse scheme to page_pool fragments, registers
the RX (and PTP RX) xdp_rxq with MEM_TYPE_PAGE_POOL, and drops the
PageFlips/PageReuses/PageFrees ethtool counters that described the old
scheme. A few questions below about the ownership transfers and about
what the commit message describes.
> net: atlantic: convert RX path to page_pool
[Severity: Low]
The subject and the body scope this to the RX path:
"ensures every RX path either keeps the ring's fragment reference for
reposting or hands it to the skb/xdp_buff for later recycling"
but the patch also changes TX completion behaviour in aq_ring_tx_clean(),
replacing xdp_return_frame_rx_napi() with xdp_return_frame(). That change
is only documented in an in-code comment.
The difference is real once this patch lands, because __xdp_return()
honours napi_direct only for MEM_TYPE_PAGE_POOL:
case MEM_TYPE_PAGE_POOL:
netmem = netmem_compound_head(netmem);
if (napi_direct && xdp_return_frame_no_direct())
napi_direct = false;
page_pool_put_full_netmem(netmem_get_pp(netmem), netmem,
napi_direct);
so all XDP TX completions lose direct NAPI recycling, and frames arriving
via ndo_xdp_xmit() from a foreign pool are no longer pushed into that
pool's lock-free cache. Could the commit message mention this TX-side
semantic change?
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> index 558ac9237f75..3a40d986cd67 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
[ ... ]
> @@ -1192,12 +1193,23 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
> if (err)
> goto err_exit_ptp_tx;
>
> + err = xdp_rxq_info_reg(&aq_ptp->ptp_rx.xdp_rxq, aq_nic->ndev,
> + rx_ring_idx, aq_ptp->napi.napi_id);
> + if (err < 0)
> + goto err_exit_ptp_rx;
[Severity: Low]
Is rx_ring_idx the right value for the queue_index argument here? It comes
from aq_ptp_ring_idx(), which is documented in aq_ptp.h as a hardware
descriptor ring index:
/* Index must to be 8 (8 TCs) or 16 (4 TCs).
* It depends from Traffic Class mode.
*/
static inline unsigned int aq_ptp_ring_idx(const enum aq_tc_mode tc_mode)
The peer call site in aq_vec_ring_alloc() passes the netdev queue index
instead:
if (xdp_rxq_info_reg(&ring->xdp_rxq,
aq_nic->ndev, idx,
self->napi.napi_id) < 0) {
Note it passes idx, not idx_ring. queue_index is exported to BPF as
xdp_md->rx_queue_index and is what AF_XDP compares against the bound queue
id, and 8 or 16 is outside the range the driver advertises with:
err = netif_set_real_num_rx_queues(self->ndev,
self->aq_vecs * cfg->tcs);
__xdp_rxq_info_reg() stores the value verbatim without any range check:
xdp_rxq->dev = dev;
xdp_rxq->queue_index = queue_index;
The clean paths already treat the PTP ring as queue 0 via
skb_record_rx_queue(skb, is_ptp_ring ? 0 : AQ_NIC_RING2QMAP(...)), so
should this registration use 0 (or the netdev queue id) as well?
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e1193c6719d9..9dd881710594 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
[ ... ]
> @@ -346,7 +284,11 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
> ++self->stats.tx.packets;
> self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
> u64_stats_update_end(&self->stats.tx.syncp);
> - xdp_return_frame_rx_napi(buff->xdpf);
> + /* Frames queued via ndo_xdp_xmit() may come from a
> + * page pool owned by another NAPI context: no direct
> + * recycling.
> + */
> + xdp_return_frame(buff->xdpf);
> }
[Severity: High]
This isn't a regression introduced by this patch, but the pre-existing
ndo_xdp_xmit accounting in aq_xdp_xmit() becomes more expensive once the
frames are page_pool backed. aq_xdp_xmit() keeps submitting after a
failure and returns a count:
for (i = 0; i < num_frames; i++) {
struct xdp_frame *xdpf = frames[i];
if (aq_nic_xmit_xdpf(aq_nic, ring, xdpf) == NETDEV_TX_BUSY)
drop++;
}
return num_frames - drop;
The core treats the return value as the length of a consumed prefix,
bq_xmit_all() in kernel/bpf/devmap.c:
/* If not all frames have been transmitted, it is our
* responsibility to free them
*/
for (i = sent; unlikely(i < to_send); i++)
xdp_return_frame_rx_napi(bq->q[i]);
and aq_nic_xmit_xdpf() returns NETDEV_TX_BUSY without freeing xdpf when
aq_nic_map_xdp() fails on dma_map_single()/skb_frag_dma_map():
frags = aq_nic_map_xdp(aq_nic, xdpf, tx_ring);
if (likely(frags))
err = aq_nic->aq_hw_ops->hw_ring_tx_xmit(aq_nic->aq_hw, tx_ring,
frags);
So if a middle frame fails while a later one is queued successfully, the
core frees a frame that is still sitting in the TX ring, and the frame that
actually failed is never released.
After this patch, can that first frame then be released a second time here
through xdp_return_frame() -> page_pool_put_full_netmem(), underflowing
pp_ref_count and recycling a page the NIC is still DMAing from? And does
the never-released frame hold a pool fragment forever, so the new
page_pool_destroy() call in aq_ring_free() stalls?
Would returning the length of the successfully transmitted prefix (breaking
out of the loop on the first failure) fix the accounting?
[ ... ]
> @@ -546,8 +489,11 @@ static bool aq_add_rx_fragment(struct device *dev,
> do {
> skb_frag_t *frag;
>
> - if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
> + if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
> + /* Attached frags must reach xdp_return_buff() */
> + xdp_buff_set_frags_flag(xdp);
> return true;
> + }
>
> frag = &sinfo->frags[sinfo->nr_frags++];
> buff_ = &ring->buff_ring[buff_->next];
> @@ -571,6 +517,11 @@ static bool aq_add_rx_fragment(struct device *dev,
> if (page_is_pfmemalloc(buff_->rxdata.page))
> xdp_buff_set_frag_pfmemalloc(xdp);
>
> + /* The frag's page pool reference is owned by the xdp_buff
> + * from now on.
> + */
> + buff_->rxdata.page = NULL;
> +
> } while (!buff_->is_eop);
[Severity: Low]
This isn't a bug, but the reported validation is:
"smoke-tested with native XDP PASS, DROP, and ABORTED actions"
at "MTU 1500"
Those three verdicts are exactly the paths that simply call
xdp_return_buff() in the new code. The hunks with the ownership transfers
do not appear to be covered:
- the per-fragment transfer above plus the new xdp_buff_set_frags_flag()
on the MAX_SKB_FRAGS abort
- the two buff->rxdata.page = NULL sites in __aq_ring_rx_clean()
- XDP_TX and XDP_REDIRECT in aq_xdp_run_prog(), which lost their
aq_get_rxpages_xdp() reference acquisition and now rely entirely on the
caller having nulled buff->rxdata.page
- the PTP RX ring, newly registered with MEM_TYPE_PAGE_POOL and sharing
__aq_ring_xdp_clean()
Multi-buffer XDP cannot be reached at MTU 1500: aq_ring_rx_alloc() sets
self->frame_max = AQ_CFG_RX_FRAME_MAX (2048) and aq_xdp_setup() only allows
a larger MTU for frags-capable programs:
if (prog && !prog->aux->xdp_has_frags) {
if (ndev->mtu > AQ_CFG_RX_FRAME_MAX) {
so aq_add_rx_fragment() is never entered in that test. Could the multi-
buffer, XDP_TX/XDP_REDIRECT and PTP-ring paths be exercised too (for
example at MTU 9000 with an xdp.frags program), so the fragment ownership
transfers are covered by more than inspection?
[ ... ]
> @@ -851,6 +803,11 @@ static int __aq_ring_xdp_clean(struct aq_ring_s *rx_ring,
> xdp_init_buff(&xdp, frame_sz, &rx_ring->xdp_rxq);
> xdp_prepare_buff(&xdp, hard_start, rx_ring->page_offset,
> buff->len, false);
> + /* The xdp_buff owns the buffer's page pool reference from
> + * here on; it comes back through the MEM_TYPE_PAGE_POOL
> + * memory model on every XDP verdict.
> + */
> + buff->rxdata.page = NULL;
[Severity: High]
This is a pre-existing issue and the loops are untouched by the diff, but
since this patch reworks buffer ownership inside this function it may be
worth fixing here: the two descriptor-chain walks earlier in
__aq_ring_xdp_clean() are unbounded. The RSC completion walk:
do {
if (buff_->next >= rx_ring->size) {
err = -EIO;
goto err_exit;
}
next_ = buff_->next;
buff_ = &rx_ring->buff_ring[next_];
is_rsc_completed =
aq_ring_dx_in_range(rx_ring->sw_head,
next_,
rx_ring->hw_head);
if (unlikely(!is_rsc_completed))
break;
...
} while (!buff_->is_eop);
and the error-drain walk right below it follow buff->next, which for LRO
descriptors is copied straight out of the hardware write-back in
hw_atl_b0_hw_ring_rx_receive():
if (buff->is_lro) {
/* LRO */
buff->next = rxd_wb->next_desc_ptr;
Can a cyclic chain A->B->A, where both entries are inside the
[sw_head, hw_head] window and neither has is_eop set, spin here forever
with BH disabled in the NAPI poll? The non-XDP sibling
__aq_ring_rx_clean() bounds the identical walk:
if (unlikely(!is_rsc_completed) ||
frag_cnt > MAX_SKB_FRAGS) {
err = 0;
goto err_exit;
}
Should the XDP path carry the same hop counter?
> if (!buff->is_eop) {
> if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
> u64_stats_update_begin(&rx_ring->stats.rx.syncp);
[ ... ]
--
pw-bot: cr
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-08-17 16:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-07 8:56 [PATCH net-next v3] net: atlantic: convert RX path to page_pool Yangyu Chen
2026-08-08 8:56 ` sashiko-bot
2026-08-17 16:59 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox