Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Ingo Molnar]

* Roland McGrath <roland@xxxxxxxxxx> wrote:

> > do you know any such code?
> 
> I was thinking it would look more like:
> 
>  	read_lock(&tasklist_lock);
>  	p = find_task_by_pid(pid);
>  	get_task_struct(p);
>  	read_unlock(&tasklist_lock);
> 	...
> 	spin_lock_irq(&p->sighand->siglock);
> 	...

the amount of potentially affected code (assuming all the locking is 
done in a single .[ch] file) is even smaller:

  kernel/posix-cpu-timers.c
  kernel/exit.c
  kernel/posix-timers.c
  include/linux/sched.h

checked these, we dont seem to do that.

> I am not sure whether such code exists or not.  It won't look quite 
> like that, in that the siglock use may be far away from the 
> extraction.  There are things that store pointers like that (like 
> real_timer.data, and posix_timers stuff).  But it may well be that all 
> those places take tasklist_lock before using the saved task_struct, in 
> which case it's fine. (Anything doing signals stuff usually needs 
> tasklist_lock anyway in case it has to traverse the thread group.)

this reminds me about the patch below: it gets rid of tasklist_lock use 
in the POSIX timer signal delivery critical path.

> You mean those are the files that use all three of those, or what?  
> That's clearly not the complete list of siglock uses.  Any code using 
> siglock needs to be grokked adequately to see if tasklist_lock is 
> always held around looking at ->sighand.

yeah.

	Ingo

---

dont use the tasklist_lock in the POSIX timer-delivery critical path.

Signed-off-by: Ingo Molnar <mingo@xxxxxxx>

--- linux/kernel/signal.c.orig
+++ linux/kernel/signal.c
@@ -1384,7 +1384,7 @@ send_sigqueue(int sig, struct sigqueue *
 	 * going away or changing from under us.
 	 */
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
-	read_lock(&tasklist_lock);  
+	task_lock(p);  
 	spin_lock_irqsave(&p->sighand->siglock, flags);
 	
 	if (unlikely(!list_empty(&q->list))) {
@@ -1411,7 +1411,7 @@ send_sigqueue(int sig, struct sigqueue *
 
 out:
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
-	read_unlock(&tasklist_lock);
+	task_unlock(p);
 	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/