forward_signal(): Assume td is never curthread

[olce]

Summary

All callers already ensure that. Add a KASSERT() checking for that, and
remove the actual code causing an early return in this case.

MFC after: 2 week
Sponsored by: The FreeBSD Foundation

[kib]

And why?

If you have an urge to remove something from this function, the forward_signal_enabled knob is probably long outlived its usefulness.

[olce]

In D42285#964836, @kib wrote:

And why?

This was just in passing. Why not? All callers treat the case of signaling curthread separately, so this code, however small, is just redundant code.

If you have an urge to remove something from this function, the forward_signal_enabled knob is probably long outlived its usefulness.

Great. Same commit, or a separate one (if this change can't be MFCed)?

[kib]

In D42285#965118, @olce.freebsd_certner.fr wrote:

In D42285#964836, @kib wrote:

And why?

This was just in passing. Why not? All callers treat the case of signaling curthread separately, so this code, however small, is just redundant code.

Because it makes the interface of the function quite weird.

In fact, I would prefer to push both running check and curthread check into the function. Also it would be nice to have stub for UP so that #ifdef SMP braces are removed from kern_sig.c, we are long past the time when SMP was a useful differentiator in generic code.

If you have an urge to remove something from this function, the forward_signal_enabled knob is probably long outlived its usefulness.

Great. Same commit, or a separate one (if this change can't be MFCed)?

[olce]

In D42285#965559, @kib wrote:

Because it makes the interface of the function quite weird.
In fact, I would prefer to push both running check and curthread check into the function. Also it would be nice to have stub for UP so that #ifdef SMP braces are removed from kern_sig.c, we are long past the time when SMP was a useful differentiator in generic code.

Mmm... It is called forward_signal(), so really I don't expect it to start determining if it should do it or not, but just do it. Pushing the checks into the function is a clear win for callers tdsigwakeup() and sig_suspend_threads() (because the guarding test there could then be suppressed), a bit less for thread_single() which skips a whole block of code containing forward_signal() if the thread is curthread.

But more importantly, my initial analysis seems wrong (and my smoke test not effective): The passed td apparently can be something else then curthread in some cases (see for example tdsendsignal() -> sig_suspend_threads() -> forward_signal()). Consequently, I now think that the test td2 != td guarding the call to forward_signal() in sig_suspend_threads() is wrong since it doesn't check for curthread.

So yes, I'll move the tests inside forward_signal(), and rename it if you don't object, probably to signal_maybe_preempt().

As a side note, lots of kernel functions (in the signal machinery and elsewhere) take a td argument that is often, but not always, assumed to be curthread, and sometimes it is a bit hard to follow, since there are no comments or assertions to indicate that precondition. I'll try to improve that later.

I should be mostly off next week, so this review will be stale on my side during that time. When back, and after updating it, I will perform more extensive tests.

[kib]

I think the current function name is fine.