Details

Reviewers

• hselasky
kevlo

Commits

rS365648: A major update to the ure driver.

Summary

This update adds support for:
HW VLAN tagging
HW checksum offload for IPv4 and IPv6
tx and rx aggreegation (for full gige speeds)
multiple transactions

In my testing, I am able to get 900-950Mbps depending upon
TCP or UDP, which is a significant improvement over the previous
91Mbps (~8kint/sec*1500bytes/packet*1packet/int).

Test Plan

iperf3 for performance, and an attached testing script for the various
hw offload features.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 32577
Build 30034: arc lint + arc unit

Event Timeline

jmg created this revision.Jul 25 2020, 10:52 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 25 2020, 10:52 PM

jmg requested review of this revision.Jul 25 2020, 10:52 PM

Harbormaster completed remote builds in B32548: Diff 74939.Jul 25 2020, 10:52 PM

• hselasky added inline comments.Jul 26 2020, 7:42 AM

sys/dev/usb/net/if_ure.c
215	>= 4 like in previous statement?
634	style: return (m);
651	style: return (m);
686	Are you sure the right is not "len > URE_RXPKT_MAX_SEG" ?
710	avoid negative length: if (len > ETHER_CRC_LEN)
832	wrap long line.
873	Do we need to mangle with the OACTIVE flag ? Can we just always have it cleared?
1045	Make this a single line.
1060–1061	It is enough to set stall on the first TX transfer, else the USB stack will clear stall multiple times.
sys/dev/usb/net/if_urereg.h
446	The USB stack will also keep track of pending USB transfers, so it would also be fine to just start all USB transfers on ifp start() . I.E. executing of USB transfers on the same USB endpoint is always serialized.

make it relative to HEAD.. Post white space commit..

Harbormaster completed remote builds in B32577: Diff 74986.Jul 27 2020, 1:18 AM

update to address hselasky's comments.

Harbormaster completed remote builds in B32578: Diff 74992.Jul 27 2020, 1:46 AM

ok, these should address all of your comments.

sys/dev/usb/net/if_ure.c
215	I converted the other one to ==. I had tried to see if 8 would work, but got failure when allocating memory.
686	Yes, I am sure. _MAX_SEG is 2048, and when a packet that is larger (or equal) to 2048, it will return 2048 (I guess it could be ==, but this is safer). So if the length is exactly _MAX_SEG, there is no way to tell if it's just a 2048 byte packet, OR it's part of a multi segment packet. If I had the programmer's manual for the chip, it'd be easy to fix this, but I don't have access to it.
710	Length cannot be negative here due to: len = le32toh(pkt.ure_pktlen) & URE_RXPKT_LEN_MASK
873	Do we need to mangle with the OACTIVE flag ? Can we just always have it cleared? This will prevent excessive if_start calls under high packet loads, which saves unnecessary lock/unlock cycles that happens by calling it... if_handoff doesn't bother calling if_start (aka ue_start here, which does a lock/unlock cycle) if the _OACTIVE flag is set...
1060–1061	Thanks, wasn't sure what the correct process was, just did everything 4x for each xfer (except _start, but I explain why elsewhere).
sys/dev/usb/net/if_urereg.h
446	The reason I don't start all the TX transfers is that if there is only have one packet queued up, the first tx xfer will get it, and then the remaining three will get call backs, and immediately go back to sleep. That is a lot of extra calls to _write_callback that are unnecessary. It allows us to only ever have as many TX xfers in flight as needed to send the currently pending packets.

Looks good to me!

sys/dev/usb/net/if_ure.c
642	KASSERT(tlen != 0);

This revision is now accepted and ready to land.Jul 27 2020, 11:53 AM

I'll wait to get a few more reports of tests before committing..

sys/dev/usb/net/if_ure.c
642	not needed M_TRAILINGSPACE(mb) will always be non-zero, otherwise an mbuf returned by m_getm2 would have no storage space (_TRAILINGSPACE was used instead of M_SIZE because of the m_adj earlier). len is always != 0 due to loop condition.