Fix iflib zero-length fragment handling
ClosedPublic
Actions

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

Details

Reviewers

avg
erj
gallatin
shurd

Group Reviewers

iflib

Commits

rS358996: Fix iflib zero-length fragment handling

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

pkelsey created this revision.Mar 3 2020, 4:03 PM

Herald added 1 blocking reviewer(s): iflib. · View Herald TranscriptMar 3 2020, 4:03 PM

Herald added subscribers: melifaro, ae. · View Herald Transcript

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

pkelsey mentioned this in D23949: Fix if_vmx receive checksum offload bug and harden against the device skipping receive descriptors.Mar 3 2020, 5:17 PM

pkelsey mentioned this in D23950: Adjust if_vmx default receive ring size and receive ring buffers sizes to improve out-of-box performance.Mar 3 2020, 5:29 PM

gallatin accepted this revision.Mar 3 2020, 5:31 PM

This revision is now accepted and ready to land.Mar 3 2020, 5:31 PM

pkelsey added inline comments.Mar 3 2020, 6:06 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 accepted this revision.Mar 3 2020, 7:59 PM

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?

pkelsey added inline comments.Mar 4 2020, 4:40 AM

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.