Skip to content

Add lock for updating file size in OS memory provider #622

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

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Add lock for updating file size in OS memory provider #622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions src/provider/provider_os_memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,14 @@ static umf_result_t os_initialize(void *params, void **provider) {
goto err_destroy_bitmaps;
}

if (os_provider->fd > 0) {
if (util_mutex_init(&os_provider->lock_fd) == NULL) {
LOG_ERR("initializing the file size lock failed");
ret = UMF_RESULT_ERROR_UNKNOWN;
goto err_destroy_bitmaps;
}
}

os_provider->nodeset_str_buf = umf_ba_global_alloc(NODESET_STR_BUF_LEN);
if (!os_provider->nodeset_str_buf) {
LOG_INFO("allocating memory for printing NUMA nodes failed");
Expand Down Expand Up @@ -562,6 +570,10 @@ static void os_finalize(void *provider) {

os_memory_provider_t *os_provider = provider;

if (os_provider->fd > 0) {
util_mutex_destroy_not_free(&os_provider->lock_fd);
}

critnib_delete(os_provider->fd_offset_map);

free_bitmaps(os_provider);
Expand Down Expand Up @@ -624,8 +636,8 @@ static inline void assert_is_page_aligned(uintptr_t ptr, size_t page_size) {

static int os_mmap_aligned(void *hint_addr, size_t length, size_t alignment,
size_t page_size, int prot, int flag, int fd,
size_t max_fd_size, void **out_addr,
size_t *fd_size) {
size_t max_fd_size, os_mutex_t *lock_fd,
void **out_addr, size_t *fd_size) {
assert(out_addr);

size_t extended_length = length;
Expand All @@ -641,13 +653,20 @@ static int os_mmap_aligned(void *hint_addr, size_t length, size_t alignment,
size_t fd_offset = 0;

if (fd > 0) {
if (util_mutex_lock(lock_fd)) {
LOG_ERR("locking file size failed");
return -1;
}

if (*fd_size + extended_length > max_fd_size) {
util_mutex_unlock(lock_fd);
LOG_ERR("cannot grow a file size beyond %zu", max_fd_size);
return -1;
}

fd_offset = *fd_size;
*fd_size += extended_length;
util_mutex_unlock(lock_fd);
Comment on lines +656 to +669
Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like this? It probably should have better performance, and we don't need extra locks.
I'm not very proficient with atomics, so it would be good if @pbalcer or @igchor check if my proposal is valid .

Suggested change
if (util_mutex_lock(lock_fd)) {
LOG_ERR("locking file size failed");
return -1;
}
if (*fd_size + extended_length > max_fd_size) {
util_mutex_unlock(lock_fd);
LOG_ERR("cannot grow a file size beyond %zu", max_fd_size);
return -1;
}
fd_offset = *fd_size;
*fd_size += extended_length;
util_mutex_unlock(lock_fd);
do {
size_t size = *fd_size;
if (size + extended_length > max_fd_size) {
LOG_ERR("cannot grow a file size beyond %zu", max_fd_size);
return -1;
}
fd_offset = *fd_size;
} while(!__sync_bool_compare_and_swap(fd_size, size, size + extended_lenght));

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 does not work - another thread can update fd_size with exactly the same value ...

Copy link
Contributor

@lplewa lplewa Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It should not be a problem - as compare_and_swap checks for old value, and them updates it to new value. So even if one thread updates this value, second thread will detect it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it should work.
But how would you do that on Windows? I do not know any equivalent of __sync_bool_compare_and_swap on Windows ...

Copy link
Contributor

Choose a reason for hiding this comment

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

All atomic functions are available on windows - just they have stupid names.
https://learn.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange64

Copy link
Contributor

Choose a reason for hiding this comment

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

my 2 cents: since this isn't perf critical, I'd just leave it as is with the mutex. Atomics can be tricky to get right and make things more difficult to modify.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Piotr - since this is not the allocation path we should use the simplest solution

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Piotr - since this is not the allocation path we should use the simplest solution

This is an allocation path

}

void *ptr = os_mmap(hint_addr, extended_length, prot, flag, fd, fd_offset);
Expand Down Expand Up @@ -899,11 +918,11 @@ static umf_result_t os_alloc(void *provider, size_t size, size_t alignment,
errno = 0;
ret = os_mmap_aligned(NULL, size, alignment, page_size,
os_provider->protection, os_provider->visibility,
os_provider->fd, os_provider->max_size_fd, &addr,
&os_provider->size_fd);
os_provider->fd, os_provider->max_size_fd,
&os_provider->lock_fd, &addr, &os_provider->size_fd);
if (ret) {
os_store_last_native_error(UMF_OS_RESULT_ERROR_ALLOC_FAILED, errno);
LOG_PERR("memory allocation failed");
os_store_last_native_error(UMF_OS_RESULT_ERROR_ALLOC_FAILED, 0);
LOG_ERR("memory allocation failed");
return UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;
}

Expand Down
3 changes: 3 additions & 0 deletions src/provider/provider_os_memory_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "critnib.h"
#include "umf_hwloc.h"
#include "utils_common.h"
#include "utils_concurrency.h"

#ifdef __cplusplus
extern "C" {
Expand All @@ -33,6 +34,8 @@ typedef struct os_memory_provider_t {
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
os_mutex_t lock_fd; // lock for updating file size

// A critnib map storing (ptr, fd_offset + 1) pairs. We add 1 to fd_offset
// in order to be able to store fd_offset equal 0, because
// critnib_get() returns value or NULL, so a value cannot equal 0.
Expand Down
Loading