[PATCH v3] mieq: Provide better adaptability and diagnostics during mieq overflow
Peter Hutterer
peter.hutterer at who-t.net
Sun Oct 16 17:38:46 PDT 2011
On Sun, Oct 16, 2011 at 03:30:27AM -0700, Jeremy Huddleston wrote:
> This patch changes from a static length event queue (512) to one that
> starts at 128 and grows to 4096 as it overflows, logging each time it
> grows.
>
> This change also allows for multiple backtraces to be printed when the
> server is wedged rather than just one. This increased sampling should
> help identify the true hog in cases where one backtrace might be
> insufficient.
>
> Signed-off-by: Jeremy Huddleston <jeremyhu at apple.com>
> ---
> Changes since v2:
> mieqGrowQueue is done in mieqProcessInputEvents rather than mieqEnqueue
> since the latter can be called from a signal handler
>
> mi/mieq.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 100 insertions(+), 22 deletions(-)
>
> diff --git a/mi/mieq.c b/mi/mieq.c
> index b75bde9..9b50267 100644
> --- a/mi/mieq.c
> +++ b/mi/mieq.c
> @@ -59,7 +59,11 @@ in this Software without prior written authorization from The Open Group.
> # include <X11/extensions/dpmsconst.h>
> #endif
>
> -#define QUEUE_SIZE 512
> +/* Queue size must be a power of 2 */
> +#define INITIAL_QUEUE_SIZE 128
> +#define MAXIMUM_QUEUE_SIZE 4096
> +#define DROP_BACKTRACE_FREQUENCY 100
> +#define DROP_BACKTRACE_MAX 20
please "namespace" these, QUEUE_INITIAL_SIZE, QUEUE_MAXIMUM_SIZE, etc.
> #define EnqueueScreen(dev) dev->spriteInfo->sprite->pEnqueueScreen
> #define DequeueScreen(dev) dev->spriteInfo->sprite->pDequeueScreen
> @@ -74,7 +78,9 @@ typedef struct _EventQueue {
> HWEventQueueType head, tail; /* long for SetInputCheck */
> CARD32 lastEventTime; /* to avoid time running backwards */
> int lastMotion; /* device ID if last event motion? */
> - EventRec events[QUEUE_SIZE]; /* static allocation for signals */
> + EventRec *events; /* our queue as an array */
> + size_t q_size; /* the size of our queue */
size? or number of elements? even with the comment it's ambiguous without
reading the implementation too.
it's number of elements here, so nevents would be more expressive.
> + size_t dropped; /* counter for number of consecutive dropped events */
> mieqHandler handlers[128]; /* custom event handler */
> } EventQueueRec, *EventQueuePtr;
>
> @@ -99,6 +105,44 @@ static inline void wait_for_server_init(void) {
> }
> #endif
>
> +/* Pre-condition: Called with miEventQueueMutex held */
> +static void
> +mieqGrowQueue(void) {
this function should take the event queue as parameter. makes it easier to
write a test for it too (hint hint, nudge nudge)
> + size_t i;
> + size_t old_size = miEventQueue.q_size;
> + size_t new_size = old_size << 1;
> + EventRec *new_queue = calloc(new_size, sizeof(EventRec));
> +
> + if (new_queue == NULL)
> + FatalError("Unable to allocate memory for the event queue.\n");
> +
> + /* We set the q_size to 0 to indicate to mieqEnqueue that there is
> + * no queue, so it drops a couple more events while we resize.
> + */
> + miEventQueue.q_size = 0;
why don't you just call OsBlockSignals/OsReleaseSignals?
> +
> + /* First copy the existing events */
> + memcpy(new_queue, &miEventQueue.events[miEventQueue.head],
> + (old_size - miEventQueue.head) * sizeof(EventRec));
> + memcpy(&new_queue[old_size - miEventQueue.head], miEventQueue.events,
> + (miEventQueue.head) * sizeof(EventRec));
> +
> + /* Initialize the new portion */
> + for (i = old_size; i < new_size; i++) {
> + InternalEvent* evlist = InitEventList(1);
> + if (!evlist)
> + FatalError("Could not allocate event queue.\n");
> + new_queue[i].events = evlist;
> + }
if we're growing the queue and run out of memory, I don't think we should
fatal error. we should just abort growing the queue.
> +
> + /* And update our record */
> + miEventQueue.tail = (miEventQueue.tail - miEventQueue.head) % old_size;
> + miEventQueue.head = 0;
> + miEventQueue.q_size = new_size;
> + free(miEventQueue.events);
> + miEventQueue.events = new_queue;
> +}
> +
> Bool
> mieqInit(void)
> {
> @@ -109,7 +153,11 @@ mieqInit(void)
> miEventQueue.lastMotion = FALSE;
> for (i = 0; i < 128; i++)
> miEventQueue.handlers[i] = NULL;
> - for (i = 0; i < QUEUE_SIZE; i++)
> + miEventQueue.q_size = INITIAL_QUEUE_SIZE;
> + miEventQueue.events = calloc(miEventQueue.q_size, sizeof(EventRec));
> + if(miEventQueue.events == NULL)
> + FatalError("Could not allocate event queue.\n");
> + for (i = 0; i < miEventQueue.q_size; i++)
> {
> if (miEventQueue.events[i].events == NULL) {
> InternalEvent* evlist = InitEventList(1);
why don't we use mieqGrowQueue() here too?
> @@ -127,13 +175,14 @@ void
> mieqFini(void)
> {
> int i;
> - for (i = 0; i < QUEUE_SIZE; i++)
> + for (i = 0; i < miEventQueue.q_size; i++)
> {
> if (miEventQueue.events[i].events != NULL) {
> FreeEventList(miEventQueue.events[i].events, 1);
> miEventQueue.events[i].events = NULL;
> }
> }
> + free(miEventQueue.events);
> }
>
> /*
> @@ -157,6 +206,20 @@ mieqEnqueue(DeviceIntPtr pDev, InternalEvent *e)
> pthread_mutex_lock(&miEventQueueMutex);
> #endif
>
> + if (!miEventQueue.q_size) {
> + /* The event queue is being resized due to previous overflow.
> + * This event will be dropped on the floor.
> + */
> + if (!miEventQueue.dropped) {
> + ErrorF("[mi] Event discarded during EQ resize.\n");
> + }
> + miEventQueue.dropped++;
> +#ifdef XQUARTZ
> + pthread_mutex_unlock(&miEventQueueMutex);
> +#endif
> + return;
> + }
> +
> verify_internal_event(e);
>
> /* avoid merging events from different devices */
> @@ -165,26 +228,29 @@ mieqEnqueue(DeviceIntPtr pDev, InternalEvent *e)
>
> if (isMotion && isMotion == miEventQueue.lastMotion &&
> oldtail != miEventQueue.head) {
> - oldtail = (oldtail - 1) % QUEUE_SIZE;
> - }
> - else {
> - static int stuck = 0;
> + oldtail = (oldtail - 1) % miEventQueue.q_size;
> + } else if (((oldtail + 1) % miEventQueue.q_size) == miEventQueue.head) {
> /* Toss events which come in late. Usually this means your server's
> * stuck in an infinite loop somewhere, but SIGIO is still getting
> - * handled. */
> - if (((oldtail + 1) % QUEUE_SIZE) == miEventQueue.head) {
> - if (!stuck) {
> - ErrorF("[mi] EQ overflowing. The server is probably stuck "
> - "in an infinite loop.\n");
> - xorg_backtrace();
> - stuck = 1;
> + * handled.
> + */
> + miEventQueue.dropped++;
> + if (miEventQueue.dropped == 1) {
> + ErrorF("[mi] EQ overflowing. Additional events will be discarded until existing events are processed.\n");
> + xorg_backtrace();
a really useful addition here would be to leave some space in the queue as a
backup. When the queue is full, all events but button release and key
release events are dropped. This way when the events are catching up again,
we won't have stuck keys/buttons (tricky with multiple buttons down, but
covering 90% of the cases would be good enough here)
> + } else if (miEventQueue.dropped % DROP_BACKTRACE_FREQUENCY == 0 &&
> + miEventQueue.dropped / DROP_BACKTRACE_FREQUENCY <= DROP_BACKTRACE_MAX) {
> + ErrorF("[mi] EQ overflow continuing. %lu events have been dropped.\n", miEventQueue.dropped);
> + if (miEventQueue.dropped / DROP_BACKTRACE_FREQUENCY == DROP_BACKTRACE_MAX) {
> + ErrorF("[mi] No further overflow reports will be reported until the clog is cleared.\n");
> }
> + xorg_backtrace();
> + }
> +
> #ifdef XQUARTZ
> - pthread_mutex_unlock(&miEventQueueMutex);
> + pthread_mutex_unlock(&miEventQueueMutex);
> #endif
> - return;
> - }
> - stuck = 0;
> + return;
> }
>
> evlen = e->any.length;
> @@ -203,7 +269,7 @@ mieqEnqueue(DeviceIntPtr pDev, InternalEvent *e)
> miEventQueue.events[oldtail].pDev = pDev;
>
> miEventQueue.lastMotion = isMotion;
> - miEventQueue.tail = (oldtail + 1) % QUEUE_SIZE;
> + miEventQueue.tail = (oldtail + 1) % miEventQueue.q_size;
> #ifdef XQUARTZ
> pthread_mutex_unlock(&miEventQueueMutex);
> #endif
> @@ -441,7 +507,19 @@ mieqProcessInputEvents(void)
> #ifdef XQUARTZ
> pthread_mutex_lock(&miEventQueueMutex);
> #endif
> -
> +
> + if ((miEventQueue.tail - miEventQueue.head) % miEventQueue.q_size >= (miEventQueue.q_size >> 1) &&
> + miEventQueue.q_size < MAXIMUM_QUEUE_SIZE) {
> + ErrorF("[mi] Increasing EQ size to %lu to prevent dropped events.\n", miEventQueue.q_size << 1);
> + ErrorF("[mi] This may be indicative of a misbehaving driver monopolizing the server.\n");
"may be a misbehaving ..."
Cheers,
Peter
> + mieqGrowQueue();
> + }
> +
> + if (miEventQueue.dropped) {
> + ErrorF("[mi] EQ processing has resumed after %lu dropped events.\n", miEventQueue.dropped);
> + miEventQueue.dropped = 0;
> + }
> +
> while (miEventQueue.head != miEventQueue.tail) {
> e = &miEventQueue.events[miEventQueue.head];
>
> @@ -449,7 +527,7 @@ mieqProcessInputEvents(void)
> dev = e->pDev;
> screen = e->pScreen;
>
> - miEventQueue.head = (miEventQueue.head + 1) % QUEUE_SIZE;
> + miEventQueue.head = (miEventQueue.head + 1) % miEventQueue.q_size;
>
> #ifdef XQUARTZ
> pthread_mutex_unlock(&miEventQueueMutex);
> --
> 1.7.6.1
>
>
More information about the xorg-devel
mailing list