Skip to content

fix: use vm_allocate to allocate CPU backend buffer on macOS #9875

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 15 commits into from
Oct 16, 2024
Merged

fix: use vm_allocate to allocate CPU backend buffer on macOS #9875

Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
06b119a
fix: use `vm_allocate` to allocate CPU backend buffer on macOS
giladgd Oct 13, 2024
cec2d4e
fix: switch to `posix_memalign` to keep existing `free()` usages work
giladgd Oct 14, 2024
51ccdeb
feat: move `GGML_ALIGNED_MALLOC` to `ggml-backend-impl.h`, add suppor…
giladgd Oct 14, 2024
c2259e3
style: formatting
giladgd Oct 14, 2024
55298d2
fix: move const outside of `#ifndef`
giladgd Oct 14, 2024
fa79e0d
style: formatting
giladgd Oct 14, 2024
19a820f
fix: unused var
giladgd Oct 14, 2024
8352354
fix: transform `GGML_ALIGNED_MALLOC` and `GGML_ALIGNED_FREE` into fun…
giladgd Oct 14, 2024
df67ae3
fix: unused var
giladgd Oct 14, 2024
33b4308
fix: page align to `GGUF_DEFAULT_ALIGNMENT`
giladgd Oct 14, 2024
cb22464
fix: page align to `TENSOR_ALIGNMENT`
giladgd Oct 14, 2024
1516f7b
fix: convert `TENSOR_ALIGNMENT` to a macro
giladgd Oct 15, 2024
4455c7f
fix: increase page size to `32` on iOS
giladgd Oct 15, 2024
de7836a
fix: iOS page size
giladgd Oct 15, 2024
ecddb24
fix: `hbw_posix_memalign` alignment
giladgd Oct 15, 2024
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
11 changes: 11 additions & 0 deletions ggml/src/ggml-backend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#ifdef __APPLE__
#include <sys/types.h>
#include <sys/sysctl.h>
#include <unistd.h>
#include <TargetConditionals.h>
#endif


Expand Down Expand Up @@ -770,12 +772,21 @@ static const char * ggml_backend_cpu_buffer_type_get_name(ggml_backend_buffer_ty
}

static ggml_backend_buffer_t ggml_backend_cpu_buffer_type_alloc_buffer(ggml_backend_buffer_type_t buft, size_t size) {
#if defined(GGML_USE_METAL) || defined(TARGET_OS_OSX)
Copy link
Member

Choose a reason for hiding this comment

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

We cannot rely on backend macros like GGML_USE_METAL, and I don't see the point of enabling this specifically with Metal anyway, when the crash is happening in the CPU backend. Instead I would suggest using GGML_ALIGNED_MALLOC everywhere like the TODO in the comment suggests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main focus here is on TARGET_OS_OSX, but I figured that including GGML_USE_METAL covers more cases where posix_memalign is available and can be used instead of malloc.

I'll move GGML_ALIGNED_MALLOC to ggml-backend-impl.h and use it instead then.

Copy link
Member

Choose a reason for hiding this comment

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

It still needs some changes:

  • It should be in ggml-impl.h, not ggml-backend-impl.h
  • The implementation details need to be hidden in the source file, the header should only have the ggml_aligned_malloc and ggml_aligned_free functions, and the macros should be removed

I realize this is a lot more work than just fixing the bug, but we cannot add technical debt every time we fix a bug.

void * data = NULL;
int result = posix_memalign(&data, sysconf(_SC_PAGESIZE), size);
if (result != 0) {
GGML_LOG_ERROR("%s: failed to allocate buffer using posix_memalign with size %zu\n", __func__, size);
return NULL;
}
#else
size += TENSOR_ALIGNMENT; // malloc may return an address that is not aligned
void * data = malloc(size); // TODO: use GGML_ALIGNED_MALLOC (move to ggml-impl.h)
if (data == NULL) {
GGML_LOG_ERROR("%s: failed to allocate buffer of size %zu\n", __func__, size);
return NULL;
}
#endif

return ggml_backend_buffer_init(buft, ggml_backend_cpu_buffer_i, data, size);
}
Expand Down
Loading