Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
On Wed, Aug 21, 2019 at 03:31:39PM +0200, Thomas Gleixner wrote: > On Wed, 21 Aug 2019, Frederic Weisbecker wrote: > > So I propose to change the behaviour of case 1) so that $TARGET doesn't call > > posix_cpu_timers_exit(). We instead wait for $OWNER to exit and call > > exit_itimers() -> timer_delete_hook($ITIMER) -> > > posix_cpu_timer_del($ITIMER). > > It is going to find $TARGET as the target of $ITIMER but no more sighand. > > Then > > finally it removes $ITIMER from $TARGET->cputime_expires. > > We basically do the same thing as in 2) but without locking sighand since > > it's NULL > > on $TARGET at this time. > > But what do we win with that? Horrors like this: > > task Atask B task C > > arm_timer(A)arm_timer(A) > > do_exit() > > del_timer(A)del_timer(A) > no sighand no_sighand >list_del() list_del() > > Guess how well concurrent list deletion works. > > We must remove armed timers from the task/signal _before_ dropping sighand, > really. Ah right, there can be concurrent owners, nevermind.
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
On Wed, 21 Aug 2019, Frederic Weisbecker wrote: > On Tue, Aug 20, 2019 at 11:43:26PM +0200, Thomas Gleixner wrote: > > On Tue, 20 Aug 2019, Frederic Weisbecker wrote: > > No it can't do that throughout posix_cpu_timer_del() because exit_itimers() > > can only look at current->signal->posix_timers which does not contain the > > posix timers owned by a different task/process. > > > > We could of course invoke posix_cpu_timers_exit() from exit_itimers() but > > does that buy anything? > > > > > It would make things more simple to delete the timer off the target from > > > the same caller and place and we could remove posix_cpu_timers_exit*(). > > > > We can't. The foreign owned cpu timers are not in cur->signal->posix_timers > > so how should we invoke posix_cpu_timer_del() on them. Only the owner task > > can. The only thing the exiting task can do is to remove the foreign timer > > from it's expiry list which has nothing to do with > > cur->signal->posix_timers. > > That's exactly what I'm proposing. I think you're misunderstanding me. > > I want the owner to handle all the list deletion work from the target. > > Ok let's imagine a timer $ITIMER, owned by task $OWNER and whose target is > task $TARGET. > > So it's enqueued on $OWNER->signal->posix_timers and $TARGET->cputime_expires. > > Two scenarios can happen: > > 1) $TARGET exits first and is released. So it calls posix_cpu_timers_exit() >which deletes $ITIMER from $TARGET->cputime_expires. > >Later on, $OWNER exits and calls exit_itimers() -> > timer_delete_hook($ITIMER) >-> posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER > but no >more sighand. So it returns. > > 2) $OWNER exits first and calls exit_itimer() -> timer_delete_hook($ITIMER) >-> posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER > and it >finds a sighand to lock. So it deletes $ITIMER from > $TARGET->cputime_expires >(see list_del(>it.cpu.entry)). > > > So I propose to change the behaviour of case 1) so that $TARGET doesn't call > posix_cpu_timers_exit(). We instead wait for $OWNER to exit and call > exit_itimers() -> timer_delete_hook($ITIMER) -> posix_cpu_timer_del($ITIMER). > It is going to find $TARGET as the target of $ITIMER but no more sighand. Then > finally it removes $ITIMER from $TARGET->cputime_expires. > We basically do the same thing as in 2) but without locking sighand since > it's NULL > on $TARGET at this time. But what do we win with that? Horrors like this: task A task B task C arm_timer(A)arm_timer(A) do_exit() del_timer(A)del_timer(A) no sighand no_sighand list_del() list_del() Guess how well concurrent list deletion works. We must remove armed timers from the task/signal _before_ dropping sighand, really. Thanks, tglx
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
On Tue, Aug 20, 2019 at 11:43:26PM +0200, Thomas Gleixner wrote: > On Tue, 20 Aug 2019, Frederic Weisbecker wrote: > No it can't do that throughout posix_cpu_timer_del() because exit_itimers() > can only look at current->signal->posix_timers which does not contain the > posix timers owned by a different task/process. > > We could of course invoke posix_cpu_timers_exit() from exit_itimers() but > does that buy anything? > > > It would make things more simple to delete the timer off the target from > > the same caller and place and we could remove posix_cpu_timers_exit*(). > > We can't. The foreign owned cpu timers are not in cur->signal->posix_timers > so how should we invoke posix_cpu_timer_del() on them. Only the owner task > can. The only thing the exiting task can do is to remove the foreign timer > from it's expiry list which has nothing to do with cur->signal->posix_timers. That's exactly what I'm proposing. I think you're misunderstanding me. I want the owner to handle all the list deletion work from the target. Ok let's imagine a timer $ITIMER, owned by task $OWNER and whose target is task $TARGET. So it's enqueued on $OWNER->signal->posix_timers and $TARGET->cputime_expires. Two scenarios can happen: 1) $TARGET exits first and is released. So it calls posix_cpu_timers_exit() which deletes $ITIMER from $TARGET->cputime_expires. Later on, $OWNER exits and calls exit_itimers() -> timer_delete_hook($ITIMER) -> posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER but no more sighand. So it returns. 2) $OWNER exits first and calls exit_itimer() -> timer_delete_hook($ITIMER) -> posix_cpu_timer_del($ITIMER). It finds $TARGET as the target of $ITIMER and it finds a sighand to lock. So it deletes $ITIMER from $TARGET->cputime_expires (see list_del(>it.cpu.entry)). So I propose to change the behaviour of case 1) so that $TARGET doesn't call posix_cpu_timers_exit(). We instead wait for $OWNER to exit and call exit_itimers() -> timer_delete_hook($ITIMER) -> posix_cpu_timer_del($ITIMER). It is going to find $TARGET as the target of $ITIMER but no more sighand. Then finally it removes $ITIMER from $TARGET->cputime_expires. We basically do the same thing as in 2) but without locking sighand since it's NULL on $TARGET at this time. I hope I'm less confusing (if not confused). Thanks.
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
On Tue, 20 Aug 2019, Frederic Weisbecker wrote: > On Tue, Aug 20, 2019 at 07:57:37PM +0200, Thomas Gleixner wrote: > > On Tue, 20 Aug 2019, Frederic Weisbecker wrote: > > > On Mon, Aug 19, 2019 at 04:31:45PM +0200, Thomas Gleixner wrote: > > > > /* > > > > - * Clean out CPU timers still ticking when a thread exited. The task > > > > - * pointer is cleared, and the expiry time is replaced with the > > > > residual > > > > - * time for later timer_gettime calls to return. > > > > + * Clean out CPU timers which are still armed when a thread exits. The > > > > + * timers are only removed from the list. No other updates are done. > > > > The > > > > + * corresponding posix timers are still accessible, but cannot be > > > > rearmed. > > > > + * > > > > * This must be called with the siglock held. > > > > */ > > > > static void cleanup_timers(struct list_head *head) > > > > > > Indeed and I believe we could avoid that step. We remove the sighand at > > > the same > > > time so those can't be accessed anymore anyway. > > > > > > exit_itimers() takes care of the last call release and could force remove > > > from > > > the list (although it might be taken care of in your series, haven't > > > checked yet): > > > > No. The posix timer is not necessarily owned by the exiting task or > > process. It can be owned by a different entity which has permissions, > > e.g. parent. > > > > So those are not in the posix timer list of the exiting task, which gets > > cleaned up in exit_itimers(). Those are in the list of the task which armed > > the timer. The timer is merily queued in the 'active timers' list of the > > exiting task and posix_cpu_timers_exit()/posix_cpu_timers_exit_group() > > remove it before the task/signal structs go away. > > Sure, I understand there's two distinct things here: the owner that queues > timers in owner->sig->posix_timers (cleaned in exit_itimers()) and the target > that queues > in target->[signal->]cputime_expires (cleaned in > posix_cpu_timers_exit[_group](). > > So I'm wondering why we bother with posix_cpu_timers_exit[_group]() at > all when exit_itimers() could handle the list deletion from > target->[signal]->cputime_expires throughout posix_cpu_timer_del() as it > already does on targets that still have their sighands. No it can't do that throughout posix_cpu_timer_del() because exit_itimers() can only look at current->signal->posix_timers which does not contain the posix timers owned by a different task/process. We could of course invoke posix_cpu_timers_exit() from exit_itimers() but does that buy anything? > It would make things more simple to delete the timer off the target from > the same caller and place and we could remove posix_cpu_timers_exit*(). We can't. The foreign owned cpu timers are not in cur->signal->posix_timers so how should we invoke posix_cpu_timer_del() on them. Only the owner task can. The only thing the exiting task can do is to remove the foreign timer from it's expiry list which has nothing to do with cur->signal->posix_timers. cur->signal->posix_timers only contains posix timers which are owned by current not those which are owned by a different task and armed on the exiting one. exit_itimers() handles cur->signal->posix_timers, i.e. timers owned by current. posix_cpu_timers_exit() handles timers enqueued on current, which are foreign owned timers because exit_itimers() removed those which were owned by current already. posix_cpu_timers_exit_group() handles timers enqueued on current->signal, which are foreign owned timers because exit_itimers() removed those which were owned by current already. > Or is there something I'm awkwardly missing as usual? :-) I think so :) Thanks, tglx
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
On Tue, Aug 20, 2019 at 07:57:37PM +0200, Thomas Gleixner wrote: > On Tue, 20 Aug 2019, Frederic Weisbecker wrote: > > On Mon, Aug 19, 2019 at 04:31:45PM +0200, Thomas Gleixner wrote: > > > /* > > > - * Clean out CPU timers still ticking when a thread exited. The task > > > - * pointer is cleared, and the expiry time is replaced with the residual > > > - * time for later timer_gettime calls to return. > > > + * Clean out CPU timers which are still armed when a thread exits. The > > > + * timers are only removed from the list. No other updates are done. The > > > + * corresponding posix timers are still accessible, but cannot be > > > rearmed. > > > + * > > > * This must be called with the siglock held. > > > */ > > > static void cleanup_timers(struct list_head *head) > > > > Indeed and I believe we could avoid that step. We remove the sighand at the > > same > > time so those can't be accessed anymore anyway. > > > > exit_itimers() takes care of the last call release and could force remove > > from > > the list (although it might be taken care of in your series, haven't > > checked yet): > > No. The posix timer is not necessarily owned by the exiting task or > process. It can be owned by a different entity which has permissions, > e.g. parent. > > So those are not in the posix timer list of the exiting task, which gets > cleaned up in exit_itimers(). Those are in the list of the task which armed > the timer. The timer is merily queued in the 'active timers' list of the > exiting task and posix_cpu_timers_exit()/posix_cpu_timers_exit_group() > remove it before the task/signal structs go away. Sure, I understand there's two distinct things here: the owner that queues timers in owner->sig->posix_timers (cleaned in exit_itimers()) and the target that queues in target->[signal->]cputime_expires (cleaned in posix_cpu_timers_exit[_group](). So I'm wondering why we bother with posix_cpu_timers_exit[_group]() at all when exit_itimers() could handle the list deletion from target->[signal]->cputime_expires throughout posix_cpu_timer_del() as it already does on targets that still have their sighands. It would make things more simple to delete the timer off the target from the same caller and place and we could remove posix_cpu_timers_exit*(). Or is there something I'm awkwardly missing as usual? :-)
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
On Tue, 20 Aug 2019, Frederic Weisbecker wrote: > On Mon, Aug 19, 2019 at 04:31:45PM +0200, Thomas Gleixner wrote: > > /* > > - * Clean out CPU timers still ticking when a thread exited. The task > > - * pointer is cleared, and the expiry time is replaced with the residual > > - * time for later timer_gettime calls to return. > > + * Clean out CPU timers which are still armed when a thread exits. The > > + * timers are only removed from the list. No other updates are done. The > > + * corresponding posix timers are still accessible, but cannot be rearmed. > > + * > > * This must be called with the siglock held. > > */ > > static void cleanup_timers(struct list_head *head) > > Indeed and I believe we could avoid that step. We remove the sighand at the > same > time so those can't be accessed anymore anyway. > > exit_itimers() takes care of the last call release and could force remove from > the list (although it might be taken care of in your series, haven't checked > yet): No. The posix timer is not necessarily owned by the exiting task or process. It can be owned by a different entity which has permissions, e.g. parent. So those are not in the posix timer list of the exiting task, which gets cleaned up in exit_itimers(). Those are in the list of the task which armed the timer. The timer is merily queued in the 'active timers' list of the exiting task and posix_cpu_timers_exit()/posix_cpu_timers_exit_group() remove it before the task/signal structs go away. Thanks, tglx
Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
On Mon, Aug 19, 2019 at 04:31:45PM +0200, Thomas Gleixner wrote:
> The comment above cleanup_timers() is outdated. The timers are only removed
> from the task/process list heads but not modified in any other way.
>
> Signed-off-by: Thomas Gleixner
> ---
> kernel/time/posix-cpu-timers.c |7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -412,9 +412,10 @@ static void cleanup_timers_list(struct l
> }
>
> /*
> - * Clean out CPU timers still ticking when a thread exited. The task
> - * pointer is cleared, and the expiry time is replaced with the residual
> - * time for later timer_gettime calls to return.
> + * Clean out CPU timers which are still armed when a thread exits. The
> + * timers are only removed from the list. No other updates are done. The
> + * corresponding posix timers are still accessible, but cannot be rearmed.
> + *
> * This must be called with the siglock held.
> */
> static void cleanup_timers(struct list_head *head)
Indeed and I believe we could avoid that step. We remove the sighand at the same
time so those can't be accessed anymore anyway.
exit_itimers() takes care of the last call release and could force remove from
the list (although it might be taken care of in your series, haven't checked
yet):
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 0a426f4e3125..f8f4a07025fd 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -383,11 +383,7 @@ static int posix_cpu_timer_del(struct k_itimer *timer)
*/
sighand = lock_task_sighand(p, );
if (unlikely(sighand == NULL)) {
- /*
-* We raced with the reaping of the task.
-* The deletion should have cleared us off the list.
-*/
- WARN_ON_ONCE(!list_empty(>it.cpu.entry));
+ list_del(>it.cpu.entry);
} else {
if (timer->it.cpu.firing)
ret = TIMER_RETRY;
Reviewed-by: Frederic Weisbecker
[patch 04/44] posix-cpu-timers: Fixup stale comment
The comment above cleanup_timers() is outdated. The timers are only removed from the task/process list heads but not modified in any other way. Signed-off-by: Thomas Gleixner --- kernel/time/posix-cpu-timers.c |7 --- 1 file changed, 4 insertions(+), 3 deletions(-) --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -412,9 +412,10 @@ static void cleanup_timers_list(struct l } /* - * Clean out CPU timers still ticking when a thread exited. The task - * pointer is cleared, and the expiry time is replaced with the residual - * time for later timer_gettime calls to return. + * Clean out CPU timers which are still armed when a thread exits. The + * timers are only removed from the list. No other updates are done. The + * corresponding posix timers are still accessible, but cannot be rearmed. + * * This must be called with the siglock held. */ static void cleanup_timers(struct list_head *head)
