metal : pair rsets_add with rsets_rm on buffer free (fix #22593)#22595
Open
joelteply wants to merge 1 commit into
Open
metal : pair rsets_add with rsets_rm on buffer free (fix #22593)#22595joelteply wants to merge 1 commit into
joelteply wants to merge 1 commit into
Conversation
github-actions
Bot
added
ggml
|
Hi @joelteply, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
ggml_metal_device_rsets_add was called from ggml_metal_buffer_rset_init without a matching ggml_metal_device_rsets_rm on the buffer's free path — so the device's rsets->data array always retained references that ggml_metal_rsets_free then asserted on at device-free time ([rsets->data count] == 0). Fires deterministically on macOS 15+ for any consumer that allocates buffers and then tears down the device. Add the symmetric remove call in ggml_metal_buffer_rset_free, before release. The rsets_rm API existed but was unused — this wires it up.
joelteply
force-pushed
the
metal-fix-rsets-leak-on-buffer-free
branch
from
5ee7cfc to
ae2c4a5
Compare
joelteply
changed the title
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #22593.
`ggml_metal_device_rsets_add` is called from `ggml_metal_buffer_rset_init` (line 1467) for every buffer when residency sets are active. The symmetric `ggml_metal_device_rsets_rm` API exists (line 911) but was defined and never called from anywhere in the tree — so the device's `rsets->data` array always retained references the per-buffer `rset_free` had just released. On `ggml_metal_device_free` → `ggml_metal_rsets_free` the assertion `[rsets->data count] == 0` fires deterministically.
Reproducible on macOS 15+ for any consumer that allocates buffers and tears down the device. Confirmed on Apple M5 Pro / macOS 15+ via a downstream consumer.
The fix is one line: pair the add with a remove in the buffer's `rset_free` path, before release. `buf->dev` is already populated by the time `rset_free` runs (`struct ggml_metal_buffer.dev`, line 1303) so no plumbing is needed. Build-tested locally on M5 Pro / macOS 15.
The residency-sets API was added in #17766; the remove-side wiring was missed in that PR. This commit closes that asymmetry.
Why not the env-var workaround
`GGML_METAL_NO_RESIDENCY=1` (line 775) bypasses the entire feature — but that throws away the keep-alive heartbeat #17766 was specifically optimizing for. On Metal-perf-critical consumers that's the wrong shape; this PR keeps the optimization and removes the assertion.