[PATCH 10/25] mciseq: Limit concurrency when starting to play.

[Joerg-Cyril.Hoehle at t-systems.com]

Alexandre Julliard wrote:

>This makes no sense, you can't make assumptions about the generated code
>from the shape of the if statements.
Don't || and && impose an evaluation order?

> Also there's no such thing as
>"conceptually volatile", unless you add explicit memory barriers.
Forget about "conceptually volatile".  I meant to express the idea
that some variable may be updated from another thread.  This has
nothing to do with the C keyword of that name.

Once again I read some articles about memory barriers and fail to see
what's wrong with the final code (after patch 21/25):

	if (wmm->dwStatus == MCI_MODE_STOP || wmm->dwStatus == MCI_MODE_NOT_READY)
	    break;
	else if (wmm->dwStatus != MCI_MODE_PLAY || recheck)
	    continue;

0. dwStatus is aligned within HeapAlloc'ed WINE_MCIMIDI,
   so we only consider atomic reads and writes.

1. Every writer of wmm->dwStatus is already within a critical section
   (after patch 11/25).

2. I don't want the player to block in a CS.  All it needs is to poll dwStatus.

3. The player need not react immediately to status changes.
   Nothing in it depends on reacting ASAP to status changes.
   Say "stop" to children, and they'll stop eventually, but not immediately. :-)
   IOW, I don't mind if it reads a stale status value.  It may stop
   after playing another note at the next iteration.

The last sentence sounds a bit strange.  If every thread/core had 1GB local
storage, would the player never see the status update by another core?

I expect memory writes to eventually propagate to all cores.  Memory
barriers are all about ordering reads and writes.  Given dwStatus is a single
memory location, do we care about ordering? (SetEvent doesn't matter)

Changing the code to:
  if (InterlockedCompareExchange(&wmm->dwStatus, PLAY, PLAY) != PAUSE &&
      InterlockedCompareExchange(&wmm->dwStatus, PLAY, PLAY) != PLAY) break;
would be as broken as:
  if (wmm->dwStatus != PAUSE && wmm->dwStatus != PLAY) break;


I propose possible rewrites that would make it clear that dwStatus is updated
asynchronously.  Wine lacks the MemoryBarrier() macro that MSDN documents,
however InterlockedCompareExchange can be used as a no-op.  At the cost of
a goto, since a break within a while can't abort the while:

A switch (InterlockedCompareExchange(&wmm->dwStatus, PLAY, PLAY)) {
    case MODE_PLAY:           break;    
    case MODE_PAUSE:          continue;
    default:                  goto finish;
    }
    if (recheck)              continue;

or
B  InterlockedExchange(&status, wmm->dwStatus);
    if (status == MODE_PAUSE) continue;
    if (status != MODE_PLAY)  break;
    if (recheck)              continue;

but how is that different from
C  status = wmm->dwStatus;
    if (status == MODE_PAUSE) continue;
    if (status != MODE_PLAY)  break;
    if (recheck)              continue;
given point 3. i.e. I don't care whether the player aborts now or at the next round.

Would one of these solve the issue in your opinion?

To make progress meanwhile, you may consider comitting
the patches 13/14/16 (MCI_STATUS/SEEK/INFO).

Regards,
	Jörg Höhle