Page MenuHomeFreeBSD

e1000: Clean up and bug fix em_txrx
Changes PlannedPublic

Authored by kbowling on May 1 2021, 11:27 PM.

Details

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
Unit Tests Skipped

Event Timeline

kbowling created this revision.

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.

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 edited the summary of this revision. (Show Details)

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

sys/dev/e1000/em_txrx.c
186
356

Why is this change needed?

643

Why fetch it from each descriptor?

667
731
794

The indentation was right before.

sys/dev/e1000/if_em.c
909
kbowling added inline comments.
sys/dev/e1000/em_txrx.c
356

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.

643

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.