On Thu, Jan 30, 2014 at 1:02 PM, Rakib Mullick <rakib.mull...@gmail.com> wrote:
> On Thu, Jan 30, 2014 at 12:32 AM, Oleg Nesterov <o...@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.
*/
