ozone/wayland: watch fd on a dedicated thread. [chromium/src : main]
Maksim Sisov (Gerrit)
Attention is currently required from: Robert Kroeger.
Maksim Sisov would like Robert Kroeger to review this change.
ozone/wayland: watch fd on a dedicated thread.
Whenever user closes a popup window (or any additional window),
the browser process can get stuck waiting for the gpu side to
close. But the gpu thread can also get stuck because of destroying
the wl_egl_surface. That is, internally, Wayland EGL implementation
(it's something driver vendors provide) can try to also read events
from the wl_display's fd and call wl_display_prepare_read, and
then try to read the events. But it won't be possible as
wl_display_read_events will block for them, because Ozone/Wayland
has already prepared to read, but it doesn't read, because
it waits for another thread to complete.
Thus, the most correct approach is to use a dedicated non-blocking
thread that always watches the wayland fd and dispatches events
on an original thread. Also, use a non-default event queue to avoid
flushing events that may not belong to us (a toolkit or egl
implementation uses a default queue). See also
https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display_1a40039c1169b153269a3dc0796a54ddb0
that explains this.
Bug: 1117463
Change-Id: I33fa21f84bb26592e97327c3f0a9c926868ad214
---
M ui/ozone/platform/wayland/common/wayland_object.cc
M ui/ozone/platform/wayland/common/wayland_object.h
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc
M ui/ozone/platform/wayland/host/proxy/wayland_proxy.h
M ui/ozone/platform/wayland/host/proxy/wayland_proxy_impl.cc
M ui/ozone/platform/wayland/host/proxy/wayland_proxy_impl.h
M ui/ozone/platform/wayland/host/wayland_connection.cc
M ui/ozone/platform/wayland/host/wayland_connection.h
M ui/ozone/platform/wayland/host/wayland_connection_unittest.cc
M ui/ozone/platform/wayland/host/wayland_data_device_base.cc
M ui/ozone/platform/wayland/host/wayland_drm.cc
M ui/ozone/platform/wayland/host/wayland_event_source.cc
M ui/ozone/platform/wayland/host/wayland_event_source.h
M ui/ozone/platform/wayland/host/wayland_event_watcher.cc
M ui/ozone/platform/wayland/host/wayland_event_watcher.h
M ui/ozone/platform/wayland/host/wayland_keyboard.cc
M ui/ozone/platform/wayland/host/wayland_zaura_shell_unittest.cc
M ui/ozone/platform/wayland/host/wayland_zwp_linux_dmabuf.cc
M ui/ozone/platform/wayland/test/wayland_test.cc
19 files changed, 276 insertions(+), 49 deletions(-)
To view, visit change 2814598. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Robert Kroeger.
Robert Kroeger (Gerrit)
Attention is currently required from: Maksim Sisov.
3 comments:
Patchset:
I'm not convinced that this is the best way to solve the problem. In particular:
- something is blocking that shouldn't? Maybe it would be better to remove the block
- and why can't you use the I/O thread to supervise the wayland fd?
File ui/ozone/platform/wayland/host/wayland_event_watcher.h:
Patch Set #6, Line 89:
// See the |use_dedicated_polling_thread_| and also the comment in the sourceDo you have to use a custom thread? This is generally discouraged. Can you use the I/O thread / thread pool instead?
File ui/ozone/platform/wayland/host/wayland_event_watcher.cc:
Patch Set #6, Line 36:
// then started to wait until the thread, which was used by the gpu service,I don't understand. There are three processes involved in this. Can you point me at the block on ui? Can you do all of this on the I/O thread instead?
To view, visit change 2814598. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Robert Kroeger.
3 comments:
Patchset:
something is blocking that shouldn't? Maybe it would be better to remove the block
We cannot really control that. Let me explain why. Please note that this ONLY happens with --in-process-gpu when dmabuf path is not used. Instead, Wayland EGL is used.
Imagine a user clicks on a 3-dot menu and a menu opens. Then, the user clicks somewhere else and the menu closes. At this moment, a deadlock may occur. It doesn't happen always as there is a race.
First of all, our Wayland fd watching mechanism happens on the UI thread that mustn't be blocked. And to ensure Wayland's events' reading API doesn't block, the following method is introduced by Wayland and used by clients - wl_display_prepare_read(_queue). That is, the client must prepare to read before it reads the events. Internally, that results in increasing the display reader count (https://source.chromium.org/chromium/chromium/src/+/master:third_party/wayland/src/src/wayland-client.c;l=1671;drc=d5e1999ef5ebc2308f8710dc9eb4d069c3452308;bpv=1;bpt=1), which then determines what thread eventually reads the events. So, a thread must prepare to read only once and then either cancel to read (on error) or read events to decrease the counter. If 2 threads prepare to read, the counter equals to 2. Then, when one reads, the counter is decreased (https://source.chromium.org/chromium/chromium/src/+/master:third_party/wayland/src/src/wayland-client.c;l=1460;drc=d5e1999ef5ebc2308f8710dc9eb4d069c3452308;bpv=1;bpt=1). If counter is not 0, the thread that started to read first waits until somebody else reads and decreases the counter (https://source.chromium.org/chromium/chromium/src/+/master:third_party/wayland/src/src/wayland-client.c;l=1496;drc=d5e1999ef5ebc2308f8710dc9eb4d069c3452308;bpv=1;bpt=1).
While everything sounds reasonable and non-blocking, Chromium's thread synchronization and Wayland EGL (second thread that does reading) comes into play and breaks things.
So, our user opened and closed the menu. GLIB/Libevent sent OnFileCanReadWithoutBlocking on the UI browser thread and Ozone/Wayland called wl_display_prepare_read(_queue). The display read counter is 1. Also (as the menu is being closed), DesktopWindowTreeHostPlatform::CloseNow was called and ui::Compositor's dtor was called. The Compositor started to destroy LayerTreeHost, which resulted in calling the CommandBuffer to finish the operations it was in the process of doing. That means the CommandBuffer sends some commands to the gpu thread via gpu_channel_host, which waits for IPC::PendingSyncMsg to arrive back.
What does the gpu thread do? It can either close the gpu side surfaces (NativeViewGLSurfaceEGL and wl_egl_surface) or call SwapBuffers. But, the internal EGL implementation for Wayland (we can't control it, and it actually does things correctly) can also call wl_display_prepare_read(_queue) (the display read counter is 2 now) and then wl_display_read_events. The wl_display_read_events decreases the counter and it becomes 1. The read cannot be completed as there is another thread that will also read, and this read operation waits until another thread reads the events. And here is the deadlock - the browser UI thread prepared to read and started to destroy ui::Compositor, which flushed/sent messages and started to wait until the gpu thread is done. However, the gpu thread was completing SwapBuffers or was destroying the egl resources, which made LibEGL (internally) also read events from the Wayland fd into own queue, but it couldn't complete reading as another thread has already prepared to read and didn't complete. And that another thread is our browser UI thread that cannot complete reading, because it waits the GPU thread to complete. So, it's a deadlock.
|-------------|
|----------------| calls | LibEGL + |
| Wayland read/ |<-------------------< | Wayland |
| preapare read | |-------------|
|----------------| |
|----------| calls | |-----------| SwapBuffers/ |
| | >------' | | DestroySurface |
| UI | Wait | GPU |>-----------------'
| thread | >------------->| thread |
|----------| PendingSyncMsg |-----------|
So, answering to your question
something is blocking that shouldn't? Maybe it would be better to remove the block
No, we cannot remove the block. The only solution is to move reading events to another non-blocking thread. Otherwise, using Wayland EGL or any other toolkit that uses Wayland is used along with the Ozone/Wayland can result in a deadlock.
You may say one can cancel reads before destroying. I'd argue that is not really possible as another synchronization can happen between the gpu and the ui thread, which may result in the same deadlock. So, you never know when it happens.I hope you're convinced now.
The bt of the browser UI thread -
(gdb) bt
#0 futex_wait_cancelable (private=<optimized out>, expected=0, futex_word=0x7fffffffc870) at ../sysdeps/nptl/futex-internal.h:183
#1 __pthread_cond_wait_common (abstime=0x0, clockid=0, mutex=0x7fffffffc820, cond=0x7fffffffc848) at pthread_cond_wait.c:508
#2 __pthread_cond_wait (cond=0x7fffffffc848, mutex=0x7fffffffc820) at pthread_cond_wait.c:638
#3 0x00007ffff7ea90e0 in Wait() () at ../../base/synchronization/condition_variable_posix.cc:79
#4 0x00007ffff7eaa821 in TimedWait() () at ../../base/synchronization/waitable_event_posix.cc:211
#5 0x00007ffff7eaa27d in Wait() () at ../../base/synchronization/waitable_event_posix.cc:157
#6 0x00007ffff3d88ebe in Send() () at ../../gpu/ipc/client/gpu_channel_host.cc:110
#7 0x00007ffff3d84df0 in Send() () at ../../gpu/ipc/client/command_buffer_proxy_impl.cc:674
#8 0x00007ffff3d85195 in WaitForGetOffsetInRange() () at ../../gpu/ipc/client/command_buffer_proxy_impl.cc:337
#9 0x00007ffff3d3dca1 in WaitForGetOffsetInRange() () at ../../gpu/command_buffer/client/cmd_buffer_helper.cc:166
#10 0x00007ffff3d3de21 in Finish() () at ../../gpu/command_buffer/client/cmd_buffer_helper.cc:226
#11 0x00007fffe5918334 in FinishHelper() () at ../../gpu/command_buffer/client/gles2_implementation.cc:1589
#12 0x00007fffe5919bbb in Finish() () at ../../gpu/command_buffer/client/gles2_implementation.cc:1566
#13 0x00007ffff21ee370 in ReleaseLayerTreeFrameSink() () at ../../cc/trees/layer_tree_host_impl.cc:3654
#14 0x00007ffff222f679 in Stop() () at ../../cc/trees/single_thread_proxy.cc:355
#15 0x00007ffff21d3bc7 in ~LayerTreeHost() () at ../../cc/trees/layer_tree_host.cc:231
#16 0x00007ffff21d408e in cc::LayerTreeHost::~LayerTreeHost() () at ../../cc/trees/layer_tree_host.cc:207
#17 0x00007ffff12c379d in operator() () at ../../buildtools/third_party/libc++/trunk/include/memory:1335
#18 reset () at ../../buildtools/third_party/libc++/trunk/include/memory:1596
#19 ~Compositor() () at ../../ui/compositor/compositor.cc:288
#20 0x00007ffff12c3ebe in ui::Compositor::~Compositor() () at ../../ui/compositor/compositor.cc:271
#21 0x00007ffff0427dcb in CloseNow() () at ../../ui/views/widget/desktop_aura/desktop_window_tree_host_platform.cc:312
The bt of the GPU thread(gdb) thread 26
[Switching to thread 26 (Thread 0x7fffd1a69700 (LWP 17448))]
#0 futex_wait_cancelable (private=<optimized out>, expected=0, futex_word=0x1e444439da4) at ../sysdeps/nptl/futex-internal.h:183
183 in ../sysdeps/nptl/futex-internal.h
(gdb) bt
#0 futex_wait_cancelable (private=<optimized out>, expected=0, futex_word=0x1e444439da4) at ../sysdeps/nptl/futex-internal.h:183
#1 __pthread_cond_wait_common (abstime=0x0, clockid=0, mutex=0x1e444439d48, cond=0x1e444439d78) at pthread_cond_wait.c:508
#2 __pthread_cond_wait (cond=0x1e444439d78, mutex=0x1e444439d48) at pthread_cond_wait.c:638
#3 0x00007fffe2c57c03 in wl_display_read_events () at /lib/x86_64-linux-gnu/libwayland-client.so.0
#4 0x00007fffe2c583f9 in wl_display_dispatch_queue () at /lib/x86_64-linux-gnu/libwayland-client.so.0
#5 0x00007fffca652dec in () at /lib/x86_64-linux-gnu/libEGL_mesa.so.0
#6 0x00007fffca648b1a in () at /lib/x86_64-linux-gnu/libEGL_mesa.so.0
#7 0x00007fffca641083 in () at /lib/x86_64-linux-gnu/libEGL_mesa.so.0
#8 0x00007ffff3be266d in SwapBuffers() () at ../../ui/gl/gl_surface_egl.cc:1662
#9 0x00007fffefed9c02 in SwapBuffers() () at ../../ui/ozone/platform/wayland/gpu/gl_surface_wayland.cc:74
#10 0x00007ffff3bbd44a in SwapBuffers() () at ../../ui/gl/gl_surface.cc:334
#11 0x00007fffe5ce53b8 in SwapBuffers() () at ../../gpu/ipc/service/pass_through_image_transport_surface.cc:53
#12 0x00007fffe6372b9b in SwapBuffers() () at ../../components/viz/service/display_embedder/skia_output_device_gl.cc:303
#13 0x00007fffe6386688 in PostSubmit() () at ../../components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc:163
File ui/ozone/platform/wayland/host/wayland_event_watcher.h:
Patch Set #6, Line 89:
// See the |use_dedicated_polling_thread_| and also the comment in the source
Do you have to use a custom thread? This is generally discouraged. […]
See my answer above.
File ui/ozone/platform/wayland/host/wayland_event_watcher.cc:
Patch Set #6, Line 36:
// then started to wait until the thread, which was used by the gpu service,
I don't understand. There are three processes involved in this. […]
Not processes. Threads. This only happens with --in-process-gpu.
In this case, the performance degrades as it looks like the frequency of fd watching is less. Secondly, it also leads to a problem with a deadlock as it can start to spin indefinitely as wl_display_prepare_read returns EAGAIN and we need to dispatch event queue on the UI thread (but we can't as UI thread is blocked) and it blocks other stuff, which somehow leads to blocking Chrome_InProcGpu thread.
To view, visit change 2814598. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Attention is currently required from: Robert Kroeger.
CQ is trying the patch.
Note: The patchset #7 "add TODO" sent to CQ was uploaded after this CL was CR+1-ed.
Reviewer, please verify there is nothing unexpected https://chromium-review.googlesource.com/c/2814598/7
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/2814598/7
Bot data: {"action": "start", "triggered_at": "2021-04-16T13:08:19.0Z", "revision": "cd10070ac3c12c725fe0b4fda6a67a6037aba06b"}
Maksim Sisov (Gerrit)
Attention is currently required from: Robert Kroeger.
Patch set 7:Commit-Queue +2
4 comments:
Patchset:
I am convinced that an extra thread is required unless (maybe) we remove IPC::PendingSyncMsg and mak […]
Yeah. I'm afraid it will take quite a lot of time to do this refactoring including learning the stuff that I haven't worked with yet.
lgtm with some nits/questions. […]
As discussed, I will investigate again watching on I/O.
File ui/ozone/platform/wayland/host/wayland_event_watcher.h:
Patch Set #6, Line 82:
bool use_dedicated_polling_thread_ = true;This is only required with in-process-gpu right? Should / can it be restricted to that case?
this is only for ozone_unittests. We will use a separate thread for out of process gpu as well as it doesn't hurt to do so.
Patch Set #6, Line 89:
// See the |use_dedicated_polling_thread_| and also the comment in the source
So you convinced me (thanks for the awesome explanation) that we must have another thread. […]
added a TODO
To view, visit change 2814598. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
ozone/wayland: watch fd on a dedicated thread.
Whenever user closes a popup window (or any additional window),
the browser process can get stuck waiting for the gpu side to
close. But the gpu thread can also get stuck because of destroying
the wl_egl_surface. That is, internally, Wayland EGL implementation
(it's something driver vendors provide) can try to also read events
from the wl_display's fd and call wl_display_prepare_read, and
then try to read the events. But it won't be possible as
wl_display_read_events will block for them, because Ozone/Wayland
has already prepared to read, but it doesn't read, because
it waits for another thread to complete.
Thus, the most correct approach is to use a dedicated non-blocking
thread that always watches the wayland fd and dispatches events
on an original thread. Also, use a non-default event queue to avoid
flushing events that may not belong to us (a toolkit or egl
implementation uses a default queue). See also
https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display_1a40039c1169b153269a3dc0796a54ddb0
that explains this.
Bug: 1117463
Change-Id: I33fa21f84bb26592e97327c3f0a9c926868ad214
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2814598
Commit-Queue: Maksim Sisov <msi...@igalia.com>
Reviewed-by: Robert Kroeger <rjkr...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#873303}
---
M ui/ozone/platform/wayland/common/wayland_object.cc
M ui/ozone/platform/wayland/common/wayland_object.h
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc
M ui/ozone/platform/wayland/host/proxy/wayland_proxy.h
M ui/ozone/platform/wayland/host/proxy/wayland_proxy_impl.cc
M ui/ozone/platform/wayland/host/proxy/wayland_proxy_impl.h
M ui/ozone/platform/wayland/host/wayland_connection.cc
M ui/ozone/platform/wayland/host/wayland_connection.h
M ui/ozone/platform/wayland/host/wayland_connection_unittest.cc
M ui/ozone/platform/wayland/host/wayland_data_device_base.cc
M ui/ozone/platform/wayland/host/wayland_drm.cc
M ui/ozone/platform/wayland/host/wayland_event_source.cc
M ui/ozone/platform/wayland/host/wayland_event_source.h
M ui/ozone/platform/wayland/host/wayland_event_watcher.cc
M ui/ozone/platform/wayland/host/wayland_event_watcher.h
M ui/ozone/platform/wayland/host/wayland_keyboard.cc
M ui/ozone/platform/wayland/host/wayland_zaura_shell_unittest.cc
M ui/ozone/platform/wayland/host/wayland_zwp_linux_dmabuf.cc
M ui/ozone/platform/wayland/test/wayland_test.cc
19 files changed, 277 insertions(+), 49 deletions(-)
To view, visit change 2814598. To unsubscribe, or for help writing mail filters, visit settings.
Lan Wei (Gerrit)
Lan Wei has created a revert of this change.
To view, visit change 2814598. To unsubscribe, or for help writing mail filters, visit settings.
Fergal Daly (Gerrit)
Fergal Daly has created a revert of this change.