[PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
From: Andy Ritger v2: Uses BUG_WARN_MSG to also provide a stack trace. (Peter Hutterer) Signed-off-by: Keith Packard Signed-off-by: Andy Ritger --- mi/mieq.c | 12 1 file changed, 12 insertions(+) diff --git a/mi/mieq.c b/mi/mieq.c index c7dd0ad..01f74ac 100644 --- a/mi/mieq.c +++ b/mi/mieq.c @@ -572,11 +572,20 @@ mieqProcessInputEvents(void) InternalEvent event; DeviceIntPtr dev = NULL, master = NULL; size_t n_enqueued; +static Bool inProcessInputEvents = FALSE; #ifdef XQUARTZ pthread_mutex_lock(&miEventQueueMutex); #endif +/* + * report an error if mieqProcessInputEvents() is called recursively; + * this can happen, e.g., if something in the mieqProcessDeviceEvent() + * call chain calls UpdateCurrentTime() instead of UpdateCurrentTimeIf() + */ +BUG_WARN_MSG(inProcessInputEvents, "[mi] mieqProcessInputEvents() called recursively.\n"); +inProcessInputEvents = TRUE; + /* Grow our queue if we are reaching capacity: < 2 * QUEUE_RESERVED_SIZE remaining */ n_enqueued = mieqNumEnqueued(&miEventQueue); if (n_enqueued >= (miEventQueue.nevents - (2 * QUEUE_RESERVED_SIZE)) && @@ -631,6 +640,9 @@ mieqProcessInputEvents(void) pthread_mutex_lock(&miEventQueueMutex); #endif } + +inProcessInputEvents = FALSE; + #ifdef XQUARTZ pthread_mutex_unlock(&miEventQueueMutex); #endif -- 1.7.10 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
On Jun 15, 2012, at 4:27 PM, Keith Packard wrote: > Jeremy Huddleston writes: > > >> Can you please put the XQuartz mutex lock inside of the >> inProcessInputEvents grab/release. I know the number of instructions >> is small, but there's no reason to hold the lock when checking or >> setting inProcessInputEvents. It's a mutex on miEventQueue. > > Uh. That would require that the test/set of the inProcessInputEvents be > atomic; True. I'll change that as a followup down the road. > placing that test inside the mutex ensures that the check works > correctly. > > I know it would 'probably' work just fine, but it's not nice to see > obviously incorrect code like that lying around. > > -- > keith.pack...@intel.com ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
Andy Ritger writes: > Also, remove the unnecessary static qualifier on mieqProcessInputEvents()'s > 'event' local variable. following the 'each patch does one thing' rule, this should probably be split out. In any case, I reviewed where this code came from, and a static qualifier is not necessary -- the code used to need to deal with events that could change sizes, and so mieqProcessInputEvents had a static pointer to storage that got resized as needed; a useful optimization, but not necessary. In any case, for this hunk, Reviewed-by: Keith Packard -- keith.pack...@intel.com pgppCDDs55blV.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
Jeremy Huddleston writes: > Can you please put the XQuartz mutex lock inside of the > inProcessInputEvents grab/release. I know the number of instructions > is small, but there's no reason to hold the lock when checking or > setting inProcessInputEvents. It's a mutex on miEventQueue. Uh. That would require that the test/set of the inProcessInputEvents be atomic; placing that test inside the mutex ensures that the check works correctly. I know it would 'probably' work just fine, but it's not nice to see obviously incorrect code like that lying around. -- keith.pack...@intel.com pgp2fNoXBwBsf.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
Can you please put the XQuartz mutex lock inside of the inProcessInputEvents
grab/release. I know the number of instructions is small, but there's no
reason to hold the lock when checking or setting inProcessInputEvents. It's a
mutex on miEventQueue.
Thanks,
Jeremy
On Jun 14, 2012, at 9:15 AM, Andy Ritger wrote:
> Also, remove the unnecessary static qualifier on mieqProcessInputEvents()'s
> 'event' local variable.
>
> Signed-off-by: Andy Ritger
> ---
> mi/mieq.c | 16 +++-
> 1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/mi/mieq.c b/mi/mieq.c
> index e117a8d..8339afb 100644
> --- a/mi/mieq.c
> +++ b/mi/mieq.c
> @@ -569,14 +569,25 @@ mieqProcessInputEvents(void)
> {
> EventRec *e = NULL;
> ScreenPtr screen;
> -static InternalEvent event;
> +InternalEvent event;
> DeviceIntPtr dev = NULL, master = NULL;
> size_t n_enqueued;
> +static Bool inProcessInputEvents = FALSE;
>
> #ifdef XQUARTZ
> pthread_mutex_lock(&miEventQueueMutex);
> #endif
>
> +/*
> + * report an error if mieqProcessInputEvents() is called recursively;
> + * this can happen, e.g., if something in the mieqProcessDeviceEvent()
> + * call chain calls UpdateCurrentTime() instead of UpdateCurrentTimeIf()
> + */
> +if (inProcessInputEvents) {
> +ErrorF("[mi] mieqProcessInputEvents() called recursively.\n");
> +}
> +inProcessInputEvents = TRUE;
> +
> /* Grow our queue if we are reaching capacity: < 2 * QUEUE_RESERVED_SIZE
> remaining */
> n_enqueued = mieqNumEnqueued(&miEventQueue);
> if (n_enqueued >= (miEventQueue.nevents - (2 * QUEUE_RESERVED_SIZE)) &&
> @@ -631,6 +642,9 @@ mieqProcessInputEvents(void)
> pthread_mutex_lock(&miEventQueueMutex);
> #endif
> }
> +
> +inProcessInputEvents = FALSE;
> +
> #ifdef XQUARTZ
> pthread_mutex_unlock(&miEventQueueMutex);
> #endif
> --
> 1.7.7
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
On Thu, Jun 14, 2012 at 04:47:30PM -0700, Alan Coopersmith wrote:
> On 06/14/12 09:15 AM, Andy Ritger wrote:
> > +if (inProcessInputEvents) {
> > +ErrorF("[mi] mieqProcessInputEvents() called recursively.\n");
>
> Would it be useful to throw a backtrace in the log as well, to help us figure
> out how we looped back around to that point?
just use BUG_WARN_MSG(inProcessInputEvents, "blah blah"); and that'll print
a backtrace.
Cheers,
Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
On 06/14/12 09:15 AM, Andy Ritger wrote:
> +if (inProcessInputEvents) {
> +ErrorF("[mi] mieqProcessInputEvents() called recursively.\n");
Would it be useful to throw a backtrace in the log as well, to help us figure
out how we looped back around to that point?
--
-Alan Coopersmith- alan.coopersm...@oracle.com
Oracle Solaris Engineering - http://blogs.oracle.com/alanc
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 2/2] mi: Log an error if mieqProcessInputEvents() recurses.
Also, remove the unnecessary static qualifier on mieqProcessInputEvents()'s
'event' local variable.
Signed-off-by: Andy Ritger
---
mi/mieq.c | 16 +++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/mi/mieq.c b/mi/mieq.c
index e117a8d..8339afb 100644
--- a/mi/mieq.c
+++ b/mi/mieq.c
@@ -569,14 +569,25 @@ mieqProcessInputEvents(void)
{
EventRec *e = NULL;
ScreenPtr screen;
-static InternalEvent event;
+InternalEvent event;
DeviceIntPtr dev = NULL, master = NULL;
size_t n_enqueued;
+static Bool inProcessInputEvents = FALSE;
#ifdef XQUARTZ
pthread_mutex_lock(&miEventQueueMutex);
#endif
+/*
+ * report an error if mieqProcessInputEvents() is called recursively;
+ * this can happen, e.g., if something in the mieqProcessDeviceEvent()
+ * call chain calls UpdateCurrentTime() instead of UpdateCurrentTimeIf()
+ */
+if (inProcessInputEvents) {
+ErrorF("[mi] mieqProcessInputEvents() called recursively.\n");
+}
+inProcessInputEvents = TRUE;
+
/* Grow our queue if we are reaching capacity: < 2 * QUEUE_RESERVED_SIZE
remaining */
n_enqueued = mieqNumEnqueued(&miEventQueue);
if (n_enqueued >= (miEventQueue.nevents - (2 * QUEUE_RESERVED_SIZE)) &&
@@ -631,6 +642,9 @@ mieqProcessInputEvents(void)
pthread_mutex_lock(&miEventQueueMutex);
#endif
}
+
+inProcessInputEvents = FALSE;
+
#ifdef XQUARTZ
pthread_mutex_unlock(&miEventQueueMutex);
#endif
--
1.7.7
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
