[Xcb] [RFC PATCH] New XCB socket handoff mechanism for Xlib/XCB and other libraries

[Jamey Sharp]

Thanks very much for reviewing the patches so carefully, Ian!

On Sun, Mar 16, 2008 at 03:20:43PM -0700, Ian Osgood wrote:
> It wasn't obviously clear to me what some of the extra fields were,  
> like socket_moving and real_bufmax. Perhaps you could capture some of  
> your tricky threading design in code commentary?

We should add more documentation everywhere in XCB, frankly.

The threading design in XCB's new handoff mechanism doesn't seem tricky
to us.  To answer your question about socket_moving, it's just a flag
indicating whether some thread is currently changing the ownership of
XCB's X connection socket; because XCB needs to drop its lock before
calling the callback, it needs some way to prevent other XCB threads
from simultaneously attempting handoff.  This explanation should
probably go into a comment on get_socket_back.

real_bufmax and the other buffer pointer manipulations in Xlib/XCB do
represent a bit of magic.  In fact, this is the key idea of the new
implementation: we can force Xlib requests to call _XFlush first by
making the output buffer appear full.  We originally proposed keeping
the output buffer closed all the time, to force Xlib to call _XFlush
before every request.  Keith proposed using the same hook as part of the
handoff mechanism.

When Xlib/XCB doesn't own the write side of the X socket, it closes the
output buffer by setting dpy->bufmax = dpy->buffer, preventing any Xlib
stub from writing without first calling _XFlush.  Our _XFlush then asks
XCB for ownership of the write side of the socket, using
xcb_take_socket, and opens Xlib's buffer for writing.  When XCB asks for
ownership back, via Xlib/XCB's return_socket callback, that callback
closes Xlib's buffer again.  Since Xlib calculates the buffer size in
XOpenDisplay and allocates the buffer, we needed somewhere to store the
real size of the buffer, so we put it in dpy->xcb->real_bufmax.  This
explanation should probably live somewhere in Xlib/XCB, either in an
appropriately placed comment or in some pile of "magic things you never
wanted to know about Xlib".

> Is there any danger that one workaround would overwrite another? Do  
> the workarounds need to be masks rather than an enum? Or separate enums?

The workarounds mechanism is currently as simple as possible, and no
simpler.  As purely internal mechanism, they can change at any time
without API/ABI concerns, so we can keep them simple until they need to
become more complicated.  Right now, any given pending_reply will have
only one workaround, because requests made by an external socket owner
will not result in any workarounds set in XCB.  If in the future we need
to support multiple workarounds in the same pending_reply, we can change
the workarounds to bitwise flags.

> Why "offsetof" but "container_of" with an underscore? As local  
> macros, should they be XCB_ALLCAPS, like the preceding  
> XCB_SEQUENCE_COMPARE? If they are common practice, is there some  
> other header you could include to get them?

"offsetof" because the C99 standard spells it that way, and we only
define it because the C compiler might not have it.

"container_of" because the Linux kernel spells it that way, and we
haven't seen it spelled otherwise elsewhere.

> How does _xcb_in_expect_reply differ from _xcb_in_expect_replies?

It doesn't, as we realized today.  We plan to release a revised patch
series soon, and it includes the removal of _xcb_in_expect_replies; the
caller now calls _xcb_in_expect_reply and passes the new workaround.

> Does Xlib configure.ac still need both X11_REQUIRES and  
> X11_EXTRA_DEPS for xcb?

It isn't obvious that the two need to be separate, but they are both
still used, and in different places.  We made the obvious, minimally
invasive change.  Merging the two seems like a potentailly sensible
cleanup later on, given careful inspection of how they're used.

> In OpenDis.c, this looks better to me:
> +#if USE_XCB
> +    dpy->bufmax = dpy->buffer;
> +    dpy->xcb->real_bufmax = dpy->buffer + conn_buf_size;
> +#else
>       dpy->bufmax = dpy->buffer + conn_buf_size;
> +#endif

We debated using that variant.  Jamey argues that the two lines we
currently have,
	dpy->xcb->real_bufmax = dpy->bufmax;
	dpy->bufmax = dpy->buffer;
look like "close up the buffer", and the compiler will optimize away the
first assignment to dpy->bufmax.  Josh thinks that the double assignment to
dpy->bufmax seems tacky, regardless of whether it will go away in the
optimizer.  In any case, both ways work, so we opted for the one we
already had typed in.  :)

> Why did the dpy->bigreq_size determination move?

A minor optimization: it reduces the number of times that ownership of
the socket is transferred back and forth between XCB and Xlib.  When
Xlib first asks XCB to open the connection, XCB owns the socket.  With
the previous ordering, Xlib would take the socket to call XCreateGC,
then XCB would take it back to sniff out BIG-REQUESTS, and then Xlib
would take it back again to make more requests.  By moving the
BIG-REQUESTS determination before the call to XCreateGC, we reduce this
to one handoff.

- Josh Triplett and Jamey Sharp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg/attachments/20080317/810598e1/attachment.pgp>