Messages in this thread Patch in this message | | | Date | Sat, 1 Feb 2014 00:53:42 +0600 | Subject | Re: Do we really need curr_target in signal_struct ? | From | Rakib Mullick <> |
| |
On Thu, Jan 30, 2014 at 1:02 PM, Rakib Mullick <rakib.mullick@gmail.com> wrote: > On Thu, Jan 30, 2014 at 12:32 AM, Oleg Nesterov <oleg@redhat.com> wrote: >> On 01/29, Rakib Mullick wrote: > >>> Are you thinking that , since things are not broken, then we shouldn't >>> try to do anything? >> >> Hmm. No. >> >> I am thinking that, since you misunderstood the purpose of ->curr_target, >> I should probably try to argue with your patch which blindly removes this >> optimization ? >> > Since the optimization (usages of ->curr_target) isn't perfect, so there's > chance of being misunderstood. This optimization is misleading too (I think), > cause curr_target don't have anything to do with wants_signal() and > ->curr_target is used only for this optimization and to get this optimization > needs to maintain it properly, and this maintenance does have cost and if > we don't get benefited too much, then it doesn't worth it (my pov). > >> I also think that this logic doesn't look perfect, but this is another >> story. > > Yes, this logic seems need to improve. > As you've made few points about the current logic of thread picking in complete_signal(), I took a look and found that using while_each_thread() can make things better than current. Although my initial intent of this thread wasn't related to complete_signal() logic, but as you've pointed out that things could be better, so I prepared the attached patch (just to address complete_signal()'s thread finding logic) not using ->curr_target but it's been kept. What do you think?
Thanks, Rakib diff --git a/kernel/signal.c b/kernel/signal.c index 52f881d..064f81f 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -944,7 +944,7 @@ static inline int wants_signal(int sig, struct task_struct *p) static void complete_signal(int sig, struct task_struct *p, int group) { struct signal_struct *signal = p->signal; - struct task_struct *t; + struct task_struct *t = p; /* * Now find a thread we can wake up to take the signal off the queue. @@ -952,33 +952,26 @@ static void complete_signal(int sig, struct task_struct *p, int group) * If the main thread wants the signal, it gets first crack. * Probably the least surprising to the average bear. */ - if (wants_signal(sig, p)) - t = p; - else if (!group || thread_group_empty(p)) - /* - * There is just one thread and it does not need to be woken. - * It will dequeue unblocked signals before it runs again. - */ - return; - else { - /* - * Otherwise try to find a suitable thread. - */ - t = signal->curr_target; - while (!wants_signal(sig, t)) { - t = next_thread(t); - if (t == signal->curr_target) - /* - * No thread needs to be woken. - * Any eligible threads will see - * the signal in the queue soon. - */ - return; + if (!group || thread_group_empty(p)) { + if (wants_signal(sig, t)) + goto found; + } else { + while_each_thread(p, t) { + if (wants_signal(sig, t)) + goto found; } - signal->curr_target = t; } /* + * No thread needs to be woken. + * Any eligible threads will see + * the signal in the queue soon. + */ + return; +found: + signal->curr_target = t; + + /* * Found a killable thread. If the signal will be fatal, * then start taking the whole group down immediately. */ | |