Page MenuHomeFreeBSD

Allocate sufficient number of chain frames in mps(4) and mpr(4).
ClosedPublic

Authored by mav on Feb 8 2018, 4:29 AM.

Details

Summary

Quite a lot of people are complaining on confusing "Out of chain frames" reported by mps(4) and mpr(4) drivers during periods of high load. Aside of pure cosmetics, it causes request retrial, that is found to not be handled properly by some software, and possibly also reduces performance. Investigation shows that present count of chain frames is underestimated by 5 times comparing to potentially needed (2048 vs 10240).

Proposed patch removed hard limits on number of chain frames in the drivers, allocating as much of them as needed to satisfy all declared number of requests at maximum data size with worst possible fragmentation. With the default value MAXPHYS of 128KB the price of this change is about 1MB of memory per HBA, that does not sound very serious these days for server-grade HBA.

People, that for some reason want that limitation back, can still do it via the hw.mps.max_chains loader tunable, same as before.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mav created this revision.Feb 8 2018, 4:29 AM
ken added a comment.Feb 8 2018, 4:29 PM

This is a good idea. I have been bumping this up on our builds at Spectra, and I currently have the maximum number of chain frames set to 32768.

I'd rather not limit maxio to MAXPHYS, though. I can probably live with it, but I'd rather get rid of the limit someday if we can. The asynchronous pass(4) interface already allows users to bypass MAXPHYS because it doesn't use struct buf, but rather mallocs buffers to send down.

mpr/mpr.c
443 ↗(On Diff #39033)

I would rather not limit maxio to MAXPHYS. We can already do I/Os larger than MAXPHYS via the asynchronous pass(4) interface, and I hope we can remove the limitation in other places as well. Let's limit it to whatever the hardware supports.

mps/mps.c
418 ↗(On Diff #39033)

See above, I would rather not limit this to MAXPHYS.

mav updated this revision to Diff 39045.Feb 8 2018, 4:59 PM

Makes sense. I have an idea for that. This new patch uses MAXPHYS as an optimization hint, while reporting real hardware limitation to CAM. It should allow bigger I/Os, just without resources guaranteed.

scottl requested changes to this revision.Feb 8 2018, 5:26 PM

Unfortunately, there are some big drawbacks to this approach. The larger you make the chain frame pool, the more pressure you put on the contigmalloc allocator. This is usually (though not always!) not a problem at boot, but would be a big problem with loading the driver as a module after boot. I agree with the desire to make the chain frame pool be self-tuning, but I think that to do it the right way, the pool must be allocated in multiple smaller segments of no more than 2MB each (i.e. 16384 frames). It should also handle potential allocation failures. Without this, I feel that your approach will be too fragile.

This revision now requires changes to proceed.Feb 8 2018, 5:26 PM
mav added a comment.Feb 8 2018, 6:20 PM

Yes, I had that concern about memory allocation too. I agree that allocation code should be rewritten to not require more continuous pages then required by hardware. But actually present setting of the MAXPHYS and MPR_REQ_FRAMES easily fit into your proposed 2MB/16384 threshold. What would you say about committing this patch with MPR_CHAIN_FRAMES set to some limit sufficient for default configuration like 16384, just to protect against somebody who may have increased MAXPHYS until the allocation is fixed?

imp added a comment.Feb 8 2018, 6:22 PM
In D14261#299111, @mav wrote:

Yes, I had that concern about memory allocation too. I agree that allocation code should be rewritten to not require more continuous pages then required by hardware. But actually present setting of the MAXPHYS and MPR_REQ_FRAMES easily fit into your proposed 2MB/16384 threshold. What would you say about committing this patch with MPR_CHAIN_FRAMES set to some limit sufficient for default configuration like 16384, just to protect against somebody who may have increased MAXPHYS until the allocation is fixed?

Netflix runs with a 8MB MAXPHYS, so we'd need something from the get go...

mav added a comment.EditedFeb 8 2018, 6:29 PM
In D14261#299113, @imp wrote:

Netflix runs with a 8MB MAXPHYS, so we'd need something from the get go...

Are you running with default 2048 chain frames or you are tuning those also? I'd guess you should start hitting that limit hard as soon as memory get fragmented. Or you just never reaching high number of simultaneous I/Os while using large I/O sizes to hit that?

This revision was not accepted when it landed; it landed in state Needs Revision.Feb 10 2018, 12:56 AM
This revision was automatically updated to reflect the committed changes.