Re: [PATCH 4/4] posix-timers: turn it_signal into it_valid flag

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, 6 Sep 2011, Oleg Nesterov wrote:

> On 09/06, Thomas Gleixner wrote:
> >
> > On Tue, 6 Sep 2011, Oleg Nesterov wrote:
> > >
> > > The problem is, it can be already dequeued.
> >
> > Right, but we can solve this by moving the whole detach code into rcu.
> 
> Hmm, I don't understand...
> 
> > --- linux-2.6.orig/kernel/posix-timers.c
> > +++ linux-2.6/kernel/posix-timers.c
> > @@ -495,22 +495,30 @@ static void k_itimer_rcu_free(struct rcu
> >  {
> >  	struct k_itimer *tmr = container_of(head, struct k_itimer, it.rcu);
> >  
> > +	put_pid(tmr->it_pid);
> > +	sigqueue_free(tmr->sigq);
> >  	kmem_cache_free(posix_timers_cache, tmr);
> 
> Why do we need to move put_pid/sigqueue_free ?
> 
> The caller of release_posix_timer() should cancel the timer, we can can
> do this even before idr_remove() with or without this patch.

All callers cancel the timer. Right, put_pid() and sigqueue_free() can
stay where they are.
 
> >  static void release_posix_timer(struct k_itimer *tmr, int it_id_set)
> >  {
> > -	if (it_id_set) {
> > -		unsigned long flags;
> > -		spin_lock_irqsave(&idr_lock, flags);
> > -		idr_remove(&posix_timers_id, tmr->it_id);
> > -		spin_unlock_irqrestore(&idr_lock, flags);
> > -	}
> > -	put_pid(tmr->it_pid);
> > -	sigqueue_free(tmr->sigq);
> > -	call_rcu(&tmr->it.rcu, k_itimer_rcu_free);
> > +	if (it_id_set)
> > +		call_rcu(&tmr->it.rcu, k_itimer_rcu_free_idr);
> 
> But how this can help? Suppose that the task is preempted right
> after dequeue_signal() drops ->siglock. We need rcu_read_lock()
> before unlock then, no?

Crap, you are right, but that's fortunately an easy to solve one :)

> And. This breaks the accounting logic. I mean the patch from Andi
> which adds the limits.

That's a different problem and really, it does not break it by any
means. When the timer is released, then the count is decreased and we
can safely assume that the memory is going to be freed in the next
grace period. If that's not the case, then we have a totally different
problem which is not fixable by any limits to the number of timers per
process.

Thanks,

	tglx