Page MenuHomeFreeBSD

Enable em(4) multiqueue as a compile time option
ClosedPublic

Authored by sbruno on Mar 1 2015, 11:11 PM.
Tags
None
Referenced Files
F52239168: D1994.id4104.diff
Mon, Nov 28, 6:17 AM
Unknown Object (File)
Apr 26 2017, 3:47 PM
Unknown Object (File)
Apr 26 2017, 3:46 PM
Unknown Object (File)
Apr 18 2017, 8:18 AM
Unknown Object (File)
Apr 17 2017, 2:07 PM
Unknown Object (File)
Apr 11 2017, 8:14 AM
Unknown Object (File)
Apr 10 2017, 1:07 PM
Unknown Object (File)
Apr 9 2017, 1:22 AM

Details

Summary

em(4) has multiqueue support, toggle the right compile bits
to enable the support. Update man page with instructions and flush
out missing hw.em tuneables.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sbruno retitled this revision from to Enable em(4) multiqueue as a compile time option.Mar 1 2015, 11:11 PM
sbruno updated this object.
sbruno edited the test plan for this revision. (Show Details)
sbruno added reviewers: hiren, erj.
erj edited edge metadata.

Added spelling fixes inline, but it looks good.

Though, for MSI-X, can you check to see how many vectors your adapter supports? Mine says 3, but the datasheet says 5.

share/man/man4/em.4
54–56 ↗(On Diff #4056)

Note: Activating EM_MULTIQUEUE support will disable MSI-X features
by default. Until these issues are resolved, activating MSI-X will crash
your system with EM_MULTIQUEUE enabled.

211 ↗(On Diff #4056)

Disable or Enable Energy Efficient Ethernet. Default 1 (disabled).

sys/conf/NOTES
2988 ↗(On Diff #4056)

options EM_MULTIQUEUE # Activate multiqueue features/disable MSI-X

This revision is now accepted and ready to land.Mar 2 2015, 11:48 PM
hiren edited edge metadata.

You may want to mention in commitlog that this gets us 2 queues but the other queue is not being used right now. i.e. packets are not being received on it.

The code is not acceptable as is right now, Eric and I are discussing changes.

sbruno edited edge metadata.

Capture Eric's man page and comment typo fixes.

This revision now requires review to proceed.Mar 3 2015, 5:51 PM
In D1994#10, @jfvogel wrote:

The code is not acceptable as is right now, Eric and I are discussing changes.

  1. The driver is missing the RSS hash programming bits to distribute across the 2 receive queues. Should be trivial.
  2. The driver is missing the TX distribution across the 2 TX queues. This is probably going to be a rip off of logic from igb(4)
  3. The driver in its multiqueue form doesn't DTRT with regards to MSI-X. This might be just a simple matter of handling the number MSI-X vectors that the h/w reports as opposed to a hardwired value.

Is this what you are discussing? Because this is what I am working on right now. Want to collaborate a bit?

erj requested changes to this revision.Mar 3 2015, 7:28 PM
erj edited edge metadata.

So, I'm going to remove my approval until the Tx/Rx changes get added, but for point #3, there's a potential can of worms there.

By default, the EEPROM only exposes three MSIX vectors, but there's a diff that I've put up here that will re-write a specific word in the EEPROM to make it expose five vectors if you configure the driver to use two queues. It seems the hardware is designed around using an interrupt for each tx/rx queue.

This change works on 82574's that I've used here, but this operation/mode would be completely unsupported by Intel. It may end up being a better idea to rewrite the interrupt code to use three vectors and assign one interrupt to each tx/rx pair if EM_MULTIQUEUE is defined or two queues are used, but changing the EEPROM to expose the new vectors would avoid that.

If the five vector change ends up working for you/actually making a performance difference, then there should be a gigantic disclaimer when EM_MULTIQUEUE is defined when the driver loads/is compiled saying it's an unsupported mode of functionality for the driver.

This revision now requires changes to proceed.Mar 3 2015, 7:28 PM

Even just enabling RSS calculation would be beneficial - it doesn't have to be multiple queues.

sbruno edited edge metadata.

Merge Eric's MSI-X Vector changes for testing and arguments.

sbruno edited edge metadata.

Well ... when I thought I was merging Eric's feature I was actually
replacing the entirety of the review. Don't do that and really do
a merge.

Add numerous bits stolen from DragonFly BSD emx(4) driver to begin RSS
support and multiqueue enablement. TX queues seem to be moving now and I get
small periodic stalls in ethernet traffic. RX queues don't seem to be working
at all.

Remove references to disabling MSI-X when EM_MULTIQUEUE is enabled.

The complete opposite is now true as we *need* MSI-X to give us more
vectors to handle this hackery.

Underp the RXCSUM bits so that recieve queues are really processed.

Fun times. This diff is indeed working, somewhat. It does seem to have some transmit issues, that seem vaguely familiar from my days working on igb(4):

em0: Watchdog timeout -- resetting
em0: Queue(0) tdh = 2846, hw tdt = 2718
em0: TX(0) desc avail = 31,Next TX to Clean = 2749
em0: Link is Down
em0: Link is up 1000 Mbps Full Duplex
em0: Watchdog timeout -- resetting
em0: Queue(0) tdh = 993, hw tdt = 3937
em0: TX(0) desc avail = 31,Next TX to Clean = 3968
em0: Link is Down
em0: Link is up 1000 Mbps Full Duplex

Fixup compile for non-multiqueue case.

Add missing KASSERT, flush out taskqueue entries to display different
queue ids.

  • split num_queues into seperate tx/rx queue configurations.
    • assing tx or rx queue variables to control looping across most of the driver.
    • modify netmap code for em(4) to deal with tx vs rx queue configuration.

This is a capture all the things commit before I create a project branch
in svn for this. I think that the review process is inappropriate
for this feature developement at this time.

I'm closing this out as I'm off doing development in a svn proj branch now.

Updating and reopening. This matches production code that is currently
being load tested.

Note, that in production, we have ALWAYS disabled TSO4 for our em(4) code
and that my testing seems to show a TSO4 induced hang/deadlock with the
82574L that is even worse in MQ mode.

sbruno added a subscriber: gnn.

Pending Intel driver maintainer review/approval/discussion. This modification is being requested/tested/supported by Limelight Networks.

gnn added a reviewer: gnn.
sbruno edited edge metadata.

Update with 2 changes.

Add errata to handle TARC bits that need to be set for proper MQ ops

Add a soft interrupt to the msix link handler to avoid racy conditions
with non-tx/rx handling.

Resolve 2 review comments

sbruno added a subscriber: j-nitrology.com.
sbruno added inline comments.
share/man/man4/em.4
54–56 ↗(On Diff #5910)

Do you guys want this *verbatim* in the man page? Sounds kind of scary. :-)

wblock added inline comments.
share/man/man4/em.4
54–56 ↗(On Diff #5910)

Passive->active: s/will disable/disables/
Avoid "you" and "your": "the system when EM_MULTIQUEUE is enabled."

Agreed with sbruno. This kind of implies "not done". Maybe it should be in the BUGS section?

209 ↗(On Diff #5910)

Capitalizing "enable" implies that it is a value the user sets. Should be lower case. (And 1 to disable is unexpected, but that has probably been mentioned already.)

The second sentence should be on a new line:

Disable or enable Energy Efficient Ethernet.
Default 1 (disabled).

211 ↗(On Diff #5910)

As above, don't capitalize "disable", and start a new sentence on a new line.

213 ↗(On Diff #5910)

As above.

215 ↗(On Diff #5910)

New sentence should be on a new line.

248 ↗(On Diff #5910)

"h/w" would be better as just "hardware", and "we" is unnecessary. "Max 2" is a sentence fragment, probably best added to the previous sentence as an aside. Also, new sentences on new lines:

Number of hardware queues to be run on this adapter (maximum of 2).
Defaults to 1.

Address man page updates via wblock.

Address MSI-X <-> EM_MULTIQUEUE incompatibility via BUGS section
as per erj

Punched "done" buttons for man page updates.

White space alignment in the TARC defines made my brain itch.

Aren't all the comments saying MSI-X is disabled when EM_MULTIQUEUE is enabled are wrong now? I was under the impression that with the changes you've made, you should use MSI-X when using EM_MULTIQUEUE.

And, you should move any defines you've added to e1000_defines.h to if_em.h because e1000_defines.h is a shared code file. Unless someone here wants to patch the shared code to add those *cough*Jack*cough*.

jfv edited edge metadata.

Thanks for all the work you put into this Sean, go for it :)

In D1994#51596, @erj wrote:

Aren't all the comments saying MSI-X is disabled when EM_MULTIQUEUE is enabled are wrong now? I was under the impression that with the changes you've made, you should use MSI-X when using EM_MULTIQUEUE.

bah, you *have* to use MSI-X when using EM_MULTIQUEUE now. Its required as it needs 5x msi-x vectors. right?

And, you should move any defines you've added to e1000_defines.h to if_em.h because e1000_defines.h is a shared code file. Unless someone here wants to patch the shared code to add those *cough*Jack*cough*.

Hrm ... Not sure what to do with these two values. The appropriate place for them is in e1000_defines.h as they are valid values for the MRQC register. I'd like to not have them get overridden if a change happens upstream here, but I'll yield to your opinions in this matter if there is a strong desire to do one thing vs another.

erj edited edge metadata.

Alright, it's fine to leave the defines in e1000_defines.h. With the MSI-X comments being changed, this is ok with me.

This revision is now accepted and ready to land.Jun 3 2015, 4:41 PM

The shared code changes just require some legal stuff if they are taken back into our code base, if you leave it as is we can cope with that later, its not like the E1000 shared code has a lot of churn at this point.

sbruno edited edge metadata.

EM_MULTIQUEUE *requires* MSI-X on the 82574. Fix comment about this.

This revision now requires review to proceed.Jun 3 2015, 4:50 PM
hiren edited edge metadata.
This revision is now accepted and ready to land.Jun 3 2015, 4:52 PM
This revision was automatically updated to reflect the committed changes.