Page MenuHomeFreeBSD
Differential D25809

major update to if_ure
ClosedPublic
Actions

Authored by jmg on Jul 25 2020, 10:52 PM.
Tags
None
Referenced Files
F114941643: D25809.id76633.diff
Fri, Apr 18, 8:08 PM
Unknown Object (File)
Tue, Apr 8, 2:34 PM
Unknown Object (File)
Sun, Mar 23, 4:44 PM
Unknown Object (File)
Mar 6 2025, 9:03 AM
Unknown Object (File)
Mar 5 2025, 2:54 PM
Unknown Object (File)
Feb 7 2025, 10:02 PM
Unknown Object (File)
Jan 31 2025, 8:49 AM
Unknown Object (File)
Jan 28 2025, 6:18 AM
View All Files
Subscribers
ganbold
imp

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 33376
Build 30680: 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
218
>= 4 like in previous statement?
637

style: return (m);

654

style: return (m);

689

Are you sure the right is not "len > URE_RXPKT_MAX_SEG" ?

713

avoid negative length: if (len > ETHER_CRC_LEN)

836

wrap long line.

880

Do we need to mangle with the OACTIVE flag ? Can we just always have it cleared?

1079

Make this a single line.

1093

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
445

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.

jmg updated this revision to Diff 74986.Jul 27 2020, 1:18 AM

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

Harbormaster completed remote builds in B32577: Diff 74986.Jul 27 2020, 1:18 AM
jmg updated this revision to Diff 74992.Jul 27 2020, 1:46 AM
jmg marked 10 inline comments as done.

update to address hselasky's comments.

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

ok, these should address all of your comments.

sys/dev/usb/net/if_ure.c
218

I converted the other one to ==. I had tried to see if 8 would work, but got failure when allocating memory.

689

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.

713

Length cannot be negative here due to:
len = le32toh(pkt.ure_pktlen) & URE_RXPKT_LEN_MASK

880

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...

1093

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
445

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.

• hselasky accepted this revision.Jul 27 2020, 11:53 AM

Looks good to me!

sys/dev/usb/net/if_ure.c
645

KASSERT(tlen != 0);

This revision is now accepted and ready to land.Jul 27 2020, 11:53 AM
jmg marked an inline comment as done.Jul 27 2020, 6:28 PM

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

sys/dev/usb/net/if_ure.c
645

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.

ganbold added a subscriber: ganbold.Aug 19 2020, 8:35 AM

Works for me.

jmg updated this revision to Diff 76524.Sep 1 2020, 11:50 PM
jmg marked an inline comment as done.

move some debug prints to before the error...

Add a sysctl for dumping the chip version.

This revision now requires review to proceed.Sep 1 2020, 11:50 PM
Harbormaster completed remote builds in B33317: Diff 76524.Sep 1 2020, 11:50 PM
• hselasky accepted this revision.Sep 2 2020, 7:25 AM
This revision is now accepted and ready to land.Sep 2 2020, 7:25 AM
jmg updated this revision to Diff 76633.Sep 4 2020, 1:37 AM

don't overwrite the RCR register, keep some bits set. This gets
MTU up to 4096 - mumble working w/o my devices crashing...

This revision now requires review to proceed.Sep 4 2020, 1:37 AM
Harbormaster completed remote builds in B33376: Diff 76633.Sep 4 2020, 1:37 AM
jmg updated this revision to Diff 76779.Sep 8 2020, 5:50 PM

minor update to include some new register definitions.. unused for now..

Harbormaster completed remote builds in B33445: Diff 76779.Sep 8 2020, 5:50 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 12 2020, 12:33 AM
Closed by commit rS365648: A major update to the ure driver. (authored by jmg). · Explain Why
This revision was automatically updated to reflect the committed changes.
jmg added a commit: rS365648: A major update to the ure driver..

Revision Contents
Changeset List

PathSize
sys/
dev/
usb/
net/
677 lines
49 lines
modules/
usb/
ure/
2 lines

Diff 76633

View Options

sys/dev/usb/net/if_ure.c

Show First 20 Lines • Show All 90 Lines • ▼ Show 20 Lines
} }
bool bool
kinst_excluded(const char *name) kinst_excluded(const char *name)
{ {
if (kinst_md_excluded(name)) if (kinst_md_excluded(name))
return (true); return (true);
/* /*
markjUnsubmitted
Done
Inline Actions

Please add a comment explaining why: cpu_switch() modifies the value of curthread, but kinst_invop() assumes that this won't happen.

markj: Please add a comment explaining why: cpu_switch() modifies the value of `curthread`, but…
* cpu_switch() can cause a crash if it modifies the value of curthread
* while in probe context.
*/
if (strcmp(name, "cpu_switch") == 0)
return (true);
/*
* Anything beginning with "dtrace_" may be called from probe context * Anything beginning with "dtrace_" may be called from probe context
* unless it explicitly indicates that it won't be called from probe * unless it explicitly indicates that it won't be called from probe
* context by using the prefix "dtrace_safe_". * context by using the prefix "dtrace_safe_".
*/ */
if (strncmp(name, "dtrace_", strlen("dtrace_")) == 0 && if (strncmp(name, "dtrace_", strlen("dtrace_")) == 0 &&
strncmp(name, "dtrace_safe_", strlen("dtrace_safe_")) != 0) strncmp(name, "dtrace_safe_", strlen("dtrace_safe_")) != 0)
return (true); return (true);
▲ Show 20 Lines • Show All 213 Lines • Show Last 20 Lines
View Options

sys/dev/usb/net/if_urereg.h

Show First 20 Lines • Show All 90 Lines • ▼ Show 20 Lines
} }
bool bool
kinst_excluded(const char *name) kinst_excluded(const char *name)
{ {
if (kinst_md_excluded(name)) if (kinst_md_excluded(name))
return (true); return (true);
/* /*
markjUnsubmitted
Done
Inline Actions

Please add a comment explaining why: cpu_switch() modifies the value of curthread, but kinst_invop() assumes that this won't happen.

markj: Please add a comment explaining why: cpu_switch() modifies the value of `curthread`, but…
* cpu_switch() can cause a crash if it modifies the value of curthread
* while in probe context.
*/
if (strcmp(name, "cpu_switch") == 0)
return (true);
/*
* Anything beginning with "dtrace_" may be called from probe context * Anything beginning with "dtrace_" may be called from probe context
* unless it explicitly indicates that it won't be called from probe * unless it explicitly indicates that it won't be called from probe
* context by using the prefix "dtrace_safe_". * context by using the prefix "dtrace_safe_".
*/ */
if (strncmp(name, "dtrace_", strlen("dtrace_")) == 0 && if (strncmp(name, "dtrace_", strlen("dtrace_")) == 0 &&
strncmp(name, "dtrace_safe_", strlen("dtrace_safe_")) != 0) strncmp(name, "dtrace_safe_", strlen("dtrace_safe_")) != 0)
return (true); return (true);
▲ Show 20 Lines • Show All 213 Lines • Show Last 20 Lines
View Options

sys/modules/usb/ure/Makefile

Show First 20 Lines • Show All 90 Lines • ▼ Show 20 Lines
} }
bool bool
kinst_excluded(const char *name) kinst_excluded(const char *name)
{ {
if (kinst_md_excluded(name)) if (kinst_md_excluded(name))
return (true); return (true);
/* /*
markjUnsubmitted
Done
Inline Actions

Please add a comment explaining why: cpu_switch() modifies the value of curthread, but kinst_invop() assumes that this won't happen.

markj: Please add a comment explaining why: cpu_switch() modifies the value of `curthread`, but…
* cpu_switch() can cause a crash if it modifies the value of curthread
* while in probe context.
*/
if (strcmp(name, "cpu_switch") == 0)
return (true);
/*
* Anything beginning with "dtrace_" may be called from probe context * Anything beginning with "dtrace_" may be called from probe context
* unless it explicitly indicates that it won't be called from probe * unless it explicitly indicates that it won't be called from probe
* context by using the prefix "dtrace_safe_". * context by using the prefix "dtrace_safe_".
*/ */
if (strncmp(name, "dtrace_", strlen("dtrace_")) == 0 && if (strncmp(name, "dtrace_", strlen("dtrace_")) == 0 &&
strncmp(name, "dtrace_safe_", strlen("dtrace_safe_")) != 0) strncmp(name, "dtrace_safe_", strlen("dtrace_safe_")) != 0)
return (true); return (true);
▲ Show 20 Lines • Show All 213 Lines • Show Last 20 Lines