[Bug 45524] New: Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

Bug ID: 45524
           Summary: Add a futex-based implementation of condition
                    variables
           Product: Wine
           Version: 3.13
          Hardware: x86
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: ntdll
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

Created attachment 61910
  --> https://bugs.winehq.org/attachment.cgi?id=61910
[PATCH] ntdll: Add a futex-based condition variable implementation.

Titanfall 2 has reportedly high wineserver overhead, and this seems to help
with that.

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

--- Comment #1 from Dmitry Timoshkov  ---
Wouldn't it be more natural to move the futexes support to the underlying
ntdll implementations of NtWaitForKeyedEvent, etc?

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

--- Comment #2 from Zebediah Figura  ---
(In reply to Dmitry Timoshkov from comment #1)
> Wouldn't it be more natural to move the futexes support to the underlying
> ntdll implementations of NtWaitForKeyedEvent, etc?

I'm not sure that would work; I had assumed that keyed events didn't have to
use pointers to valid memory as a key. Is this not the case?

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

--- Comment #3 from Dmitry Timoshkov  ---
(In reply to Zebediah Figura from comment #2)
> > Wouldn't it be more natural to move the futexes support to the underlying
> > ntdll implementations of NtWaitForKeyedEvent, etc?
>
> I'm not sure that would work; I had assumed that keyed events didn't have to
> use pointers to valid memory as a key. Is this not the case?

I don't see how is this related, I'm just suggesting instead of introducing
new layer of code with futexes and still having separate ntdll fallback
implementation have everything in one place - ntdll.

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

--- Comment #4 from Dmitry Timoshkov  ---
(In reply to Dmitry Timoshkov from comment #3)
> (In reply to Zebediah Figura from comment #2)
> > > Wouldn't it be more natural to move the futexes support to the underlying
> > > ntdll implementations of NtWaitForKeyedEvent, etc?
> >
> > I'm not sure that would work; I had assumed that keyed events didn't have to
> > use pointers to valid memory as a key. Is this not the case?
>
> I don't see how is this related, I'm just suggesting instead of introducing
> new layer of code with futexes and still having separate ntdll fallback
> implementation have everything in one place - ntdll.

Sorry, should have read as: have everything in one place - ntdll conditional
variables APIs.

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

--- Comment #5 from Zebediah Figura  ---
I'm sorry; I'm not sure I understand. If you mean implementing keyed events on
top of futexes; I'm not sure that'll work since I don't think the key has to be
a valid pointer (whereas for futexes it absolutely must be). If your concern is
with the architecture of the patch, I just followed the model used in critical
sections.

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

--- Comment #6 from Dmitry Timoshkov  ---
(In reply to Zebediah Figura from comment #5)
> I'm sorry; I'm not sure I understand. If you mean implementing keyed events
> on top of futexes; I'm not sure that'll work since I don't think the key has
> to be a valid pointer (whereas for futexes it absolutely must be). If your
> concern is with the architecture of the patch, I just followed the model
> used in critical sections.

I don't understand your concern. For instance look at your new implementation

void WINAPI RtlWakeConditionVariable( RTL_CONDITION_VARIABLE *variable )
 {
     if (interlocked_dec_if_nonzero( (int *)&variable->Ptr ))
-        NtReleaseKeyedEvent( keyed_event, &variable->Ptr, FALSE, NULL );
+    {
+        NTSTATUS ret;
+        if ((ret = fast_wake( variable, 1 )) == STATUS_NOT_IMPLEMENTED)
+            NtReleaseKeyedEvent( keyed_event, &variable->Ptr, FALSE, NULL );
+    }
 }

Why not move fast_wake() call into the NtReleaseKeyedEvent() implementation?

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

Alistair Leslie-Hughes  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |STAGED
                 CC|                            |[e-mail]
    Staged patchset|                            |https://github.com/wine-sta
                   |                            |ging/wine-staging/tree/mast
                   |                            |er/patches/ntdll-futex-cond
                   |                            |ition-var

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

--- Comment #7 from Zebediah Figura  ---
(In reply to Dmitry Timoshkov from comment #6)
> Why not move fast_wake() call into the NtReleaseKeyedEvent() implementation?

If you're envisioning using the key as the pointer to the futex word, that
won't work. The key doesn't have to be a valid pointer; there are tests for
this in ntdll:om.

If you're envisioning using some internal field as the futex word (and then
copying most of the logic from the server to ntdll) I think that wouldn't work
since keyed events need to be cross-process.

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

Zebediah Figura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |be541f1b0cf2bec93b59d3a2fde
                   |                            |2b6c0593c011b
             Status|STAGED                      |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Zebediah Figura  ---
This is committed now,
https://source.winehq.org/git/wine.git/commit/be541f1b0cf2bec93b59d3a2fde2b6c0593c011b.

[Bug 45524] Add a futex-based implementation of condition variables

https://bugs.winehq.org/show_bug.cgi?id=45524

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #9 from Alexandre Julliard  ---
Closing bugs fixed in 4.2.