Re: [PATCH v4 02/15] hw/display/apple-gfx: Introduce ParavirtualizedGraphics.Framework support

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/10/28 23:13, Phil Dennis-Jordan wrote:
> 
> 
> On Mon, 28 Oct 2024 at 15:02, Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2024/10/28 22:31, Phil Dennis-Jordan wrote:
>      >
>      >
>      > On Mon, 28 Oct 2024 at 10:00, Phil Dennis-Jordan
>     <phil@philjordan.eu <mailto:phil@philjordan.eu>
>      > <mailto:phil@philjordan.eu <mailto:phil@philjordan.eu>>> wrote:
>      >
>      >
>      >          >      >
>      >          >      > Hmm. I think if we were to use that, we would
>     need to
>      >         create a new
>      >          >      > QemuEvent for every job and destroy it afterward,
>      >         which seems
>      >          >     expensive.
>      >          >      > We can't rule out multiple concurrent jobs being
>      >         submitted, and the
>      >          >      > QemuEvent system only supports a single producer as
>      >         far as I can
>      >          >     tell.
>      >          >      >
>      >          >      > You can probably sort of hack around it with
>     just one
>      >         QemuEvent by
>      >          >      > putting the qemu_event_wait into a loop and turning
>      >         the job.done
>      >          >     flag
>      >          >      > into an atomic (because it would now need to be
>      >         checked outside the
>      >          >      > lock) but this all seems unnecessarily complicated
>      >         considering the
>      >          >      > QemuEvent uses the same mechanism QemuCond/
>     QemuMutex
>      >         internally
>      >          >     on macOS
>      >          >      > (the only platform relevant here), except we
>     can use it as
>      >          >     intended with
>      >          >      > QemuCond/QemuMutex rather than having to work
>     against the
>      >          >     abstraction.
>      >          >
>      >          >     I don't think it's going to be used concurrently. It
>      >         would be difficult
>      >          >     to reason even for the framework if it performs memory
>      >          >     unmapping/mapping/reading operations concurrently.
>      >          >
>      >          >
>      >          > I've just performed a very quick test by wrapping the job
>      >         submission/
>      >          > wait in the 2 mapMemory callbacks and the 1 readMemory
>      >         callback with
>      >          > atomic counters and logging whenever a counter went
>     above 1.
>      >          >
>      >          >   * Overall, concurrent callbacks across all types were
>      >         common (many per
>      >          > second when the VM is busy). It's not exactly a
>     "thundering
>      >         herd" (I
>      >          > never saw >2) but it's probably not a bad idea to use
>     a separate
>      >          > condition variable for each job type. (task map,
>     surface map,
>      >         memory read)
>      >          >   * While I did not observe any concurrent memory mapping
>      >         operations
>      >          > *within* a type of memory map (2 task mappings or 2
>     surface
>      >         mappings) I
>      >          > did see very occasional concurrent memory *read*
>     callbacks.
>      >         These would,
>      >          > as far as I can tell, not be safe with QemuEvents,
>     unless we
>      >         placed the
>      >          > event inside the job struct and init/destroyed it on every
>      >         callback
>      >          > (which seems like excessive overhead).
>      >
>      >         I think we can tolerate that overhead. init/destroy
>     essentially
>      >         sets the
>      >         fields in the data structure and I estimate its total size is
>      >         about 100
>      >         bytes. It is probably better than waking an irrelevant thread
>      >         up. I also
>      >         hope that keeps the code simple; it's not worthwhile
>     adding code to
>      >         optimize this.
>      >
>      >
>      >     At least pthread_cond_{init,destroy} and
>      >     pthread_mutex_{init,destroy} don't make any syscalls, so yeah
>     it's
>      >     probably an acceptable overhead.
>      >
>      >
>      > I've just experimented with QemuEvents created on-demand and ran
>     into
>      > some weird deadlocks, which then made me sit down and think about it
>      > some more. I've come to the conclusion that creating (and crucially,
>      > destroying) QemuEvents on demand in this way is not safe.
>      >
>      > Specifically, you must not call qemu_event_destroy() - which
>      > transitively destroys the mutex and condition variable - unless
>     you can
>      > guarantee that the qemu_event_set() call on that event object has
>     completed.
>      >
>      > In qemu_event_set, the event object's value is atomically set to
>     EV_SET.
>      > If the previous value was EV_BUSY, qemu_futex_wake() is called.
>     All of
>      > this is outside any mutex, however, so apart from memory coherence
>      > (there are barriers) this can race with the waiting thread.
>      > qemu_event_wait() reads the event's value. If EV_FREE, it's
>     atomically
>      > set to EV_BUSY. Then the mutex is locked, the value is checked
>     again,
>      > and if it's still EV_BUSY, it waits for the condition variable,
>      > otherwise the mutex is immediately unlocked again. If the trigger
>      > thread's qemu_event_set() flip to EV_SET occurs between the waiting
>      > thread's two atomic reads of the value, the waiting thread will
>     never
>      > wait for the condition variable, but the trigger thread WILL try to
>      > acquire the mutex and signal the condition variable in
>      > qemu_futex_wake(), by which  time the waiting thread may have
>     advanced
>      > outside of qemu_event_wait().
> 
>     Sorry if I'm making a mistake again, but the waiting thread won't
>     set to
>     EV_BUSY unless the value is EV_FREE on the second read so the trigger
>     thread will not call qemu_futex_wake() if it manages to set to EV_SET
>     before the second read, will it?
> 
> 
> This sequence of events will cause the problem:
> 
> WAITER (in qemu_event_wait):
> value = qatomic_load_acquire(&ev->value);
> -> EV_FREE
> 
> TRIGGER (in qemu_event_set):
> qatomic_read(&ev->value) != EV_SET
> -> EV_FREE (condition is false)
> 
> WAITER:
> qatomic_cmpxchg(&ev->value, EV_FREE, EV_BUSY) == EV_SET
> -> cmpxchg returns EV_FREE, condition false.
> ev->value =  EV_BUSY.
> > TRIGGER:
>          int old = qatomic_xchg(&ev->value, EV_SET);
>          smp_mb__after_rmw();
>          if (old == EV_BUSY) {
> -> old = EV_BUSY, condition true.
> ev->value = EV_SET
> 
> WAITER (in qemu_futex_wait(ev, EV_BUSY)):
>      pthread_mutex_lock(&ev->lock);
>      if (ev->value == val) {
> -> false, because value is EV_SET
> 
> WAITER:
>      pthread_mutex_unlock(&ev->lock);
>      …
>      qemu_event_destroy(&job->done_event);
> 
> TRIGGER (in qemu_futex_wake(ev, INT_MAX)):
>      pthread_mutex_lock(&ev->lock);
> -> hangs, because mutex has been destroyed

Thanks for clarification. This is very insightful.


> 
>      >
>      > This is all fine usually, BUT if you destroy the QemuEvent
>     immediately
>      > after the qemu_event_wait() call, qemu_futex_wake() may try to
>     lock a
>      > mutex that has been destroyed, or signal a condition variable
>     which has
>      > been destroyed. I don't see a reasonable way of making this safe
>     other
>      > than using long-lived mutexes and condition variables. And
>     anyway, we
>      > have much, MUCH bigger contention/performance issues coming from
>     almost
>      > everything being covered by the BQL. (If waking these callbacks
>     can even
>      > be considered an issue: I haven't seen it show up in profiling,
>     whereas
>      > BQL contention very much does.)
>      >
>      > I'll submit v5 of this patch set with separate condition
>     variables for
>      > each job type. This should make the occurrence of waking the wrong
>      > thread quite rare, while reasoning about correctness is pretty
>      > straightforward. I think that's good enough.

What about using QemuSemaphore then? It does not seem to have the 
problem same with QemuEvent.