On Sat Jan 25 04:28:32 2025 +0000, William Horvath wrote:
About flakiness: I could up the iteration count for yields (0 timeout waits) drastically without it increasing the total test time much. In practice, I've never seen this check fail on my fast machine under no load, and it's probably even less likely to fail on the comparatively sluggish test VM, but it wouldn't hurt. About the yield after the server call: we only yield if we timed out, which I guess is what's supposed to emulate the "if it's hit by an event then it gets a priority boost" behavior, as "hit by an event" means the we didn't get `STATUS_TIMEOUT`. It's a bit backwards in that we're instead giving the other events a priority **penalty**, but I think that's the logic.
Your patch doesn't change whether NtYieldExecution is called or not, and thus has no chance to affect any host "priority boost". I actually doubt that it works this way now, giving any additional "boost" after we already waited for server call, but this is a different story unrelated to this patch. And especially that NtYieldExecution worth calling after non-zero timeout; if the thread slept for some time it did perform the yield, no way around. in a much more sure way than what NtYieldExecution does which is de-facto no-op most often. But even with zero timeout server call did a blocking wait.
_NO_YIELD_PERFORMED only reflects reality when there were no blocking native system calls on the way of NtDelayExecution(). Basing this status on the sole return of NtYieldExecution in server_wait() on that path is plainly wrong, it doesn't take into account yield performed during server call. So returning _NO_YIELD_PERFORMED based on that addition NtYieldExecution may only confuse the app, hinting it to wait additionally. As a bonus, ignoring that from server_wait() reduces the functional part of the patch to 2-3 lines.