catch put_task_struct RCU handling up to mainline

Hello!

Due to the separate -rt and mainline evolution of RCU signal handling, the -rt patchset now makes each task struct go through two RCU grace periods, with one call_rcu() in release_task() and with another in put_task_struct(). Only the call_rcu() in release_task() is required, since this is the one that is associated with tearing down the task structure.

This patch removes the extra call_rcu() in put_task_struct(), synching this up with mainline. Tested lightly on i386.

CC Oleg Nesterov <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---

include/linux/sched.h |   10 ----------
kernel/fork.c         |   21 ---------------------
2 files changed, 31 deletions(-)

diff -urpNa -X dontdiff linux-2.6.17-rt5/include/linux/sched.h linux-2.6.17-rt5-2RCU/include/linux/sched.h

include/linux/sched.h +0 -10

@@ -1105,15 +1105,6 @@ static inline int pid_alive(struct task_
 extern void free_task(struct task_struct *tsk);
 #define get_task_struct(tsk) do { atomic_inc(&(tsk)->usage); } while(0)
 
-#ifdef CONFIG_PREEMPT_RT
-extern void __put_task_struct_cb(struct rcu_head *rhp);
-
-static inline void put_task_struct(struct task_struct *t)
-{
-	if (atomic_dec_and_test(&t->usage))
-		call_rcu(&t->rcu, __put_task_struct_cb);
-}
-#else
 extern void __put_task_struct(struct task_struct *t);
 
 static inline void put_task_struct(struct task_struct *t)

@@ -1121,7 +1112,6 @@ static inline void put_task_struct(struc
 	if (atomic_dec_and_test(&t->usage))
 		__put_task_struct(t);
 }
-#endif
 
 /*
 * Per process flags

diff -urpNa -X dontdiff linux-2.6.17-rt5/kernel/fork.c linux-2.6.17-rt5-2RCU/kernel/fork.c

kernel/fork.c +0 -22

@@ -120,26 +120,6 @@ void free_task(struct task_struct *tsk)
 }
 EXPORT_SYMBOL(free_task);
 
-#ifdef CONFIG_PREEMPT_RT
-void __put_task_struct_cb(struct rcu_head *rhp)
-{
-	struct task_struct *tsk = container_of(rhp, struct task_struct, rcu);
-
-	BUG_ON(atomic_read(&tsk->usage));
-	WARN_ON(!(tsk->flags & PF_DEAD));
-	WARN_ON(!(tsk->exit_state & (EXIT_DEAD | EXIT_ZOMBIE)));
-	WARN_ON(tsk == current);
-
-	security_task_free(tsk);
-	free_uid(tsk->user);
-	put_group_info(tsk->group_info);
-
-	if (!profile_handoff_task(tsk))
-		free_task(tsk);
-}
-
-#else
-
 void __put_task_struct(struct task_struct *tsk)
 {
 	WARN_ON(!(tsk->exit_state & (EXIT_DEAD | EXIT_ZOMBIE)));

@@ -154,7 +134,6 @@ void __put_task_struct(struct task_struc
 	if (!profile_handoff_task(tsk))
 		free_task(tsk);
 }
-#endif
 
 void __init fork_init(unsigned long mempages)
 {
-

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 9 lines · Paul E. McKenney

Hello!

Due to the separate -rt and mainline evolution of RCU signal handling, the -rt patchset now makes each task struct go through two RCU grace periods, with one call_rcu() in release_task() and with another in put_task_struct(). Only the call_rcu() in release_task() is required, since this is the one that is associated with tearing down the task structure.

Quoted 3 lines

This patch removes the extra call_rcu() in put_task_struct(), synching this up with mainline. Tested lightly on i386.

The extra call_rcu() has an advantage:
It defers work away from the task doing the last put_task_struct(). It could be a priority 99 task with hard latency requirements doing some PI boosting, forinstance. The extra call_rcu() defers non-RT work to a low priority task. This is in generally a very good idea in a real-time system.
So unless you can argue that the work defered is as small as the work of doing a call_rcu() I would prefer the extra call_rcu().

Quoted 15 lines · Esben Nielsen

Quoted 9 lines · Paul E. McKenney

Hello!

Due to the separate -rt and mainline evolution of RCU signal handling, the -rt patchset now makes each task struct go through two RCU grace periods, with one call_rcu() in release_task() and with another in put_task_struct(). Only the call_rcu() in release_task() is required, since this is the one that is associated with tearing down the task structure.

Quoted 3 lines

This patch removes the extra call_rcu() in put_task_struct(), synching this up with mainline. Tested lightly on i386.

Quoted 8 lines

The extra call_rcu() has an advantage:
It defers work away from the task doing the last put_task_struct(). It could be a priority 99 task with hard latency requirements doing some PI boosting, forinstance. The extra call_rcu() defers non-RT work to a low priority task. This is in generally a very good idea in a real-time system.
So unless you can argue that the work defered is as small as the work of doing a call_rcu() I would prefer the extra call_rcu().

I would instead argue that the only way that the last put_task_struct() is an unrelated high-priority task is if it manipulating an already-exited task. In particular, I believe that the sys_exit() path prohibits your example of priority-boosting an already-exited task by removing the exiting task from the various lists before doing the release_task() on itself.

Please let me know what I am missing here!

Thanx, Paul

Quoted 25 lines · Paul E. McKenney

Quoted 15 lines · Esben Nielsen

Quoted 9 lines · Paul E. McKenney

Hello!

Due to the separate -rt and mainline evolution of RCU signal handling, the -rt patchset now makes each task struct go through two RCU grace periods, with one call_rcu() in release_task() and with another in put_task_struct(). Only the call_rcu() in release_task() is required, since this is the one that is associated with tearing down the task structure.

Quoted 3 lines

This patch removes the extra call_rcu() in put_task_struct(), synching this up with mainline. Tested lightly on i386.

Quoted 8 lines

The extra call_rcu() has an advantage:
It defers work away from the task doing the last put_task_struct(). It could be a priority 99 task with hard latency requirements doing some PI boosting, forinstance. The extra call_rcu() defers non-RT work to a low priority task. This is in generally a very good idea in a real-time system.
So unless you can argue that the work defered is as small as the work of doing a call_rcu() I would prefer the extra call_rcu().

Quoted 8 lines

I would instead argue that the only way that the last put_task_struct() is an unrelated high-priority task is if it manipulating an already-exited task. In particular, I believe that the sys_exit() path prohibits your example of priority-boosting an already-exited task by removing the exiting task from the various lists before doing the release_task() on itself.

Please let me know what I am missing here!

You could very well be right (I don't know the details that well). But in that case the get/put_task_struct() in the PI code is not needed? I think, however, it is needed because the task doing the (de)boosting gets a pointer to a task, enables preemption and drops all locks. It then uses the pointer. The task could have been deleted a long time ago if it wasn't used protected by get/put_task_struct().

This is an examble of why using reference counting in a RT system is a bad
idea: Suddenly a highpriority task can end up doing the cleanup for low priority tasks.

The work should be defered to a low priority task. Using rcu is probably overkill because it also introduces other delays. A tasklet or a dedicated task would be better.

Quoted 43 lines · Esben Nielsen

Quoted 25 lines · Paul E. McKenney

Quoted 15 lines · Esben Nielsen

Quoted 9 lines · Paul E. McKenney

Hello!

Due to the separate -rt and mainline evolution of RCU signal handling, the -rt patchset now makes each task struct go through two RCU grace periods, with one call_rcu() in release_task() and with another in put_task_struct(). Only the call_rcu() in release_task() is required, since this is the one that is associated with tearing down the task structure.

Quoted 3 lines

This patch removes the extra call_rcu() in put_task_struct(), synching this up with mainline. Tested lightly on i386.

Quoted 8 lines

The extra call_rcu() has an advantage:
It defers work away from the task doing the last put_task_struct(). It could be a priority 99 task with hard latency requirements doing some PI boosting, forinstance. The extra call_rcu() defers non-RT work to a low priority task. This is in generally a very good idea in a real-time system.
So unless you can argue that the work defered is as small as the work of doing a call_rcu() I would prefer the extra call_rcu().

Quoted 8 lines

I would instead argue that the only way that the last put_task_struct() is an unrelated high-priority task is if it manipulating an already-exited task. In particular, I believe that the sys_exit() path prohibits your example of priority-boosting an already-exited task by removing the exiting task from the various lists before doing the release_task() on itself.

Please let me know what I am missing here!

You could very well be right (I don't know the details that well). But in that case the get/put_task_struct() in the PI code is not needed? I think, however, it is needed because the task doing the (de)boosting gets a pointer to a task, enables preemption and drops all locks. It then uses the pointer. The task could have been deleted a long time ago if it wasn't used protected by get/put_task_struct().

Quoted 3 lines

This is an examble of why using reference counting in a RT system is a bad
idea: Suddenly a highpriority task can end up doing the cleanup for low priority tasks.

My belief is that the scenarios that lead to this situation involve error situations -- in which case the dying task will be missing whatever deadlines it had anyway, because it died before it could complete its work. So, I agree that we need the get/put_task_struct() calls, but I believe that their job is to keep the system running in face of application errors. Otherwise, it would be really hard to debug the application, right?

That said, it is entirely possible that I am missing a code path where a correctly written application could legitimately force a high-priority task to do cleanup work on behalf of a low-priority path.

Quoted 3 lines

The work should be defered to a low priority task. Using rcu is probably overkill because it also introduces other delays. A tasklet or a dedicated task would be better.

Agreed -- if there is in fact a legitimate non-error code path, then a patch that used some deferral mechanism would be good. But RCU is overkill, and misleading overkill at that!

Thanx, Paul

Quoted 1 line · Paul E. McKenney

On Sat, Jul 08, 2006 at 02:59:37PM +0100, Esben Nielsen wrote:

Quoted 8 lines

Quoted 3 lines

The work should be defered to a low priority task. Using rcu is probably overkill because it also introduces other delays. A tasklet or a dedicated task would be better.

Agreed -- if there is in fact a legitimate non-error code path, then a patch that used some deferral mechanism would be good. But RCU is overkill, and misleading overkill at that!

I think this is a legitimate situation. lock 1 is owned by B which is blocked on lock 2 which is owned by C

CPU1: CPU2
RT task A locks lock 1 C runs something
A boosts B to RT
A does get_task_struct B
A enables interrupts C unlocks lock 2
An very long interrupt is running B unlocks lock 2
B unlocks lock 1 B is deboosted B exits
A gets CPU1 again
A does put_task_struct B

I don't know if the timing is realistic, but theoretically it is possible. It might also be possible the B exits on another CPU even without the long interrupt handler. If A has cpu affinity to CPU1 it is enough if a higher priority task preempts it on CPU1.

Quoted 2 lines · Esben Nielsen

Quoted 1 line · Paul E. McKenney

On Sat, Jul 08, 2006 at 02:59:37PM +0100, Esben Nielsen wrote:

[ . . . ]

Quoted 24 lines

Quoted 8 lines

Quoted 3 lines

The work should be defered to a low priority task. Using rcu is probably overkill because it also introduces other delays. A tasklet or a dedicated task would be better.

Agreed -- if there is in fact a legitimate non-error code path, then a patch that used some deferral mechanism would be good. But RCU is overkill, and misleading overkill at that!

I think this is a legitimate situation. lock 1 is owned by B which is blocked on lock 2 which is owned by C

CPU1: CPU2
RT task A locks lock 1 C runs something
A boosts B to RT
A does get_task_struct B
A enables interrupts C unlocks lock 2
An very long interrupt is running B unlocks lock 2
B unlocks lock 1 B is deboosted B exits
A gets CPU1 again
A does put_task_struct B

Quoted 4 lines

I don't know if the timing is realistic, but theoretically it is possible. It might also be possible the B exits on another CPU even without the long interrupt handler. If A has cpu affinity to CPU1 it is enough if a higher priority task preempts it on CPU1.

For this to happen, either A has to be at a lower priority than the irq tasks or the interrupt has to be a hard irq (e.g., scheduling clock interrupt). In the first case, the added cleanup processing seems inconsequential compared to (say) an interrupt doing network protocol processing. In the second case, B does not do its put_task_struct() until after the hard irq returns (because the put_task_struct() is invoked from a call_rcu() callback), which makes the above scenario unlikely, though perhaps not impossible.

If the second scenario is in fact possible, would you be willing to supply the appropriate deferral code? I believe we both agree that RCU is not really the right deferral mechanism in this situation.

Thanx, Paul

Quoted 44 lines · Paul E. McKenney

Quoted 2 lines · Esben Nielsen

Quoted 1 line · Paul E. McKenney

On Sat, Jul 08, 2006 at 02:59:37PM +0100, Esben Nielsen wrote:

[ . . . ]

Quoted 24 lines

Quoted 8 lines

Quoted 3 lines

The work should be defered to a low priority task. Using rcu is probably overkill because it also introduces other delays. A tasklet or a dedicated task would be better.

Agreed -- if there is in fact a legitimate non-error code path, then a patch that used some deferral mechanism would be good. But RCU is overkill, and misleading overkill at that!

I think this is a legitimate situation. lock 1 is owned by B which is blocked on lock 2 which is owned by C

CPU1: CPU2
RT task A locks lock 1 C runs something
A boosts B to RT
A does get_task_struct B
A enables interrupts C unlocks lock 2
An very long interrupt is running B unlocks lock 2
B unlocks lock 1 B is deboosted B exits
A gets CPU1 again
A does put_task_struct B

Quoted 4 lines

I don't know if the timing is realistic, but theoretically it is possible. It might also be possible the B exits on another CPU even without the long interrupt handler. If A has cpu affinity to CPU1 it is enough if a higher priority task preempts it on CPU1.

For this to happen, either A has to be at a lower priority than the irq tasks or the interrupt has to be a hard irq (e.g., scheduling clock interrupt). In the first case, the added cleanup processing seems inconsequential compared to (say) an interrupt doing network protocol processing. In the second case, B does not do its put_task_struct() until after the hard irq returns (because the put_task_struct() is invoked from a call_rcu() callback), which makes the above scenario unlikely, though perhaps not impossible.

Quoted 4 lines

If the second scenario is in fact possible, would you be willing to supply the appropriate deferral code? I believe we both agree that RCU is not really the right deferral mechanism in this situation.

Let your patch go through. I'll stop complaining :-)
Is there anywhere where we can make a list of known issues like this? I can't promise I will get time to fix this one :-(

Hello!

Due to the separate -rt and mainline evolution of RCU signal handling, the -rt patchset now makes each task struct go through two RCU grace periods, with one call_rcu() in release_task() and with another in put_task_struct(). Only the call_rcu() in release_task() is required, since this is the one that is associated with tearing down the task structure.

This patch removes the extra call_rcu() in put_task_struct(), synching this up with mainline. Tested lightly on i386.