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.
Tags
None
Referenced Files
F81604633: D14261.diff
Thu, Apr 18, 8:13 PM
Unknown Object (File)
Mon, Apr 15, 8:59 PM
Unknown Object (File)
Mar 19 2024, 2:36 PM
Unknown Object (File)
Mar 17 2024, 3:55 AM
Unknown Object (File)
Mar 7 2024, 8:01 PM
Unknown Object (File)
Jan 31 2024, 5:40 PM
Unknown Object (File)
Dec 22 2023, 10:51 PM
Unknown Object (File)
Dec 2 2023, 7:27 PM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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

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?

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...

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.