On Mon, Oct 27, 2025 at 06:18:01PM +0100, Alexander Lobakin wrote:
> From: Simon Horman <[email protected]>
> Date: Mon, 27 Oct 2025 16:23:28 +0000
>
> > On Mon, Oct 06, 2025 at 06:20:53PM +0200, Alexander Lobakin wrote:
> >> Add second page_pool for header buffers to each Rx queue and ability
> >> to toggle the header split on/off using Ethtool (default to off to
> >> match the current behaviour).
> >> Unlike idpf, all HW backed up by ice doesn't require any W/As and
> >> correctly splits all types of packets as configured: after L4 headers
> >> for TCP/UDP/SCTP, after L3 headers for other IPv4/IPv6 frames, after
> >> the Ethernet header otherwise (in case of tunneling, same as above,
> >> but after innermost headers).
> >> This doesn't affect the XSk path as there are no benefits of having
> >> it there.
> >>
> >> Signed-off-by: Alexander Lobakin <[email protected]>
> >> ---
> >> Applies on top of Tony's next-queue, depends on Michał's Page Pool
> >> conversion series.
> >>
> >> Sending for review and validation purposes.
> >>
> >> Testing hints: traffic testing with and without header split enabled.
> >> The header split can be turned on/off using Ethtool:
> >>
> >> sudo ethtool -G <iface> tcp-data-split on (or off)
> >
> > Nice, I'm very pleased to see this feature in the pipeline for the ice
> > driver.
>
> This is a prereq for io_uring/devmem support in ice which I'm currently
> finishing :>
>
> (I know it's a bit overdue already, but I couldn't find a free time slot
> earlier to implement this)
Great, I'm very pleased to hear io_uring/devmem support is in the pipeline.
>
> >
> > ...
> >
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c
> >> b/drivers/net/ethernet/intel/ice/ice_txrx.c
> >
> > ...
> >
> >> @@ -836,6 +858,20 @@ bool ice_alloc_rx_bufs(struct ice_rx_ring *rx_ring,
> >> unsigned int cleaned_count)
> >> */
> >> rx_desc->read.pkt_addr = cpu_to_le64(addr);
> >>
> >> + if (!hdr_fq.pp)
> >> + goto next;
> >> +
> >> + addr = libeth_rx_alloc(&hdr_fq, ntu);
> >> + if (addr == DMA_MAPPING_ERROR) {
> >> + rx_ring->ring_stats->rx_stats.alloc_page_failed++;
> >> +
> >> + libeth_rx_recycle_slow(fq.fqes[ntu].netmem);
> >> + break;
> >> + }
> >> +
> >> + rx_desc->read.hdr_addr = cpu_to_le64(addr);
> >> +
> >> +next:
> >
> > Is performance the reason that a goto is used here, rather than, say,
> > putting
> > the conditional code in an if condition? Likewise in ice_clean_rx_irq?
>
> Not the performance, but the thing that I can avoid introducing +1
> indentation level for 9 lines. I don't like big `if` blocks.
> IIRC there's no strong rule regarding this?
>
> (same for ice_clean_rx_irq)
Ok. I'd lean towards an if condition. But I don't feel strongly about it.
>
> >
> >> rx_desc++;
> >> ntu++;
> >> if (unlikely(ntu == rx_ring->count)) {
> >> @@ -933,14 +969,16 @@ static int ice_clean_rx_irq(struct ice_rx_ring
> >> *rx_ring, int budget)
> >> unsigned int size;
> >> u16 stat_err_bits;
> >> u16 vlan_tci;
> >> + bool rxe;
> >>
> >> /* get the Rx desc from Rx ring based on 'next_to_clean' */
> >> rx_desc = ICE_RX_DESC(rx_ring, ntc);
> >>
> >> - /* status_error_len will always be zero for unused descriptors
> >> - * because it's cleared in cleanup, and overlaps with hdr_addr
> >> - * which is always zero because packet split isn't used, if the
> >> - * hardware wrote DD then it will be non-zero
> >> + /*
> >> + * The DD bit will always be zero for unused descriptors
> >> + * because it's cleared in cleanup or when setting the DMA
> >> + * address of the header buffer, which never uses the DD bit.
> >> + * If the hardware wrote the descriptor, it will be non-zero.
> >> */
> >
> > The update to this comment feels like it could be a separate patch.
> > (I know, I often say something like that...)
>
> But this update is tied closely to the header split itself. Before this
> patch, this update would make no sense as there are no header buffers.
> After this patch, this comment will be outdated already without this
> update :D
Thanks for the clarification, I was expecting you would say something like
that.
I think everything I mentioned in my previous email was
covered by your response. So feel free to add:
Reviewed-by: Simon Horman <[email protected]>
