Skip to content
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

Add devdax memory provider #671

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Add devdax memory provider #671

merged 3 commits into from
Sep 13, 2024

Conversation

ldorau
Copy link
Contributor

@ldorau ldorau commented Aug 9, 2024 •
edited
Loading

Description

Add devdax memory provider.

Checklist

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

@ldorau ldorau force-pushed the Add_devdax_memory_provider branch 2 times, most recently from aa65331 to ac12c5c Compare August 12, 2024 09:32
@ldorau ldorau changed the title [DRAFT] Add devdax memory provider Add devdax memory provider Aug 12, 2024
@ldorau ldorau marked this pull request as ready for review August 12, 2024 09:33
@ldorau ldorau requested a review from a team as a code owner August 12, 2024 09:33
@ldorau ldorau mentioned this pull request Aug 12, 2024
Closed
kswiecicki
kswiecicki reviewed Aug 12, 2024
View reviewed changes
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
@lplewa
Copy link
Contributor

lplewa commented Aug 12, 2024

What is a design and use case of the dax provider? Only to help "mmap" it? As free is not supported?

lukaszstolarczuk
lukaszstolarczuk reviewed Aug 12, 2024
View reviewed changes
.github/workflows/devdax.yml Outdated Show resolved Hide resolved
.github/workflows/devdax.yml Outdated Show resolved Hide resolved
.github/workflows/devdax.yml Outdated Show resolved Hide resolved
.github/workflows/devdax.yml Outdated Show resolved Hide resolved
test/ipc_devdax_prov.sh Outdated Show resolved Hide resolved
test/provider_devdax_memory.cpp Outdated Show resolved Hide resolved
@ldorau
Copy link
Contributor Author

ldorau commented Aug 13, 2024

What is a design and use case of the dax provider? Only to help "mmap" it? As free is not supported?

To be able to provide memory from a /dev/dax (not extendable file).

kswiecicki
kswiecicki approved these changes Aug 14, 2024
View reviewed changes
test/provider_devdax_memory.cpp Show resolved Hide resolved
README.md Show resolved Hide resolved
@ldorau ldorau force-pushed the Add_devdax_memory_provider branch 4 times, most recently from 0982211 to 110329d Compare August 14, 2024 13:23
@ldorau
Copy link
Contributor Author

ldorau commented Sep 2, 2024

@lplewa please resolve your issues.

@ldorau
Copy link
Contributor Author

ldorau commented Sep 2, 2024

@bratpiorka please review

bratpiorka
bratpiorka requested changes Sep 3, 2024
View reviewed changes
.github/workflows/devdax.yml Outdated Show resolved Hide resolved
.github/workflows/devdax.yml Outdated Show resolved Hide resolved
.github/workflows/devdax.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
include/umf/providers/provider_devdax_memory.h Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
@ldorau ldorau force-pushed the Add_devdax_memory_provider branch 2 times, most recently from 15462b0 to ca35caa Compare September 4, 2024 06:39
lukaszstolarczuk
lukaszstolarczuk reviewed Sep 5, 2024
View reviewed changes
Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

it looks like one of the devdax test has failed

README.md Show resolved Hide resolved
@ldorau
Copy link
Contributor Author

ldorau commented Sep 5, 2024

it looks like one of the devdax test has failed

Fixed

lukaszstolarczuk
lukaszstolarczuk reviewed Sep 5, 2024
View reviewed changes
README.md Show resolved Hide resolved
test/provider_devdax_memory.cpp Show resolved Hide resolved
test/provider_devdax_memory.cpp Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
include/umf/providers/provider_devdax_memory.h Outdated Show resolved Hide resolved
@ldorau ldorau force-pushed the Add_devdax_memory_provider branch 2 times, most recently from cb2439e to 69bb1e6 Compare September 6, 2024 10:12
lplewa
lplewa requested changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@lplewa lplewa left a comment

Choose a reason for hiding this comment

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

If we are not supporting free, why we just do not mmap entire devdax at init? We can remove half of the code from this provider? mmap before first access do not cost us, so it should not be a problem.

test/provider_devdax_memory.cpp Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory_internal.h Outdated Show resolved Hide resolved
src/provider/provider_devdax_memory.c Outdated Show resolved Hide resolved
@ldorau ldorau force-pushed the Add_devdax_memory_provider branch 2 times, most recently from c218854 to 62b9cc7 Compare September 9, 2024 19:43
@ldorau
Copy link
Contributor Author

ldorau commented Sep 10, 2024

If we are not supporting free, why we just do not mmap entire devdax at init? We can remove half of the code from this provider? mmap before first access do not cost us, so it should not be a problem.

Because it is a memory provider that is supposed to handle many calls to alloc(size, alignment) ...

lplewa
lplewa reviewed Sep 10, 2024
View reviewed changes
src/provider/provider_devdax_memory.c Outdated
}

/* devdax requires UMF_MEM_MAP_SHARED */
result = os_translate_mem_visibility_flag(UMF_MEM_MAP_SHARED,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need dax/pmem mmap magic here? MAP_SYNC(and maybe MAP_SYNC_VALIDATE).

If yes, please add it, and second more important add test enuring that we are not going thru page cache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lplewa mapping with MAP_SHARED_VALIDATE | MAP_SYNC added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works:

32: [PID:167087 TID:167087 DEBUG UMF] os_devdax_mmap: devdax mapped with the (MAP_SHARED_VALIDATE | MAP_SYNC) flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test checking if devdax was mapped with the MAP_SYNC flag.

Done

@ldorau ldorau force-pushed the Add_devdax_memory_provider branch 3 times, most recently from f32e1ee to a24ba42 Compare September 11, 2024 08:46
@ldorau ldorau requested a review from lplewa September 11, 2024 08:56
lplewa
lplewa approved these changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@lplewa lplewa left a comment

Choose a reason for hiding this comment

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

There is refactor needed - we should not have dependencies between providers. It might be better, to merge this PR(and propably file provider too) and fix this problem in the separate PR as it might be not small change. So i conditionally approve this pr, so if you decide to do refactor in the separate PR, you can merge this one.

src/provider/provider_devdax_memory_internal.h
#ifndef UMF_DEVDAX_MEMORY_PROVIDER_INTERNAL_H
#define UMF_DEVDAX_MEMORY_PROVIDER_INTERNAL_H

#include <umf/providers/provider_os_memory.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not have dependencies between providers. If we need share code, we should move it to utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree, but it will be a huge change, so I will do that in a separate PR (I have already started to do that).

@bratpiorka bratpiorka merged commit 8717302 into oneapi-src:main Sep 13, 2024
70 checks passed
@ldorau ldorau deleted the Add_devdax_memory_provider branch September 13, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bratpiorka bratpiorka bratpiorka approved these changes

@lplewa lplewa lplewa approved these changes

@kswiecicki kswiecicki kswiecicki 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.
5 participants
@ldorau @lplewa @bratpiorka @lukaszstolarczuk @kswiecicki