Fixes #1556 double free #3347
Merged
Fixes #1556 double free #3347
Conversation
This file contains 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
|
If you run cc @WoosukKwon as you responded to the original issue. |
|
@hmellor, done. |
cadedaniel
approved these changes
Mar 12, 2024
Comment on lines
+396
to
+398
| blocks_to_free = (block_table[-self.block_sliding_window:] | ||
| if self.block_sliding_window is not None else | ||
| block_table) |
There was a problem hiding this comment.
Can we add a comment for this line?
| @@ -312,7 +312,7 @@ def fork(self, parent_seq: Sequence, child_seq: Sequence) -> None: | |||
| # Thus, it is always safe from OOM. | |||
| src_block_table = self.block_tables[parent_seq.seq_id] | |||
| self.block_tables[child_seq.seq_id] = src_block_table.copy() | |||
| for block in src_block_table: | |||
| for block in set(src_block_table): | |||
There was a problem hiding this comment.
Can we add comment why we need to deduplicate?
| @@ -274,3 +274,48 @@ def test_reset(): | |||
| # Resetting block manager frees all allocated blocks. | |||
| block_manager.reset() | |||
| assert block_manager.get_num_free_gpu_blocks() == original_blocks | |||
|
|
|||
|
|
|||
| def test_sliding_window_multi_seq(): | |||
There was a problem hiding this comment.
Can we add a small docstring explaining what runtime property this test verifies and the basic methodology?
|
@cadedaniel here you go! Let me know if there's anything else missing. |
|
Just waiting for CI to finish 👍 |
Yard1
approved these changes
Mar 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes issue #1556.
Conditions for the bug to appear:
The double free error occurs when there are multiple sequences sharing blocks and the context window exceeds the model's sliding-window.
The test function in this PR will fail on the
block_manager.pyin main with the "double free" error mentioned in #1556.What is the underlying cause of the bug?
The reason this happens is that in
append_slot, when a new block needs to be allocated because an existing block cannot be reused (because it is referenced by other sequences), its ref count is decremented for the current sequence: https://github.com/vllm-project/vllm/blob/main/vllm/core/block_manager.py#L307This block is thus considered not to be referenced anymore by this sequence.
Later, when
_free_block_tableis called for the block table of this sequence, vllm tries to free this block once again: https://github.com/vllm-project/vllm/blob/main/vllm/core/block_manager.py#L396, leading to the error.What is the fix?
There are two small parts:
ref_countof shared blocks was being wrongly incremented onforkbecause all blocks in the block table were being incremented. This is wrong because if the context exceeds the sliding window, blocks in the block tables will be reused. The fix makes sure to only increment theref_countby 1, by wrapping the table inset()._free_block_tableshould not free already freed blocks. To this end, the method was changed to just free the lastsliding_windowblocks (if defined).