Re: [PATCH net] igc: track rx frame boundaries independent of skbs

[Paul Moses]

On Monday, October 26th, 2026 at 2:46 PM, Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx> wrote:

> On Sun, Oct 25, 2026 at 06:52:06AM +0000, Paul Moses wrote:
> > TSIP remains set on continuation descriptors, but the inline timestamp
> > header exists only in the first buffer. Stripping it from every
> > descriptor truncates split frames and can underflow short buffers.
> >
> > skb ownership cannot identify frame boundaries when XDP consumes the
> > first buffer or AF_XDP uses zero-copy.
> >
> > Track rx_in_frame per ring and update it as descriptors are consumed.
> > Strip timestamps only from first buffers, reject unsupported split
> > frames before XDP and drain continuations. Account discarded
> > descriptors against NAPI work without counting successful RX.
> >
> > Fixes: e1ed4f92a625 ("igc: Refactor Rx timestamp handling")
> > Fixes: 26575105d6ed ("igc: Add initial XDP support")
> > Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Paul Moses <p@xxxxxxx>
> > ---
> > - This supersedes "igc: only strip RX timestamp header from first buffer"
> >   following feedback from Simon Horman, Maciej Fijalkowski and Sashiko.
> >
> > Changes:
> > - track frame boundaries independently of skb ownership
> > - guard timestamp stripping and drain unsupported split frames
> > - cover the AF_XDP zero-copy RX path
> > ---
> >  drivers/net/ethernet/intel/igc/igc.h      |  1 +
> >  drivers/net/ethernet/intel/igc/igc_main.c | 87 ++++++++++++++++-------
> >  2 files changed, 64 insertions(+), 24 deletions(-)
> 
> Hi Paul,
> 
> I think this implementation went off track. We should make sure we *never*
> enter data path for XDP/AF_XDP multi-buffer scenario until we add real
> support to it within this driver. Now you sorta handle fragmented packets
> in data path.
> 
> I tried to argue with bot reports that it hallucinated for
> igc_clean_rx_irq() content (which was concise and to the point) and agreed
> the AF_XDP should *not* be exposed to any fragmented traffic, which should
> be addressed on HW RX ring configuration side, not within data path.
> 
> Also this should have been a v3 with a link to previous revisions so we
> could keep track what has been changing between versions.
> 
> Here's something that would address ZC fragmentation issue. Could you give
> it a spin on your side as I don't currently have igc in my setup handy.
> 
> 
> From a3edec7db135f13a567565a652da5cc850505f7f Mon Sep 17 00:00:00 2001
> From: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> Date: Mon, 26 Oct 2026 15:00:00 +0100
> Subject: [PATCH net] igc: fix AF_XDP receive buffer size for 2K UMEM chunks
> 
> SRRCTL.BSIZEPKT has a granularity of 1 KiB. With a 2 KiB UMEM
> chunk and the default XDP headroom, xsk_pool_get_rx_frame_size()
> returns 1792. Programming this value truncates the hardware receive
> buffer size to 1 KiB, even though an MTU-sized frame fits in the
> usable UMEM area.
> 
> XDP on igc is limited to MTU 1500, so configure a 2 KiB receive
> buffer for XSK rings. Limit RLPML to the maximum supported Ethernet
> frame size while XDP is attached, regardless of PAGE_SIZE, to avoid
> oversized frames spanning descriptors or exceeding the usable UMEM
> space.
> 
> RLPML is currently programmed from igc_set_rx_mode(), which can run
> without RTNL, for example on an IPv6 multicast join, and could then
> use a stale XDP state. RLPML does not depend on the address lists.
> Program it from igc_setup_rctl() instead, next to RCTL.LPE, and from
> igc_xdp_set_prog() while the rings are disabled. This avoids
> netif_addr_lock_bh() usage.
> 
> Validate that the XSK frame size accommodates a maximum-size frame
> with two VLAN tags, FCS and the optional inline RX timestamp. Check
> against the maximum supported XDP MTU, rather than the current MTU,
> which can change after the socket is bound.
> 
> Fixes: fc9df2a0b520 ("igc: Enable RX via AF_XDP zero-copy")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  1 +
>  drivers/net/ethernet/intel/igc/igc_main.c | 33 +++++++++++++++++------
>  drivers/net/ethernet/intel/igc/igc_xdp.c  | 11 +++++---
>  3 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 17f213cc93e4..ad1062f8531a 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -765,6 +765,7 @@ static inline s32 igc_read_phy_reg(struct igc_hw *hw, u32 offset, u16 *data)
>  	return -EOPNOTSUPP;
>  }
> 
> +void igc_set_rlpml(struct igc_adapter *adapter);
>  void igc_reinit_locked(struct igc_adapter *);
>  struct igc_nfc_rule *igc_get_nfc_rule(struct igc_adapter *adapter,
>  				      u32 location);
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c..38a5702647f4 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -674,8 +674,11 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
>  	ring->next_to_clean = 0;
>  	ring->next_to_use = 0;
> 
> +	/* igc's XDP limits MTU to 1500; all XSK RX buffers can use 2kb;
> +	 * RLPML limits the receive length to fit after UMEM headroom.
> +	 */
>  	if (ring->xsk_pool)
> -		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> +		buf_size = IGC_RXBUFFER_2048;
>  	else if (ring_uses_large_buffer(ring))
>  		buf_size = IGC_RXBUFFER_3072;
>  	else
> @@ -830,6 +833,25 @@ static void igc_setup_mrqc(struct igc_adapter *adapter)
>  	wr32(IGC_MRQC, mrqc);
>  }
> 
> +void igc_set_rlpml(struct igc_adapter *adapter)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 rlpml = MAX_JUMBO_FRAME_SIZE;
> +
> +#if (PAGE_SIZE < 8192)
> +	if (adapter->max_frame_size <= IGC_MAX_FRAME_BUILD_SKB)
> +		rlpml = IGC_MAX_FRAME_BUILD_SKB;
> +#endif
> +	/* igc's XDP currently does not support jumbo frames. Limit the
> +	 * accepted packet size regardless of PAGE_SIZE so it fits in a 2kb
> +	 * XSK chunk.
> +	 */
> +	if (igc_xdp_is_enabled(adapter))
> +		rlpml = ETH_FRAME_LEN + VLAN_HLEN * 2 + ETH_FCS_LEN;
> +
> +	wr32(IGC_RLPML, rlpml);
> +}
> +
>  /**
>   * igc_setup_rctl - configure the receive control registers
>   * @adapter: Board private structure
> @@ -874,6 +896,7 @@ static void igc_setup_rctl(struct igc_adapter *adapter)
>  			  IGC_RCTL_CFIEN); /* Disable VLAN CFIEN Filter */
>  	}
> 
> +	igc_set_rlpml(adapter);
>  	wr32(IGC_RCTL, rctl);
>  }
> 
> @@ -4116,7 +4139,7 @@ static void igc_set_rx_mode(struct net_device *netdev)
>  {
>  	struct igc_adapter *adapter = netdev_priv(netdev);
>  	struct igc_hw *hw = &adapter->hw;
> -	u32 rctl = 0, rlpml = MAX_JUMBO_FRAME_SIZE;
> +	u32 rctl = 0;
>  	int count;
> 
>  	/* Check for Promiscuous and All Multicast modes */
> @@ -4146,12 +4169,6 @@ static void igc_set_rx_mode(struct net_device *netdev)
>  	/* update state of unicast and multicast */
>  	rctl |= rd32(IGC_RCTL) & ~(IGC_RCTL_UPE | IGC_RCTL_MPE);
>  	wr32(IGC_RCTL, rctl);
> -
> -#if (PAGE_SIZE < 8192)
> -	if (adapter->max_frame_size <= IGC_MAX_FRAME_BUILD_SKB)
> -		rlpml = IGC_MAX_FRAME_BUILD_SKB;
> -#endif
> -	wr32(IGC_RLPML, rlpml);
>  }
> 
>  /**
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
> index 9eb47b4beb06..a0f1ecb2ed5f 100644
> --- a/drivers/net/ethernet/intel/igc/igc_xdp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> @@ -34,6 +34,8 @@ int igc_xdp_set_prog(struct igc_adapter *adapter, struct bpf_prog *prog,
>  	}
> 
>  	old_prog = xchg(&adapter->xdp_prog, prog);
> +	if (if_running && need_update)
> +		igc_set_rlpml(adapter);
>  	if (old_prog)
>  		bpf_prog_put(old_prog);
> 
> @@ -69,11 +71,12 @@ static int igc_xdp_enable_pool(struct igc_adapter *adapter,
>  		return -EINVAL;
> 
>  	frame_size = xsk_pool_get_rx_frame_size(pool);
> -	if (frame_size < ETH_FRAME_LEN + VLAN_HLEN * 2) {
> +	if (frame_size < ETH_FRAME_LEN + VLAN_HLEN * 2 +
> +			 ETH_FCS_LEN + IGC_TS_HDR_LEN) {
>  		/* When XDP is enabled, the driver doesn't support frames that
> -		 * span over multiple buffers. To avoid that, we check if xsk
> -		 * frame size is big enough to fit the max ethernet frame size
> -		 * + vlan double tagging.
> +		 * span multiple buffers. Check the maximum supported MTU, not
> +		 * current MTU, as it may change while the pool is bound. Account
> +		 * for double VLAN tags, FCS and inline RX timestamping.
>  		 */
>  		return -EOPNOTSUPP;
>  	}
> --
> 2.43.0
> 
> 
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> > index 17f213cc93e4..635d49701e62 100644
> > --- a/drivers/net/ethernet/intel/igc/igc.h
> > +++ b/drivers/net/ethernet/intel/igc/igc.h
> > @@ -198,6 +198,7 @@ struct igc_ring {
> >  			struct igc_rx_packet_stats pkt_stats;
> >  			struct u64_stats_sync rx_syncp;
> >  			struct sk_buff *skb;
> > +			bool rx_in_frame;
> >  		};
> >  	};
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 1fb5f3cbe93c..9ca5558c762c 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -474,6 +474,7 @@ static void igc_clean_rx_ring(struct igc_ring *ring)
> >  		igc_clean_rx_ring_page_shared(ring);
> >
> >  	clear_ring_uses_large_buffer(ring);
> > +	ring->rx_in_frame = false;
> >
> >  	ring->next_to_alloc = 0;
> >  	ring->next_to_clean = 0;
> > @@ -673,6 +674,7 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
> >  	/* reset next-to- use/clean to place SW in sync with hardware */
> >  	ring->next_to_clean = 0;
> >  	ring->next_to_use = 0;
> > +	ring->rx_in_frame = false;
> >
> >  	if (ring->xsk_pool)
> >  		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> > @@ -2111,10 +2113,8 @@ static bool igc_can_reuse_rx_page(struct igc_rx_buffer *rx_buffer,
> >   * @rx_ring: Rx ring being processed
> >   * @rx_desc: Rx descriptor for current buffer
> >   *
> > - * This function updates next to clean.  If the buffer is an EOP buffer
> > - * this function exits returning false, otherwise it will place the
> > - * sk_buff in the next buffer to be chained and return true indicating
> > - * that this is in fact a non-EOP buffer.
> > + * Advance next to clean and retain frame position for the next descriptor.
> > + * Return true if the consumed descriptor does not end the frame.
> >   */
> >  static bool igc_is_non_eop(struct igc_ring *rx_ring,
> >  			   union igc_adv_rx_desc *rx_desc)
> > @@ -2127,10 +2127,8 @@ static bool igc_is_non_eop(struct igc_ring *rx_ring,
> >
> >  	prefetch(IGC_RX_DESC(rx_ring, ntc));
> >
> > -	if (likely(igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)))
> > -		return false;
> > -
> > -	return true;
> > +	rx_ring->rx_in_frame = !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP);
> > +	return unlikely(rx_ring->rx_in_frame);
> >  }
> >
> >  /**
> > @@ -2601,7 +2599,7 @@ static void igc_update_rx_stats(struct igc_q_vector *q_vector,
> >
> >  static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  {
> > -	unsigned int total_bytes = 0, total_packets = 0;
> > +	unsigned int total_bytes = 0, total_packets = 0, work_done = 0;
> >  	struct igc_adapter *adapter = q_vector->adapter;
> >  	struct igc_ring *rx_ring = q_vector->rx.ring;
> >  	struct sk_buff *skb = rx_ring->skb;
> > @@ -2609,13 +2607,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  	int xdp_status = 0, rx_buffer_pgcnt;
> >  	int xdp_res = 0;
> >
> > -	while (likely(total_packets < budget)) {
> > +	while (likely(work_done < budget)) {
> >  		struct igc_xdp_buff ctx = { .rx_ts = NULL };
> > +		bool first = !rx_ring->rx_in_frame;
> >  		struct igc_rx_buffer *rx_buffer;
> >  		union igc_adv_rx_desc *rx_desc;
> >  		unsigned int size, truesize;
> >  		int pkt_offset = 0;
> >  		void *pktbuf;
> > +		bool tsip;
> >
> >  		/* return some buffers to hardware, one at a time is too slow */
> >  		if (cleaned_count >= IGC_RX_BUFFER_WRITE) {
> > @@ -2635,21 +2635,34 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  		dma_rmb();
> >
> >  		rx_buffer = igc_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
> > +		tsip = first && igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP);
> > +
> > +		if ((!first && !skb) ||
> > +		    (first && igc_xdp_is_enabled(adapter) &&
> > +		     !igc_test_staterr(rx_desc, IGC_RXD_STAT_EOP)) ||
> > +		    unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> > +			if (first)
> > +				dev_core_stats_rx_dropped_inc(rx_ring->netdev);
> > +			xdp_res = IGC_XDP_CONSUMED;
> > +			goto xdp_done;
> > +		}
> > +
> >  		truesize = igc_get_rx_frame_truesize(rx_ring, size);
> >
> >  		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
> >
> > -		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
> > +		if (tsip) {
> >  			ctx.rx_ts = pktbuf;
> >  			pkt_offset = IGC_TS_HDR_LEN;
> >  			size -= IGC_TS_HDR_LEN;
> >  		}
> >
> > -		if (igc_fpe_is_pmac_enabled(adapter) &&
> > +		if (first && igc_fpe_is_pmac_enabled(adapter) &&
> >  		    igc_fpe_handle_mpacket(adapter, rx_desc, size, pktbuf + pkt_offset)) {
> >  			/* Advance the ring next-to-clean */
> >  			igc_is_non_eop(rx_ring, rx_desc);
> >  			cleaned_count++;
> > +			work_done++;
> >  			continue;
> >  		}
> >
> > @@ -2664,6 +2677,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  			xdp_res = igc_xdp_run_prog(adapter, &ctx.xdp);
> >  		}
> >
> > +		if (xdp_res) {
> > +			total_bytes += size;
> > +			total_packets++;
> > +		}
> > +
> > +xdp_done:
> >  		if (xdp_res) {
> >  			switch (xdp_res) {
> >  			case IGC_XDP_CONSUMED:
> > @@ -2676,8 +2695,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  				break;
> >  			}
> >
> > -			total_packets++;
> > -			total_bytes += size;
> > +			work_done++;
> >  		} else if (skb)
> >  			igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
> >  		else if (ring_uses_build_skb(rx_ring))
> > @@ -2700,8 +2718,15 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  		if (igc_is_non_eop(rx_ring, rx_desc))
> >  			continue;
> >
> > +		if (xdp_res) {
> > +			skb = NULL;
> > +			continue;
> > +		}
> > +
> > +		work_done++;
> > +
> >  		/* verify the packet layout is correct */
> > -		if (xdp_res || igc_cleanup_headers(rx_ring, rx_desc, skb)) {
> > +		if (igc_cleanup_headers(rx_ring, rx_desc, skb)) {
> >  			skb = NULL;
> >  			continue;
> >  		}
> > @@ -2717,7 +2742,6 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  		/* reset skb pointer */
> >  		skb = NULL;
> >
> > -		/* update budget accounting */
> >  		total_packets++;
> >  	}
> >
> > @@ -2732,7 +2756,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >  	if (cleaned_count)
> >  		igc_alloc_rx_buffers(rx_ring, cleaned_count);
> >
> > -	return total_packets;
> > +	return work_done;
> >  }
> >
> >  static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
> > @@ -2797,10 +2821,10 @@ static struct igc_xdp_buff *xsk_buff_to_igc_ctx(struct xdp_buff *xdp)
> >
> >  static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  {
> > +	int total_bytes = 0, total_packets = 0, work_done = 0;
> >  	struct igc_adapter *adapter = q_vector->adapter;
> >  	struct igc_ring *ring = q_vector->rx.ring;
> >  	u16 cleaned_count = igc_desc_unused(ring);
> > -	int total_bytes = 0, total_packets = 0;
> >  	u16 ntc = ring->next_to_clean;
> >  	struct bpf_prog *prog;
> >  	bool failure = false;
> > @@ -2810,11 +2834,13 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >
> >  	prog = READ_ONCE(adapter->xdp_prog);
> >
> > -	while (likely(total_packets < budget)) {
> > +	while (likely(work_done < budget)) {
> > +		bool first = !ring->rx_in_frame;
> >  		union igc_adv_rx_desc *desc;
> >  		struct igc_rx_buffer *bi;
> >  		struct igc_xdp_buff *ctx;
> >  		unsigned int size;
> > +		bool eop, tsip;
> >  		int res;
> >
> >  		desc = IGC_RX_DESC(ring, ntc);
> > @@ -2829,11 +2855,20 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  		dma_rmb();
> >
> >  		bi = &ring->rx_buffer_info[ntc];
> > +		tsip = first && igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP);
> > +		eop = igc_test_staterr(desc, IGC_RXD_STAT_EOP);
> > +
> > +		if (!first || !eop || unlikely(tsip && size < IGC_TS_HDR_LEN)) {
> > +			if (first)
> > +				dev_core_stats_rx_dropped_inc(ring->netdev);
> > +			res = IGC_XDP_CONSUMED;
> > +			goto xdp_done;
> > +		}
> >
> >  		ctx = xsk_buff_to_igc_ctx(bi->xdp);
> >  		ctx->rx_desc = desc;
> >
> > -		if (igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP)) {
> > +		if (tsip) {
> >  			ctx->rx_ts = bi->xdp->data;
> >
> >  			bi->xdp->data += IGC_TS_HDR_LEN;
> > @@ -2851,6 +2886,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  		xsk_buff_dma_sync_for_cpu(bi->xdp);
> >
> >  		res = __igc_xdp_run_prog(adapter, prog, bi->xdp);
> > +		total_bytes += size;
> > +		total_packets++;
> > +
> > +xdp_done:
> >  		switch (res) {
> >  		case IGC_XDP_PASS:
> >  			igc_dispatch_skb_zc(q_vector, desc, ctx);
> > @@ -2865,8 +2904,8 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  		}
> >
> >  		bi->xdp = NULL;
> > -		total_bytes += size;
> > -		total_packets++;
> > +		ring->rx_in_frame = !eop;
> > +		work_done++;
> >  		cleaned_count++;
> >  		ntc++;
> >  		if (ntc == ring->count)
> > @@ -2889,10 +2928,10 @@ static int igc_clean_rx_irq_zc(struct igc_q_vector *q_vector, const int budget)
> >  			xsk_set_rx_need_wakeup(ring->xsk_pool);
> >  		else
> >  			xsk_clear_rx_need_wakeup(ring->xsk_pool);
> > -		return total_packets;
> > +		return work_done;
> >  	}
> >
> > -	return failure ? budget : total_packets;
> > +	return failure ? budget : work_done;
> >  }
> >
> >  static void igc_update_tx_stats(struct igc_q_vector *q_vector,
> >
> 

Looks good to me, passes basic testing, and I would rather you/intel do this patch

For posterity, I did email Tony with handoff notes stating I'd rather not do the 
xdp/xsk/af_xdp/fpe side of things, but hadn't heard back so I kept going.