Re: Do we really need curr_target in signal_struct ?
On 02/04, Rakib Mullick wrote: > > On Mon, Feb 3, 2014 at 10:39 PM, Oleg Nesterov 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(>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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 02/04, Rakib Mullick wrote: On Mon, Feb 3, 2014 at 10:39 PM, Oleg Nesterov o...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Mon, Feb 3, 2014 at 10:39 PM, Oleg Nesterov 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 02/02, Rakib Mullick wrote: > > On Sat, Feb 1, 2014 at 10:51 PM, Oleg Nesterov 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
Re: Do we really need curr_target in signal_struct ?
On 02/02, Rakib Mullick wrote: On Sat, Feb 1, 2014 at 10:51 PM, Oleg Nesterov o...@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
Re: Do we really need curr_target in signal_struct ?
On Mon, Feb 3, 2014 at 10:39 PM, Oleg Nesterov o...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Sat, Feb 1, 2014 at 10:51 PM, Oleg Nesterov wrote: > On 02/01, Rakib Mullick wrote: >> >> On Thu, Jan 30, 2014 at 1:02 PM, Rakib Mullick >> wrote: >> > On Thu, Jan 30, 2014 at 12:32 AM, Oleg Nesterov 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Sat, Feb 1, 2014 at 10:51 PM, Oleg Nesterov o...@redhat.com wrote: On 02/01, Rakib Mullick wrote: 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() 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 02/01, Rakib Mullick wrote: > > On Thu, Jan 30, 2014 at 1:02 PM, Rakib Mullick > wrote: > > On Thu, Jan 30, 2014 at 12:32 AM, Oleg Nesterov 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 02/01, Rakib Mullick wrote: 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() 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Thu, Jan 30, 2014 at 1:02 PM, Rakib Mullick wrote: > On Thu, Jan 30, 2014 at 12:32 AM, Oleg Nesterov 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. */
Re: Do we really need curr_target in signal_struct ?
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. */
Re: Do we really need curr_target in signal_struct ?
On Thu, Jan 30, 2014 at 12:32 AM, Oleg Nesterov 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 01/29, Rakib Mullick wrote: > > On Wed, Jan 29, 2014 at 8:55 PM, Oleg Nesterov wrote: > > On 01/29, Rakib Mullick wrote: > >> > >> On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Wed, Jan 29, 2014 at 8:55 PM, Oleg Nesterov wrote: > On 01/29, Rakib Mullick wrote: >> >> On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 01/29, Rakib Mullick wrote: > > On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 01/29, Rakib Mullick wrote: On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov o...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Wed, Jan 29, 2014 at 8:55 PM, Oleg Nesterov o...@redhat.com wrote: On 01/29, Rakib Mullick wrote: On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov o...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On 01/29, Rakib Mullick wrote: On Wed, Jan 29, 2014 at 8:55 PM, Oleg Nesterov o...@redhat.com wrote: On 01/29, Rakib Mullick wrote: On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov o...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
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. Thanks, Rakib -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Wed, Jan 29, 2014 at 10:09 AM, Rakib Mullick wrote: > On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Do we really need curr_target in signal_struct ?
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(>wait_chldexit); - sig->curr_target = tsk; + //sig->curr_target = tsk; init_sigpending(>shared_pending); INIT_LIST_HEAD(>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; } /* -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Do we really need curr_target in signal_struct ?
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; } /* -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov o...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Do we really need curr_target in signal_struct ?
On Wed, Jan 29, 2014 at 10:09 AM, Rakib Mullick rakib.mull...@gmail.com wrote: On Tue, Jan 28, 2014 at 10:43 PM, Oleg Nesterov o...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/