Re: [PATCH 2.6.13-rc6-rt9] PI aware dynamic priority adjustment

[George Anzinger]

Thomas Gleixner wrote:
> On Sat, 2005-08-20 at 18:10 +0400, Oleg Nesterov wrote:
>
>
> > posix_timer_event() first checks that the thread (SIGEV_THREAD_ID
> > case) does not have PF_EXITING flag, then it calls send_sigqueue()
> > which locks task list. But if the thread exits in between the kernel
> > will oops.
>
>
> > posix_timer_event() runs under k_itimer.it_lock, but this does not
> > help if that thread was not the only one in thread group, in this
> > case we don't call exit_itimers().
>
>
> I added exit_notify_itimers() for that case. It is synchronized vs.
> posix_timer_event() via the timer lock and therefor protects against the
> race described above.

It seems to me that exit (or exec) calling the timer release code 
covered this.  I haven't gone through the exact sequence being 
discussed, however.  In any case, I don't think we should send the 
signal to the group leader instead, but rather should release the timer 
and cancel any pending signal.  AFAIKT there is no reason the group 
leader can not exit prior to other threads, but I may have missed this...

George
> It solves the problem for the only user of send_sigqueue for now, but I
> think we should have a more general interface to such functionality to
> allow simple usage.
>
> tglx
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>
>
> diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/include/linux/sched.h linux-2.6.13-rc6.work/include/linux/sched.h
> --- linux-2.6.13-rc6/include/linux/sched.h	2005-08-13 12:25:56.000000000 +0200
> +++ linux-2.6.13-rc6.work/include/linux/sched.h	2005-08-20 17:43:36.000000000 +0200
> @@ -1051,6 +1051,7 @@ extern void __exit_signal(struct task_st
>  extern void exit_sighand(struct task_struct *);
>  extern void __exit_sighand(struct task_struct *);
>  extern void exit_itimers(struct signal_struct *);
> +extern void exit_notify_itimers(struct signal_struct *);
>  
>  extern NORET_TYPE void do_group_exit(int);
>  
> diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/kernel/posix-timers.c linux-2.6.13-rc6.work/kernel/posix-timers.c
> --- linux-2.6.13-rc6/kernel/posix-timers.c	2005-08-13 12:25:58.000000000 +0200
> +++ linux-2.6.13-rc6.work/kernel/posix-timers.c	2005-08-20 17:43:36.000000000 +0200
> @@ -1169,6 +1169,33 @@ void exit_itimers(struct signal_struct *
>  }
>  
>  /*
> + * This is called by __exit_signal, when there are still references to
> + * the shared signal_struct
> + */
> +void exit_notify_itimers(struct signal_struct *sig)
> +{
> +	struct k_itimer *timer;
> +	struct list_head *tmp;
> +	unsigned long flags;
> +
> +	list_for_each(tmp, &sig->posix_timers) {
> +
> +		timer = list_entry(tmp, struct k_itimer, list);
> +
> +		/* Protect against posix_timer_fn */
> +		spin_lock_irqsave(&timer->it_lock, flags);
> +		if (timer->it_process == current) {
> +			
> +			if (timer->it_sigev_notify == (SIGEV_SIGNAL|SIGEV_THREAD_ID))
> +				timer->it_sigev_notify = SIGEV_SIGNAL;
> +			
> +			timer->it_process = timer->it_process->group_leader;
> +		} 
> +		spin_lock_irqrestore(&timer->it_lock, flags);
> +	}
> +}
> +
> +/*
>   * And now for the "clock" calls
>   *
>   * These functions are called both from timer functions (with the timer
> diff -uprN --exclude-from=/usr/local/bin/diffit.exclude linux-2.6.13-rc6/kernel/signal.c linux-2.6.13-rc6.work/kernel/signal.c
> --- linux-2.6.13-rc6/kernel/signal.c	2005-08-13 12:25:58.000000000 +0200
> +++ linux-2.6.13-rc6.work/kernel/signal.c	2005-08-20 17:57:46.000000000 +0200
> @@ -390,6 +390,7 @@ void __exit_signal(struct task_struct *t
>  		sig->nvcsw += tsk->nvcsw;
>  		sig->nivcsw += tsk->nivcsw;
>  		sig->sched_time += tsk->sched_time;
> +		exit_notify_itimers(sig);
>  		spin_unlock(&sighand->siglock);
>  		sig = NULL;	/* Marker for below.  */
>  	}
> @@ -1381,13 +1382,12 @@ send_sigqueue(int sig, struct sigqueue *
>  	int ret = 0;
>  
>  	/*
> -	 * We need the tasklist lock even for the specific
> -	 * thread case (when we don't need to follow the group
> -	 * lists) in order to avoid races with "p->sighand"
> -	 * going away or changing from under us.
> +	 * No need to hold tasklist lock here. 
> +	 * posix_timer_event() is synchronized via 
> +	 * exit_itimers() and exit_notify_itimers() to 
> +	 * be protected against p->sighand == NULL
>  	 */
>  	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> -	read_lock(&tasklist_lock);  
>  	spin_lock_irqsave(&p->sighand->siglock, flags);
>  	
>  	if (unlikely(!list_empty(&q->list))) {
> @@ -1414,7 +1414,6 @@ send_sigqueue(int sig, struct sigqueue *
>  
>  out:
>  	spin_unlock_irqrestore(&p->sighand->siglock, flags);
> -	read_unlock(&tasklist_lock);
>  	return(ret);
>  }
>  
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
George Anzinger   george@xxxxxxxxxx
HRT (High-res-timers):  http://sourceforge.net/projects/high-res-timers/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/