* [PATCH net-next v3] net: atlantic: convert RX path to page_pool
@ 2026-08-07 8:56 Yangyu Chen
2026-08-17 16:59 ` Jakub Kicinski
0 siblings, 1 reply; 2+ 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] 2+ 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-17 16:59 ` Jakub Kicinski
0 siblings, 0 replies; 2+ 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] 2+ messages in thread
end of thread, other threads:[~2026-08-17 16:59 UTC | newest]
Thread overview: 2+ 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-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