[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-xorg-devel
Subject: Re: [PATCH] os: Fix timer race conditions
From: Nikhil Mahale <nmahale () nvidia ! com>
Date: 2014-11-26 6:50:19
Message-ID: 1469106.Tmv7tukVRJ () linux ! site
[Download RAW message or body]
On Monday, November 24, 2014 10:52:37 PM Julien Cristau wrote:
> On Fri, Nov 14, 2014 at 23:52:55 +0530, Nikhil Mahale wrote:
> > From 40f5100cb73aad8f90ba2c926dd08c4e15789de0 Mon Sep 17 00:00:00 2001
> > From: Nikhil Mahale <nmahale@nvidia.com>
> > Date: Tue, 11 Nov 2014 17:44:28 +0530
> > Subject: [PATCH] os: Fix timer race conditions
> >
> > Fixing following kind of race-conditions -
> >
> > WaitForSomething()
> >
> > ----> // timers -> timer-1 -> timer-2 -> null
> >
> > while (timers && (int) (timers->expires - now) <= 0)
> >
> > // prototype - DoTimer(OsTimerPtr timer, CARD32 now,
> > OsTimerPtr *prev)
> > DoTimer(timers, now, &timers)
> >
> >
> > ----> OsBlockSignals(); .... OS Signal comes just before
> > blocking it,
> >
> > .... timer-1 handler gets called.
> >
> > // timer-1 gets served and
> > scheduled again;
> > // timers -> timer-2 ->
> > timer-1 -> null
> >
> > ....
> >
> > *prev = timer->next;
> >
> > timer->next = NULL; // timers -> null
> > // timers list gets corrupted here and timer-2 gets
> > removed from list.
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=86288
> > Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
> > ---
> >
> > os/WaitFor.c | 31 +++++++++++++++++++++----------
> > 1 file changed, 21 insertions(+), 10 deletions(-)
>
> As far as I can tell after this change DoTimer is always called with
> signals disabled, so it doesn't need to disable them itself?
>
ok, here is updated patch.
> Cheers,
> Julien
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
["0001-os-Fix-timer-race-conditions.patch" (0001-os-Fix-timer-race-conditions.patch)]
From 5c797ce9cdb596107f9ebcdf8345ca234c587b22 Mon Sep 17 00:00:00 2001
From: Nikhil Mahale <nmahale@nvidia.com>
Date: Tue, 11 Nov 2014 17:44:28 +0530
Subject: [PATCH] os: Fix timer race conditions
Fixing following kind of race-conditions -
WaitForSomething()
|
----> // timers -> timer-1 -> timer-2 -> null
while (timers && (int) (timers->expires - now) <= 0)
// prototype - DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev)
DoTimer(timers, now, &timers)
|
|
----> OsBlockSignals(); .... OS Signal comes just before blocking it,
.... timer-1 handler gets called.
// timer-1 gets served and scheduled again;
// timers -> timer-2 -> timer-1 -> null
....
*prev = timer->next;
timer->next = NULL; // timers -> null
// timers list gets corrupted here and timer-2 gets removed from list.
https://bugs.freedesktop.org/show_bug.cgi?id=86288
Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
---
os/WaitFor.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/os/WaitFor.c b/os/WaitFor.c
index 39fedd9..e768356 100644
--- a/os/WaitFor.c
+++ b/os/WaitFor.c
@@ -123,7 +123,7 @@ struct _OsTimerRec {
static void DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev);
static void CheckAllTimers(void);
-static OsTimerPtr timers = NULL;
+static volatile OsTimerPtr timers = NULL;
/*****************
* WaitForSomething:
@@ -263,11 +263,14 @@ WaitForSomething(int *pClientsReady)
if ((int) (timers->expires - now) <= 0)
expired = 1;
- while (timers && (int) (timers->expires - now) <= 0)
- DoTimer(timers, now, &timers);
+ if (expired) {
+ OsBlockSignals();
+ while (timers && (int) (timers->expires - now) <= 0)
+ DoTimer(timers, now, &timers);
+ OsReleaseSignals();
- if (expired)
return 0;
+ }
}
}
else {
@@ -281,11 +284,14 @@ WaitForSomething(int *pClientsReady)
if ((int) (timers->expires - now) <= 0)
expired = 1;
- while (timers && (int) (timers->expires - now) <= 0)
- DoTimer(timers, now, &timers);
+ if (expired) {
+ OsBlockSignals();
+ while (timers && (int) (timers->expires - now) <= 0)
+ DoTimer(timers, now, &timers);
+ OsReleaseSignals();
- if (expired)
return 0;
+ }
}
}
if (someReady)
@@ -405,13 +411,12 @@ DoTimer(OsTimerPtr timer, CARD32 now, OsTimerPtr *prev)
{
CARD32 newTime;
- OsBlockSignals();
*prev = timer->next;
timer->next = NULL;
+
newTime = (*timer->callback) (timer, now, timer->arg);
if (newTime)
TimerSet(timer, 0, newTime, timer->callback, timer->arg);
- OsReleaseSignals();
}
OsTimerPtr
@@ -515,8 +520,12 @@ TimerCheck(void)
{
CARD32 now = GetTimeInMillis();
- while (timers && (int) (timers->expires - now) <= 0)
- DoTimer(timers, now, &timers);
+ if (timers && (int) (timers->expires - now) <= 0) {
+ OsBlockSignals();
+ while (timers && (int) (timers->expires - now) <= 0)
+ DoTimer(timers, now, &timers);
+ OsReleaseSignals();
+ }
}
void
--
1.8.1.5
[Attachment #4 (text/plain)]
_______________________________________________
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic