On Mon, Jul 07, 2025 at 03:58:37PM -0700, Jacob Keller wrote:
>
>
> On 7/4/2025 9:18 AM, Michal Kubiak wrote:
> > @@ -1075,16 +780,17 @@ void ice_clean_ctrl_rx_irq(struct ice_rx_ring
> > *rx_ring)
> > static int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
[...]
> > @@ -1144,27 +841,35 @@ static int ice_clean_rx_irq(struct ice_rx_ring
> > *rx_ring, int budget)
> > if (ice_is_non_eop(rx_ring, rx_desc))
> > continue;
> >
> > - ice_get_pgcnts(rx_ring);
> > xdp_verdict = ice_run_xdp(rx_ring, xdp, xdp_prog, xdp_ring,
> > rx_desc);
> > if (xdp_verdict == ICE_XDP_PASS)
> > goto construct_skb;
> > - total_rx_bytes += xdp_get_buff_len(xdp);
> > - total_rx_pkts++;
> >
> > - ice_put_rx_mbuf(rx_ring, xdp, &xdp_xmit, ntc, xdp_verdict);
> > + if (xdp_verdict & (ICE_XDP_TX | ICE_XDP_REDIR))
> > + xdp_xmit |= xdp_verdict;
> > + total_rx_bytes += xdp_get_buff_len(&xdp->base);
> > + total_rx_pkts++;
> >
> > + xdp->data = NULL;
> > + rx_ring->first_desc = ntc;
> > + rx_ring->nr_frags = 0;
> > continue;
> > construct_skb:
> > - skb = ice_build_skb(rx_ring, xdp);
> > + skb = xdp_build_skb_from_buff(&xdp->base);
> > +
> > /* exit if we failed to retrieve a buffer */
> > if (!skb) {
> > rx_ring->ring_stats->rx_stats.alloc_page_failed++;
>
> This is not your fault, but we've been incorrectly incrementing
> alloc_page_failed here instead of alloc_buf_failed.
>
Sure. It's a good idea to fix it while we're rewriting the Rx path.
Will be addressed in v2.
> > xdp_verdict = ICE_XDP_CONSUMED;
>
> xdp_verdict is no longer used, so I don't think we need to modify it
> further here. It was previously being used as part of the call to
> ice_put_rx_mbuf.
>
You're right. I'll remove it in v2.
> > + xdp->data = NULL;
> > + rx_ring->first_desc = ntc;
> > + rx_ring->nr_frags = 0;
> > + break;
> > }
> > - ice_put_rx_mbuf(rx_ring, xdp, &xdp_xmit, ntc, xdp_verdict);
> >
> > - if (!skb)
> > - break;
> > + xdp->data = NULL;
> > + rx_ring->first_desc = ntc;
> > + rx_ring->nr_frags = 0;
> >
>
> The failure case for !skb does the same as this, so would it make sense
> to move this block up to before !skb and just check the skb pointer
> afterwards?
>
Yeah. Together with Olek we re-organized the logic here, so in v2 it
should be simplified.
> > stat_err_bits = BIT(ICE_RX_FLEX_DESC_STATUS0_RXE_S);
> > if (unlikely(ice_test_staterr(rx_desc->wb.status_error0
Thanks,
Michal
