| 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:
On 05/21, Andrea Calabrese wrote: > > Looking for a suggestion here: I have the objdumps before and after, > and I see that the assembly with this modification does change. OK, lets forget it, lets stay with V2. Oleg. To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
> Il giorno 22 mag 2026, alle ore 17:24, Oleg Nesterov <oleg@redhat.com> ha scritto: > > On 05/21, Andrea Calabrese wrote: >> >> Looking for a suggestion here: I have the objdumps before and after, >> and I see that the assembly with this modification does change. > > OK, lets forget it, lets stay with V2. > > Oleg. > Thank you! Andrea To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
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(-)