[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