Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Core] Locked memory manager #2276

Merged
merged 5 commits into from
Apr 5, 2021
Merged

[Core] Locked memory manager #2276

merged 5 commits into from
Apr 5, 2021

Conversation

random-zebra
Copy link

First commit adapts btc d7d187e.
Then backport the last 3 commits of bitcoin#8753, which were left out in #1488.

@random-zebra random-zebra added this to the 6.0.0 milestone Mar 29, 2021
@random-zebra random-zebra self-assigned this Mar 29, 2021
furszy
furszy reviewed Mar 31, 2021
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@random-zebra
Copy link
Author

I think this can be merged as is.

Fuzzbawls
Fuzzbawls reviewed Apr 4, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good. few nits that could simply be addressed in #2266 if this is merged first. Other than that, src/support/lockedpool.cpp is being included in multiple libs when it doesn't need to be for CMake builds.

src/support/allocators/secure.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/support/allocators/zeroafterfree.h Outdated Show resolved Hide resolved
src/support/lockedpool.cpp Outdated Show resolved Hide resolved
src/support/lockedpool.cpp Outdated Show resolved Hide resolved
@Fuzzbawls Fuzzbawls added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Apr 4, 2021
random-zebra and others added 5 commits April 4, 2021 09:19
@random-zebra
allocators: split allocators and pagelocker
6ca6470 
>>> 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.
@laanwj @random-zebra
support: Add LockedPool
2d2e331 
>>> 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.
@laanwj @random-zebra
rpc: Add getmemoryinfo call
cd34088 
>>> 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/
```
@laanwj @random-zebra
bench: Add benchmark for lockedpool allocation/deallocation
68a3f8f
@random-zebra
[Trivial] Use C++11 nulltpr instead of NULL macro in allocators files
1c04a35
@random-zebra
Copy link
Author

src/support/lockedpool.cpp is being included in multiple libs when it doesn't need to be for CMake builds

Yeah, that's because allocators.cpp was originally included in both libs (for CMake, as well as for native builds).
Fixed now.
Also rebased on master and fixed NULL-->nullptr.

Fuzzbawls
Fuzzbawls approved these changes Apr 4, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1c04a35

furszy
furszy approved these changes Apr 5, 2021
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good 👍 , we can tackle the next PRs in the list in another branch.

ACK 1c04a35 .

@furszy furszy merged commit 7793f18 into PIVX-Project:master Apr 5, 2021
Fuzzbawls added a commit to Fuzzbawls/PIVX that referenced this pull request Apr 14, 2021
@Fuzzbawls
Use secure.h header for secure allocators
cecbf6c 
PIVX-Project#2276 split the `allocators.h` header and has been merged
@furszy furszy mentioned this pull request Aug 4, 2021
Merged
@random-zebra random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Aug 24, 2021
furszy added a commit that referenced this pull request Sep 11, 2021
@furszy
Merge #2493: Locked memory manager updates
19d00a6 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

@furszy furszy furszy approved these changes

Assignees

@random-zebra random-zebra

Labels
Refactoring RPC Upstream Wallet
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@random-zebra @furszy @Fuzzbawls @laanwj