Details

Reviewers

markj
marius
grehan
gallatin
olivier

Group Reviewers

Intel Networking
Restricted Owners Package	(Owns No Changed Paths)

Commits

rGd483953e9975: e1000: lem(4)/em(4) ifcaps, TSO and hwcsum fixes
rGbd07d5c45199: e1000: lem(4)/em(4) ifcaps, TSO and hwcsum fixes
rGc45dba3890e4: e1000: Some fixes for em(4) TSO setup
rG918c25677d88: e1000: lem(4)/em(4) ifcaps, TSO and hwcsum fixes
rG95f7b36e8fac: e1000: lem(4)/em(4) ifcaps, TSO and hwcsum fixes
rG201c4b7c29da: e1000: Some fixes for em(4) TSO setup
rG670a17e9f978: e1000: Format em_txrx
rGdac6aa05f8da: e1000: Add SPDX tag to em_txrx
rG0b6e5abfb855: e1000: Add SPDX tag to em_txrx
rG7d387749dcdd: e1000: Format em_txrx
rG5253d74e4650: e1000: Format em_txrx
rG034f38cdf864: e1000: Add SPDX tag to em_txrx

Summary

This is a general cleanup of em_txrx.

Correct the E1000_TXD_CMD_IP bit (8254x SDM4.0 page 45, and PCIe GbE SDM2.5 page 63)
Move the documentation about csum offload contexts and the DONT_FORCE_CTX define inline so it explains what is going on closer to the subject
Move HWCSUM disables out of txrx to setup (< e1000_82543 already disabled), new if statement handles 82547 limitations
Correct lem-class VLAN tagging (need E1000_RXD_SPC_VLAN_MASK)
Add CSUM_IPV6 and TSO6 for em-class. TSO is disabled by default but this should now work.

Test Plan

TSO tested on I219.

Interesting observation: performance is lower on this particular SPT-era NIC. This is due to the Intel-recommended workaround for " i218-i219 Specification Update 1.5.4.5" -- this lowers the throughput by 200mbit/s. If I disable the code (E1000_TARC0_CB_MULTIQ_2_REQ), I am able to get full performance at well under half the CPU usage. I'll see if there is a better way to do this but I may not be able to change it without help from Intel. It's just an older generation part so the newer I219s can do TSO after a careful reading of the spec update, I'll consider that after more testing.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kbowling requested review of this revision.May 1 2021, 11:27 PM

kbowling created this revision.

kbowling mentioned this in D30073: e1000: Clean up igb_txrx.May 1 2021, 11:34 PM

kbowling mentioned this in D30074: ixgbe: Clean up ix_txrx.May 1 2021, 11:43 PM

kbowling edited the test plan for this revision. (Show Details)May 2 2021, 4:44 AM

kbowling edited the test plan for this revision. (Show Details)May 2 2021, 4:50 AM

kbowling edited the test plan for this revision. (Show Details)May 2 2021, 8:57 AM

Also, the same comments regarding rx checksum __predict() as in the other 2 intel driver reviews

sys/dev/e1000/em_txrx.c
618	I would have gone for the ifp too, but the "iflib way" is to avoid accessing this directly when possible. Can you get what you need from scctx->isc_capenable ?

Could you please upload patches with context? Otherwise it's somewhat challenging to read. For raw diffs, pass -U99999. If you use git, arc can upload diffs with context automatically.

sys/dev/e1000/em_txrx.c
758	I'd expect this to be rare too?
814	`case` should be on the same indentation level as `switch`.

markj added inline comments.May 3 2021, 2:50 PM

sys/dev/e1000/em_txrx.c
662	I believe iflib ensures that this field is zeroed. Otherwise, note that we assume `ri->iri_flags` is zeroed when ORing `M_VLANTAG` below.

kbowling planned changes to this revision.May 31 2021, 4:27 PM

kbowling mentioned this in D30463: iflib:correct vlan register/unregister.

Kevin Bowling <kbowling@FreeBSD.org> mentioned this in rG51e46835e10a: ixgbe: Clean up ix_txrx.Jul 18 2021, 6:26 AM

Kevin Bowling <kbowling@FreeBSD.org> mentioned this in rGfe6803c18e81: ixgbe: Clean up ix_txrx.Jul 30 2021, 12:31 AM

Kevin Bowling <kbowling@FreeBSD.org> mentioned this in rGfd350dd7ff94: ixgbe: Clean up ix_txrx.Jul 30 2021, 12:42 AM

rebase on main, csum fix/cleanups broken out to a different review. address some of the feedback

rebase

kbowling marked 3 inline comments as done.Aug 9 2021, 10:09 PM

markj added inline comments.Aug 11 2021, 6:21 PM

sys/dev/e1000/em_txrx.c
185–200
355	Why is this change needed?
642	Why fetch it from each descriptor?
664
726
780	The indentation was right before.
sys/dev/e1000/if_em.c
909

kbowling planned changes to this revision.Aug 11 2021, 9:17 PM

kbowling added inline comments.

sys/dev/e1000/em_txrx.c
355	It's insufficient as is, we need to properly handle legacy, context and data descriptors for lem/em and I haven't wrapped my brain around it yet to make the encap code look and work nice. The primary issue is we are supposed to put the vlan tag and mark the VLE bit in the last data descriptor following a context descriptor. The lem versus em datasheet is confusing, it says put the tag and VLE in the first data descriptor if using the context/data descriptors for hwcsum non-TSE (aka non-TSO), and last if TSE (TSO) so I'm still trying to understand it.
642	I had the same concern, trying to unify the igb and ixgbe txrx so I will prep an update for those to move it outside the loop.