Do we really need curr_target in signal_struct ?

Do we really need curr_target in signal_struct ?

[Rakib Mullick]

I was wondering what might be the possible use of curr_target in signal_struct (atleast,
it's not doing what it's comment says). Also, I'm not seeing any real use of it except
in kernel/signal.c::complete_signal() where it's use as loop breaking condition in thread's
list traversing. As an alternative of using curr_target we can use get_nr_thread() count
to get # of threads in a group and can remove curr_target completely. This will also help
us to get rid from overhead of maintaining ->curr_target at fork()/exit() path. Below is
rough patch, I can prepare a good one if everyone agrees.

Or we really need it?

---
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 485234d..1fd65b7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -554,7 +554,7 @@ struct signal_struct {
 	wait_queue_head_t	wait_chldexit;	/* for wait4() */
 
 	/* current thread group signal load-balancing target: */
-	struct task_struct	*curr_target;
+	//struct task_struct	*curr_target;
 
 	/* shared signal handling: */
 	struct sigpending	shared_pending;
diff --git a/kernel/exit.c b/kernel/exit.c
index 1e77fc6..4a2cf37 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -113,8 +113,8 @@ static void __exit_signal(struct task_struct *tsk)
 		if (sig->notify_count > 0 && !--sig->notify_count)
 			wake_up_process(sig->group_exit_task);
 
-		if (tsk == sig->curr_target)
-			sig->curr_target = next_thread(tsk);
+		//if (tsk == sig->curr_target)
+		//	sig->curr_target = next_thread(tsk);
 		/*
 		 * Accumulate here the counters for all threads but the
 		 * group leader as they die, so they can be added into
diff --git a/kernel/fork.c b/kernel/fork.c
index 2f11bbe..8de4928 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1041,7 +1041,7 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	tsk->thread_node = (struct list_head)LIST_HEAD_INIT(sig->thread_head);
 
 	init_waitqueue_head(&sig->wait_chldexit);
-	sig->curr_target = tsk;
+	//sig->curr_target = tsk;
 	init_sigpending(&sig->shared_pending);
 	INIT_LIST_HEAD(&sig->posix_timers);
 
diff --git a/kernel/signal.c b/kernel/signal.c
index 940b30e..1a1280a 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -945,6 +945,7 @@ static void complete_signal(int sig, struct task_struct *p, int group)
 {
 	struct signal_struct *signal = p->signal;
 	struct task_struct *t;
+	int i;
 
 	/*
 	 * Now find a thread we can wake up to take the signal off the queue.
@@ -961,21 +962,16 @@ static void complete_signal(int sig, struct task_struct *p, int group)
 		 */
 		return;
 	else {
-		/*
-		 * Otherwise try to find a suitable thread.
-		 */
-		t = signal->curr_target;
-		while (!wants_signal(sig, t)) {
+		i = get_nr_threads(p);
+		t = p;
+		do {
+			--i;
 			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.
-				 */
+			if (!i)
 				return;
-		}
-		signal->curr_target = t;
+		} while (!wants_signal(sig, t));
+	
+		//signal->curr_target = t;
 	}
 
 	/*

Re: Do we really need curr_target in signal_struct ?

[Oleg Nesterov]

On 01/28, Rakib Mullick wrote:
>
> As an alternative of using curr_target we can use get_nr_thread() count

We do not even need get_nr_thread() if we want to kill curr_target,

> @@ -961,21 +962,16 @@ static void complete_signal(int sig, struct task_struct *p, int group)
>  		 */
>  		return;
>  	else {
> -		/*
> -		 * Otherwise try to find a suitable thread.
> -		 */
> -		t = signal->curr_target;
> -		while (!wants_signal(sig, t)) {
> +		i = get_nr_threads(p);
> +		t = p;
> +		do {
> +			--i;
>  			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.
> -				 */
> +			if (!i)
>  				return;
> -		}
> -		signal->curr_target = t;
> +		} while (!wants_signal(sig, t));

You could simply do while_each_thread(p, t) to find a thread which
wants_signal(..).

But I guess ->curr_target was added exactly to avoid this loop if
possible, assuming that wants_signal(->current_targer) should be
likely true. Although perhaps this optimization is too simple.

Oleg.

Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/28, Rakib Mullick wrote:
>
> You could simply do while_each_thread(p, t) to find a thread which
> wants_signal(..).
>
Yes, while_each_thread() is much nicer than get_nr_thread(), thanks for
the pointer.

> But I guess ->curr_target was added exactly to avoid this loop if
> possible, assuming that wants_signal(->current_targer) should be
> likely true. Although perhaps this optimization is too simple.
>
Well, this code block will only hit when first check for wants_signal()
will miss, that means we need to find some other thread of the group.
AFAIU, ->current_target is only a loop breaker to avoid infinite loop,
but - by using while_each_thread() we can remove it completely, thus
helps to get rid from maintaining it too.

I'll prepare a proper patch with you suggestions for reviewing.

Thanks,
Rakib

Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

On Wed, Jan 29, 2014 at 10:09 AM, Rakib Mullick <rakib.mullick@gmail.com> wrote:
> On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 01/28, Rakib Mullick wrote:
>>
>> You could simply do while_each_thread(p, t) to find a thread which
>> wants_signal(..).
>>
> Yes, while_each_thread() is much nicer than get_nr_thread(), thanks for
> the pointer.
>
Or, should we use for_each_thread()? Just noticed that, you've plan to remove
while_each_thread().

Thanks,
Rakib

Re: Do we really need curr_target in signal_struct ?

[Oleg Nesterov]

On 01/29, Rakib Mullick wrote:
>
> On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> > But I guess ->curr_target was added exactly to avoid this loop if
> > possible, assuming that wants_signal(->current_targer) should be
> > likely true. Although perhaps this optimization is too simple.
> >
> Well, this code block will only hit when first check for wants_signal()
> will miss,

Yes,

> that means we need to find some other thread of the group.

Yes,

> AFAIU, ->current_target is only a loop breaker to avoid infinite loop,

No. It caches the last result of "find a thread which can handle this
group-wide signal".

> but - by using while_each_thread() we can remove it completely, thus
> helps to get rid from maintaining it too.

... and remove the optimization above.

> I'll prepare a proper patch with you suggestions for reviewing.

I am not sure we want this patch. Once again, I do not know how much
->curr_target helps, and certainaly it can't help always. But you
should not blindly remove it just because yes, sure, it is not strictly
needed to find a wants_signal() thread.

Oleg.

Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

On Wed, Jan 29, 2014 at 8:55 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/29, Rakib Mullick wrote:
>>
>> On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>>
>
>> AFAIU, ->current_target is only a loop breaker to avoid infinite loop,
>
> No. It caches the last result of "find a thread which can handle this
> group-wide signal".
>
The reason behind of my understanding is the following comments:

                                /*
                                 * No thread needs to be woken.
                                 * Any eligible threads will see
                                 * the signal in the queue soon.
                                 */

What if, there's no thread in a group wants_signal()? Or it can't
practically happen?

>> but - by using while_each_thread() we can remove it completely, thus
>> helps to get rid from maintaining it too.
>
> ... and remove the optimization above.
>
>> I'll prepare a proper patch with you suggestions for reviewing.
>
> I am not sure we want this patch. Once again, I do not know how much
> ->curr_target helps, and certainaly it can't help always. But you
> should not blindly remove it just because yes, sure, it is not strictly
> needed to find a wants_signal() thread.
>
Are you thinking that , since things are not broken, then we shouldn't
try to do anything?

Thanks,
Rakib

Re: Do we really need curr_target in signal_struct ?

[Oleg Nesterov]

On 01/29, Rakib Mullick wrote:
>
> On Wed, Jan 29, 2014 at 8:55 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 01/29, Rakib Mullick wrote:
> >>
> >> On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >>
> >
> >> AFAIU, ->current_target is only a loop breaker to avoid infinite loop,
> >
> > No. It caches the last result of "find a thread which can handle this
> > group-wide signal".
> >
> The reason behind of my understanding is the following comments:
>
>                                 /*
>                                  * No thread needs to be woken.
>                                  * Any eligible threads will see
>                                  * the signal in the queue soon.
>                                  */
>
> What if, there's no thread in a group wants_signal()?

then complete_signal() returns without signal_wake_up().

> Or it can't
> practically happen?

It can. Say, all threads has blocked this signal. And other reasons.

> >> but - by using while_each_thread() we can remove it completely, thus
> >> helps to get rid from maintaining it too.
> >
> > ... and remove the optimization above.
> >
> >> I'll prepare a proper patch with you suggestions for reviewing.
> >
> > I am not sure we want this patch. Once again, I do not know how much
> > ->curr_target helps, and certainaly it can't help always. But you
> > should not blindly remove it just because yes, sure, it is not strictly
> > needed to find a wants_signal() thread.
> >
> 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 ?

I also think that this logic doesn't look perfect, but this is another
story.

Oleg.

Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

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.

Thanks,
Rakib

Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

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

[-- Attachment #2: signal_completion.patch --]
[-- Type: text/x-patch, Size: 1742 bytes --]

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.
 	 */

Re: Do we really need curr_target in signal_struct ?

[Oleg Nesterov]

On 02/01, Rakib Mullick wrote:
>
> 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()

can't understand... curr_target is obviously connected to wants_signal() ?
As I already said it caches the last wants_signal(t) thread?

> > 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).

but you need to prove this somehow.

> I took a look and found that using while_each_thread()
> can make things better than current.

Why?

> What do you think?

The patch is technically wrong, a group-wide signal doesn't check all
threads after this change. And I do not understand why complete_signal()
still updates ->curr_target. Plus thread_group_empty() doesn't buy too
much if we use while_each_thread(). But this doesn't really matter, easy
to fix.

Rakib. It is not that I like ->curr_target very much. But let me repeat,
if you want to remove this optimization you need to explain why it doesn't
make sense. You claim that this "can make things better" without any
argument.

As for me - I simply do not know. This logic is very old, I am not even
sure that the current usage of ->curr_signal matches the original intent.
But it can obviously help in some cases, and you need to explain why we
do not care.

So I won't argue if you submit the technically correct patch, but you
need to convince Andrew or Ingo to take it. I think the right change in
complete_signal() is something like below. It can be even simpler and use
the single do/while loop, but then we need to check "group" inside that
loop. With the change below ->curr_target can be simply removed.

Oleg.

--- x/kernel/signal.c
+++ x/kernel/signal.c
@@ -944,7 +944,7 @@ static inline int wants_signal(int sig, 
 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,32 +952,16 @@ static void complete_signal(int sig, str
 	 * 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 (!wants_signal(sig, t)) {
+		if (group) {
+			while_each_thread(p, t) {
+				if (wants_signal(sig, t))
+					goto notify;
+			}
 		}
-		signal->curr_target = t;
+		return;
 	}
-
+ notify:
 	/*
 	 * Found a killable thread.  If the signal will be fatal,
 	 * then start taking the whole group down immediately.

Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

On Sat, Feb 1, 2014 at 10:51 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/01, Rakib Mullick wrote:
>>
>> 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()
>
> can't understand... curr_target is obviously connected to wants_signal() ?
No. I meant, curr_target doesn't makes sure that it really wants the
signal, it's likely
choice.

> As I already said it caches the last wants_signal(t) thread?
Yes, you said. But, gets messed up at exit path, not useful everytime.
If fails, p gets checked twice.

>> > 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).
>
> but you need to prove this somehow.
>
Right, I'll try, I'll do it as per my understanding and everyone might
not agree.

>> I took a look and found that using while_each_thread()
>> can make things better than current.
>
> Why?
>
using while_each_thread() we can start thread traversing from p, which
is a likely
pick and also gets away from redundant checking of p.

>> What do you think?
>
> The patch is technically wrong, a group-wide signal doesn't check all
> threads after this change.
If group is empty, we don't need to check other than t.

> And I do not understand why complete_signal()
> still updates ->curr_target.
Didn't want to remove it blindly :-/

> Plus thread_group_empty() doesn't buy too
> much if we use while_each_thread(). But this doesn't really matter, easy
> to fix.
>
> Rakib. It is not that I like ->curr_target very much. But let me repeat,
> if you want to remove this optimization you need to explain why it doesn't
> make sense. You claim that this "can make things better" without any
> argument.
>
Well, I had other way of looking at it and didn't find any real usages
of ->curr_target and got confused though the code comment in curr_target.

> As for me - I simply do not know. This logic is very old, I am not even
> sure that the current usage of ->curr_signal matches the original intent.
> But it can obviously help in some cases, and you need to explain why we
> do not care.
>
Actually, this is exactly what i wanted to know. What is the purpose
of ->curr_signal, *do we really need it?* If I knew or seen any reason
I'd have never asked this question. You guys knows this domain better than
me, that's why I asked. Git log didn't help much, neither it's documentation
. As a result, I asked and thought about cleaning it up, (as i rarely do if
it meets my capability of course), so appended a sort of rough patch.

> So I won't argue if you submit the technically correct patch, but you
> need to convince Andrew or Ingo to take it. I think the right change in
> complete_signal() is something like below. It can be even simpler and use
> the single do/while loop, but then we need to check "group" inside that
> loop. With the change below ->curr_target can be simply removed.
>
I'll try to make points to convince Andrew or Ingo, I'll try to show
up my points,
and the rest will be upon them.

And here's what i think about ->curr_target removal (as per my understanding):

 a) ->curr_target is only might be useful in group wide case.
 b) Usually signals are sent particularly to a thread so ->curr_signal
 doesn't makes sense.
 c) If ->curr_signal pointed thread is killed, curr_signal points to
the next thread,
but nothing can make sure that it's a right pick.
 d) also current in implementation p is checked twice.
 e) One use case of ->curr_signal is, it always points to the lastly
created thread,
as it's a better candidate for want_signal cause newly created thread don't
usually have any signal pending. We can acheive the same without ->curr_signal
by traversing thread group from the lastly created thread.

So, this is what I think. Let me know if these reason's looks reasonable to you,
cause before Ingo or Andrew taking it, it requires your ack.

Thanks,
Rakib.

Re: Do we really need curr_target in signal_struct ?

[Oleg Nesterov]

On 02/02, Rakib Mullick wrote:
>
> On Sat, Feb 1, 2014 at 10:51 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 02/01, Rakib Mullick wrote:
> >>
> >> > 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()
> >
> > can't understand... curr_target is obviously connected to wants_signal() ?
> No. I meant, curr_target doesn't makes sure that it really wants the
> signal, it's likely
> choice.

Sure. But why this is "misleading" and "don't have anything to do with
wants_signal()" ? OK, nevermind.

> > As I already said it caches the last wants_signal(t) thread?
> Yes, you said. But, gets messed up at exit path, not useful everytime.

Yes.

> If fails, p gets checked twice.

Yes, but this is minor, I think.

> >> I took a look and found that using while_each_thread()
> >> can make things better than current.
> >
> > Why?
> >
> using while_each_thread() we can start thread traversing from p, which
> is a likely
> pick and also gets away from redundant checking of p.

Heh. We always check "p" first. And (in general) we do not want to start
from "p" if we want to find a wants_signal() thread, and ->curr_target can
help to avoid this.

> >> What do you think?
> >
> > The patch is technically wrong, a group-wide signal doesn't check all
> > threads after this change.
> If group is empty, we don't need to check other than t.

I didn't meant the thread_group_empty() case. Please look at your code:


	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;
		}
	}

Suppose that group == T, thread_group_empty(p) == F. Suppose that all
sub-threads except "p" blocked this signal. With this change "p" (and
thus the whole thread group) won't be notified. IOW, with your change
we do not check "p" at all. This is wrong.

> > Rakib. It is not that I like ->curr_target very much. But let me repeat,
> > if you want to remove this optimization you need to explain why it doesn't
> > make sense. You claim that this "can make things better" without any
> > argument.
> >
> Well, I had other way of looking at it and didn't find any real usages
> of ->curr_target and got confused though the code comment in curr_target.

The only user of ->curr_target is complete_signal(), you have found it.

> > As for me - I simply do not know. This logic is very old, I am not even
> > sure that the current usage of ->curr_signal matches the original intent.
> > But it can obviously help in some cases, and you need to explain why we
> > do not care.
> >
> Actually, this is exactly what i wanted to know. What is the purpose
> of ->curr_signal, *do we really need it?* If I knew or seen any reason
> I'd have never asked this question. You guys knows this domain better than
> me, that's why I asked.

I can only read the current code. I do not know the original intent.

> > So I won't argue if you submit the technically correct patch, but you
> > need to convince Andrew or Ingo to take it. I think the right change in
> > complete_signal() is something like below. It can be even simpler and use
> > the single do/while loop, but then we need to check "group" inside that
> > loop. With the change below ->curr_target can be simply removed.
> >
> I'll try to make points to convince Andrew or Ingo, I'll try to show
> up my points,
> and the rest will be upon them.
>
> And here's what i think about ->curr_target removal (as per my understanding):
>
>  a) ->curr_target is only might be useful in group wide case.

Yes.

>  b) Usually signals are sent particularly to a thread

Really?

> so ->curr_signal
> doesn't makes sense.

Well. I do not know what else I can say ;)

>  c) If ->curr_signal pointed thread is killed, curr_signal points to
> the next thread,
> but nothing can make sure that it's a right pick.

Yes (except a thread can't be killed), so what? Obviously, if ->curr_targer
exits we should update this pointer. We could even nullify it.

>  d) also current in implementation p is checked twice.

Yes, "p" can be checked twice. I don't think this is that bad, and I
do not think this particular "problem" should be fixed.

>  e) One use case of ->curr_signal is, it always points to the lastly
> created thread,
> as it's a better candidate for want_signal cause newly created thread don't
> usually have any signal pending.

I simply can't understand. Why? I do not think so.

> We can acheive the same without ->curr_signal
> by traversing thread group from the lastly created thread.

We certainly can't "achieve the same" this way, although I am not sure
what this "the same" actually means.

> So, this is what I think. Let me know if these reason's looks reasonable to you,

No. Contrary, whatever I personally think about ->curr_signal, I feel
that you do not understand the code you are trying to change. Sorry,
I can be wrong. But I still do not see any argument.

> cause before Ingo or Andrew taking it, it requires your ack.

Not really. And of course I'll review the patch correctness-wise, and
I already sent the change in complete_signal() which looks right to me.

But I am not going to ack the behaviour change, simply because I have
no idea how this can impact the existing applications. Perhaps nobody
will notice this change, but we can't know this.

Oleg.

Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

On Mon, Feb 3, 2014 at 10:39 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/02, Rakib Mullick wrote:
>
>> > As I already said it caches the last wants_signal(t) thread?
>> Yes, you said. But, gets messed up at exit path, not useful everytime.
>
> Yes.
>
>> If fails, p gets checked twice.
>
> Yes, but this is minor, I think.
>
Right.

>> >> I took a look and found that using while_each_thread()
>> >> can make things better than current.
>> >
>> > Why?
>> >
>> using while_each_thread() we can start thread traversing from p, which
>> is a likely
>> pick and also gets away from redundant checking of p.
>
> Heh. We always check "p" first. And (in general) we do not want to start
> from "p" if we want to find a wants_signal() thread, and ->curr_target can
> help to avoid this.
>
>> >> What do you think?
>> >
>> > The patch is technically wrong, a group-wide signal doesn't check all
>> > threads after this change.
>> If group is empty, we don't need to check other than t.
>
> I didn't meant the thread_group_empty() case. Please look at your code:
>
>
>         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;
>                 }
>         }
>
> Suppose that group == T, thread_group_empty(p) == F. Suppose that all
> sub-threads except "p" blocked this signal. With this change "p" (and
> thus the whole thread group) won't be notified. IOW, with your change
> we do not check "p" at all. This is wrong.
>
Oh, sorry, my bad. That was wrong.

> The only user of ->curr_target is complete_signal(), you have found it.
>
Indeed.

>
> I can only read the current code. I do not know the original intent.
>
This is where things are confusing.


> Really?
>
Sometimes, 100% correct (!group case) ;-).

>
> Yes (except a thread can't be killed), so what? Obviously, if ->curr_targer
> exits we should update this pointer. We could even nullify it.
>
That's makes ->curr_target less useful, that's what I meant.

>
> Yes, "p" can be checked twice. I don't think this is that bad, and I
> do not think this particular "problem" should be fixed.
>
Yes, it's minor.

>
> I simply can't understand. Why? I do not think so.
>
Cause, want_signal logic checks these thread attributes to find whether it's
eligible or not.

>> We can acheive the same without ->curr_signal
>> by traversing thread group from the lastly created thread.
>
> We certainly can't "achieve the same" this way, although I am not sure
> what this "the same" actually means.
>
>> So, this is what I think. Let me know if these reason's looks reasonable to you,
>
> No. Contrary, whatever I personally think about ->curr_signal, I feel
> that you do not understand the code you are trying to change. Sorry,
> I can be wrong. But I still do not see any argument.
>
Yes, right. I do not fully understand this code, also how it exactly puts impact
on signaling subsystems. And, therefore, I think I should not make any
changes in this code.

>> cause before Ingo or Andrew taking it, it requires your ack.
>
> Not really. And of course I'll review the patch correctness-wise, and
> I already sent the change in complete_signal() which looks right to me.
>
> But I am not going to ack the behaviour change, simply because I have
> no idea how this can impact the existing applications. Perhaps nobody
> will notice this change, but we can't know this.
>
Yes, I'm not also sure about the behavior change and it's impact over
existing applications, so, I'm skipping it.

I usually try to make small fixes, cleanup; cause it's less error-prone and
requires less follow-up. Since the things here becoming sort of "don't know"
thing, I think I should stop. But, thank you for helping and replying in this
thread.

Again thanks,
Rakib.

Re: Do we really need curr_target in signal_struct ?

[Oleg Nesterov]

On 02/04, Rakib Mullick wrote:
>
> On Mon, Feb 3, 2014 at 10:39 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> > I can only read the current code. I do not know the original intent.
> >
> This is where things are confusing.

Yes, I agree.

Once again, I can understand what this code does, but I am not sure
I understand why, and I am not sure this logic was actually "designed".
The usual problem with the ancient code.

> > I simply can't understand. Why? I do not think so.
> >
> Cause, want_signal logic checks these thread attributes to find whether it's
> eligible or not.

Ah, wants_signal()->signal_pending() doesn't mean "eligible".
sigismember(&p->blocked) does mean.

This signal_pending() checks allows to notify multiple threads, so that
they can run the signal handlers in parallel. And otoh, if signal_pending()
is true then we obviously do not need signal_wake_up().

> And, therefore, I think I should not make any
> changes in this code.

No ;) not at all.

We all do mistakes, and in this particular case I am not even 100% sure
I was right.

> > But I am not going to ack the behaviour change, simply because I have
> > no idea how this can impact the existing applications. Perhaps nobody
> > will notice this change, but we can't know this.
> >
> Yes, I'm not also sure about the behavior change and it's impact over
> existing applications, so, I'm skipping it.

Yes, this is the main reason why I disliked this change from the very
beginning.

Oleg.