Fix devdax_open_ipc_handle() and devdax_close_ipc_handle() #838

ldorau · 2024-10-24T20:45:52Z

Description

Fix devdax_open_ipc_handle() and devdax_close_ipc_handle()

devdax_open_ipc_handle() has to use the path of the remote
/dev/dax got from the IPC handle, not the local one.

devdax_open_ipc_handle() has to use also the memory protection
got from the IPC handle, so let's add the memory protection
to the IPC handle.

devdax_open_ipc_handle() should mmap only the required range of memory,
not the whole /dev/dax device, so let's add the length of the allocation
to the IPC handle.

devdax_close_ipc_handle() should unmap only the required range of memory,
not the whole /dev/dax device.

Fixes: #846

Checklist

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

src/provider/provider_devdax_memory.c

vinser52 · 2024-10-25T11:51:41Z

src/provider/provider_devdax_memory.c

+    // length and offset passed to mmap() have to be page-aligned
+    size_t offset_aligned = devdax_ipc_data->offset;
+    size_t length_aligned = devdax_ipc_data->length;
+    utils_align_ptr_down_size_up((void **)&offset_aligned, &length_aligned,


Do we really need this call to the utils_align_ptr_down_size_up function? Offset and length should be aligned during the allocation? I mean that IPC handle represents allocation that is done by this memory provider and it already should be page-aligned. Probably here and in the devdax_open_ipc_handle we need to check that offset and length are page-aligned.

Yes, it is needed. alloc() aligns pointer's address to the alignment given as the alloc()'s parameter, not to the page size, but mmap()requires the page size alignment.

Ok, I got it. In case of arbitrary alignment such logic makes sense, but does arbitrary alignment in case of any memory provider (not only devdax) makes sense. I can assume that alignment always is a multiply of the page size. Did I miss something?

User-requested alignment (given as alloc(size, alignment)) "must be a power of two and a multiple of sizeof(void *)." as POSIX_MEMALIGN(3) states, so it does not have to be a multiply of the page size.

Yeah, but the memory provider API is closer to the mmap.
The umfPoolMalloc() is similar to regular malloc, but memory providers are responsible for a coarse-grain allocations.

I think the main issue is that we cannot require the size (in alloc()) to be a multiple of 2MB - we cannot force jemalloc or any other pool manager or a user of a memory provider to call alloc() only with size being a multiple of 2MB.

I think with alloc function everything is easy, we can support 4KB allocations because we just update the offset and do actual mapping at the init phase. The question is about ope_ip_handle how should we implement it.

@vinser52 please review the new version - there is no up-alignment in open/close_IPC_handle any more
(The devdax CI jobs pass: https://github.com/oneapi-src/unified-memory-framework/actions/runs/11595816096)

The new IPC tests (#845) pass too: https://github.com/oneapi-src/unified-memory-framework/actions/runs/11596112796/job/32286377438

@vinser52 Done.

ldorau · 2024-10-30T11:57:52Z

Fixes: #846

src/provider/provider_devdax_memory.c

Signed-off-by: Lukasz Dorau <[email protected]>

Add utils_align_ptr_down_size_up() to align a pointer down and a size up (for mmap()/munmap()). Signed-off-by: Lukasz Dorau <[email protected]>

Signed-off-by: Lukasz Dorau <[email protected]>

Fix handling arguments of devdax_alloc(): - alignment has to be a power of 2 and a divider or a multiple of the page size (2MB), Signed-off-by: Lukasz Dorau <[email protected]>

devdax_open_ipc_handle() has to use the path of the remote /dev/dax got from the IPC handle, not the local one. devdax_open_ipc_handle() has to use also the memory protection got from the IPC handle, so let's add the memory protection to the IPC handle. devdax_open_ipc_handle() should mmap only the required range of memory, not the whole /dev/dax device, so let's add the length of the allocation to the IPC handle. devdax_close_ipc_handle() should unmap only the required range of memory, not the whole /dev/dax device. Fixes: oneapi-src#846 Signed-off-by: Lukasz Dorau <[email protected]>

vinser52 · 2024-10-31T13:27:13Z

src/provider/provider_devdax_memory.c

+    // alignment must be a power of two and a multiple or a divider of the page size
+    if (alignment && ((alignment & (alignment - 1)) ||
+                      ((alignment % DEVDAX_PAGE_SIZE_2MB) &&
+                       (DEVDAX_PAGE_SIZE_2MB % alignment)))) {
+        LOG_ERR("wrong alignment: %zu (not a power of 2 or a multiple or a "
+                "divider of the page size (%zu))",
+                alignment, DEVDAX_PAGE_SIZE_2MB);


To ensure I understand it correctly, will 4 KB also pass this check?

Yes, 4kB is a divider of 2MB

Alignment passed to `file_alloc()` must be a power of two and a multiple or a divider of the page size. Align up the alignment in `file_alloc()` to the page size. file_open_ipc_handle() has to use the memory protection and visibility got from the IPC handle, so let's add the memory protection and visibility to the IPC handle. A length and an offset passed to mmap() in `file_open_ipc_handle()` and `file_close_ipc_handle()` have to be aligned to the page size. Ref: oneapi-src#838 Fixes: oneapi-src#848 Signed-off-by: Lukasz Dorau <[email protected]>

Alignment passed to `file_alloc()` must be a power of two and a multiple or a divider of the page size. Align up the alignment in `file_alloc()` to the page size. file_open_ipc_handle() has to use the memory protection and visibility got from the IPC handle, so let's add the memory protection and visibility to the IPC handle. Ref: oneapi-src#838 Fixes: oneapi-src#848 Signed-off-by: Lukasz Dorau <[email protected]>

ldorau requested a review from a team as a code owner October 24, 2024 20:45

This was referenced Oct 24, 2024

Fix devdax_open_ipc_handle() and devdax_close_ipc_handle() #834

Closed

Add internal IPC-consumer-only mode to the devdax provider #831

Closed

ldorau force-pushed the Fix_devdax_open_ipc_handle branch 2 times, most recently from 5ce9442 to 7b2a4ba Compare October 24, 2024 20:51

ldorau requested review from vinser52 and bratpiorka October 25, 2024 06:51

bratpiorka reviewed Oct 25, 2024

View reviewed changes

src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved

ldorau force-pushed the Fix_devdax_open_ipc_handle branch from 7b2a4ba to 45950bb Compare October 25, 2024 08:17

ldorau requested a review from bratpiorka October 25, 2024 08:18

bratpiorka approved these changes Oct 25, 2024

View reviewed changes

vinser52 requested changes Oct 25, 2024

View reviewed changes

ldorau force-pushed the Fix_devdax_open_ipc_handle branch 5 times, most recently from 18dee99 to a4f1c8b Compare October 28, 2024 09:39

ldorau requested a review from vinser52 October 28, 2024 09:39

ldorau mentioned this pull request Oct 28, 2024

Add IPC tests (umfIpcTest) to the devdax provider #845

Merged

3 tasks

ldorau force-pushed the Fix_devdax_open_ipc_handle branch from a4f1c8b to 3129474 Compare October 28, 2024 16:23

ldorau requested a review from pbalcer October 28, 2024 16:26

ldorau force-pushed the Fix_devdax_open_ipc_handle branch from 3129474 to a4f1c8b Compare October 28, 2024 16:56

ldorau mentioned this pull request Oct 29, 2024

IPC API of the devdax provider does not work (IPC tests segfault) #846

Closed

ldorau force-pushed the Fix_devdax_open_ipc_handle branch 7 times, most recently from e0262fc to cd6e0be Compare October 30, 2024 09:00

ldorau force-pushed the Fix_devdax_open_ipc_handle branch from 8c3735a to 903c467 Compare October 30, 2024 11:56

ldorau marked this pull request as ready for review October 30, 2024 11:57

ldorau marked this pull request as draft October 30, 2024 13:47

ldorau force-pushed the Fix_devdax_open_ipc_handle branch 2 times, most recently from 641be78 to ccd1784 Compare October 30, 2024 14:58

ldorau marked this pull request as ready for review October 30, 2024 14:59

vinser52 reviewed Oct 30, 2024

View reviewed changes

src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved

ldorau mentioned this pull request Oct 30, 2024

Align size of allocation to provider's page size in arena_extent_alloc() #853

Closed

3 tasks

ldorau force-pushed the Fix_devdax_open_ipc_handle branch from ccd1784 to 1a2ce63 Compare October 31, 2024 07:45

ldorau added 4 commits October 31, 2024 14:12

Rename utils_align_ptr_size() to utils_align_ptr_up_size_down()

2034c4e

Signed-off-by: Lukasz Dorau <[email protected]>

Add utils_align_ptr_down_size_up()

d182403

Add utils_align_ptr_down_size_up() to align a pointer down and a size up (for mmap()/munmap()). Signed-off-by: Lukasz Dorau <[email protected]>

The default page size for the devdax mode is 2 MB

454fcd9

Signed-off-by: Lukasz Dorau <[email protected]>

Fix handling arguments of devdax_alloc()

a677109

Fix handling arguments of devdax_alloc(): - alignment has to be a power of 2 and a divider or a multiple of the page size (2MB), Signed-off-by: Lukasz Dorau <[email protected]>

ldorau force-pushed the Fix_devdax_open_ipc_handle branch from 1a2ce63 to 52fd361 Compare October 31, 2024 13:15

ldorau force-pushed the Fix_devdax_open_ipc_handle branch from 52fd361 to 74b91bb Compare October 31, 2024 13:19

ldorau requested a review from vinser52 October 31, 2024 13:20

vinser52 reviewed Oct 31, 2024

View reviewed changes

vinser52 approved these changes Oct 31, 2024

View reviewed changes

vinser52 requested a review from bratpiorka October 31, 2024 13:45

bratpiorka approved these changes Oct 31, 2024

View reviewed changes

bratpiorka merged commit 77ef32a into oneapi-src:main Oct 31, 2024
74 checks passed

ldorau deleted the Fix_devdax_open_ipc_handle branch October 31, 2024 15:14

ldorau mentioned this pull request Nov 5, 2024

Fix file_alloc() and file_open/close_ipc_handle() #865

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix devdax_open_ipc_handle() and devdax_close_ipc_handle() #838

Fix devdax_open_ipc_handle() and devdax_close_ipc_handle() #838

Uh oh!

ldorau commented Oct 24, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

vinser52 Oct 25, 2024

Uh oh!

ldorau Oct 25, 2024 •

edited

Loading

Uh oh!

vinser52 Oct 25, 2024

Uh oh!

ldorau Oct 25, 2024 •

edited

Loading

Uh oh!

vinser52 Oct 25, 2024

Uh oh!

ldorau Oct 29, 2024

Uh oh!

vinser52 Oct 29, 2024

Uh oh!

ldorau Oct 30, 2024 •

edited

Loading

Uh oh!

ldorau Oct 30, 2024 •

edited

Loading

Uh oh!

ldorau Oct 31, 2024

Uh oh!

ldorau commented Oct 30, 2024

Uh oh!

Uh oh!

vinser52 Oct 31, 2024

Uh oh!

ldorau Oct 31, 2024

Uh oh!

Uh oh!

Uh oh!

Fix devdax_open_ipc_handle() and devdax_close_ipc_handle() #838

Fix devdax_open_ipc_handle() and devdax_close_ipc_handle() #838

Uh oh!

Conversation

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

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldorau commented Oct 30, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldorau commented Oct 24, 2024 •

edited

Loading

ldorau Oct 25, 2024 •

edited

Loading

ldorau Oct 25, 2024 •

edited

Loading

ldorau Oct 30, 2024 •

edited

Loading

ldorau Oct 30, 2024 •

edited

Loading