fix segfault caused by memory allocated in one pool and freed in another - #533
fix segfault caused by memory allocated in one pool and freed in another#533jp-embedded wants to merge 1 commit into
Conversation
|
Thanks for investigating this! I added some debug go mempool.c locally, and I can see other cases where items are allocated in one thread and freed in another (eg: the parser runs in a worker thread which calls tent_tree_add_dup(), and later updater.c calls tent_tree_rm() in the main thread). That probably doesn't have the same danger of causing a segfault, but it still is a memory leak. What are your thoughts on removing the thread locality entirely and just using locking for all mempools? Unfortunately I don't recall what performance test I was running in 2020 when memory pools were originally added. |
|
Yes that would be simpler and much less error prone. I do have another idea though - i just did not wan't to fix this with a bigger change. I did an experiment a long time ago doing a lock-free pool based memory allocator: https://github.com/jp-embedded/jp_alloc Although it was just an experiment and maybe needs a bit of cleanup it works pretty well. Could probably be optimized a bit more also in some corners. I tested it on gimp, which does a lot of allocations, and it made it perform better. In most cases an allocation goes through like 5 lines of code or so. If you like it, we can just grab the required parts of it. Although its in c++ it should link fine together with the c code - just an idea. I would also be happy to try it out on a branch? |
|
A lock-free memory allocator sounds interesting. I tried to do a quick test with it, though it didn't compile out-of-the-box on Macos & Windows, so I'm not sure how involved it would be to support those. I'm also kinda on the fence on introducing C++ just for the mempool allocator :). In any case, I tested building the gittup.org repo with the existing _Thread_local mempools and compared to using global mempools with mutex locking, and the performance difference was negligible for my setup. That's not to say there won't be a performance impact with a different workload (maybe higher job concurrency and/or faster jobs with more file accesses, for example), but for now it seems unlikely to make an impact for compiling regular C programs. If you have different performance statistics for your case, I'd be interested to hear them. For now I'm going to commit the locking approach, so at least the crashes should be fixed. Let me know if you continue to have issues here. |
On large projects tup ends up in a segfault because mem_pool::next_alloc_size wraps around to 0 so it allocates a new block of 0 bytes instead of 4GB. next_alloc_size should be of type size_t to match the pointer size to avoid this.
However it turns out that the underlying issue is that all file_entry entries are allocated in the fuse server thread but put back on the free-list in the worker threads where they cannot be reused by the fuse server thread. The result is that the fuse server thread keeps allocating new entries and the worker threads free-list keeps growing.
This PR fixes both.
This fixes #532