Re: [NEW PATCH] POSIX timers for 2.3.26

Robert de Vries (rhdv@rhdv.cistron.nl)
Wed, 10 Nov 1999 20:55:12 +0100 (CET)


On Wed, 10 Nov 1999, Alan Cox wrote:

> > > > + timers = current->posix_timers = itimer_struct_new();
> > >
> > > What if two threads sharing timers execute this at the same time. I suspect
> > > you need to guard this with the tasklist lock perhaps ?
> >
> > If you have two threads sharing timers the current->posix_timers is
> > already initialized in fork.c when the clone_flags has the CLONE_ITIMERS
> > bit set.
>
> Good point.
>
> > > Also consider exec and a setuid program getting started with suprise signals
> > > pending. I suspect execve needs to kill these timers ?
> >
> > The timers are not inherited/copied across a fork, so do pending
> > signals.
>
> exec not fork. Consider executing a setuid program. I think an exec should
> clear the timers ?

You're right, I forgot that one. Here is the extra patch:

--- linux-2.3.26-vanilla/fs/exec.c Tue Nov 2 19:24:05 1999
+++ linux-2.3.26-devt-timer/fs/exec.c Wed Nov 10 19:58:25 1999
@@ -500,6 +500,7 @@

flush_signal_handlers(current);
flush_old_files(current->files);
+ exit_itimers(current);

return 0;

However, I'm not sure this is the _correct_ patch. I believe that there is
a possible problem here.
Imagine the following when executing the function flush_old_exec() in
which the above patch is located.
First the new empty list of signals is installed and then the itimers are
removed.
If the timer expires in the middle of all this, a signal is put into the
new list.

Another problem is that the struct k_itimer contains a pointer to a
task_struct to which it wants to send the signal. If the itimer is
installed by the current task, it will still send the signal there.

This makes me think of an even more fundamental problem, when the clone()
is performed with the CLONE_ITIMERS flag set, the timers point to the
parent task. A new timer set up by the child will send its signal to the
child and not the parent. It should add the signal to the task queue of
the group of threads.

I have also reorganized the patch in exit.c a bit. I moved the deletion of
the itimers to itimer.c to function itimer_struct_delete(). And the
function __exit_itimers() always sets current->posix_timers to NULL. As in
the following patch:

--- linux-2.3.26-vanilla/kernel/exit.c Tue Nov 2 19:24:09 1999
+++ linux-2.3.26-devt-timer/kernel/exit.c Wed Nov 10 19:59:06 1999
@@ -262,6 +262,21 @@
mmdrop(active_mm);
}

+static inline void __exit_itimers(struct task_struct *tsk)
+{
+ struct itimer_struct *timers = tsk->posix_timers;
+
+ if (timers) {
+ itimer_struct_delete(timers);
+ tsk->posix_timers = NULL;
+ }
+}
+
+void exit_itimers(struct task_struct *tsk)
+{
+ __exit_itimers(tsk);
+}
+
/*
* Turn us into a lazy TLB process if we
* aren't already..
@@ -410,6 +425,7 @@
__exit_files(tsk);
__exit_fs(tsk);
__exit_sighand(tsk);
+ __exit_itimers(tsk);
exit_thread();
tsk->state = TASK_ZOMBIE;
tsk->exit_code = code;

>
> > Signal queues cannot be shared between threads. I recall that this is one
> > of the remaining incompatibilities of LinuxThreads with POSIX threads.
>
> If you have ideas how to sort the queue sharing btw lots of people want this.

I have some ideas, and I'm willing to look into the matter. It's tricky
stuff though, especially in the real-time field (which is what I do for a
living).

In the context of a multi threaded app the task_struct has a shared
signal_queue/sigset with shared handlers and a global signal
mask. Each thread has also a private signal mask. Sending a signal by any
means to any of the threads results in the addition of the signal to the
shared set/queue. I have to check the sigaltstack stuff.
This would solve the problem above with the sending of the signal to the
right task vs. the right signal queue. I'll have to think a bit more on
this.

Robert

-- 
Robert de Vries
rhdv@rhdv.cistron.nl

- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/