Page MenuHomeFreeBSD

Fix iflib zero-length fragment handling
ClosedPublic

Authored by pkelsey on Mar 3 2020, 4:03 PM.

Details

Summary

There is some explicit handling for zero-length fragments in iflib. This fixes the existing support and extends it to include well-defined behavior for packets that consist entirely of zero-length fragments.

  • Don't unload the dmamap for zero-length fragments so that the cluster-reuse logic in _iflib_fl_refill() works as intended.
  • Handle all zero-length fragments in the assemble_segments() path so that the cluster-reuse logic there does not have to be replicated in the small-single-fragment-packet path of iflib_rxd_pkt_get().
  • Handle the case of a packet consisting entirely of zero-length fragments, which results in a NULL mbuf pointer. This allows drivers (such as the vmx driver, via a following patch) to take advantage of this in the case where a descriptor for a received packet is marked with an error indicator. The advantage is that refill of the descriptor(s) for that received-with-error packet are handled via the existing iflib machinery without having to duplicate parts of that machinery in the driver for the sake of that error case.
Test Plan

This has been tested on a 12.1 system (with r356932 applied) with em and vmx devices.

This builds for 13-current, but testing on 13-current is waiting for the package system to get fixed.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

LGTM.
I assume that a change to vmxnet3 that takes advantage of this improvement is coming.

This revision is now accepted and ready to land.Mar 3 2020, 5:31 PM
sys/net/iflib.c
2678–2679 ↗(On Diff #69126)

This check doesn't actually have to be added here, as there isn't a zero-fragment length case in this path.

In D23945#526265, @avg wrote:

LGTM.
I assume that a change to vmxnet3 that takes advantage of this improvement is coming.

Yes, that is part of https://reviews.freebsd.org/D23949.

erj added inline comments.
sys/net/iflib.c
2678–2679 ↗(On Diff #69126)

Then why keep it here? Does it make a static analysis tool happy?

sys/net/iflib.c
2678–2679 ↗(On Diff #69126)

I'm going to remove it pre-commit - it's something I noticed after posting the review.

In the patch as it was developed based on 12.1, the check for a NULL mbuf came after the if/else, before the mbuf pkthdr and flags are set. In updating the patch to -current, in response to the change to rxd_frag_to_sd() since 12.1 (it now returns an mbuf), I pulled the NULL mbuf check up into each branch of the if/else because I thought it was clearer. I just didn't follow the thought all the way through at that point to realize this particular check was truly unnecessary.

This revision was automatically updated to reflect the committed changes.