Re: [PATCH v5] r8169: migrate Rx path to page_pool, prepare for XDP

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260713035049.25310-1-atharvapotdar07@gmail.com?part=1
---
r8169: migrate Rx path to page_pool, prepare for XDP

This commit replaces the alloc_pages() + dma_map_page() + skb_copy() Rx
model with a page_pool-backed napi_build_skb() path. It keeps MACs up to
RTL_GIGA_MAC_VER_06 on the copy path, and reserves XDP_PACKET_HEADROOM
on newer MACs. It also decouples cur_rx from dirty_rx and registers an
xdp_rxq for MEM_TYPE_PAGE_POOL.

A few questions on the new code below.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21f..c548e7e8db87 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -4221,12 +4228,57 @@ static int rtl8169_rx_fill(struct rtl8169_private *tp)
>  
>  static int rtl8169_init_ring(struct rtl8169_private *tp)
>  {
> +	struct page_pool_params params = {0};
> +	int err;
> +
>  	rtl8169_init_ring_indexes(tp);
>  
> +	if (tp->mac_version <= RTL_GIGA_MAC_VER_06) {
> +		tp->rx_headroom = 0;
> +		tp->rx_buf_sz = R8169_RX_BUF_SIZE;
> +	} else {
> +		tp->rx_headroom = XDP_PACKET_HEADROOM;
> +		tp->rx_buf_sz = SZ_16K -
> +						SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) -
> +						XDP_PACKET_HEADROOM;
> +	}

[High]

Can this silently break jumbo Rx on RTL8125/RTL8126? For those MACs
rtl_jumbo_max() returns JUMBO_16K (16362 bytes) and dev->max_mtu is
left unchanged by this patch, but tp->rx_buf_sz here becomes:

    SZ_16K - SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
           - XDP_PACKET_HEADROOM

which is roughly 15808 bytes. This value is written into every
descriptor by rtl8169_mark_to_asic():

    WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | rx_buf_sz));

while rtl_set_rx_max_size() still programs RxMaxSize based on
R8169_RX_BUF_SIZE + 1 = 16384. When a frame larger than ~15808 bytes
arrives at an MTU near 16362, the hardware will split it across
descriptors, and rtl8169_fragmented_frame() then drops it and bumps
rx_length_errors:

    if (unlikely(rtl8169_fragmented_frame(status))) {
        dev->stats.rx_dropped++;
        dev->stats.rx_length_errors++;
        goto release_descriptor;
    }

Should dev->max_mtu (or rtl_jumbo_max()) be adjusted for the newer
MACs so this reduction in the per-descriptor buffer size does not
silently drop max-size jumbo frames?

Relatedly, rtl8169_change_mtu() does not recompute rx_buf_sz or
repopulate the ring, so a change from a smaller MTU up towards
JUMBO_16K will not re-evaluate this either. Is that intentional?

Since the commit message describes this only as XDP preparation, could
the MTU-visible behaviour change be called out there as well?

[ ... ]

> +	tp->rx_pool = page_pool_create(&params);
> +	if (IS_ERR(tp->rx_pool)) {
> +		err = PTR_ERR(tp->rx_pool);
> +		tp->rx_pool = NULL;
> +		return err;
> +	}
> +
> +	err = xdp_rxq_info_reg(&tp->xdp_rxq, tp->dev, 0, tp->napi.napi_id);
> +	if (err)
> +		goto err_free_pool;

[Medium]

At this point, is tp->napi.napi_id actually assigned yet? rtl_open()
calls rtl8169_init_ring() before rtl8169_up() -> napi_enable(), and
napi_hash_add() (which populates napi_id) runs from
napi_enable_locked(). netif_napi_add() at probe time does not assign a
napi_id either.

If so, xdp_rxq_info_reg() is called with napi_id == 0 and the xdp_rxq
loses the association intended for busy-poll, netlink queue
introspection and XDP redirect wakeups.

Would deferring xdp_rxq_info_reg() until after napi_enable(), or using
a persistent napi_config-based id, keep the association the commit
message is establishing?

[ ... ]

> @@ -4311,8 +4363,20 @@ static void rtl_reset_work(struct rtl8169_private *tp)
>  
>  	rtl8169_cleanup(tp);
>  
> -	for (i = 0; i < NUM_RX_DESC; i++)
> -		rtl8169_mark_to_asic(tp->RxDescArray + i);
> +	for (i = 0; i < NUM_RX_DESC; i++) {
> +		if (tp->Rx_databuff[i])
> +			continue;
> +		tp->Rx_databuff[i] =
> +			rtl8169_alloc_rx_data(tp, tp->RxDescArray + i);
> +		if (!tp->Rx_databuff[i])
> +			break;
> +	}
> +
> +	for (i = 0; i < NUM_RX_DESC; i++) {
> +		if (!tp->Rx_databuff[i])
> +			continue;
> +		rtl8169_mark_to_asic(tp->RxDescArray + i, tp->rx_buf_sz);
> +	}
>  
>  	napi_enable(&tp->napi);
>  	rtl_hw_start(tp);

[High]

Can this leave the Rx ring with permanent gaps under memory pressure?

rtl8169_cleanup() does not clear Rx_databuff[], so on entry this loop
tries to refill slots consumed by the zero-copy path. On the first
allocation failure it breaks:

    if (!tp->Rx_databuff[i])
        break;

This skips every later NULL slot, even those where a subsequent
allocation might have succeeded. The second loop only marks entries
that already have a databuff, so the gap entries keep DescOwn=0.

rtl_reset_work() is void and rtl_hw_start() runs unconditionally, so
hardware then walks the ring sequentially and stalls at the first
gap. Since the normal refill (rtl8169_rx_refill()) only iterates
between dirty_rx and cur_rx, would slots at higher indices that were
never allocated here ever be filled again?

Would it be preferable to keep retrying across all slots (rather than
break on the first failure), or to fail the reset out to a scheduled
retry when allocation fails?

[ ... ]

> @@ -4768,16 +4832,39 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
>  		skb_checksum_none_assert(skb);
>  }
>  
> +static void rtl8169_rx_refill(struct rtl8169_private *tp)
> +{
> +	while (tp->dirty_rx != tp->cur_rx) {
> +		unsigned int entry = tp->dirty_rx % NUM_RX_DESC;
> +		struct RxDesc *desc = tp->RxDescArray + entry;
> +
> +		if (!tp->Rx_databuff[entry]) {
> +			struct page *new_page = page_pool_dev_alloc_pages(tp->rx_pool);
> +
> +			if (unlikely(!new_page))
> +				break;
> +
> +			tp->Rx_databuff[entry] = new_page;
> +
> +			desc->addr = cpu_to_le64(page_pool_get_dma_addr(new_page) +
> +				tp->rx_headroom);
> +		}
> +		rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
> +
> +		tp->dirty_rx++;
> +	}
> +}
> +
>  static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget)
>  {
>  	struct device *d = tp_to_dev(tp);
>  	int count;
>  
> -	for (count = 0; count < budget; count++, tp->cur_rx++) {
> +	for (count = 0; count < budget;) {
>  		unsigned int pkt_size, entry = tp->cur_rx % NUM_RX_DESC;
>  		struct RxDesc *desc = tp->RxDescArray + entry;
>  		struct sk_buff *skb;
> -		const void *rx_buf;
> +		void *rx_buf;
>  		dma_addr_t addr;
>  		u32 status;
>  
> @@ -4820,21 +4907,40 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  			goto release_descriptor;
>  		}
>  
> -		skb = napi_alloc_skb(&tp->napi, pkt_size);
> -		if (unlikely(!skb)) {
> -			dev->stats.rx_dropped++;
> -			goto release_descriptor;
> -		}
> +		if (unlikely(!tp->Rx_databuff[entry]))
> +			break;

[High]

Can this pair of paths leave the ring wedged with hardware waiting on a
DescOwn=0 slot that software has already given up on?

If rtl8169_rx_refill() runs after some pages were consumed by the
zero-copy path and page_pool_dev_alloc_pages() fails, the loop breaks
without calling rtl8169_mark_to_asic() and without advancing dirty_rx:

    if (unlikely(!new_page))
        break;
    ...
    rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
    tp->dirty_rx++;

So the descriptor stays with DescOwn=0 and Rx_databuff[entry] == NULL.

On the next NAPI iteration, when rtl_rx() reaches that entry the new
early exit:

    if (unlikely(!tp->Rx_databuff[entry]))
        break;

leaves rtl_rx() without advancing tp->cur_rx and without re-marking
the descriptor. Hardware is also stuck at this same slot, so if there
is no other IRQ source (e.g. TX completions) to schedule NAPI again,
does anything actually recover the Rx queue?

The pre-patch code always called rtl8169_mark_to_asic(desc) on the
drop path in rtl_rx(), so this stalled state was not structurally
reachable. Should the alloc-failure path here retain ownership with
the hardware (mark_to_asic with a placeholder / retry later) rather
than leaving DescOwn=0?

[ ... ]