| Message ID | 20260520062849.183621-2-andrea.calabrese@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 05/20, Andrea Calabrese wrote: > > static inline bool has_pending_signals(sigset_t *signal, sigset_t *blocked) > { > - unsigned long ready; > - long i; > - > - switch (_NSIG_WORDS) { > - default: > - for (i = _NSIG_WORDS, ready = 0; --i >= 0 ;) > - ready |= signal->sig[i] &~ blocked->sig[i]; > - break; > - > - case 4: ready = signal->sig[3] &~ blocked->sig[3]; > - ready |= signal->sig[2] &~ blocked->sig[2]; > - ready |= signal->sig[1] &~ blocked->sig[1]; > - ready |= signal->sig[0] &~ blocked->sig[0]; > - break; > - > - case 2: ready = signal->sig[1] &~ blocked->sig[1]; > - ready |= signal->sig[0] &~ blocked->sig[0]; > - break; > - > - case 1: ready = signal->sig[0] &~ blocked->sig[0]; > - } > - return ready != 0; > + unsigned long ready = 0; > + for (long i = 0; i < _NSIG_WORDS; i++) > + ready |= signal->sig[i] & ~blocked->sig[i]; > + return ready != 0; > } Note that with your patch the main loop doesn't stop when ready becomes != 0... OK, I guess the modern compilers doesn't need this hint even if _NSIG_WORDS > 1, so Acked-by: Oleg Nesterov <oleg@redhat.com> But since your patch is just a cleanup, you could do for (int i = 0; i < _NSIG_WORDS; i++) { if (signal->sig[i] & ~blocked->sig[i]) return true; } return false; but this is subjective, I won't insist. Oleg. To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On Thu, 2026-05-21 at 14:50 +0200, Oleg Nesterov wrote: > On 05/20, Andrea Calabrese wrote: > > > > static inline bool has_pending_signals(sigset_t *signal, sigset_t > > *blocked) > > { > > - unsigned long ready; > > - long i; > > - > > - switch (_NSIG_WORDS) { > > - default: > > - for (i = _NSIG_WORDS, ready = 0; --i >= 0 ;) > > - ready |= signal->sig[i] &~ blocked- > > >sig[i]; > > - break; > > - > > - case 4: ready = signal->sig[3] &~ blocked->sig[3]; > > - ready |= signal->sig[2] &~ blocked->sig[2]; > > - ready |= signal->sig[1] &~ blocked->sig[1]; > > - ready |= signal->sig[0] &~ blocked->sig[0]; > > - break; > > - > > - case 2: ready = signal->sig[1] &~ blocked->sig[1]; > > - ready |= signal->sig[0] &~ blocked->sig[0]; > > - break; > > - > > - case 1: ready = signal->sig[0] &~ blocked->sig[0]; > > - } > > - return ready != 0; > > + unsigned long ready = 0; > > + for (long i = 0; i < _NSIG_WORDS; i++) > > + ready |= signal->sig[i] & ~blocked->sig[i]; > > + return ready != 0; > > } > > Note that with your patch the main loop doesn't stop when ready > becomes != 0... > > OK, I guess the modern compilers doesn't need this hint even if > _NSIG_WORDS > 1, so > > Acked-by: Oleg Nesterov <oleg@redhat.com> > > > But since your patch is just a cleanup, you could do > > for (int i = 0; i < _NSIG_WORDS; i++) { > if (signal->sig[i] & ~blocked->sig[i]) > return true; > } > > return false; > Good idea, I will send a v3 with this! > but this is subjective, I won't insist. > > Oleg. To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On Thu, 2026-05-21 at 15:34 +0200, Andrea Calabrese wrote: > On Thu, 2026-05-21 at 14:50 +0200, Oleg Nesterov wrote: > > On 05/20, Andrea Calabrese wrote: > > > > > > static inline bool has_pending_signals(sigset_t *signal, > > > sigset_t > > > *blocked) > > > { > > > - unsigned long ready; > > > - long i; > > > - > > > - switch (_NSIG_WORDS) { > > > - default: > > > - for (i = _NSIG_WORDS, ready = 0; --i >= 0 ;) > > > - ready |= signal->sig[i] &~ blocked- > > > > sig[i]; > > > - break; > > > - > > > - case 4: ready = signal->sig[3] &~ blocked->sig[3]; > > > - ready |= signal->sig[2] &~ blocked->sig[2]; > > > - ready |= signal->sig[1] &~ blocked->sig[1]; > > > - ready |= signal->sig[0] &~ blocked->sig[0]; > > > - break; > > > - > > > - case 2: ready = signal->sig[1] &~ blocked->sig[1]; > > > - ready |= signal->sig[0] &~ blocked->sig[0]; > > > - break; > > > - > > > - case 1: ready = signal->sig[0] &~ blocked->sig[0]; > > > - } > > > - return ready != 0; > > > + unsigned long ready = 0; > > > + for (long i = 0; i < _NSIG_WORDS; i++) > > > + ready |= signal->sig[i] & ~blocked->sig[i]; > > > + return ready != 0; > > > } > > > > Note that with your patch the main loop doesn't stop when ready > > becomes != 0... > > > > OK, I guess the modern compilers doesn't need this hint even if > > _NSIG_WORDS > 1, so > > > > Acked-by: Oleg Nesterov <oleg@redhat.com> > > > > > > But since your patch is just a cleanup, you could do > > > > for (int i = 0; i < _NSIG_WORDS; i++) { > > if (signal->sig[i] & ~blocked->sig[i]) > > return true; > > } > > > > return false; > > > Good idea, I will send a v3 with this! > Looking for a suggestion here: I have the objdumps before and after, and I see that the assembly with this modification does change. I am not sure whether the new one is ok in terms of performance, as it adds one more instruction. Based on the objdumps, I have the situation before the change:
diff --git a/kernel/signal.c b/kernel/signal.c index 2d102e025883..799ee98cf03e 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -130,28 +130,10 @@ static bool sig_ignored(struct task_struct *t, int sig, bool force) */ static inline bool has_pending_signals(sigset_t *signal, sigset_t *blocked) { - unsigned long ready; - long i; - - switch (_NSIG_WORDS) { - default: - for (i = _NSIG_WORDS, ready = 0; --i >= 0 ;) - ready |= signal->sig[i] &~ blocked->sig[i]; - break; - - case 4: ready = signal->sig[3] &~ blocked->sig[3]; - ready |= signal->sig[2] &~ blocked->sig[2]; - ready |= signal->sig[1] &~ blocked->sig[1]; - ready |= signal->sig[0] &~ blocked->sig[0]; - break; - - case 2: ready = signal->sig[1] &~ blocked->sig[1]; - ready |= signal->sig[0] &~ blocked->sig[0]; - break; - - case 1: ready = signal->sig[0] &~ blocked->sig[0]; - } - return ready != 0; + unsigned long ready = 0; + for (long i = 0; i < _NSIG_WORDS; i++) + ready |= signal->sig[i] & ~blocked->sig[i]; + return ready != 0; } #define PENDING(p,b) has_pending_signals(&(p)->signal, (b))
In has_pending_signals there was a switch/case used for optimizations. However, today's compilers perform loop unrolling efficiently, thus it is not needed anymore. put i inside the for declaration so we do not risk its escape from the scope. Moreover, i starts now from 0 and counts up, as it is a more usual pattern. Signed-off-by: Andrea Calabrese <andrea.calabrese@amarulasolutions.com> --- The difference with V1 is in the description, now accurate thanks to the review. As before, the patch has been tested on gcc x86 and generates the same code. kernel/signal.c | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-)