Re: [patch 04/44] posix-cpu-timers: Fixup stale comment

From: Frederic Weisbecker <frederic@kernel.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Oleg Nesterov <oleg@redhat.com>, Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	John Stultz <john.stultz@linaro.org>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Anna-Maria Behnsen <anna-maria@linutronix.de>
Subject: Re: [patch 04/44] posix-cpu-timers: Fixup stale comment
Date: Tue, 20 Aug 2019 22:48:03 +0200	[thread overview]
Message-ID: <20190820204803.GH2093@lenoir> (raw)
In-Reply-To: <alpine.DEB.2.21.1908201946320.2223@nanos.tec.linutronix.de>

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? :-) 