oneapi-src/unified-memory-frameworkPublic

NotificationsYou must be signed in to change notification settings
Fork42
Star75

Implement IPC cache for umfOpenIPCHandle function#736

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

bratpiorka merged 8 commits intooneapi-src:mainfromvinser52:svinogra_ipc_cache

Nov 13, 2024

Merged

Implement IPC cache for umfOpenIPCHandle function#736

bratpiorka merged 8 commits intooneapi-src:mainfromvinser52:svinogra_ipc_cache

Nov 13, 2024

Conversation

Copy link

Contributor

vinser52 commentedSep 17, 2024•
edited by lukaszstolarczuk
Loading

Description

This PR adds initial set of changes to implement cache for opened IPC handles.
The new third-party data structure (uthash - has map implementation) is added. The same data structure is used by MPI to implement IPC cache. Here is theuthash documentation. Here is theoriginal source repo

Fixes:#403

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly
All newly added source files have a license
All newly added source files are referenced in CMake files

bratpiorka reviewed

Sep 18, 2024

View reviewed changes

src/uthash/uthash.h OutdatedShow resolvedHide resolved

vinser52 force-pushed thesvinogra_ipc_cache branch 3 times, most recently fromab7b9ee tob969903Compare

October 1, 2024 14:18

vinser52 force-pushed thesvinogra_ipc_cache branch 6 times, most recently from4fd30e3 to8a96eb0Compare

October 13, 2024 00:00

vinser52 mentioned this pull request

Oct 13, 2024

Fix umfPoolDestroy to destroy providers in the right order#794

Merged

3 tasks

vinser52 force-pushed thesvinogra_ipc_cache branch from8a96eb0 tob84c05cCompare

October 13, 2024 16:47

vinser52 mentioned this pull request

Oct 13, 2024

Fix issue in trackingGetIpcHandle#795

Closed

3 tasks

vinser52 force-pushed thesvinogra_ipc_cache branch 4 times, most recently from50b1d4e to3d012dfCompare

October 14, 2024 21:25

vinser52 marked this pull request as ready for review

October 14, 2024 21:26

vinser52 requested a review froma team as acode owner

October 14, 2024 21:26

bratpiorka requested a review fromlplewa

October 15, 2024 08:43

vinser52 force-pushed thesvinogra_ipc_cache branch 2 times, most recently from0823eaf tocb6248fCompare

October 20, 2024 21:09

vinser52 commented

Oct 20, 2024

View reviewed changes

src/uthash/uthash.h OutdatedShow resolvedHide resolved

vinser52 commented

Oct 20, 2024

View reviewed changes

src/uthash/uthash.hShow resolvedHide resolved

vinser52 force-pushed thesvinogra_ipc_cache branch 2 times, most recently fromfc2f15b to8b3626eCompare

October 21, 2024 14:36

vinser52 requested a review frombratpiorka

October 22, 2024 21:15

vinser52 force-pushed thesvinogra_ipc_cache branch from8b3626e toa239fa1Compare

October 23, 2024 14:28

vinser52 requested a review fromldorau

October 25, 2024 13:24

bratpiorka requested changes

Oct 28, 2024

View reviewed changes

src/ipc.cShow resolvedHide resolved

src/ipc_cache.c OutdatedShow resolvedHide resolved

src/ipc_cache.cShow resolvedHide resolved

src/ipc_cache.c OutdatedShow resolvedHide resolved

src/ipc_cache.cShow resolvedHide resolved

src/ipc_cache.h OutdatedShow resolvedHide resolved

src/ipc_cache.c OutdatedShow resolvedHide resolved

src/libumf.c OutdatedShow resolvedHide resolved

src/provider/provider_tracking.c OutdatedShow resolvedHide resolved

lplewa requested changes

Nov 4, 2024

View reviewed changes

src/ipc_cache.cShow resolvedHide resolved

src/ipc_cache.c OutdatedShow resolvedHide resolved

src/ipc_cache.h OutdatedShow resolvedHide resolved

src/ipc_cache.cShow resolvedHide resolved

src/provider/provider_tracking.cShow resolvedHide resolved

src/ipc_cache.cShow resolvedHide resolved

vinser52 force-pushed thesvinogra_ipc_cache branch 4 times, most recently fromd1e2e55 tod10b8a8Compare

November 4, 2024 22:56

ldorau requested changes

Nov 5, 2024

View reviewed changes

Copy link

Contributor

ldorau left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Please enable lines no: 356 and 359:

unified-memory-framework/test/ipcFixtures.hpp

Lines 355 to 359 incdc0310

	// TODO fix it - it does not work in case of IPC cache hit
	// EXPECT_EQ(stat.allocCount, stat.getCount);
	EXPECT_EQ(stat.getCount, stat.putCount);
	// TODO fix it - it does not work in case of IPC cache hit
	// EXPECT_EQ(stat.openCount, stat.getCount);

It seems that the check in the line no 356:

EXPECT_EQ(stat.allocCount,stat.getCount);

still fails in theumf-provider_devdax_memory test:

[ RUN      ] DevDaxProviderDifferentPoolsTest/umfIpcTest.AllocFreeAllocTest/2/home/ldorau/work/unified-memory-framework/test/ipcFixtures.hpp:354: FailureExpected equality of these values:  stat.allocCount    Which is: 5  stat.getCount    Which is: 1[  FAILED  ] DevDaxProviderDifferentPoolsTest/umfIpcTest.AllocFreeAllocTest/2, where GetParam() = (0x7f3e254f5660, NULL, 0x7f3e254f5200, 0x55cdc299f760, 0x55cdc299f048, false) (0 ms)...[  FAILED  ] 1 test, listed below:[  FAILED  ] DevDaxProviderDifferentPoolsTest/umfIpcTest.AllocFreeAllocTest/2, where GetParam() = (0x7f3e254f5660, NULL, 0x7f3e254f5200, 0x55cdc299f760, 0x55cdc299f048, false) 1 FAILED TEST

lplewa approved these changes

Nov 5, 2024

View reviewed changes

Copy link

Contributor

lplewa left a comment

Choose a reason for hiding this comment

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

Please next time do not include half done features. Untested/unused features deceases code coverage, but also they can be detected by static analyze tools as an bugs - in short they might cause troubles after the merge. Also nothing is more permanent that temporary solution, but i trust you that it isn't a case here ;)

Copy link

ContributorAuthor

vinser52 commentedNov 5, 2024

Please enable lines no: 356 and 359:
unified-memory-framework/test/ipcFixtures.hpp
Lines 355 to 359 incdc0310
// TODO fix it - it does not work in case of IPC cache hit
// EXPECT_EQ(stat.allocCount, stat.getCount);
EXPECT_EQ(stat.getCount, stat.putCount);
// TODO fix it - it does not work in case of IPC cache hit
// EXPECT_EQ(stat.openCount, stat.getCount);
It seems that the check in the line no 356:
EXPECT_EQ(stat.allocCount,stat.getCount);
still fails in theumf-provider_devdax_memory test:
[ RUN      ] DevDaxProviderDifferentPoolsTest/umfIpcTest.AllocFreeAllocTest/2/home/ldorau/work/unified-memory-framework/test/ipcFixtures.hpp:354: FailureExpected equality of these values:  stat.allocCount    Which is: 5  stat.getCount    Which is: 1[  FAILED  ] DevDaxProviderDifferentPoolsTest/umfIpcTest.AllocFreeAllocTest/2, where GetParam() = (0x7f3e254f5660, NULL, 0x7f3e254f5200, 0x55cdc299f760, 0x55cdc299f048, false) (0 ms)...[  FAILED  ] 1 test, listed below:[  FAILED  ] DevDaxProviderDifferentPoolsTest/umfIpcTest.AllocFreeAllocTest/2, where GetParam() = (0x7f3e254f5660, NULL, 0x7f3e254f5200, 0x55cdc299f760, 0x55cdc299f048, false) 1 FAILED TEST

Done. The issue was caused by the implementation of a scalable pool. When we do the firstumfPoolMalloc the scalable pool internally does five 2MB allocations from the memory provider. So checkingEXPECT_EQ(stat.allocCount, stat.getCount) does not make sense. I removed it, butEXPECT_EQ(stat.openCount, stat.getCount); should pass. I tested it with the OS provider and it works. I have no system with devdax, so let's see if CI passes.

ldorau approved these changes

Nov 5, 2024

View reviewed changes

Copy link

Contributor

ldorau commentedNov 6, 2024

@vinser52 CI builds fail

Copy link

ContributorAuthor

vinser52 commentedNov 6, 2024

@vinser52 CI builds fail

Yeah, I saw that. I cannot reproduce it locally. My guess is that in case of CI there are some limits exceed with scalable pool because it uses 2MB slabs. So I can try to decrease memory pressure by decreasing number of allocations in corresponding test or disable test with scalable pool for now. I will investigate today.

vinser52 force-pushed thesvinogra_ipc_cache branch from5e688a1 toa39b9fcCompare

November 6, 2024 11:17

Copy link

ContributorAuthor

vinser52 commentedNov 6, 2024

@ldorau I have moved the test with the scalable pool into a separate PR#860. Now this PR should pass CI validation.
I am going to introduce a config API for a scalable pool. It allows us to set the slab size to something less than 2MB and use such a config for the test.