[xdp-hints] Re: [PATCH bpf-next v12 5/5] igc: Add launch time support to XDP ZC

["Bouska, Zdenek" <zdenek.bouska@siemens.com>]

> -----Original Message-----
> From: Song, Yoong Siang <yoong.siang.song@intel.com>
> Sent: Friday, March 7, 2025 3:21 PM
> 
> On Friday, March 7, 2025 9:25 PM, Bouska, Zdenek
> <zdenek.bouska@siemens.com> wrote:
> 
> [...]
> 
> >> @@ -2996,7 +3035,13 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
> >>  	ntu = ring->next_to_use;
> >>  	budget = igc_desc_unused(ring);
> >>
> >> -	while (xsk_tx_peek_desc(pool, &xdp_desc) && budget--) {
> >> +	/* Packets with launch time require one data descriptor and one context
> >> +	 * descriptor. When the launch time falls into the next Qbv cycle, we
> >> +	 * may need to insert an empty packet, which requires two more
> >> +	 * descriptors. Therefore, to be safe, we always ensure we have at least
> >> +	 * 4 descriptors available.
> >> +	 */
> >> +	while (xsk_tx_peek_desc(pool, &xdp_desc) && budget >= 4) {
> >
> >I think that here is a bug: some frames could be missed if budget < 4.
> >I was able to reproduce it by sending 100000x 60 B frames with minimal IPG
> >(672 ns between starts of frames) on 1Gbit/s. Always 1026 frames were not sent
> >and were missing a AF_XDP competition. Interesting was that then even when I
> >sent more
> >frames for hours it still was 1026 frames not sent and missing competition.
> >
> >Bug seems to be fixed when I change this line to:
> >
> >	while (budget >= 4 && xsk_tx_peek_desc(pool, &xdp_desc)) {
> >
> >Do you think this is a good fix?
> >
> >I think this bug is also in original code base, but I was only able to reproduce
> >it with launch time.
> >
> 
> Thank you for pointing out this issue and for providing a detailed
> explanation of your findings. I personally agree with your proposed fix
> that make sure there is enough budget in the driver, before go peek the xsk
> descriptor. Do you plan to submit bug fix patch to iwl-net?
> 

Yes, I plan to submit bug fix patch.

Best regards,
Zdenek Bouska

--
Siemens, s.r.o
Foundational Technologies