mmap #29
mmap #29
Conversation
|
OK but if we got this "sys_map" can't we remove "dont_map" flag all code related to it? |
| @@ -96,6 +96,11 @@ void memtier_delete_memtier_memory(struct memtier_memory *memory); | |||
| /// | |||
| void *memtier_malloc(struct memtier_memory *memory, size_t size); | |||
|
|
|||
| // comment TODO | |||
There was a problem hiding this comment.
What does this TODO relate to?
src/memkind.c
Outdated
| @@ -72,6 +74,8 @@ extern struct memkind_ops MEMKIND_HBW_GBTLB_OPS; | |||
| extern struct memkind_ops MEMKIND_HBW_PREFERRED_GBTLB_OPS; | |||
| extern struct memkind_ops MEMKIND_GBTLB_OPS; | |||
|
|
|||
| thread_local bool dont_mmap; | |||
There was a problem hiding this comment.
Why isn't it static as well?
Importing using extern is ugly and might be problematic in the future. If the variable is only used inside this compilation unit, it should be static; if it's used elsewhere, a small function wrapper would be welcome.
"Static" is automatically assumed only for thread_local variables of a block scope.
src/memkind_memtier.c
Outdated
| @@ -44,6 +44,8 @@ | |||
| static atomic_size_t g_failed_adds_free=0; | |||
| #endif | |||
|
|
|||
| extern thread_local bool dont_mmap; | |||
There was a problem hiding this comment.
This kind of extern import usually leads to mess... Using explicit function wrapper inside an explicitly included file would be better. Btw, some coding standards (e.g. MISRA C) forbid this kind of in-place extern usage.
| @@ -284,6 +286,7 @@ memtier_policy_static_ratio_get_kind(struct memtier_memory *memory, | |||
| dest_tier = i; | |||
| } | |||
| } | |||
| //log_info("kind: %d", dest_tier); | |||
There was a problem hiding this comment.
Maybe we could remove this line? If not, put it under some #def?
| long ret = syscall(SYS_mmap, addr, length, prot, flags, fd, off); | ||
| if (ret == -EPERM && !off && (flags&MAP_ANON) && !(flags&MAP_FIXED)) | ||
| ret = -ENOMEM; | ||
| if (ret > -4096 && ret < 0) { |
There was a problem hiding this comment.
Why > - 4096 ? Please at least add a comment about an origin of this magick number...
src/memkind_memtier.c
Outdated
| return -1; | ||
| } | ||
|
|
||
| #include <pthread.h> |
There was a problem hiding this comment.
Why isn't this include at the top of the file?
src/memkind_memtier.c
Outdated
| } | ||
|
|
||
| #include <pthread.h> | ||
| pthread_mutex_t mutex; |
There was a problem hiding this comment.
Not static... Is it ever used as "extern"? if yes, this is probably not the best way to use a mutex, I've seen horrible things happen because of such approach - ugly code, synchronization errors and deadlocks, among others... It would be best to:
- Describe what this mutex protects exactly, preferrably by giving it self-explanatory name
- Keep the data it protects (if it's about data access synchronisation) as close to it as possible - preferrably in the same structure
| dont_mmap = true; | ||
|
|
||
| pthread_mutex_lock(&mutex); | ||
| void* ret = memtier_malloc(memory, length); |
There was a problem hiding this comment.
Isn't malloc supposed to be thread safe?
We only use variables specified as arguments here, do we use the mutex to protect them?
There was a problem hiding this comment.
This does not support splittings big mmaps into multiple smaller remaps on different regions... I thought this was supposed to be the key feature
| #include <stdatomic.h> | ||
| #define MEMKIND_ATOMIC _Atomic | ||
| #else | ||
| #define MEMKIND_ATOMIC |
There was a problem hiding this comment.
Is it really how we want to handle atomics in this case?
tiering/memtier.c
Outdated
| @@ -131,6 +163,12 @@ MEMTIER_EXPORT int posix_memalign(void **memptr, size_t alignment, size_t size) | |||
|
|
|||
| MEMTIER_EXPORT void free(void *ptr) | |||
| { | |||
| // TODO!!! | |||
There was a problem hiding this comment.
This function is not implemented at all... Would be best to add some log, TODOs might be overlooked
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @kilobyte)
There was a problem hiding this comment.
Reviewed 7 of 9 files at r3, all commit messages.
Reviewable status: 8 of 10 files reviewed, 12 unresolved discussions (waiting on @bratpiorka and @kilobyte)
src/memkind_memtier.c, line 1238 at r3 (raw file):
while (todo > 0) { size_t len = (todo > SLICE) ? SLICE : todo; int sl = sel++ % (tier1_ratio + tier2_ratio); /* non-atomic inc, doesn't hurt */
please use memory->get_kind(memory, size) here
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @kilobyte)
src/memkind_memtier.c, line 1247 at r4 (raw file):
mbind(sladdr, len, MPOL_PREFERRED, &nodemask, sizeof(nodemask)*8, 0); } else fprintf(stderr, "Kind without nodemask\n");
shouldn't we fail whole mmap() here?
tiering/memtier.c, line 180 at r4 (raw file):
/* MEMTIER_EXPORT int munmap(void *addr, size_t length)
why this is defined in memkind_memtier.c and not here? why we export mmap() and don't munmap()?
This change is