[PATCH v3 0/1] MR137: vkd3d: Leave the command queue op mutex locked after a partial flush.
All return paths in d3d12_command_queue_flush_ops_locked() must leave the op mutex locked. -- v3: vkd3d: Leave the command queue op mutex locked after a partial flush. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/137
From: Conor McCarthy <cmccarthy(a)codeweavers.com> All return paths in d3d12_command_queue_flush_ops_locked() must leave the op mutex locked. --- libs/vkd3d/command.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index e5ead7d3..08057128 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -6781,10 +6781,9 @@ static bool d3d12_command_queue_op_array_append(struct d3d12_command_queue_op_ar return true; } -static HRESULT d3d12_command_queue_fixup_after_flush(struct d3d12_command_queue *queue, unsigned int done_count) +static HRESULT d3d12_command_queue_fixup_and_lock_after_flush(struct d3d12_command_queue *queue, + unsigned int done_count) { - HRESULT hr; - queue->aux_op_queue.count -= done_count; memmove(queue->aux_op_queue.ops, &queue->aux_op_queue.ops[done_count], queue->aux_op_queue.count * sizeof(*queue->aux_op_queue.ops)); @@ -6798,11 +6797,7 @@ static HRESULT d3d12_command_queue_fixup_after_flush(struct d3d12_command_queue queue->aux_op_queue.count = 0; queue->is_flushing = false; - hr = d3d12_command_queue_record_as_blocked(queue); - - vkd3d_mutex_unlock(&queue->op_mutex); - - return hr; + return d3d12_command_queue_record_as_blocked(queue); } static HRESULT d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any) @@ -6855,7 +6850,7 @@ static HRESULT d3d12_command_queue_flush_ops_locked(struct d3d12_command_queue * if (op->u.wait.value > fence->max_pending_value) { vkd3d_mutex_unlock(&fence->mutex); - return d3d12_command_queue_fixup_after_flush(queue, i); + return d3d12_command_queue_fixup_and_lock_after_flush(queue, i); } d3d12_command_queue_wait_locked(queue, fence, op->u.wait.value); d3d12_fence_decref(fence); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/137
The patch looks correct, but the locking here also seems fragile and clumsy. Can we do better? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/137#note_28841
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/137
On Tue Apr 4 12:56:22 2023 +0000, Henri Verbeet wrote:
The patch looks correct, but the locking here also seems fragile and clumsy. Can we do better? I guess it depends a lot on what you consider fragile or clumsy. In principle `d3d12_command_queue_fixup_and_lock_after_flush()` could be inlined inside `d3d12_command_queue_flush_ops_locked()`, which would "rebalance" the locking and unlocking operations. The reason why I avoided that are because 1) for code readability, in order to avoid overloading the switch statement in `d3d12_command_queue_flush_ops_locked()`, and 2) to acknowledge that in principle there could be other ops than `OP_WAIT` that can block, and in that case it would be nice to factor the queue fixup together. I personally like the way the code is structured and I don't feel too hard about imbalanced lock/unlock operations, so the current organization suits me, but I will admit none of my reasons is currently too compelling.
Another option would be to move the locking outside of `d3d12_command_queue_fixup_and_lock_after_flush()`, which would mean that an additional `memmove()` would (uselessly) happen inside of the critical region. I doubt in practice much will change, but I'm not a fan of that. All in all, I don't currently see options what I would consider better, but that of course also depends on my tastes. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/137#note_28845
I guess it depends a lot on what you consider fragile or clumsy.
Well, to make it a bit more concrete, at least 3 people originally missed this issue in commit bb2fa97c33fd1591097741b809854763c0623697, and it took me a bit more time than I would've liked to review this MR. Arguably a good chunk of that is down to my shortcomings as a reviewer, but perhaps we could try to cater to those? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/137#note_28852
On Tue Apr 4 13:23:50 2023 +0000, Henri Verbeet wrote:
I guess it depends a lot on what you consider fragile or clumsy. Well, to make it a bit more concrete, at least 3 people originally missed this issue in commit bb2fa97c33fd1591097741b809854763c0623697, and it took me a bit more time than I would've liked to review this MR. Arguably a good chunk of that is down to my shortcomings as a reviewer, but perhaps we could try to cater to those? Sure, and for the sake of completeness I also take the share of responsibility due to my inability to invalidate assumptions across patch rewritings, but I am not sure of what you would consider better. Does any of the two alternatives I wrote above work better for you? Or would you propose something else?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/137#note_28860
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)