[PATCH 2/3] ipc/sem.c: handle spurious wakeups

[PATCH 2/3] ipc/sem.c: handle spurious wakeups

[Manfred Spraul]

semtimedop() does not handle spurious wakeups, it returns -EINTR to user space.
Most other schedule() users would just loop and not return to user space.
The patch adds such a loop to semtimedop()

Reported-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/sem.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index fb13be1..227948f 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1426,6 +1426,8 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 
 	queue.status = -EINTR;
 	queue.sleeper = current;
+
+sleep_again:
 	current->state = TASK_INTERRUPTIBLE;
 	sem_unlock(sma);
 
@@ -1478,6 +1480,13 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 	 */
 	if (timeout && jiffies_left == 0)
 		error = -EAGAIN;
+
+	/*
+	 * If the wakeup was spurious, just retry
+	 */
+	if (error == -EINTR && !signal_pending(current))
+		goto sleep_again;
+
 	unlink_queue(sma, &queue);
 
 out_unlock_free:
-- 
1.7.6

Re: [PATCH 2/3] ipc/sem.c: handle spurious wakeups

[Andrew Morton]

On Sat, 24 Sep 2011 19:37:11 +0200
Manfred Spraul <manfred@colorfullife.com> wrote:

> semtimedop() does not handle spurious wakeups, it returns -EINTR to user space.
> Most other schedule() users would just loop and not return to user space.
> The patch adds such a loop to semtimedop()

What is a "spurious wakeup" and how can a process receive one?

I'm wondering about the userspace-visible effects of this change, and
any compatibility issues?

Re: [PATCH 2/3] ipc/sem.c: handle spurious wakeups

[Peter Zijlstra]

On Tue, 2011-10-11 at 14:54 -0700, Andrew Morton wrote:
> On Sat, 24 Sep 2011 19:37:11 +0200
> Manfred Spraul <manfred@colorfullife.com> wrote:
> 
> > semtimedop() does not handle spurious wakeups, it returns -EINTR to user space.
> > Most other schedule() users would just loop and not return to user space.
> > The patch adds such a loop to semtimedop()
> 
> What is a "spurious wakeup" and how can a process receive one?

Its a wakeup unrelated to the condition its waiting for. They shouldn't
happen (often) but all wait loops should deal with them. Sadly of course
most out of core wait loops don't :-(

> I'm wondering about the userspace-visible effects of this change, and
> any compatibility issues?

For this particular case it would be returning to userspace with -EINTR
without a signal having been raised what so ever. Not a big deal as
userspace it supposed to be able to deal with -EINTR and retry.

Also, these spurious wakeups hardly ever happen in the current kernel, I
only noticed it because I made them slightly more likely with a patch
currently on the back-burner until I figure out a sane way to audit all
1400+ schedule() and co. callsites.

Re: [PATCH 2/3] ipc/sem.c: handle spurious wakeups

[Manfred Spraul]

On 10/11/2011 11:54 PM, Andrew Morton wrote:
> On Sat, 24 Sep 2011 19:37:11 +0200
> Manfred Spraul<manfred@colorfullife.com>  wrote:
>
>> semtimedop() does not handle spurious wakeups, it returns -EINTR to user space.
>> Most other schedule() users would just loop and not return to user space.
>> The patch adds such a loop to semtimedop()
> What is a "spurious wakeup" and how can a process receive one?
A spurious wakeup means that someone calls wake_up_process() without a 
proper reason.
The most common case would be a wake_up_process() that was somehow delayed.

Peter's patch made such delayed wakeups very common, this is how we 
found the issue.

The "standard" kernel primitives handle such wakeups, ipc/sem.c doesn't 
handle that.

> I'm wondering about the userspace-visible effects of this change, and
> any compatibility issues?
This change has no userspace visible effects.

--
     Manfred