mmdevapi: Avoid lock contention after SetEvent.

Hi,

what's up with this patch?

I wrote:
> SetEvent is one of those calls with a high probability that the
> Linux scheduler will switch to another thread, e.g. the one receiving
> the event.  As the critical section is still hold, it'll prevent the
> receiver from invoking mmdevapi, e.g. GetCurrentPadding, without two
> more thread switches (forth and back).  We don't need that.

Now that This->event is constant between IAC_Start and IAC_Release,
thanks to my "SetEventHandle is allowed once only" patch,
it's safe to move SetEvent(This->event) outside of the CS.

Unlike the rejected patch about a CreateTimerQueue race condition
internal to Wine (and only relevant in lock-less mode),
this one is between the driver and the client app that blocks when calling e.g.
GetCurrentPadding, what almost all apps will do, including our DSound
http://source.winehq.org/source/dlls/dsound/mixer.c#L679

Regards,
        Jörg Höhle

[Andrew Eikum]

On Thu, Dec 20, 2012 at 02:03:06PM +0100, [e-mail] wrote:
> what's up with this patch?
>
> I wrote:
> > SetEvent is one of those calls with a high probability that the
> > Linux scheduler will switch to another thread, e.g. the one receiving
> > the event.  As the critical section is still hold, it'll prevent the
> > receiver from invoking mmdevapi, e.g. GetCurrentPadding, without two
> > more thread switches (forth and back).  We don't need that.
>
> Now that This->event is constant between IAC_Start and IAC_Release,
> thanks to my "SetEventHandle is allowed once only" patch,
> it's safe to move SetEvent(This->event) outside of the CS.
>
> Unlike the rejected patch about a CreateTimerQueue race condition
> internal to Wine (and only relevant in lock-less mode),
> this one is between the driver and the client app that blocks when calling e.g.
> GetCurrentPadding, what almost all apps will do, including our DSound
> http://source.winehq.org/source/dlls/dsound/mixer.c#L679

I don't have any problems with this patch, but I don't know our
synchronization methods well enough to know if this is correct. Is it
true that SetEvent() causes a context switch? I see it eventually
resolves down to a write() in <server/thread.c:send_thread_wakeup()>,
but I don't know what that implies.

Julliard, any thoughts?

Andrew

Andrew Eikum asked:
> Is it true that SetEvent() causes a context switch?

Looking at +tid log files, I've seen switches happen after SetEvent
often enough.  That motivated some changes to winmm notification in
the last years, i.e. call SetEvent after all local work is finished
and objects are in a sane state again.

Moreover, the semantics of SetEvent is that it puts any thread waiting
for the event into runnable state.  If it's runnable, the OS may as
well decide to run it.  And that's exactly what happens.

On a multi-core system, the signaled thread may start running
immediately on another core.  You don't want it to stop shortly later,
blocking on wineXYZ*.drv's sole critical section when invoking mmdevapi.

LeaveCriticalSection is another place where context switches happen,
both according to log files and by design (see RtlUnWaitCriticalSection).
So could moving SetEvent after LeaveCS disturb the dynamic behaviour
of winealsa.drv?  I argue that there's no harm, because mmdevapi's
design is such that the app has nearly one period time after receiving
the event to supply new samples.  Thus if an app manages to sneak in
a call unblocked by LeaveCS, e.g. GetPosition, before SetEvent, there
should still be enough time left afterwards.

In any case, lock contention by an app calling GetPosition during the
period callback is presumably rare, while lock contention by SetEvent
vs. GetCurrentPadding in the signaled thread, e.g. DSound is
systematic.  So let's avoid it.

(Please don't skip that patch waiting for the lock-less one which will
render it superfluous...).

Regards,
    Jörg Höhle