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 33317
Build 30634: 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
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.

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

• 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/
662 lines
50 lines
modules/
usb/
ure/
2 lines

Diff 76524

View Options

sys/dev/usb/net/if_ure.c

Loading...
View Options

sys/dev/usb/net/if_urereg.h

Show First 20 Lines • Show All 385 Lines • ▼ Show 20 Linesstruct ure_intrpkt {
uint8_t ure_gep_msr; uint8_t ure_gep_msr;
uint8_t ure_waksr; uint8_t ure_waksr;
uint8_t ure_txok_cnt; uint8_t ure_txok_cnt;
uint8_t ure_rxlost_cnt; uint8_t ure_rxlost_cnt;
uint8_t ure_crcerr_cnt; uint8_t ure_crcerr_cnt;
uint8_t ure_col_cnt; uint8_t ure_col_cnt;
} __packed; } __packed;
#define URE_RXPKT_ALIGN 8
struct ure_rxpkt { struct ure_rxpkt {
uint32_t ure_pktlen; uint32_t ure_pktlen;
#define URE_RXPKT_MAX_SEG 2048
#define URE_RXPKT_LEN_MASK 0x7fff #define URE_RXPKT_LEN_MASK 0x7fff
uint32_t ure_rsvd0; uint32_t ure_csum;
uint32_t ure_rsvd1; /* Linux driver has this in ure_misc, but my device has it in ure_csum */
#define URE_RXPKT_VLAN_MASK 0xffff
#define URE_RXPKT_RX_VLAN_TAG (1 << 16)
#define URE_RXPKT_IPV4_CS (1 << 19)
#define URE_RXPKT_IPV6_CS (1 << 20)
#define URE_RXPKT_TCP_CS (1 << 22)
#define URE_RXPKT_UDP_CS (1 << 23)
uint32_t ure_misc;
#define URE_RXPKT_TCP_F (1 << 21)
#define URE_RXPKT_UDP_F (1 << 22)
#define URE_RXPKT_IP_F (1 << 23)
uint32_t ure_rsvd2; uint32_t ure_rsvd2;
uint32_t ure_rsvd3; uint32_t ure_rsvd3;
uint32_t ure_rsvd4; uint32_t ure_rsvd4;
} __packed; } __packed;
#define URE_TXPKT_ALIGN 4
struct ure_txpkt { struct ure_txpkt {
uint32_t ure_pktlen; uint32_t ure_pktlen;
#define URE_TKPKT_TX_FS (1 << 31) #define URE_TKPKT_TX_FS (1 << 31)
#define URE_TKPKT_TX_LS (1 << 30) #define URE_TKPKT_TX_LS (1 << 30)
#define URE_TXPKT_LEN_MASK 0xffff #define URE_TXPKT_LEN_MASK 0xffff
uint32_t ure_rsvd0; uint32_t ure_csum;
#define URE_L4_OFFSET_MAX 0x7ff
#define URE_L4_OFFSET_SHIFT 17
#define URE_TXPKT_VLAN_MASK 0xffff
#define URE_TXPKT_VLAN (1 << 16)
#define URE_TXPKT_IPV6_CS (1 << 28)
#define URE_TXPKT_IPV4_CS (1 << 29)
#define URE_TXPKT_TCP_CS (1 << 30)
#define URE_TXPKT_UDP_CS (1 << 31)
/* Lower 12 bits are the VLAN tag */
} __packed; } __packed;
enum { #define URE_N_TRANSFER 4
URE_BULK_DT_WR, #define URE_TRANSFER_SIZE 16384
URE_BULK_DT_RD,
URE_N_TRANSFER,
};
struct ure_softc { struct ure_softc {
struct usb_ether sc_ue; struct usb_ether sc_ue;
struct mtx sc_mtx; struct mtx sc_mtx;
struct usb_xfer *sc_xfer[URE_N_TRANSFER]; struct usb_xfer *sc_rx_xfer[URE_N_TRANSFER];
struct usb_xfer *sc_tx_xfer[URE_N_TRANSFER];
int sc_rxstarted;
struct usb_xfer *sc_txavail[URE_N_TRANSFER];
/*
hselaskyUnsubmitted
Done
Inline Actions

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.

hselasky: The USB stack will also keep track of pending USB transfers, so it would also be fine to just…
jmgAuthorUnsubmitted
Done
Inline Actions

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.

jmg: The reason I don't start all the TX transfers is that if there is only have one packet queued…
* Position of next available xfer for TX. If
* sc_txpos == URE_N_TRANSFER, no tx xfer's are available.
* Pop xfer: sc->sc_txavail[sc->sc_txpos++]
* Push xfer: sc->sc_txavail[(--(sc->sc_txpos))] = xfer
*/
int sc_txpos;
int sc_phyno; int sc_phyno;
u_int sc_flags; u_int sc_flags;
#define URE_FLAG_LINK 0x0001 #define URE_FLAG_LINK 0x0001
#define URE_FLAG_8152 0x1000 /* RTL8152 */ #define URE_FLAG_8152 0x1000 /* RTL8152 */
u_int sc_chip; u_int sc_chip;
u_int sc_ver;
#define URE_CHIP_VER_4C00 0x01 #define URE_CHIP_VER_4C00 0x01
#define URE_CHIP_VER_4C10 0x02 #define URE_CHIP_VER_4C10 0x02
#define URE_CHIP_VER_5C00 0x04 #define URE_CHIP_VER_5C00 0x04
#define URE_CHIP_VER_5C10 0x08 #define URE_CHIP_VER_5C10 0x08
#define URE_CHIP_VER_5C20 0x10 #define URE_CHIP_VER_5C20 0x10
#define URE_CHIP_VER_5C30 0x20 #define URE_CHIP_VER_5C30 0x20
}; };
#define URE_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx) #define URE_LOCK(_sc) mtx_lock(&(_sc)->sc_mtx)
#define URE_UNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx) #define URE_UNLOCK(_sc) mtx_unlock(&(_sc)->sc_mtx)
#define URE_LOCK_ASSERT(_sc, t) mtx_assert(&(_sc)->sc_mtx, t) #define URE_LOCK_ASSERT(_sc, t) mtx_assert(&(_sc)->sc_mtx, t)
View Options

sys/modules/usb/ure/Makefile

# $FreeBSD$ # $FreeBSD$
.PATH: ${SRCTOP}/sys/dev/usb/net .PATH: ${SRCTOP}/sys/dev/usb/net
KMOD= if_ure KMOD= if_ure
SRCS+= if_ure.c usbdevs.h SRCS+= if_ure.c usbdevs.h
SRCS+= bus_if.h device_if.h miibus_if.h usb_if.h \ SRCS+= bus_if.h device_if.h miibus_if.h usb_if.h \
opt_bus.h opt_inet.h opt_usb.h opt_bus.h opt_inet.h opt_inet6.h opt_usb.h
.include <bsd.kmod.mk> .include <bsd.kmod.mk>