Skip to content

Add support for MAP_SHARED for Linux only #429

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
Merged
merged 5 commits into from
May 7, 2024
Merged

Add support for MAP_SHARED for Linux only #429

merged 5 commits into from
May 7, 2024

Conversation

ldorau
Copy link
Contributor

@ldorau ldorau commented Apr 10, 2024 •
edited
Loading

Description

  1. Add support for MAP_SHARED to OS memory provider
  2. Add support for MAP_SHARED to the proxy library
  3. Implement the IPC hooks in OS memory provider
  4. Add a basic shared memory (SHM) IPC test (ipc_os_prov) using only the OS memory provider API (not using the API from ipc.h).

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly

@ldorau ldorau requested a review from a team as a code owner April 10, 2024 13:47
igchor
igchor requested changes Apr 10, 2024
View reviewed changes
Copy link
Member

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Is the main purpose of this PR to enable shared memory for forked processes?

I removed the SHARED flag a while back because it was not clear how to allow other processes to use this memory (#236) other than by using fork. I think this requires more research and we should probably consider using IPC (@vinser52 did we have any discussion about this on any of the issues?)

Also, @ldorau please extend existing bind tests to test MAP_SHARED as well. I'm not sure whether hwloc_set_area_membind will work with MAP_SHARED. According to the documentation of mbind: The specified policy will be ignored for any MAP_SHARED mappings in the specified memory range. Rather the pages will be allocated according to the memory policy of the thread that caused the page to be allocated.

@bratpiorka
Copy link
Contributor

@igchor this PR is a part of the requirements for CAL library

@vinser52
Copy link
Contributor

How is this shared region supposed to be used?

As Igor said, one of the use cases for shared memory on a host is IPC.
Before introducing support of shared memory I would suggest extending IPC tests: we need to create IPC tests that can be run with memory providers that support IPC (today only L0 supports it). having such test allows us to validate the changes introduced in this PR.

Regarding CAL, do we already have exact requirements?

bratpiorka
bratpiorka reviewed Apr 10, 2024
View reviewed changes
@bratpiorka
Copy link
Contributor

How is this shared region supposed to be used?
Regarding CAL, do we already have exact requirements?

I asked CAL team about that. I thought this should be as simple as adding MAP_SHARED flag but now I think it should be changed to a draft version until we get the requirements.

@bratpiorka bratpiorka marked this pull request as draft April 10, 2024 16:20
@ldorau ldorau changed the title Add support for MAP_SHARED [DRAFT] Add support for MAP_SHARED Apr 11, 2024
@ldorau ldorau changed the title [DRAFT] Add support for MAP_SHARED [DRAFT] Add support for MAP_SHARED for Linux only Apr 12, 2024
@bratpiorka bratpiorka mentioned this pull request Apr 12, 2024
Closed
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch 12 times, most recently from de5544d to 84666de Compare April 17, 2024 08:34
@ldorau ldorau mentioned this pull request Apr 17, 2024
Merged
3 tasks
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch 2 times, most recently from 9f8f85f to 7aaa1f4 Compare April 17, 2024 13:26
@ldorau ldorau requested a review from igchor April 17, 2024 14:11
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch 4 times, most recently from 871035e to 493f038 Compare April 19, 2024 13:58
@ldorau ldorau requested a review from lukaszstolarczuk April 29, 2024 07:37
@ldorau
Copy link
Contributor Author

ldorau commented Apr 29, 2024

@vinser52 please re-review

vinser52
vinser52 reviewed Apr 29, 2024
View reviewed changes
src/provider/provider_os_memory.c Outdated
int fd; // file descriptor for memory mapping
size_t size_fd; // size of file used for memory mapping
size_t max_size_fd; // maximum size of file used for memory mapping
// A critnib map storing (ptr, fd_offset + 1) pairs (+ 1 to be able to store fd_offset == 0).
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we cannot store fd_offset == 0 in the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

critnib_get() returns value or NULL, so a value cannot equal 0

Copy link
Contributor

@vinser52 vinser52 Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, got it.
Could you please explicitly mention this in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Done

igchor
igchor reviewed Apr 29, 2024
View reviewed changes
@ldorau ldorau mentioned this pull request Apr 30, 2024
Open
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch from 950345c to 2e9b682 Compare April 30, 2024 07:35
@ldorau ldorau requested a review from vinser52 April 30, 2024 09:05
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch from 2e9b682 to 6445425 Compare April 30, 2024 10:01
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch 3 times, most recently from 51b272c to d526d7f Compare May 6, 2024 08:15
@ldorau ldorau requested review from igchor and vinser52 May 6, 2024 08:15
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch 2 times, most recently from df7ef79 to c510e59 Compare May 6, 2024 10:53
ldorau added 5 commits May 7, 2024 15:47
@ldorau
Add support for MAP_SHARED to OS memory provider for Linux only
0241dbe 
Add support for MAP_SHARED to OS memory provider for Linux only
and for the UMF_NUMA_MODE_DEFAULT only.

Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
@ldorau
Add support for MAP_SHARED to the proxy library for Linux only
c4c7526 
Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
@ldorau
Implement IPC hooks in OS memory provider
6b94829 
Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
@ldorau
Add an IPC shared memory test
3331207 
Add an IPC shared memory test
using only the OS memory provider API
(not using the API from ipc.h).

Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
@ldorau
Extract common sources for Linux and MacOSX
aeb0c72 
Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
@ldorau ldorau force-pushed the Add_support_for_MAP_SHARED branch from c510e59 to aeb0c72 Compare May 7, 2024 13:47
@bratpiorka bratpiorka merged commit 4865d89 into oneapi-src:main May 7, 2024
70 checks passed
@ldorau ldorau deleted the Add_support_for_MAP_SHARED branch May 8, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinser52 vinser52 vinser52 approved these changes

@pbalcer pbalcer pbalcer left review comments

@bratpiorka bratpiorka bratpiorka approved these changes

@igchor igchor igchor approved these changes

@lukaszstolarczuk lukaszstolarczuk Awaiting requested review from lukaszstolarczuk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ldorau @bratpiorka @vinser52 @pbalcer @igchor @lukaszstolarczuk