[PATCH] os: Don't write the reply that overflows output buffer
Kristian Høgsberg
krh at bitplanet.net
Mon Aug 2 08:29:55 PDT 2010
The io code has a small optimization where it will add the reply buffer
that causes the client output buffer to overflow to the writev that
flushes the buffer. The idea is that since we're doing a syscall anyway
we may as well use writev and write as much as possible to the client.
However, this optimization significantly complicates the FlushClient()
implementation and worse, it means that the server can not reply on
WriteToClient() to buffer a reply until after a request has been
processed and control returns to Dispatch(). This matters for the damage
extension, which writes events to the client buffer before the
driver sees the rendering request, but wants to make sure that once the
output buffer is flushed, all the rendering has been submitted.
Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
---
Here's the other approach for fixing damage vs flush client. In writing the
commit message, it certainly does feel like damage should just be fixed to
not reply on that behaviour. And writing the patch, I realize that the
optimization is not just about saving a syscall, but also about avoiding
having to realloc the output buffer to hold a request that's bigger than
BUFSIZE (hello, XkbSendMap()). The current code only does that when it
fails to write the output buffer + extra request and the remaining bytes
don't fit in a standard sized buffer.
Kristian
os/connection.c | 2 +-
os/io.c | 137 +++++++++++++++---------------------------------------
os/osdep.h | 4 +-
3 files changed, 40 insertions(+), 103 deletions(-)
diff --git a/os/connection.c b/os/connection.c
index 77910be..2248edc 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -1037,7 +1037,7 @@ CloseDownConnection(ClientPtr client)
CallCallbacks(&FlushCallback, NULL);
if (oc->output && oc->output->count)
- FlushClient(client, oc, (char *)NULL, 0);
+ FlushClient(client, oc);
#ifdef XDMCP
XdmcpCloseDisplay(oc->fd);
#endif
diff --git a/os/io.c b/os/io.c
index e2df2e3..86e92c9 100644
--- a/os/io.c
+++ b/os/io.c
@@ -85,7 +85,7 @@ CallbackListPtr ReplyCallback;
CallbackListPtr FlushCallback;
static ConnectionInputPtr AllocateInputBuffer(void);
-static ConnectionOutputPtr AllocateOutputBuffer(void);
+static ConnectionOutputPtr AllocateOutputBuffer(int min_size);
/* check for both EAGAIN and EWOULDBLOCK, because some supposedly POSIX
* systems are broken and return EWOULDBLOCK when they should return EAGAIN
@@ -645,7 +645,7 @@ FlushAllOutput(void)
NewOutputPending = TRUE;
}
else
- (void)FlushClient(client, oc, (char *)NULL, 0);
+ (void)FlushClient(client, oc);
}
}
#else /* WIN32 */
@@ -665,7 +665,7 @@ FlushAllOutput(void)
NewOutputPending = TRUE;
}
else
- (void)FlushClient(client, oc, (char *)NULL, 0);
+ (void)FlushClient(client, oc);
}
XFD_COPYSET(&newOutputPending, &OutputPending);
#endif /* WIN32 */
@@ -700,7 +700,7 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
{
OsCommPtr oc;
ConnectionOutputPtr oco;
- int padBytes;
+ int padBytes, ret;
const char *buf = __buf;
#ifdef DEBUG_COMMUNICATION
Bool multicount = FALSE;
@@ -749,14 +749,35 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
}
#endif
+ padBytes = padlength[count & 3];
+
+ if (oco && oco->count + count + padBytes > oco->size)
+ {
+ FD_CLR(oc->fd, &OutputPending);
+ if(!XFD_ANYSET(&OutputPending)) {
+ CriticalOutputPending = FALSE;
+ NewOutputPending = FALSE;
+ }
+
+ if (FlushCallback)
+ CallCallbacks(&FlushCallback, NULL);
+
+ ret = FlushClient(who, oc);
+ if (ret < 0)
+ return ret;
+
+ oco = NULL;
+ }
+
if (!oco)
{
- if ((oco = FreeOutputs))
+ if (count + padBytes < BUFSIZE && (oco = FreeOutputs))
{
FreeOutputs = oco->next;
}
- else if (!(oco = AllocateOutputBuffer()))
+ else if (!(oco = AllocateOutputBuffer(count + padBytes)))
{
+
if (oc->trans_conn) {
_XSERVTransDisconnect(oc->trans_conn);
_XSERVTransClose(oc->trans_conn);
@@ -768,8 +789,6 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
oc->output = oco;
}
- padBytes = padlength[count & 3];
-
if(ReplyCallback)
{
ReplyInfoRec replyinfo;
@@ -812,19 +831,6 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
}
}
#endif
- if (oco->count + count + padBytes > oco->size)
- {
- FD_CLR(oc->fd, &OutputPending);
- if(!XFD_ANYSET(&OutputPending)) {
- CriticalOutputPending = FALSE;
- NewOutputPending = FALSE;
- }
-
- if (FlushCallback)
- CallCallbacks(&FlushCallback, NULL);
-
- return FlushClient(who, oc, buf, count);
- }
NewOutputPending = TRUE;
FD_SET(oc->fd, &OutputPending);
@@ -844,64 +850,28 @@ WriteToClient (ClientPtr who, int count, const void *__buf)
**********************/
int
-FlushClient(ClientPtr who, OsCommPtr oc, const void *__extraBuf, int extraCount)
+FlushClient(ClientPtr who, OsCommPtr oc)
{
ConnectionOutputPtr oco = oc->output;
int connection = oc->fd;
XtransConnInfo trans_conn = oc->trans_conn;
- struct iovec iov[3];
- static char padBuffer[3];
- const char *extraBuf = __extraBuf;
long written;
- long padsize;
long notWritten;
long todo;
if (!oco)
return 0;
written = 0;
- padsize = padlength[extraCount & 3];
- notWritten = oco->count + extraCount + padsize;
+ notWritten = oco->count;
todo = notWritten;
while (notWritten) {
long before = written; /* amount of whole thing written */
- long remain = todo; /* amount to try this time, <= notWritten */
- int i = 0;
long len;
-
- /* You could be very general here and have "in" and "out" iovecs
- * and write a loop without using a macro, but what the heck. This
- * translates to:
- *
- * how much of this piece is new?
- * if more new then we are trying this time, clamp
- * if nothing new
- * then bump down amount already written, for next piece
- * else put new stuff in iovec, will need all of next piece
- *
- * Note that todo had better be at least 1 or else we'll end up
- * writing 0 iovecs.
- */
-#define InsertIOV(pointer, length) \
- len = (length) - before; \
- if (len > remain) \
- len = remain; \
- if (len <= 0) { \
- before = (-len); \
- } else { \
- iov[i].iov_len = len; \
- iov[i].iov_base = (pointer) + before; \
- i++; \
- remain -= len; \
- before = 0; \
- }
-
- InsertIOV ((char *)oco->buf, oco->count)
- InsertIOV ((char *)extraBuf, extraCount)
- InsertIOV (padBuffer, padsize)
errno = 0;
- if (trans_conn && (len = _XSERVTransWritev(trans_conn, iov, i)) >= 0)
+ if (trans_conn &&
+ (len = _XSERVTransWrite(trans_conn,
+ (char *) oco->buf + before, todo)) >= 0)
{
written += len;
notWritten -= len;
@@ -933,41 +903,10 @@ FlushClient(ClientPtr who, OsCommPtr oc, const void *__extraBuf, int extraCount)
written = 0;
}
}
- else
- {
- written -= oco->count;
- oco->count = 0;
- }
-
- if (notWritten > oco->size)
- {
- unsigned char *obuf;
-
- obuf = (unsigned char *)realloc(oco->buf,
- notWritten + BUFSIZE);
- if (!obuf)
- {
- _XSERVTransDisconnect(oc->trans_conn);
- _XSERVTransClose(oc->trans_conn);
- oc->trans_conn = NULL;
- MarkClientException(who);
- oco->count = 0;
- return -1;
- }
- oco->size = notWritten + BUFSIZE;
- oco->buf = obuf;
- }
-
- /* If the amount written extended into the padBuffer, then the
- difference "extraCount - written" may be less than 0 */
- if ((len = extraCount - written) > 0)
- memmove ((char *)oco->buf + oco->count,
- extraBuf + written,
- len);
oco->count = notWritten; /* this will include the pad */
- /* return only the amount explicitly requested */
- return extraCount;
+
+ return written;
}
#ifdef EMSGSIZE /* check for another brain-damaged OS bug */
else if (errno == EMSGSIZE)
@@ -1009,7 +948,7 @@ FlushClient(ClientPtr who, OsCommPtr oc, const void *__extraBuf, int extraCount)
FreeOutputs = oco;
}
oc->output = (ConnectionOutputPtr)NULL;
- return extraCount; /* return only the amount explicitly requested */
+ return written;
}
static ConnectionInputPtr
@@ -1034,20 +973,20 @@ AllocateInputBuffer(void)
}
static ConnectionOutputPtr
-AllocateOutputBuffer(void)
+AllocateOutputBuffer(int min)
{
ConnectionOutputPtr oco;
oco = malloc(sizeof(ConnectionOutput));
if (!oco)
return NULL;
- oco->buf = calloc(1, BUFSIZE);
+ oco->size = max(BUFSIZE, min);
+ oco->buf = calloc(1, oco->size);
if (!oco->buf)
{
free(oco);
return NULL;
}
- oco->size = BUFSIZE;
oco->count = 0;
return oco;
}
diff --git a/os/osdep.h b/os/osdep.h
index 1d87592..e7565d7 100644
--- a/os/osdep.h
+++ b/os/osdep.h
@@ -175,9 +175,7 @@ typedef struct _osComm {
extern int FlushClient(
ClientPtr /*who*/,
- OsCommPtr /*oc*/,
- const void * /*extraBuf*/,
- int /*extraCount*/
+ OsCommPtr /*oc*/
);
extern void FreeOsBuffers(
--
1.7.2
More information about the xorg
mailing list