[PATCH v4 0/28] MR8955: ntdll: NtSetEventBoostPriority stub regression fixes for bug #58688
NtSetEventBoostPriority stub regression fixes for: https://bugs.winehq.org/show_bug.cgi?id=58688 Regressive commit: ed9f31120b68e7d684c1544c05d94c38b25cb759 Other stubs were also changed by regress commit and may still be broken. I'll test the other stubs and fix if necessary. In the meantime I'm submitting this merge for review in case I missed anything that may be needed for the other stub fixes. -- v4: ntdll: ZwSetEventBoostPriority typo correction for bug #58688 This merge request has too many patches to be relayed via email. Please visit the URL below to see the contents of the merge request. https://gitlab.winehq.org/wine/wine/-/merge_requests/8955
On Mon Sep 15 09:23:59 2025 +0000, Elizabeth Figura wrote:
It's not clear, but I think this should actually set the event (i.e. it's "set while boosting priority", not "set the event's boost priority"). Tests would be helpful to confirm or refute this. Indeed in my testing this is actually doing the former ("set while boosting priority"), but the actual priority boost happening seems to be negligible.
Maybe this also depends on the priority of the caller. My assumption would be that the idea here is to make a waiting thread on the event immediately runnable, so maybe a `NtSetEvent` + `NtYieldExecution` could also be potentially appropriate. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115850
On Mon Sep 15 09:23:59 2025 +0000, Marc-Aurel Zent wrote:
Indeed in my testing this is actually doing the former ("set while boosting priority"), but the actual priority boost happening seems to be negligible. Maybe this also depends on the priority of the caller. My assumption would be that the idea here is to make a waiting thread on the event immediately runnable, so maybe a `NtSetEvent` + `NtYieldExecution` could also be potentially appropriate. Yes, to be clear I don't think the "priority boost" behaviour is going to be testable, let alone implementable, and probably it doesn't even deserve a FIXME.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115880
On Mon Sep 15 17:01:50 2025 +0000, Elizabeth Figura wrote:
Yes, to be clear I don't think the "priority boost" behaviour is going to be testable, let alone implementable, and probably it doesn't even deserve a FIXME. Based on finding no other examples in the wild, I agree this issue probably doesn't even deserve a FIXME.
Without any tests as the basis for implementation, it's practically a black hole. I'm open to closing this merge request as unnecessary. I just quickly stubbed before researching so in hindsight I probably wouldn't have even bothered. If Xenia-Canary team can provide use cases and examples then it may be worth some more effort but as of right now the project seems very experimental and lacking of documentation details needed to proceed. Say the word and I'll close this merge request. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115888
On Mon Sep 15 17:56:53 2025 +0000, Stian Low wrote:
Based on finding no other examples in the wild, I agree this issue probably doesn't even deserve a FIXME. Without any tests as the basis for implementation, it's practically a black hole. I'm open to closing this merge request as unnecessary. I just quickly stubbed before researching so in hindsight I probably wouldn't have even bothered. If Xenia-Canary team can provide use cases and examples then it may be worth some more effort but as of right now the project seems very experimental and lacking of documentation details needed to proceed. Say the word and I'll close this merge request. I don't see why it would need to be closed. If an application needs it then it's valid. Rarity isn't generally a criteria for inclusion, only for priority.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115889
On Mon Sep 15 18:08:45 2025 +0000, Elizabeth Figura wrote:
I don't see why it would need to be closed. If an application needs it then it's valid. Rarity isn't generally a criteria for inclusion, only for priority. Very well. Leaving open for now.
I'll reach out to Xenia Canary for a NtSetEventBoostPriority use case. If they can provide an example of a game that benefits from it vs without then it may provide some leads. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115890
On Mon Sep 15 18:13:41 2025 +0000, Stian Low wrote:
Very well. Leaving open for now. I'll reach out to Xenia Canary for a NtSetEventBoostPriority use case. If they can provide an example of a game that benefits from it vs without then it may provide some leads. I don't really see why that's necessary either? It seems to me we should just add a quick test that it behaves like NtSetEvent(), and call NtSetEvent() from it if so.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115894
On Mon Sep 15 18:31:54 2025 +0000, Elizabeth Figura wrote:
I don't really see why that's necessary either? It seems to me we should just add a quick test that it behaves like NtSetEvent(), and call NtSetEvent() from it if so. Thanks for reviewing, guidance, and bearing with my confusion while deep dive these parts of WINE for the first time.
I have another pending patch that calls NetSetEvent from NtSetEventBoostPriority but the condition under which NetSetEvent should be called is still unclear to me. Looking deeper at Xenia Canary SetBoostPriority() calls: https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe...: `// SetEvent, but if there is a waiter we immediately transfer execution to it` This comment seems inline with @mzent assumptions that threads awaiting the event are made immediately runnable. APU audio decoding and GPU write pointer updates seem to be the two major use cases by Xenia Canary: https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe... ``` if (r >= XmaRegister::Context0Kick && r <= XmaRegister::Context9Kick) { // Context kick command. // This will kick off the given hardware contexts. // Basically, this kicks the SPU and says "hey, decode that audio!" // XMAEnableContext work_event_->SetBoostPriority(); ``` https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe... ``` void CommandProcessor::UpdateWritePointer(uint32_t value) { XE_UNLIKELY_IF(cvars::log_ringbuffer_kickoff_initiator_bts) { LogKickoffInitator(value); } write_ptr_index_ = value; write_ptr_index_event_->SetBoostPriority(); } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115983
On Tue Sep 16 16:44:24 2025 +0000, Stian Low wrote:
Thanks for reviewing, guidance, and bearing with my confusion while deep dive these parts of WINE for the first time. I have another pending patch that calls NetSetEvent from NtSetEventBoostPriority but the condition under which NetSetEvent should be called is still unclear to me. Looking deeper at Xenia Canary SetBoostPriority() calls: https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe...: `// SetEvent, but if there is a waiter we immediately transfer execution to it` This comment seems inline with @mzent assumptions that threads awaiting the event are made immediately runnable. APU audio decoding and GPU write pointer updates seem to be the two major use cases by Xenia Canary: https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe... ``` if (r >= XmaRegister::Context0Kick && r <= XmaRegister::Context9Kick) { // Context kick command. // This will kick off the given hardware contexts. // Basically, this kicks the SPU and says "hey, decode that audio!" // XMAEnableContext work_event_->SetBoostPriority(); ```
https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe... ``` void CommandProcessor::UpdateWritePointer(uint32_t value) { XE_UNLIKELY_IF(cvars::log_ringbuffer_kickoff_initiator_bts) { LogKickoffInitator(value); } write_ptr_index_ = value; write_ptr_index_event_->SetBoostPriority(); } ``` Also, this is my merge request and it seems I created kind of a mess of it.
I realize my commits should have been to a dedicated branch instead of master. Worse, I pulled commits by others to resolve Gitlab pipeline errors. In hindsight that seems unnecessary and not advised? Should I start over with a cleaner merge request once a solution ready? Forgive me while I spin myself up on typical WineHQ Gitlab workflows. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_115984
I have another pending patch that calls NetSetEvent from NtSetEventBoostPriority but the condition under which NetSetEvent should be called is still unclear to me.
Unconditionally, by my understanding.
Looking deeper at Xenia Canary SetBoostPriority() calls:
https://github.com/xenia-canary/xenia-canary/blob/canary_experimental/src/xe...:
`// SetEvent, but if there is a waiter we immediately transfer execution to it`
This comment seems inline with @mzent assumptions that threads awaiting the event are made immediately runnable.
From a user space perspective, that's how NtSetEvent behaves too. If there's a difference it's in some slight nuances of how threads are prioritized by the scheduler, and that behaviour doesn't necessarily match between Windows and the host system, nor is it probably configurable.
I realize my commits should have been to a dedicated branch instead of master.
There's nothing wrong with using master on your local fork. It's not special.
Worse, I pulled commits by others to resolve Gitlab pipeline errors.
In hindsight that seems unnecessary and not advised?
It's not necessary in general.
Should I start over with a cleaner merge request once a solution ready?
No need for that; in fact it's better to avoid it, and keep this merge request in one place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8955#note_116003
participants (3)
-
Elizabeth Figura (＠zfigura) -
Marc-Aurel Zent (＠mzent)
-
Stian Low (＠stianlow)