[Core] Locked memory manager#2276
Conversation
furszy
left a comment
There was a problem hiding this comment.
initial quick code review ack 7eab0b5 .
I spent more time checking what should go next than reviewing this code, would be good to go further and look at: bitcoin#9063 ,bitcoin#9070, bitcoin#15117 for example. Plus the algo change introduced in bitcoin#12048.
We could leave them for another PR or port all at once here. I'm fine either way, all of this changes should be pretty scoped to the private key memory allocation.
|
I think this can be merged as is. |
>>> backports bitcoin/bitcoin@d7d187e Pagelocker is only needed for secure (usually wallet) operations, so don't make the zero-after-free allocator depend on it.
>>> backports bitcoin/bitcoin@4536148 Add a pool for locked memory chunks, replacing LockedPageManager. This is something I've been wanting to do for a long time. The current approach of locking objects where they happen to be on the stack or heap in-place causes a lot of mlock/munlock system call overhead, slowing down any handling of keys. Also locked memory is a limited resource on many operating systems (and using a lot of it bogs down the system), so the previous approach of locking every page that may contain any key information (but also other information) is wasteful.
>>> backports bitcoin/bitcoin@6567999 ``` getmemoryinfo Returns an object containing information about memory usage. Result: { "locked": { (json object) Information about locked memory manager "used": xxxxx, (numeric) Number of bytes used "free": xxxxx, (numeric) Number of bytes available in current arenas "total": xxxxxxx, (numeric) Total number of bytes managed "locked": xxxxxx, (numeric) Amount of bytes that succeeded locking. If this number is smaller than total, locking pages failed at some point and key data could be swapped to disk. } } Examples: > bitcoin-cli getmemoryinfo > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getmemoryinfo", "params": [] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/ ```
7eab0b5 to
1c04a35
Compare
Yeah, that's because |
PIVX-Project#2276 split the `allocators.h` header and has been merged
9295568 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz) f7d88f5 Define ARENA_DEBUG in GA test runs (furszy) cae2e13 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz) d19359f fix nits: variable naming, typos (Martin Ankerl) 5cbd967 Use best-fit strategy in Arena, now O(log(n)) instead O(n) (Martin Ankerl) 1ccd62e Fix out-of-bounds write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked (practicalswift) 7ad7157 LockedPool: avoid quadratic-time allocation (Kaz Wesley) 39e1b3b LockedPool: fix explosion for illegal-sized alloc (Kaz Wesley) adb1f1c LockedPool: test handling of invalid allocations (Kaz Wesley) 626b865 Do not shadow variable, use deprecated MAP_ANON if MAP_ANONYMOUS is not defined. (Pavel Janík) Pull request description: Follow up to #2276. Completing the locked memory manager todo list. Back porting: * bitcoin#9063. * bitcoin#9070. * bitcoin#12048. * bitcoin#15117. * bitcoin#16161. ACKs for top commit: random-zebra: ACK 9295568 Fuzzbawls: ACK 9295568 Tree-SHA512: 0d14c7e8231386ee32f1fefac08de9ee278c02d6b0e5172c055851d33c803b0be3398fc5173457e539179712a13d4736033a926f318e576789b4d57b67eb0596
First commit adapts btc d7d187e.
Then backport the last 3 commits of bitcoin#8753, which were left out in #1488.