Re: [Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Michel Dänzer]

[ Resurrecting an old thread... ]

On 2017-10-24 8:24 p.m., Henri Verbeet wrote:
> On 24 October 2017 at 20:13, Fredrik Höglund  wrote:
>> On Tuesday 24 October 2017, Henri Verbeet wrote:
>>> On 24 October 2017 at 16:11, Fredrik Höglund  wrote:
> @@ -934,9 +938,18 @@ x11_manage_fifo_queues(void *state)
>
>while (chain->last_present_msc < target_msc) {
>   xcb_generic_event_t *event =
> -xcb_wait_for_special_event(chain->conn, 
> chain->special_event);
> - if (!event)
> -goto fail;
> +xcb_poll_for_special_event(chain->conn, 
> chain->special_event);
> + if (!event) {
> +int ret = poll(, 1, 100);

 There is a race condition here where another thread can read the event
 from the file descriptor in the time between the calls to
 xcb_poll_for_special_event() and poll().

>>> Is that a scenario we care about? Unless I'm misunderstanding
>>> something, the same kind of thing could happen between
>>> x11_present_to_x11() and xcb_wait_for_special_event().
>>
>> It cannot, because if another thread reads a special event, xcb will
>> insert it into the corresponding special event queue and wake the
>> waiting thread. That's the point of having special event queues.
>>
>> But the reason I know this to be a problem is that I have tried to fix
>> this bug in the same way, and I noticed that it resulted in frequent
>> random stutters in some apps because poll() was timing out.
>>
> Oh, I think I see what you mean. The event wouldn't get lost, but we'd
> have to wait for the poll to timeout to get to it because it's already
> read from the fd into the queue.

One way to address this is as in
src/loader/loader_dri3_helper.c:dri3_wait_for_event_locked: the first
thread to enter waits for and processes a special event, then wakes up
other threads which have entered in the meantime.


https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/5368 is my
proposed solution for the same issue in dri3_wait_for_event_locked. (In
contrast to this patch, it explicitly checks if the window still exists
and only bails early if it doesn't)


-- 
Earthling Michel Dänzer   |   https://redhat.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Henri Verbeet]

On 24 October 2017 at 20:13, Fredrik Höglund  wrote:
> On Tuesday 24 October 2017, Henri Verbeet wrote:
>> On 24 October 2017 at 16:11, Fredrik Höglund  wrote:
>> >> @@ -934,9 +938,18 @@ x11_manage_fifo_queues(void *state)
>> >>
>> >>while (chain->last_present_msc < target_msc) {
>> >>   xcb_generic_event_t *event =
>> >> -xcb_wait_for_special_event(chain->conn, 
>> >> chain->special_event);
>> >> - if (!event)
>> >> -goto fail;
>> >> +xcb_poll_for_special_event(chain->conn, 
>> >> chain->special_event);
>> >> + if (!event) {
>> >> +int ret = poll(, 1, 100);
>> >
>> > There is a race condition here where another thread can read the event
>> > from the file descriptor in the time between the calls to
>> > xcb_poll_for_special_event() and poll().
>> >
>> Is that a scenario we care about? Unless I'm misunderstanding
>> something, the same kind of thing could happen between
>> x11_present_to_x11() and xcb_wait_for_special_event().
>
> It cannot, because if another thread reads a special event, xcb will
> insert it into the corresponding special event queue and wake the
> waiting thread. That's the point of having special event queues.
>
> But the reason I know this to be a problem is that I have tried to fix
> this bug in the same way, and I noticed that it resulted in frequent
> random stutters in some apps because poll() was timing out.
>
Oh, I think I see what you mean. The event wouldn't get lost, but we'd
have to wait for the poll to timeout to get to it because it's already
read from the fd into the queue.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Fredrik Höglund]

On Tuesday 24 October 2017, Henri Verbeet wrote:
> On 24 October 2017 at 16:11, Fredrik Höglund  wrote:
> >> @@ -934,9 +938,18 @@ x11_manage_fifo_queues(void *state)
> >>
> >>while (chain->last_present_msc < target_msc) {
> >>   xcb_generic_event_t *event =
> >> -xcb_wait_for_special_event(chain->conn, chain->special_event);
> >> - if (!event)
> >> -goto fail;
> >> +xcb_poll_for_special_event(chain->conn, chain->special_event);
> >> + if (!event) {
> >> +int ret = poll(, 1, 100);
> >
> > There is a race condition here where another thread can read the event
> > from the file descriptor in the time between the calls to
> > xcb_poll_for_special_event() and poll().
> >
> Is that a scenario we care about? Unless I'm misunderstanding
> something, the same kind of thing could happen between
> x11_present_to_x11() and xcb_wait_for_special_event().

It cannot, because if another thread reads a special event, xcb will
insert it into the corresponding special event queue and wake the
waiting thread. That's the point of having special event queues.

But the reason I know this to be a problem is that I have tried to fix
this bug in the same way, and I noticed that it resulted in frequent
random stutters in some apps because poll() was timing out.

Fredrik

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Henri Verbeet]

On 24 October 2017 at 20:31, Emil Velikov  wrote:
> On 17 October 2017 at 15:18, Henri Verbeet  wrote:
>> Note that the usage of xcb_poll_for_special_event() requires a version
>> of libxcb that includes commit fad81b63422105f9345215ab2716c4b804ec7986
>> to work properly.
>>
> What should we expect if we're using xcb w/o said commit? It's worth
> mentioning in the commit message.

For reference, 
https://cgit.freedesktop.org/xcb/libxcb/commit/?id=fad81b63422105f9345215ab2716c4b804ec7986

What happens without that commit is that xcb_poll_for_special_event()
will fail to read the event, effectively preventing any subsequent
presents. That's obviously bad. As mentioned in the libxcb commit, a
similar issue exists for x11_acquire_next_image_poll_x11() with a
timeout, although I suppose that scenario is less common.

> it seems like there's no release with the commit, should we bribe Uli
> to roll one ;-)
>
Probably.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Emil Velikov]

On 17 October 2017 at 15:18, Henri Verbeet  wrote:
> In particular, if the window was destroyed before the present request
> completed, xcb_wait_for_special_event() may never return.
>
> Note that the usage of xcb_poll_for_special_event() requires a version
> of libxcb that includes commit fad81b63422105f9345215ab2716c4b804ec7986
> to work properly.
>
What should we expect if we're using xcb w/o said commit? It's worth
mentioning in the commit message.
it seems like there's no release with the commit, should we bribe Uli
to roll one ;-)

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Henri Verbeet]

On 24 October 2017 at 16:11, Fredrik Höglund  wrote:
>> @@ -934,9 +938,18 @@ x11_manage_fifo_queues(void *state)
>>
>>while (chain->last_present_msc < target_msc) {
>>   xcb_generic_event_t *event =
>> -xcb_wait_for_special_event(chain->conn, chain->special_event);
>> - if (!event)
>> -goto fail;
>> +xcb_poll_for_special_event(chain->conn, chain->special_event);
>> + if (!event) {
>> +int ret = poll(, 1, 100);
>
> There is a race condition here where another thread can read the event
> from the file descriptor in the time between the calls to
> xcb_poll_for_special_event() and poll().
>
Is that a scenario we care about? Unless I'm misunderstanding
something, the same kind of thing could happen between
x11_present_to_x11() and xcb_wait_for_special_event().
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Fredrik Höglund]

On Tuesday 17 October 2017, Henri Verbeet wrote:
> In particular, if the window was destroyed before the present request
> completed, xcb_wait_for_special_event() may never return.
> 
> Note that the usage of xcb_poll_for_special_event() requires a version
> of libxcb that includes commit fad81b63422105f9345215ab2716c4b804ec7986
> to work properly.
> 
> Signed-off-by: Henri Verbeet 
> ---
> This applies on top of "vulkan/wsi: Free the event in 
> x11_manage_fifo_queues()."
> ---
>  src/vulkan/wsi/wsi_common_x11.c | 19 ---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/src/vulkan/wsi/wsi_common_x11.c b/src/vulkan/wsi/wsi_common_x11.c
> index 22b067b..ceb0d66 100644
> --- a/src/vulkan/wsi/wsi_common_x11.c
> +++ b/src/vulkan/wsi/wsi_common_x11.c
> @@ -908,10 +908,14 @@ static void *
>  x11_manage_fifo_queues(void *state)
>  {
> struct x11_swapchain *chain = state;
> +   struct pollfd pfds;
> VkResult result;
>  
> assert(chain->base.present_mode == VK_PRESENT_MODE_FIFO_KHR);
>  
> +   pfds.fd = xcb_get_file_descriptor(chain->conn);
> +   pfds.events = POLLIN;
> +
> while (chain->status == VK_SUCCESS) {
>/* It should be safe to unconditionally block here.  Later in the loop
> * we blocks until the previous present has landed on-screen.  At that
> @@ -934,9 +938,18 @@ x11_manage_fifo_queues(void *state)
>  
>while (chain->last_present_msc < target_msc) {
>   xcb_generic_event_t *event =
> -xcb_wait_for_special_event(chain->conn, chain->special_event);
> - if (!event)
> -goto fail;
> +xcb_poll_for_special_event(chain->conn, chain->special_event);
> + if (!event) {
> +int ret = poll(, 1, 100);

There is a race condition here where another thread can read the event
from the file descriptor in the time between the calls to
xcb_poll_for_special_event() and poll().

I think what is really needed here is an
xcb_wait_for_special_event_with_timeout().

> +if (ret < 0) {
> +   result = VK_ERROR_OUT_OF_DATE_KHR;
> +   goto fail;
> +} else if (chain->status != VK_SUCCESS) {
> +   return NULL;
> +}
> +
> +continue;
> + }
>  
>   result = x11_handle_dri3_present_event(chain, (void *)event);
>   free(event);
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] vulkan/wsi: Avoid waiting indefinitely for present completion in x11_manage_fifo_queues().

[Henri Verbeet]

In particular, if the window was destroyed before the present request
completed, xcb_wait_for_special_event() may never return.

Note that the usage of xcb_poll_for_special_event() requires a version
of libxcb that includes commit fad81b63422105f9345215ab2716c4b804ec7986
to work properly.

Signed-off-by: Henri Verbeet 
---
This applies on top of "vulkan/wsi: Free the event in x11_manage_fifo_queues()."
---
 src/vulkan/wsi/wsi_common_x11.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/vulkan/wsi/wsi_common_x11.c b/src/vulkan/wsi/wsi_common_x11.c
index 22b067b..ceb0d66 100644
--- a/src/vulkan/wsi/wsi_common_x11.c
+++ b/src/vulkan/wsi/wsi_common_x11.c
@@ -908,10 +908,14 @@ static void *
 x11_manage_fifo_queues(void *state)
 {
struct x11_swapchain *chain = state;
+   struct pollfd pfds;
VkResult result;
 
assert(chain->base.present_mode == VK_PRESENT_MODE_FIFO_KHR);
 
+   pfds.fd = xcb_get_file_descriptor(chain->conn);
+   pfds.events = POLLIN;
+
while (chain->status == VK_SUCCESS) {
   /* It should be safe to unconditionally block here.  Later in the loop
* we blocks until the previous present has landed on-screen.  At that
@@ -934,9 +938,18 @@ x11_manage_fifo_queues(void *state)
 
   while (chain->last_present_msc < target_msc) {
  xcb_generic_event_t *event =
-xcb_wait_for_special_event(chain->conn, chain->special_event);
- if (!event)
-goto fail;
+xcb_poll_for_special_event(chain->conn, chain->special_event);
+ if (!event) {
+int ret = poll(, 1, 100);
+if (ret < 0) {
+   result = VK_ERROR_OUT_OF_DATE_KHR;
+   goto fail;
+} else if (chain->status != VK_SUCCESS) {
+   return NULL;
+}
+
+continue;
+ }
 
  result = x11_handle_dri3_present_event(chain, (void *)event);
  free(event);
-- 
2.1.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev