Page MenuHomeFreeBSD

iflib rollup patch.
AbandonedPublic

Authored by shurd on Sep 5 2017, 9:15 PM.

Details

Summary
Roll up iflib commits from github.  This pulls in most of the work done
by Matt Macy as well as other changes which he has accepted via pull
request to his github repo at https://github.com/mattmacy/networking/

This should bring -CURRENT and the github repo into close enough sync to
allow small feature branches rather than a large chain of interdependant
patches being developed out of tree.  The reset of the synchronization
should be able to be completed on github by splitting the remaining
changes that are not yet ready into short feature branches for later
review as smaller commits.

Here is a summary of changes included in this patch:

1)  More checks when INVARIANTS are enabled for eariler problem
    detection
2)  Group Task Queue cleanups
    - Fix use of duplicate shortdesc for gtaskqueue malloc type.
      Some interfaces such as memguard(9) use the short description to
      identify malloc types, so duplicates should be avoided.
3)  Allow gtaskqueues to use ithreads in addition to taskqueues
    - In some cases, this can improve performance
4)  Better logging when taskqgroup_attach*() fails to set interrupt
    affinity.
5)  Do not start gtaskqueues until they're needed
6)  Have mp_ring enqueue function enter the ABDICATED rather than BUSY
    state.  This moves the TX to the gtaskq and allows processing to
    continue faster as well as make TX batching more likely.
7)  Add an ift_txd_errata function to struct if_txrx.  This allows
    drivers to inspect/modify mbufs before transmission.
8)  Add a new IFLIB_NEED_ZERO_CSUM for drivers to indicate they need
    checksums zeroed for checksum offload to work.  This avoids modifying
    packet data in the TX path when possible.
9)  Use ithreads for iflib I/O instead of taskqueues
10) Clean up ioctl and support async ioctl functions
11) Prefetch two cachlines from each mbuf instead of one up to 128B.  We
    often need to parse packet header info beyond 64B.
12) Fix potential memory corruption due to fence post error in
    bit_nclear() usage.
13) Improved hang detection and handling
14) If the packet is smaller than MTU, disable the TSO flags.
    This avoids extra packet parsing when not needed.
15) Move TCP header parsing inside the IS_TSO?() test.
    This avoids extra packet parsing when not needed.
16) Pass chains of mbufs that are not consumed by lro to if_input()
    rather call if_input() for each mbuf.
17) Re-arrange packet header loads to get as much work as possible done
    before a cache stall.
18) Lock the context when calling IFDI_ATTACH_PRE()/IFDI_ATTACH_POST()/
    IFDI_DETACH();
19) Attempt to distribute RX/TX tasks across cores more sensibly,
    especially when RX and TX share an interrupt.  RX will attempt to
    take the first threads on a core, and TX will attempt to take
    successive threads.
20) Allow iflib_softirq_alloc_generic() to request affinity to the same
    cpus an interrupt has affinity with.  This allows TX queues to
    ensure they are serviced by the socket the device is on.
21) Add new iflib sysctls to net.iflib:
    - timer_int - interval at which to run per-queue timers in ticks
    - force_busdma
22) Add new per-device iflib sysctls to dev.X.Y.iflib
    - rx_budget allows tuning the batch size on the RX path
    - watchdog_events Count of watchdog events seen since load
23) Fix error where netmap_rxq_init() could get called before
    IFDI_INIT()
24) e1000: Fixed version of r323008: post-cold sleep instead of DELAY
    when waiting for firmware
    - After interrupts are enabled, convert all waits to sleeps
    - Eliminates e1000 software/firmware synchronization busy waits after
      startup
25) e1000: Remove special case for budget=1 in em_txrx.c
    - Premature optimization which may actually be incorrect with
      multi-segment packets
26) e1000: Split out TX interrupt rather than share an interrupt for
    RX and TX.
    - Allows better performance by keeping RX and TX paths separate
27) e1000: Separate igb from em code where suitable
    Much easier to understand separate functions and "if (is_igb) {" than
    previous tests like "if (reg_icr & (E1000_ICR_RXSEQ | E1000_ICR_LSC)) {"
Test Plan

Test with em, igb, and bnxt (iflib drivers) using netmap and forwarding

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11534
Build 11888: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Sep 6 2017, 5:52 AM

Some small cleanups, and fix universe builds

This revision now requires review to proceed.Sep 6 2017, 7:54 PM

Please also review the summary, as I plan to use it as the commit message after it's cleaned up and appropriate tags are addded.

cramerj_intel.com added inline comments.
sys/dev/e1000/em_txrx.c
527

Why? Is the compiler not doing what we would expect?

sys/net/iflib.c
605

Are these sysctls per-device? They probably should be, no? And there doesn't seem to be any code using these knobs. If that's WIP, no big deal, just curious.

sys/net/mp_ring.c
233

Is this just a coding style preference? I guess I would prefer the do-while since the code must be run at least once.

shurd marked an inline comment as done.Sep 7 2017, 5:29 PM
shurd added inline comments.
sys/dev/e1000/em_txrx.c
527

In the old driver, that was part of this commit:

https://svnweb.freebsd.org/base/head/sys/dev/e1000/if_em.c?revision=293331&view=markup

Apparently only the low 8 bits of the status_error word need to be zeroed.

sys/net/iflib.c
605

They are not per-device... I don't see a reason they couldn't be, but they're also fairly blunt tools that hopefully wouldn't need to be used in a production system.

force_busdma and iflib_timer_int are both used, but enable_msix is not. I'll remove enable_msix.

sys/net/mp_ring.c
233

The new code adds cpu_sinpwait() after failure.

shurd marked an inline comment as done.

Remve unised enable_msix iflib tunable

Just a note, the changes in bnxt aside from the softirq rid change are from -CURRENT, not part of this review.

This revision is now accepted and ready to land.Sep 12 2017, 9:13 PM

Fix CTX lock handing during detach

CTX lock was being destroyed before calling IFDI_DETACH. It should
remain and be locked when IFDI_DETACH() is called.

This revision now requires review to proceed.Sep 12 2017, 10:20 PM

Fix em pci resource freeing

Since em is using separate TX/RX interrupts now, we need to free both
TX and RX in em_free_pci_resources().

This revision is now accepted and ready to land.Sep 13 2017, 1:15 AM

Looks like this patch causing issues with bnxt driver unload.

Let me explain.

With Rev 323513: It is good, able to unload the bnxt driver
With Rev 323530: 'kldunload bnxt' hangs forever. no Crash but kldunload never completes.

When I instrument driver code:-

bnxt_disable_intr:1615 Entry ...
bnxt_stop:1143 Entry ...
bnxt_stop:1147 Exit ...
bnxt_detach:928 Entry ...
bnxt_detach:959 Exit ...
bnxt_queues_free:419 Entry ...
bnxt_queues_free:436 Exit ... <== In failure case, seeing only till this line during unload.
bnxt0: detached
pci16: <network, ethernet> at device 0.0 (no driver attached
<== Not seeing above Two lines in failure case where as seeing in working case

Please let me know if I'm missing anything here.

This revision now requires changes to proceed.Sep 13 2017, 2:33 PM