Add proxy library (for Linux only now) #226

ldorau · 2024-02-12T13:31:19Z

src/proxy_lib/proxy_lib_linux.c

src/proxy_lib/proxy_lib.h

src/proxy_lib/proxy_lib.c

ldorau · 2024-02-13T12:06:33Z

@igchor There is an issue with #229. I run ubench with proxy library in LD_PRELOAD:

$ LD_PRELOAD=./lib/libumf_proxy.so ./benchmark/ubench

The constructor of libumf_proxy.so is called.
The constructor runs umfMemoryProviderCreate(umfOsMemoryProviderOps()) that creates the global base allocator no. 1
The constructor runs umfPoolCreate(umfJemallocPoolOps()) that creates the global base allocator no. 2 and allocates je_malloc pool inside it.
The benchmark runs and ends.
atexit(umf_ba_destroy_global); is called and the global base allocator no. 2 is destroyed.
The destructor of libumf_proxy.so is called.
The destructor of libumf_proxy.so runs umfPoolDestroy(pool) and it blows up, because the memory it uses has already been freed in 5)

See: https://github.com/oneapi-src/unified-memory-framework/actions/runs/7887458469/job/21523176454?pr=226

github-advanced-security · 2024-02-13T12:26:42Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

src/proxy_lib/proxy_lib.c

src/CMakeLists.txt

ldorau · 2024-02-13T13:56:13Z

Build issue on Windows:
https://github.com/oneapi-src/unified-memory-framework/actions/runs/7877470520/job/21493692037?pr=226

igchor · 2024-02-13T15:55:51Z

Build issue on Windows: https://github.com/oneapi-src/unified-memory-framework/actions/runs/7877470520/job/21493692037?pr=226

This seems to be caused by the fact that we include stdlib.h where malloc and others are defined (probably with dllimport). The fix is to not include stdlib.h in the file that implements proxy_lib

src/utils/utils_common.h

igchor · 2024-02-15T19:40:32Z

IMO the scenario where someone uses both the proxy and the UMF API is invalid. I don't think we should support this, or at least it shouldn't be our priority right now. LGTM

To be honest, I think it SHOULD work in this scenario. If both proxy_lib and app uses libumf.so (dynamic lib) then there should be only one base_alloc and tracker. I think there is some other issue that we should solve here.

ldorau · 2024-02-16T09:27:00Z

IMO the scenario where someone uses both the proxy and the UMF API is invalid. I don't think we should support this, or at least it shouldn't be our priority right now. LGTM

To be honest, I think it SHOULD work in this scenario. If both proxy_lib and app uses libumf.so (dynamic lib) then there should be only one base_alloc and tracker. I think there is some other issue that we should solve here.

The occurring issues can be caused by the fact that there is only one base_alloc and one tracker in case of dynamic linking. I think that the proxy library should use a separate base_alloc and a separate tracker than an app.

vinser52 · 2024-02-16T09:46:53Z

IMO the scenario where someone uses both the proxy and the UMF API is invalid. I don't think we should support this, or at least it shouldn't be our priority right now. LGTM

Did we discussed the scenarios that we are going to cover with this proxy library at our Memory WG? We also have not discussed the architecture and potential issues. Probably TBB malloc already solves such issues somehow, we have not investigated this. Furthermore, if we consider migrating TBB malloc and building it on top of UMF then there is a TBB malloc proxy that intercepts regular malloc/free flow- do we need to duplicate such functionality with umf_proxy? Why not to discuss this first and get feedback from our Memory WG attendees? Do we have any urgency with this proxy library?
Also, as I remember CAL needs some functionality to intercept malloc/free for GPU use cases.

ldorau · 2024-02-16T10:36:02Z

IMO the scenario where someone uses both the proxy and the UMF API is invalid. I don't think we should support this, or at least it shouldn't be our priority right now. LGTM

To be honest, I think it SHOULD work in this scenario. If both proxy_lib and app uses libumf.so (dynamic lib) then there should be only one base_alloc and tracker. I think there is some other issue that we should solve here.

@bratpiorka @igchor
It DOES WORK in this scenario too with scalable pool. It turned out that those issues occur only with jemalloc pool, so there can be an issue with jemalloc pool.

The proxy library with scalable pool works well.

igchor · 2024-02-16T16:53:42Z

@ldorau Will you add some tests to this PR?

I have added a separate workflow running:
          LD_PRELOAD=./lib/libumf_proxy.so /usr/bin/date
          LD_PRELOAD=./lib/libumf_proxy.so /usr/bin/ls
          LD_PRELOAD=./lib/libumf_proxy.so ./benchmark/ubench
@igchor is it enough?

For now that's fine I think. But we should add some extra tests with a lot of mallocs/callocs/reallocs (even from multiple threads) as a next step.

src/proxy_lib/proxy_lib.c

ldorau · 2024-02-19T07:39:41Z

The linear base allocator does not implement free(), so we can only destroy the whole allocator and free all allocations or - the second option - free nothing. We do not know which allocations come from _dl_init() and which come from hwloc for example. So the only possible option is to free nothing. When there were three allocators, two of them were linear base allocators. The first was a "leak" allocator and the second was destroyed, but this was wrong. It turned out that none of them can be destroyed, so only one "leak" linear base allocator is enough.

@bratpiorka @igchor Moreover, for the same reason we cannot destroy the UMF pool either! It turned out that in the phase of loading libraries, when allocations come from the UMF pool, the program loader (maybe _dl_init()) allocates new memory for the app's environment that is read at the very end after unloading most of libraries - getenv() is called and there is a segfault caused by use-after-free, because this part of environment was freed earlier during destroying the UMF pool in the destructor of the proxy library ...

ldorau · 2024-02-20T11:39:46Z

The proxy library works correctly with all following applications now:

$ LD_PRELOAD=./lib/libumf_proxy.so ctest --output-on-failure
$ LD_PRELOAD=./lib/libumf_proxy.so ./benchmark/ubench
$ LD_PRELOAD=./lib/libumf_proxy.so ./test/umf_test-memoryPool
$ LD_PRELOAD=./lib/libumf_proxy.so /usr/bin/ls
$ LD_PRELOAD=./lib/libumf_proxy.so /usr/bin/date

bratpiorka · 2024-02-20T14:04:33Z

With the latest changes LGTM. We should make more improvements in separate PRs.

src/CMakeLists.txt

igchor · 2024-02-20T17:14:10Z

src/utils/utils_common.h

+#endif /* _WIN32 */
+
+// check if we are running in the proxy library
+static inline int is_running_in_proxy_lib(void) {


Will this work if someone links with proxy_lib? (I think we should support this scenario - from what I understand, this is how it will work on windows) Or is there no problem in that case?

Btw, it would be nice to have a test for such scenario.

Will this work if someone links with proxy_lib? (I think we should support this scenario - from what I understand, this is how it will work on windows) Or is there no problem in that case?

Do you mean the dynamic linking instead of LD_PRELOD?

Add umf_ba_linear_pool_contains_pointer(). It returns: - 0 if ptr does not belong to the pool or - size (> 0) of the memory region from ptr to the end of the pool if ptr belongs to the pool. It will be useful to implement realloc() in the proxy library. Added also the baseAllocLinearPoolContainsPointer test. Signed-off-by: Lukasz Dorau <[email protected]>

src/CMakeLists.txt

UMF proxy library is a library for intercepting user allocation requests. It intercepts following APIs: - aligned_alloc() - calloc() - free() - malloc() - malloc_usable_size() - realloc() Signed-off-by: Lukasz Dorau <[email protected]>

lukaszstolarczuk · 2024-02-21T12:35:37Z

Thx! LGTM

ldorau force-pushed the Add_proxy_library branch 4 times, most recently from 0485390 to 2e549bc Compare February 12, 2024 16:09

igchor reviewed Feb 12, 2024

View reviewed changes

src/proxy_lib/proxy_lib_linux.c Outdated Show resolved Hide resolved

src/proxy_lib/proxy_lib.h Outdated Show resolved Hide resolved

src/proxy_lib/proxy_lib.c Outdated Show resolved Hide resolved

ldorau force-pushed the Add_proxy_library branch 2 times, most recently from 253de2c to cdb5445 Compare February 12, 2024 20:23

igchor reviewed Feb 12, 2024

View reviewed changes

src/proxy_lib/proxy_lib.c Outdated Show resolved Hide resolved

ldorau force-pushed the Add_proxy_library branch 3 times, most recently from 81b5d11 to 0551c74 Compare February 13, 2024 11:22

ldorau force-pushed the Add_proxy_library branch from 0551c74 to c3f349c Compare February 13, 2024 12:12

ldorau changed the title ~~[DRAFT] Add proxy library~~ Add proxy library (for Linux only now) Feb 13, 2024

ldorau marked this pull request as ready for review February 13, 2024 12:14

ldorau requested a review from a team as a code owner February 13, 2024 12:14

ldorau force-pushed the Add_proxy_library branch from c3f349c to 7090d5e Compare February 13, 2024 12:20

ldorau force-pushed the Add_proxy_library branch from 7090d5e to 1837bc3 Compare February 13, 2024 13:45

lukaszstolarczuk reviewed Feb 13, 2024

View reviewed changes

src/proxy_lib/proxy_lib.c Outdated Show resolved Hide resolved

src/proxy_lib/proxy_lib.c Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

ldorau force-pushed the Add_proxy_library branch from 1837bc3 to 79c32f5 Compare February 13, 2024 14:44

ldorau requested review from lukaszstolarczuk, igchor and bratpiorka February 13, 2024 14:45

igchor reviewed Feb 13, 2024

View reviewed changes

src/utils/utils_common.h Outdated Show resolved Hide resolved

src/utils/utils_common.h Outdated Show resolved Hide resolved

ldorau force-pushed the Add_proxy_library branch from 79c32f5 to 5256637 Compare February 14, 2024 11:03

ldorau requested a review from igchor February 14, 2024 14:36

ldorau force-pushed the Add_proxy_library branch from fc609d5 to 8d15fb9 Compare February 14, 2024 14:37

ldorau force-pushed the Add_proxy_library branch 2 times, most recently from 886c2ae to 43f80ca Compare February 16, 2024 10:28

ldorau force-pushed the Add_proxy_library branch from 43f80ca to cec6bd4 Compare February 16, 2024 10:40

igchor reviewed Feb 16, 2024

View reviewed changes

src/proxy_lib/proxy_lib.c Show resolved Hide resolved

ldorau force-pushed the Add_proxy_library branch 6 times, most recently from 16633ba to 4b246b2 Compare February 20, 2024 11:29

ldorau requested review from igchor and pbalcer February 20, 2024 11:40

lukaszstolarczuk reviewed Feb 20, 2024

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

igchor approved these changes Feb 20, 2024

View reviewed changes

ldorau force-pushed the Add_proxy_library branch from 4b246b2 to a538b33 Compare February 21, 2024 09:27

lukaszstolarczuk reviewed Feb 21, 2024

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

ldorau force-pushed the Add_proxy_library branch from a538b33 to 47a98a4 Compare February 21, 2024 11:40

bratpiorka merged commit 1682326 into oneapi-src:main Feb 21, 2024

ldorau deleted the Add_proxy_library branch February 21, 2024 13:10

Add proxy library (for Linux only now) #226

Add proxy library (for Linux only now) #226

Uh oh!

Conversation

ldorau commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldorau commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-advanced-security bot commented Feb 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldorau commented Feb 13, 2024

Uh oh!

igchor commented Feb 13, 2024

Uh oh!

Uh oh!

Uh oh!

igchor commented Feb 15, 2024

Uh oh!

ldorau commented Feb 16, 2024

Uh oh!

vinser52 commented Feb 16, 2024

Uh oh!

ldorau commented Feb 16, 2024

Uh oh!

igchor commented Feb 16, 2024

Uh oh!

Uh oh!

ldorau commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldorau commented Feb 20, 2024

Uh oh!

bratpiorka commented Feb 20, 2024

Uh oh!

Uh oh!

igchor Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

igchor Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

ldorau Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

igchor Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukaszstolarczuk commented Feb 21, 2024

Uh oh!

Uh oh!

ldorau commented Feb 12, 2024 •

edited

Loading

ldorau commented Feb 13, 2024 •

edited

Loading

ldorau commented Feb 19, 2024 •

edited

Loading