Re: [Xcb] [PATCH xcb] don't flag extra reply in xcb_take_socket
On 14.08.2018 14:46, Julien Cristau wrote:
> +xcb@
Thanks, Julien.
And sorry for this mail getting stuck in xorg-devel's moderation queue,
I'll remove that CC if I reply again.
> On 08/09/2018 12:20 AM, Erik Kurzinger wrote:
>> If any flags are specified in a call to xcb_take_socket,
>> they should only be applied to replies for requests sent
>> after that function returns (and until the socket is
>> re-acquired by XCB).
>>
>> Previously, they would also be incorrectly applied to the
>> reply for the last request sent before the socket was taken.
>> For instance, in this example program the reply for the
>> GetInputFocus request gets discarded, even though it was
>> sent before the socket was taken. This results in the
>> call to retrieve the reply hanging indefinitely.
Thanks for figuring this out. Still, I'm slightly confused about this. I
added another GetInputFocus request after the xcb_take_socket(). If I
get the reply for this second request first, everything works fine. If I
get the reply for this second request second, getting the first reply
still hangs. (See attached file)
I fail to understand this behaviour. Since pending replies are applied
when receiving responses from the server, shouldn't the order that I
actually fetch the replies from XCB make no difference?
(Also, I patched my local xcb with just your change to
xcb_take_socket(), expecting this to cause the first request after
taking the socket to be discarded, but this did not happen either)
I feel like I am understanding less than before I started to figure this
out...
Cheers,
Uli
[...]
>> diff --git a/src/xcb_out.c b/src/xcb_out.c
>> index 3601a5f..c9593e5 100644
>> --- a/src/xcb_out.c
>> +++ b/src/xcb_out.c
>> @@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void
>> (*return_socket)(void *closure), v
>> {
>> c->out.return_socket = return_socket;
>> c->out.socket_closure = closure;
>> -if(flags)
>> -_xcb_in_expect_reply(c, c->out.request,
>> WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
>> +if(flags) {
>> +/* c->out.request + 1 will be the first request sent by the
>> external
>> + * socket owner. If the socket is returned before this request
>> is sent
>> + * it will be detected in _xcb_in_replies_done and this
>> pending_reply
>> + * will be discarded.
>> + */
>> +_xcb_in_expect_reply(c, c->out.request + 1,
>> WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
>> +}
>> assert(c->out.request == c->out.request_written);
>> *sent = c->out.request;
>> }
>>
--
- Buck, when, exactly, did you lose your mind?
- Three months ago. I woke up one morning married to a pineapple.
An ugly pineapple... But I loved her.
#include
#include
#include
static void return_socket(void *closure) {
(void) closure;
}
int main(void)
{
xcb_connection_t *c = xcb_connect(NULL, NULL);
xcb_get_input_focus_cookie_t cookie1 = xcb_get_input_focus_unchecked(c);
uint64_t seq;
xcb_take_socket(c, return_socket, NULL, XCB_REQUEST_DISCARD_REPLY, );
xcb_get_input_focus_cookie_t cookie2 = xcb_get_input_focus_unchecked(c);
xcb_generic_error_t *err;
#if 0
// This version does not hang
puts("before 2");
xcb_get_input_focus_reply(c, cookie2, );
puts("before 1");
xcb_get_input_focus_reply(c, cookie1, );
#else
// This version hangs
puts("before 1");
xcb_get_input_focus_reply(c, cookie1, );
puts("before 2");
xcb_get_input_focus_reply(c, cookie2, );
#endif
puts("done");
}
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xcb] don't flag extra reply in xcb_take_socket
On Ätvrtek 9. srpna 2018 0:20:01 CEST Erik Kurzinger wrote: > In practice, this has been causing intermittent KWin crashes when > used in combination with the proprietary NVIDIA driver such as > https://bugs.kde.org/show_bug.cgi?id=386370 since when Xlib fails to > retrieve one of these incorrectly discarded replies it triggers > an IO error. There is a downstream bug with the same issue: https://bugzilla.opensuse.org/show_bug.cgi?id=1101560 I have let the reporter try libxcb with this patch and reportedly it fixed the issue for him. Not sure if it can be used as Tested-by, but positive feedback nevertheless! Michal ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xcb] don't flag extra reply in xcb_take_socket
+xcb@
On 08/09/2018 12:20 AM, Erik Kurzinger wrote:
> If any flags are specified in a call to xcb_take_socket,
> they should only be applied to replies for requests sent
> after that function returns (and until the socket is
> re-acquired by XCB).
>
> Previously, they would also be incorrectly applied to the
> reply for the last request sent before the socket was taken.
> For instance, in this example program the reply for the
> GetInputFocus request gets discarded, even though it was
> sent before the socket was taken. This results in the
> call to retrieve the reply hanging indefinitely.
>
> static void return_socket(void *closure) {}
>
> int main(void)
> {
> Display *dpy = XOpenDisplay(NULL);
> xcb_connection_t *c = XGetXCBConnection(dpy);
>
> xcb_get_input_focus_cookie_t cookie = xcb_get_input_focus_unchecked(c);
> xcb_flush(c);
>
> uint64_t seq;
> xcb_take_socket(c, return_socket, dpy, XCB_REQUEST_DISCARD_REPLY, );
>
> xcb_generic_error_t *err;
> xcb_get_input_focus_reply(c, cookie, );
> }
>
> In practice, this has been causing intermittent KWin crashes when
> used in combination with the proprietary NVIDIA driver such as
> https://bugs.kde.org/show_bug.cgi?id=386370 since when Xlib fails to
> retrieve one of these incorrectly discarded replies it triggers
> an IO error.
>
> Signed-off-by: Erik Kurzinger
> ---
> src/xcb_in.c | 21 +++--
> src/xcb_out.c | 10 --
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/src/xcb_in.c b/src/xcb_in.c
> index 73209e0..4333ad3 100644
> --- a/src/xcb_in.c
> +++ b/src/xcb_in.c
> @@ -958,8 +958,25 @@ void _xcb_in_replies_done(xcb_connection_t *c)
> pend = container_of(c->in.pending_replies_tail, struct
> pending_reply, next);
> if(pend->workaround == WORKAROUND_EXTERNAL_SOCKET_OWNER)
> {
> -pend->last_request = c->out.request;
> -pend->workaround = WORKAROUND_NONE;
> +if (XCB_SEQUENCE_COMPARE(pend->first_request, <=,
> c->out.request)) {
> +pend->last_request = c->out.request;
> +pend->workaround = WORKAROUND_NONE;
> +} else {
> +/* The socket was taken, but no requests were actually sent
> + * so just discard the pending_reply that was created.
> + */
> +struct pending_reply *prev_pend = c->in.pending_replies;
> +if (prev_pend == pend) {
> +c->in.pending_replies = NULL;
> +c->in.pending_replies_tail = >in.pending_replies;
> +} else {
> +while (prev_pend->next != pend)
> +prev_pend = prev_pend->next;
> +prev_pend->next = NULL;
> +c->in.pending_replies_tail = _pend->next;
> +}
> +free(pend);
> +}
> }
> }
> }
> diff --git a/src/xcb_out.c b/src/xcb_out.c
> index 3601a5f..c9593e5 100644
> --- a/src/xcb_out.c
> +++ b/src/xcb_out.c
> @@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void
> (*return_socket)(void *closure), v
> {
> c->out.return_socket = return_socket;
> c->out.socket_closure = closure;
> -if(flags)
> -_xcb_in_expect_reply(c, c->out.request,
> WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
> +if(flags) {
> +/* c->out.request + 1 will be the first request sent by the
> external
> + * socket owner. If the socket is returned before this request
> is sent
> + * it will be detected in _xcb_in_replies_done and this
> pending_reply
> + * will be discarded.
> + */
> +_xcb_in_expect_reply(c, c->out.request + 1,
> WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
> +}
> assert(c->out.request == c->out.request_written);
> *sent = c->out.request;
> }
>
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xcb] don't flag extra reply in xcb_take_socket
If any flags are specified in a call to xcb_take_socket,
they should only be applied to replies for requests sent
after that function returns (and until the socket is
re-acquired by XCB).
Previously, they would also be incorrectly applied to the
reply for the last request sent before the socket was taken.
For instance, in this example program the reply for the
GetInputFocus request gets discarded, even though it was
sent before the socket was taken. This results in the
call to retrieve the reply hanging indefinitely.
static void return_socket(void *closure) {}
int main(void)
{
Display *dpy = XOpenDisplay(NULL);
xcb_connection_t *c = XGetXCBConnection(dpy);
xcb_get_input_focus_cookie_t cookie = xcb_get_input_focus_unchecked(c);
xcb_flush(c);
uint64_t seq;
xcb_take_socket(c, return_socket, dpy, XCB_REQUEST_DISCARD_REPLY, );
xcb_generic_error_t *err;
xcb_get_input_focus_reply(c, cookie, );
}
In practice, this has been causing intermittent KWin crashes when
used in combination with the proprietary NVIDIA driver such as
https://bugs.kde.org/show_bug.cgi?id=386370 since when Xlib fails to
retrieve one of these incorrectly discarded replies it triggers
an IO error.
Signed-off-by: Erik Kurzinger
---
src/xcb_in.c | 21 +++--
src/xcb_out.c | 10 --
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/xcb_in.c b/src/xcb_in.c
index 73209e0..4333ad3 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -958,8 +958,25 @@ void _xcb_in_replies_done(xcb_connection_t *c)
pend = container_of(c->in.pending_replies_tail, struct pending_reply,
next);
if(pend->workaround == WORKAROUND_EXTERNAL_SOCKET_OWNER)
{
-pend->last_request = c->out.request;
-pend->workaround = WORKAROUND_NONE;
+if (XCB_SEQUENCE_COMPARE(pend->first_request, <=, c->out.request))
{
+pend->last_request = c->out.request;
+pend->workaround = WORKAROUND_NONE;
+} else {
+/* The socket was taken, but no requests were actually sent
+ * so just discard the pending_reply that was created.
+ */
+struct pending_reply *prev_pend = c->in.pending_replies;
+if (prev_pend == pend) {
+c->in.pending_replies = NULL;
+c->in.pending_replies_tail = >in.pending_replies;
+} else {
+while (prev_pend->next != pend)
+prev_pend = prev_pend->next;
+prev_pend->next = NULL;
+c->in.pending_replies_tail = _pend->next;
+}
+free(pend);
+}
}
}
}
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..c9593e5 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void
(*return_socket)(void *closure), v
{
c->out.return_socket = return_socket;
c->out.socket_closure = closure;
-if(flags)
-_xcb_in_expect_reply(c, c->out.request,
WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+if(flags) {
+/* c->out.request + 1 will be the first request sent by the
external
+ * socket owner. If the socket is returned before this request is
sent
+ * it will be detected in _xcb_in_replies_done and this
pending_reply
+ * will be discarded.
+ */
+_xcb_in_expect_reply(c, c->out.request + 1,
WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+}
assert(c->out.request == c->out.request_written);
*sent = c->out.request;
}
--
2.18.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
