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.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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 | Note: Activating EM_MULTIQUEUE support will disable MSI-X features | |
209 | Disable or Enable Energy Efficient Ethernet. Default 1 (disabled). | |
sys/conf/NOTES | ||
2984 | options EM_MULTIQUEUE # Activate multiqueue features/disable MSI-X |
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 driver is missing the RSS hash programming bits to distribute across the 2 receive queues. Should be trivial.
- 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)
- 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?
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.
Even just enabling RSS calculation would be beneficial - it doesn't have to be multiple queues.
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.
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
- 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.
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.
Pending Intel driver maintainer review/approval/discussion. This modification is being requested/tested/supported by Limelight Networks.
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.
share/man/man4/em.4 | ||
---|---|---|
54–56 | Do you guys want this *verbatim* in the man page? Sounds kind of scary. :-) |
share/man/man4/em.4 | ||
---|---|---|
54–56 | Passive->active: s/will disable/disables/ Agreed with sbruno. This kind of implies "not done". Maybe it should be in the BUGS section? | |
209 | 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. | |
211 | As above, don't capitalize "disable", and start a new sentence on a new line. | |
213 | As above. | |
215 | New sentence should be on a new line. | |
248 | "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). |
Address man page updates via wblock.
Address MSI-X <-> EM_MULTIQUEUE incompatibility via BUGS section
as per erj
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*.
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.
Alright, it's fine to leave the defines in e1000_defines.h. With the MSI-X comments being changed, this is ok with me.
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.