2.6.13-rc6-rt9

i have released the 2.6.13-rc6-rt9 tree, which can be downloaded from the usual place:

finally found the deadlock. It was caused by IRQ flood, which was introduced by the end_irq() changes.

It doesn't compile with CONFIG_DECNET_ROUTER=y:

I'm getting the following compile errors:

<-- snip -->

...
LD .tmp_vmlinux1
drivers/built-in.o: In function `pi_init':
: multiple definition of `pi_init' kernel/built-in.o:(.bss+0x80f0): first defined here

Note: pi_init is a global function in drivers/block/paride/paride.c .

i have released the 2.6.13-rc6-rt9 tree, which can be downloaded from the usual place:

http://redhat.com/~mingo/realtime-preempt/

it's a fixes-only release. Changes since 2.6.13-rc6-rt3:

- USB irq flags use cleanups (Alan Stern)

- RCU tasklist-lock fixes (Paul McKenney, Thomas Gleixner)

- HR-timers waitqueue splitup, better HRT latencies (Thomas Gleixner)

- latency tracer fixes, irq flags tracing cleanups (Steven Rostedt, me)

- NFSd BKL unlock fix (Steven Rostedt)

- stackfootprint-max-printer fix (Steven Rostedt)

- stop_machine fix (Steven Rostedt)

- lpptest fix (me)

- turned off IOAPIC_POSTFLUSH when CONFIG_X86_IOAPIC_FAST. Now with Karsten's VIA fixes my testbox does not show PCI-POST weirnesses anymore. In case of IRQ problems please turn off IOAPIC_FAST. (me)

Ingo, can't you get rt.c to be more confusing. I mean it is too simple. We need to add a few more underscores here and there :-) Seriously, that rt.c is mind boggling. It was nice before, now it is just screaming for a cleanup (come now, do we really need the four underscores?). Same with latency.c.

On Fri, 2005-08-19 at 02:39 -0400, Steven Rostedt wrote:

I haven't thought of a good way yet to solve the race condition with dependent sleeper. (Except by turning off CONFIG_WAKEUP_TIMING :-)

Quoted 5 lines · Steven Rostedt

On Fri, 2005-08-19 at 02:39 -0400, Steven Rostedt wrote:

I haven't thought of a good way yet to solve the race condition with dependent sleeper. (Except by turning off CONFIG_WAKEUP_TIMING :-)

OK, I found one simple solution. The problem stems from max_mutex being grabbed. Since this uses the RT locks, and since tracing shouldn't really care about PI and all that, I switched this to a compat_semaphore, but only if CONFIG_WAKEUP_TIMING is set. This seems to get rid of this race condition that I have.

I found more bugs, but for now this message is about this specific race.

Quoted 5 lines · Steven Rostedt

Ingo, can't you get rt.c to be more confusing. I mean it is too simple. We need to add a few more underscores here and there :-) Seriously, that rt.c is mind boggling. It was nice before, now it is just screaming for a cleanup (come now, do we really need the four underscores?). Same with latency.c.

Ingo,

Here's one example of cleaning up rt.c. I like an extra parameter instead of having two functions that are exactly the same except for one line. I'll probably submit more.

Ingo, can't you get rt.c to be more confusing. I mean it is too simple. We need to add a few more underscores here and there :-) Seriously, that rt.c is mind boggling. It was nice before, now it is just screaming for a cleanup (come now, do we really need the four underscores?). Same with latency.c.

Quoted 5 lines · Steven Rostedt

Ingo, can't you get rt.c to be more confusing. I mean it is too simple. We need to add a few more underscores here and there :-) Seriously, that rt.c is mind boggling. It was nice before, now it is just screaming for a cleanup (come now, do we really need the four underscores?). Same with latency.c.

i agree that it's ugly, but some of that ugliness is to achieve the 7-instructions fail-through codepath for the common acquire (and release) codepath:

c03a5320 <__down_mutex>:

 c03a5320:       89 c1                   mov    %eax,%ecx
 c03a5322:       8b 15 08 76 3a c0       mov    0xc03a7608,%edx
 c03a5328:       31 c0                   xor    %eax,%eax
 c03a532a:       0f b1 51 14             cmpxchg %edx,0x14(%ecx)
 c03a532e:       85 c0                   test   %eax,%eax
 c03a5330:       75 01                   jne    c03a5333 <__down_mutex+0x13>
 c03a5332:       c3                      ret

that's how much it takes to acquire an RT lock, and i worked hard to get there. As long as the fastpath is kept this tight, feel free to do cleanups. But i really want to avoid having to write mutex_down/up in assembly for 24 architectures ...

Quoted 12 lines · Ingo Molnar

Quoted 5 lines · Steven Rostedt

Ingo, can't you get rt.c to be more confusing. I mean it is too simple. We need to add a few more underscores here and there :-) Seriously, that rt.c is mind boggling. It was nice before, now it is just screaming for a cleanup (come now, do we really need the four underscores?). Same with latency.c.

i agree that it's ugly, but some of that ugliness is to achieve the 7-instructions fail-through codepath for the common acquire (and release) codepath:

Quoted 9 lines

c03a5320 <__down_mutex>:

 c03a5320:       89 c1                   mov    %eax,%ecx
 c03a5322:       8b 15 08 76 3a c0       mov    0xc03a7608,%edx
 c03a5328:       31 c0                   xor    %eax,%eax
 c03a532a:       0f b1 51 14             cmpxchg %edx,0x14(%ecx)
 c03a532e:       85 c0                   test   %eax,%eax
 c03a5330:       75 01                   jne    c03a5333 <__down_mutex+0x13>
 c03a5332:       c3                      ret

Impressive!

Quoted 4 lines

that's how much it takes to acquire an RT lock, and i worked hard to get there. As long as the fastpath is kept this tight, feel free to do cleanups. But i really want to avoid having to write mutex_down/up in assembly for 24 architectures ...

Warning! I'm hacking hard to get rid of the global pi_lock, and I'm not worrying now about efficiency. I figure that if I can get it to work, then we can speed it up afterwards. Since it's complex enough keeping all the locks straight, I just want it to work without deadlocking.

Once I get it to work, I'll let you figure out how get it back down to 7-instructions :-)

Hi Ingo, Paul, others,

I'm trying to run a user-mode-linux guest under the RT kernel however the uml process never gets out of the calibrate delay loop. It seems as if the signal never gets through.

A non -rt host kernel does work (with a similar .config).

Could this be related to pauls task list changes?

Kind regards,

Quoted 6 lines · Peter Zijlstra

Hi Ingo, Paul, others,

I'm trying to run a user-mode-linux guest under the RT kernel however the uml process never gets out of the calibrate delay loop. It seems as if the signal never gets through.

one clarification: the guest kernel is a non -rt kernel, a modified 2.6.13-rc6 in my case.

Quoted 3 lines

A non -rt host kernel does work (with a similar .config).

Could this be related to pauls task list changes?

Quoted 7 lines · Peter Zijlstra

Quoted 6 lines · Peter Zijlstra

Hi Ingo, Paul, others,

I'm trying to run a user-mode-linux guest under the RT kernel however the uml process never gets out of the calibrate delay loop. It seems as if the signal never gets through.

Quoted 6 lines

one clarification: the guest kernel is a non -rt kernel, a modified 2.6.13-rc6 in my case.

Quoted 3 lines

A non -rt host kernel does work (with a similar .config).

Could this be related to pauls task list changes?

Possibly. What signal? This is a signal to a single process, right?

Jeff, could you help us out here?
What exactly does uml need to get out of the calibrate delay loop?

Quoted 2 lines · Peter Zijlstra

Jeff, could you help us out here?
What exactly does uml need to get out of the calibrate delay loop?

Interrupts, it's not too demanding :-)

If it's not seeing VTALRM, then it will never leave the calibration loop.

Try stracing it and see what it's getting.

Quoted 9 lines · Jeff Dike

Quoted 2 lines · Peter Zijlstra

Jeff, could you help us out here?
What exactly does uml need to get out of the calibrate delay loop?

Interrupts, it's not too demanding :-)

If it's not seeing VTALRM, then it will never leave the calibration loop.

Try stracing it and see what it's getting.

Sorry for the late reply.

Yes, that does seem to be the problem.

Even with a current -rt (2.6.14-rc2-rt5) UML does not run. The issue is indeed (as jeff pointed out) that VTALRM is never send. The small test programm below illustrates this.

On a non-rt kernel it completed in 1 second.
On a -rt kernel it waits at infinitum.

Kind regards,

Peter Zijlstra

---------------------------

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/time.h>
#include <signal.h>

volatile int quit = 0;

void sig_vtalrm(int signr, siginfo_t * si, void * arg)

{
        if (signr == SIGVTALRM) quit = 1;
}

int main()

{
        struct itimerval ival = {{0,0}, {1, 0}};

        struct sigaction sa;
        sa.sa_sigaction = sig_vtalrm;
        sigemptyset(&sa.sa_mask);
        sa.sa_flags = 0;
        sigaction(SIGVTALRM, &sa, NULL);

        setitimer(ITIMER_VIRTUAL, &ival, NULL);

        printf("wait\n");
        while (!quit) ;
        printf("done\n");
}

Signature · 1 line

Peter Zijlstra <[email protected]>

Quoted 6 lines

Even with a current -rt (2.6.14-rc2-rt5) UML does not run. The issue is indeed (as jeff pointed out) that VTALRM is never send. The small test programm below illustrates this.

On a non-rt kernel it completed in 1 second.
On a -rt kernel it waits at infinitum.

Will play with it and see what I broke...

Quoted 4 lines · Paul E. McKenney

Quoted 4 lines

Even with a current -rt (2.6.14-rc2-rt5) UML does not run. The issue is indeed (as jeff pointed out) that VTALRM is never send. The small test programm below illustrates this.

Quoted 4 lines

Quoted 2 lines

On a non-rt kernel it completed in 1 second.
On a -rt kernel it waits at infinitum.

Will play with it and see what I broke...

Paul,

you are not the culprit :)

The run_posix_cpu_timers(p) call is #ifdef'd out with PREEMPT_RT.

Thats a hard to fix issue.

It can not be run from hardirq context, as it takes a lot of locks (especially our favorites: tasklist_lock and sighand->siglock). :(

Maybe another playground for rcu, but it might also be solved by some other mechanism for accounting and delayed execution in the PREEMPT_RT case.

Quoted 1 line

you are not the culprit :)

Woo-hoo!!! Exonerated!!! This time, anyway... ;-)

Quoted 6 lines

It can not be run from hardirq context, as it takes a lot of locks (especially our favorites: tasklist_lock and sighand->siglock). :(

Maybe another playground for rcu, but it might also be solved by some other mechanism for accounting and delayed execution in the PREEMPT_RT case.

Certainly check_thread_timers() and check_process_timers() are playing with a number of task_struct fields, so it is not immediately clear to me how to safely replace tasklist_lock with RCU, at least not with a simple and small patch.

What did you have in mind for delayed execution?

i have released the 2.6.13-rc6-rt9 tree, which can be downloaded from the usual place:

http://redhat.com/~mingo/realtime-preempt/

it's a fixes-only release. Changes since 2.6.13-rc6-rt3:

- USB irq flags use cleanups (Alan Stern)

- RCU tasklist-lock fixes (Paul McKenney, Thomas Gleixner)

- HR-timers waitqueue splitup, better HRT latencies (Thomas Gleixner)

- latency tracer fixes, irq flags tracing cleanups (Steven Rostedt, me)

- NFSd BKL unlock fix (Steven Rostedt)

- stackfootprint-max-printer fix (Steven Rostedt)

- stop_machine fix (Steven Rostedt)

- lpptest fix (me)

- turned off IOAPIC_POSTFLUSH when CONFIG_X86_IOAPIC_FAST. Now with Karsten's VIA fixes my testbox does not show PCI-POST weirnesses anymore. In case of IRQ problems please turn off IOAPIC_FAST. (me)

to build a 2.6.13-rc6-rt9 tree, the following patches should be applied:

   http://kernel.org/pub/linux/kernel/v2.6/linux-2.6.12.tar.bz2
   http://kernel.org/pub/linux/kernel/v2.6/testing/patch-2.6.13-rc6.bz2
   http://redhat.com/~mingo/realtime-preempt/patch-2.6.13-rc6-rt9

	Ingo

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 5 lines · Ingo Molnar

i have released the 2.6.13-rc6-rt9 tree, which can be downloaded from the usual place:

http://redhat.com/~mingo/realtime-preempt/

I'm looking into getting HRT and RT booting on a SUMMIT NUMA machine (cyclone timer), but after s/error/warning/ in arch/i386/timers/timer.c for the HRT cyclone ifdef, I still get the following link error:

i have released the 2.6.13-rc6-rt9 tree, which can be downloaded from the usual place:

http://redhat.com/~mingo/realtime-preempt/

it's a fixes-only release. Changes since 2.6.13-rc6-rt3:

- USB irq flags use cleanups (Alan Stern)

- RCU tasklist-lock fixes (Paul McKenney, Thomas Gleixner)

- HR-timers waitqueue splitup, better HRT latencies (Thomas Gleixner)

- latency tracer fixes, irq flags tracing cleanups (Steven Rostedt, me)

- NFSd BKL unlock fix (Steven Rostedt)

- stackfootprint-max-printer fix (Steven Rostedt)

- stop_machine fix (Steven Rostedt)

- lpptest fix (me)

- turned off IOAPIC_POSTFLUSH when CONFIG_X86_IOAPIC_FAST. Now with Karsten's VIA fixes my testbox does not show PCI-POST weirnesses anymore. In case of IRQ problems please turn off IOAPIC_FAST. (me)

to build a 2.6.13-rc6-rt9 tree, the following patches should be applied:

   http://kernel.org/pub/linux/kernel/v2.6/linux-2.6.12.tar.bz2
   http://kernel.org/pub/linux/kernel/v2.6/testing/patch-2.6.13-rc6.bz2
   http://redhat.com/~mingo/realtime-preempt/patch-2.6.13-rc6-rt9

	Ingo

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I was trying to use another HRT clock source and couldn't get menuconfig to let me select acpi-pm-timer, turns out it has been disabled in arch/i386/Kconfig, but the description is still in the help...

# config HIGH_RES_TIMER_ACPI_PM
#       bool "ACPI-pm-timer"

Is the pm timer incompatible with the RT portion of this patch?

I'm looking into getting HRT and RT booting on a SUMMIT NUMA machine (cyclone timer), but after s/error/warning/ in arch/i386/timers/timer.c for the HRT cyclone ifdef, I still get the following link error:

Quoted 7 lines · Darren Hart

I was trying to use another HRT clock source and couldn't get menuconfig to let me select acpi-pm-timer, turns out it has been disabled in arch/i386/Kconfig, but the description is still in the help...

# config HIGH_RES_TIMER_ACPI_PM
#       bool "ACPI-pm-timer"

Is the pm timer incompatible with the RT portion of this patch?

The only timesource I came around to fixup is TSC in combination with PIT or preferred Local APIC. Add "lapic" to your kernel command line for UP boxen. Therefor it is disabled for now.

Quoted 1 line

I'm looking into getting HRT and RT booting on a SUMMIT NUMA machine (cyclone timer), but after s/error/warning/ in arch/i386/timers/timer.c for the HRT cyclone ifdef, I still get the following link error:

It should be simple to fix this. Just not right now. I have no such box and restricted time resources. Can you test a patch when I find a slot?

But of course you are heartely invited to fix it yourself :)

tglx

i have released the 2.6.13-rc6-rt9 tree, which can be downloaded from the usual place:

2. Drift of cyclic timers (armed by set_timer()):

Due to rounding errors and the drift adjustment code, the fixed increment which is precalculated when the timer is set up and added on rearm, I see creeping deviation from the timeline.

I have a patch lined up to base the rearm on human (nsac) units, so this effect will go away. But this is waste of time until (1.) is not solved.

George ???

Quoted 6 lines

2. Drift of cyclic timers (armed by set_timer()):

Due to rounding errors and the drift adjustment code, the fixed increment which is precalculated when the timer is set up and added on rearm, I see creeping deviation from the timeline.

Quoted 4 lines

I have a patch lined up to base the rearm on human (nsac) units, so this effect will go away. But this is waste of time until (1.) is not solved.

George ???

Could I (we) see what you have in mind?

George,

Quoted 6 lines · George Anzinger

Quoted 6 lines

2. Drift of cyclic timers (armed by set_timer()):

Due to rounding errors and the drift adjustment code, the fixed increment which is precalculated when the timer is set up and added on rearm, I see creeping deviation from the timeline.

Quoted 6 lines

Quoted 4 lines

I have a patch lined up to base the rearm on human (nsac) units, so this effect will go away. But this is waste of time until (1.) is not solved.

George ???

Could I (we) see what you have in mind?

Nothing which applies clean at the moment and I have no access to the box where the patch floats around.

It's simply explained.

Current code:

set_timer()

	calc interval->jiffies / interval->arch_cycles;
	based on it.interval

rearm()

	timer->expires += interval->jiffies;
	timer->arch_cycle_expires += interval->arch_cycles;
	normalize(timer);

Patched code:

set_timer()

	timer.interval = it.interval; 
	timer.next_expire = it.value; 
	both stored as timespec

rearm()

	next_expire += interval;
	calc timer->expires/arch_cycle_expires;

So on each rearm we eliminate the rounding errors and take the drift adjustment into account.

It adds some calculation overhead to each rearm, but ....

On another issue along this line, I have been thinking of changing the x86 TSC arch cycle size to 1ns. (NOT the resolution, the units for the arch cycle.) The reason to do this is to correctly track changes in cpu frequency as it is today, we would need to track down and update all pending HR timers when ever the frequency changed. By using a common unit all we need to do is change the conversion constants (well I guess they would not be constants any more :). I REALLY don't want to do this as it does add conversion overhead, but I can not think of another clean way to track TSC frequency changes.

send_sigqueue is called from posix_timer_fn() and acquires tasklist_lock, which makes no sense to me.

send_sigqueue()s (l)onl(e)y user is the posix_timer function (posix_timer_fn(), calling posix_timer_event()).

Each posix timer blocks the task from vanishing away by get_task_struct(), which is protected by the held tasklist_lock.

The task can neither go away nor the signal handler can change until put_task_struct() is called inside release_posix_timer(), which removes any chance to do an invalid access to either task or sighand because the relevant timer is deleted before the call to put_task_struct(). Also this call is protected by tasklist_lock().

posix_timer_event() first checks that the thread (SIGEV_THREAD_ID case) does not have PF_EXITING flag, then it calls send_sigqueue() which locks task list. But if the thread exits in between the kernel will oops.

posix_timer_event() runs under k_itimer.it_lock, but this does not help if that thread was not the only one in thread group, in this case we don't call exit_itimers().

Quoted 8 lines · Oleg Nesterov

posix_timer_event() first checks that the thread (SIGEV_THREAD_ID case) does not have PF_EXITING flag, then it calls send_sigqueue() which locks task list. But if the thread exits in between the kernel will oops.

posix_timer_event() runs under k_itimer.it_lock, but this does not help if that thread was not the only one in thread group, in this case we don't call exit_itimers().

I added exit_notify_itimers() for that case. It is synchronized vs. posix_timer_event() via the timer lock and therefor protects against the race described above.

It solves the problem for the only user of send_sigqueue for now, but I think we should have a more general interface to such functionality to allow simple usage.

tglx

Signed-off-by Thomas Gleixner <[email protected]>

diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/include/linux/sched.h linux-2.6.13-rc6.work/include/linux/sched.h

include/linux/sched.h 2005-08-20 17:43:36.000000000 +0200 renamed from include/linux/sched.h 2005-08-13 12:25:56.000000000 +0200 +1 -0

@@ -1051,6 +1051,7 @@ extern void __exit_signal(struct task_st
 extern void exit_sighand(struct task_struct *);
 extern void __exit_sighand(struct task_struct *);
 extern void exit_itimers(struct signal_struct *);
+extern void exit_notify_itimers(struct signal_struct *);
 
 extern NORET_TYPE void do_group_exit(int);
 

diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/kernel/posix-timers.c linux-2.6.13-rc6.work/kernel/posix-timers.c

kernel/posix-timers.c 2005-08-20 17:43:36.000000000 +0200 renamed from kernel/posix-timers.c 2005-08-13 12:25:58.000000000 +0200 +5 -0

@@ -1169,6 +1169,33 @@ void exit_itimers(struct signal_struct *
 }
 
 /*
+ * This is called by __exit_signal, when there are still references to
+ * the shared signal_struct
+ */
+void exit_notify_itimers(struct signal_struct *sig)
+{

Quoted 4 lines · Oleg Nesterov

posix_timer_event() first checks that the thread (SIGEV_THREAD_ID case) does not have PF_EXITING flag, then it calls send_sigqueue() which locks task list. But if the thread exits in between the kernel will oops.

Quoted 3 lines

posix_timer_event() runs under k_itimer.it_lock, but this does not help if that thread was not the only one in thread group, in this case we don't call exit_itimers().

I added exit_notify_itimers() for that case. It is synchronized vs. posix_timer_event() via the timer lock and therefor protects against the race described above.

It solves the problem for the only user of send_sigqueue for now, but I think we should have a more general interface to such functionality to allow simple usage.

tglx

Signed-off-by Thomas Gleixner <[email protected]>

diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/include/linux/sched.h linux-2.6.13-rc6.work/include/linux/sched.h

include/linux/sched.h +1 -0

@@ -1051,6 +1051,7 @@ extern void __exit_signal(struct task_st
 extern void exit_sighand(struct task_struct *);
 extern void __exit_sighand(struct task_struct *);
 extern void exit_itimers(struct signal_struct *);
+extern void exit_notify_itimers(struct signal_struct *);
 
 extern NORET_TYPE void do_group_exit(int);
 

diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/kernel/posix-timers.c linux-2.6.13-rc6.work/kernel/posix-timers.c

kernel/posix-timers.c +27 -0

@@ -1169,6 +1169,33 @@ void exit_itimers(struct signal_struct *
 }
 
 /*
+ * This is called by __exit_signal, when there are still references to
+ * the shared signal_struct
+ */
+void exit_notify_itimers(struct signal_struct *sig)
+{
+	struct k_itimer *timer;
+	struct list_head *tmp;
+	unsigned long flags;
+
+	list_for_each(tmp, &sig->posix_timers) {
+
+		timer = list_entry(tmp, struct k_itimer, list);
+
+		/* Protect against posix_timer_fn */
+		spin_lock_irqsave(&timer->it_lock, flags);
+		if (timer->it_process == current) {
+			
+			if (timer->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
+				timer->it_sigev_notify = SIGEV_SIGNAL;
+			
+			timer->it_process = timer->it_process->group_leader;
+		} 
+		spin_lock_irqrestore(&timer->it_lock, flags);
+	}
+}
+
+/*
  * And now for the "clock" calls
  *
  * These functions are called both from timer functions (with the timer

diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/kernel/signal.c linux-2.6.13-rc6.work/kernel/signal.c

kernel/signal.c +5 -7

@@ -390,6 +390,7 @@ void __exit_signal(struct task_struct *t
 		sig->nvcsw += tsk->nvcsw;
 		sig->nivcsw += tsk->nivcsw;
 		sig->sched_time += tsk->sched_time;
+		exit_notify_itimers(sig);
 		spin_unlock(&sighand->siglock);
 		sig = NULL;	/* Marker for below.  */
 	}

@@ -1381,13 +1382,12 @@ send_sigqueue(int sig, struct sigqueue *
 	int ret = 0;
 
 	/*
-	 * We need the tasklist lock even for the specific
-	 * thread case (when we don't need to follow the group
-	 * lists) in order to avoid races with "p->sighand"
-	 * going away or changing from under us.
+	 * No need to hold tasklist lock here. 
+	 * posix_timer_event() is synchronized via 
+	 * exit_itimers() and exit_notify_itimers() to 
+	 * be protected against p->sighand == NULL
 	 */
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
-	read_lock(&tasklist_lock);  
 	spin_lock_irqsave(&p->sighand->siglock, flags);
 
 	if (unlikely(!list_empty(&q->list))) {

@@ -1414,7 +1414,6 @@ send_sigqueue(int sig, struct sigqueue *
 
 out:
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
-	read_unlock(&tasklist_lock);
 	return(ret);
 }
 
 
 
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[PATCH] fix send_sigqueue() vs thread exit race

posix_timer_event() first checks that the thread (SIGEV_THREAD_ID case) does not have PF_EXITING flag, then it calls send_sigqueue() which locks task list. But if the thread exits in between the kernel will oops (->sighand == NULL after __exit_sighand).

This patch moves the PF_EXITING check into the send_sigqueue(), it must be done atomically under tasklist_lock. When send_sigqueue() detects exiting thread it returns -1. In that case posix_timer_event will send the signal to thread group.

Also, this patch fixes task_struct use-after-free in posix_timer_event.

Signed-off-by Oleg Nesterov <[email protected]>

kernel/signal.c renamed from kernel/signal.c~ +0 -5

@@ -1366,16 +1366,16 @@ send_sigqueue(int sig, struct sigqueue *
 	unsigned long flags;
 	int ret = 0;
 
-	/*
-	 * We need the tasklist lock even for the specific
-	 * thread case (when we don't need to follow the group
-	 * lists) in order to avoid races with "p->sighand"
-	 * going away or changing from under us.

Quoted 2 lines · Oleg Nesterov

[PATCH] fix send_sigqueue() vs thread exit race

kernel/signal.c send_sigqueue(int sig, struct sigqueue * · line 1376

Quoted 9 lines

.....

kernel/signal.c renamed from kernel/signal.c~ +7 -1

-     read_lock(&tasklist_lock);
+     read_lock(&tasklist_lock);
+
+     if (unlikely(p->flags & PF_EXITING)) {
+             ret = -1;
+             goto out_err;
+     }
+

It's still racy. tasklist_lock does not protect anything here.

arm timer
exit

timer event

        timr->it_process references a freed structure

If not:

Quoted 3 lines

timer event

        timr->it_process references a freed structure

No, create_timer() does get_task_struct(timr->it_process), this task may be EXIT_DEAD now, but the task_struct itself is valid, and it's ->flags has PF_EXITING flag.

arm timer
exit
timer event

CPU_0 CPU_1

release_task: posix_timer_fn:

	write_lock(tasklist);			lock(timer->it_lock);

	exit_timers:				send_sigxxx();
		lock(timer->it_lock)			read_lock(tasklist);

Deadlock.

Dear cc-list, what do you think?

Oleg,

Quoted 4 lines · Thomas Gleixner

(quoted diff) +0 -0

@@ -1427,7 +1434,18 @@ send_group_sigqueue(int sig, struct sigq
        int ret = 0;
 

kernel/signal.c send_sigqueue(int sig, struct sigqueue * · line 1388

Quoted 6 lines

kernel/signal.c renamed from kernel/signal.oleg.c +4 -1

        BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
-       read_lock(&tasklist_lock);
+retry:
+       if (unlikely(p->flags & PF_EXITING))
+               return -1;
+

I don't think this is correct. p == ->group_leader, it may have been exited and in EXIT_ZOMBIE state. But the thread group (process) is live, we should not stop posix timers.

But we can not check for p->sighand == NULL, as sighand is released after exit_itimers() so we are still deadlock prone. So I think __exit_sighand() should be called before exit_itimers(). Then we can do

retry:

        if (unlikely(!p->sighand))
                return -1;

instead of checking for PF_EXITING.

Thomas Gleixner wrote:
I think yes, and this is closely related to your and Paul "Use RCU to protect tasklist for unicast signals" patches.

We don't need RCU here, but we do need this hypothetical lock_task_sighand() (and __exit_sighand from __exit_signal change).

We still need tasklist_lock in send_group_queue for __group_complete_signal though. I hope Paul will solve this, it is needed for group_send_info() anyway.

But for the near future I don't see simple and non-intrusive fix for this deadlock. I am waiting for George to confirm that the bug exists and we are not crazy :)

It exists. It triggers on preempt-RT and I can trigger it on vanilla SMP by waiting for the timer expiry in release_task() before the __exit_signal() call. That's reasonable, as it can happen that way by chance too. It requires that the timer expires on a different CPU, but I don't see a reason why this should not happen.

Thomas Gleixner wrote:
Ok, exit_itimers()->itimer_delete() called when the last thread exits or does exec.

kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after that nobody can access this timer, so we don't need to lock timer->it_lock at all in this case. No lock - no deadlock.

But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now. However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so may be it can work?

   380  int posix_cpu_timer_del(struct k_itimer *timer)
   381  {
   382          struct task_struct *p = timer->it.cpu.task;
   383
   384          if (timer->it.cpu.firing)
   385                  return TIMER_RETRY;
   386
   387          if (unlikely(p == NULL))
   388                  return 0;
   389
   390          if (!list_empty(&timer->it.cpu.entry)) {
   391                  read_lock(&tasklist_lock);

Surely, it should be impossible to happen when process exists, otherwise it would deadlock immediately, we did write_lock(tasklist).

Thomas, do you know something about posix-cpu-timers.c?

Quoted 4 lines · Oleg Nesterov

kernel/posix-timers.c:common_timer_del() calls del_timer_sync(), after that nobody can access this timer, so we don't need to lock timer->it_lock at all in this case. No lock - no deadlock.

It still deadlocks:

CPU 0 CPU 1
write_lock(&tasklist_lock);
__exit_signal()

                                timer expires
                                base->running_timer = timer
                                  send_group_sigqueue()
                                   read_lock(&tasklist_lock();

exit_itimers()
del_timer_sync(timer)
waits for ever because waits for ever on tasklist_lock
base->running_timer == timer

I still think the last patch I sent is still necessary.

Quoted 1 line · Thomas Gleixner

I still think the last patch I sent is still necessary.

Thomas, you know that I like this change in __exit_{signal,sighand}, but i think this change is dangerous, should go in a separate patch, and needs a lot of testing. But the decision is up to Ingo and Roland.

What do you think about this:

int try_to_lock_this_beep_tasklist_lock(struct task_struct *group_leader)

{
	while (unlikely(!read_trylock(&tasklist_lock))) {
		if (group_leader->flags & PF_EXITING) {
			smp_rmb();
			if (thread_group_empty(group_leader))
				return 0;
		}
		cpu_relax();
	}

	return 1;
}

No need to re-check after we got tasklist, the signal will be flushed.

I think it's better to move the locking into the posix_timer_event, btw.

(quoted diff) +1 -0

+	/*
 +	 * We are locking ->it_lock + tasklist_lock backwards
 +	 * from release_task()->exit_itimers(), beware deadlock.

Quoted 5 lines · Oleg Nesterov

+ /*

 +     * We are locking ->it_lock + tasklist_lock backwards
 +     * from release_task()->exit_itimers(), beware deadlock.
 +     */

Quoted 9 lines

+ leader = timr->it_process->group_leader;
+ while (unlikely(!read_trylock(&tasklist_lock))) {
+ if (leader->flags & PF_EXITING) {
+ smp_rmb();
+ if (thread_group_empty(leader))
+ return 0;
+ }
+ cpu_relax();
+ }

Oh dear. Is there no way to fix this up by taking the locks in the correct order? (Whatever that is).

Quoted 17 lines · Andrew Morton

Quoted 14 lines · Oleg Nesterov

+ /*

 +     * We are locking ->it_lock + tasklist_lock backwards
 +     * from release_task()->exit_itimers(), beware deadlock.
 +     */

+ leader = timr->it_process->group_leader;
+ while (unlikely(!read_trylock(&tasklist_lock))) {
+ if (leader->flags & PF_EXITING) {
+ smp_rmb();
+ if (thread_group_empty(leader))
+ return 0;
+ }
+ cpu_relax();
+ }

Quoted 2 lines

Oh dear. Is there no way to fix this up by taking the locks in the correct order? (Whatever that is).

But I know nothing about kernel/posix-cpu-timers.c, I doubt it will work for posix_cpu_timer_del(). I don't have time to study posix-cpu-timers now. However, I see that __exit_signal() calls posix_cpu_timers_exit_xxx(), so may be it can work?

Hi all,

Quoted 2 lines · Ingo Molnar

i have released the 2.6.13-rc6-rt9 tree, which can be downloaded from the usual place:

I reworked the code for dynamically setting the priority of the hrtimer softirq to be aware of PI.

I stumbled over "//#define MUTEX_IRQS_OFF" in the first attempt. My assumption that all code protected by pi_lock (which is a raw lock) is irq save turned out to be wrong. I missed that commented define :( I guess it was introduced during the "IRQ latency contest" to squeeze out the last nsecs :)

Switching it back on is not really influencing system performance in a measurable way, but it allows to use the pi aware boosting function in irq context.

Quite contrary it makes the system more snappy and the overall test latencies go down.

Quoted 2 lines

Quite contrary it makes the system more snappy and the overall test latencies go down.

we can undo that flag - it's indeed only a couple of cycles worth of optimization, which wont count for most workloads. I've applied your patch, but we need to do those cleanups and the fact needs to be documented that !MUTEX_IRQS_OFF is not safe anymore. (most likely this means that the MUTEX_IRQS_OFF flag and all related changes needs to be gotten rid of)