Re: Do we really need curr_target in signal_struct ?

[Rakib Mullick]

On Mon, Feb 3, 2014 at 10:39 PM, Oleg Nesterov <oleg@xxxxxxxxxx> 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.
--
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/