diff mbox series

[v2] kernel: refactor: shorten has_pending_signals

Message ID 20260520062849.183621-2-andrea.calabrese@amarulasolutions.com
State New
Headers
Series [v2] kernel: refactor: shorten has_pending_signals |

Commit Message

Andrea Calabrese May 20, 2026, 6:28 a.m. UTC 
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(-)

Comments

'Abel Vesa' via Amarula Linux May 21, 2026, 12:50 p.m. UTC | #1 
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.
Andrea Calabrese May 21, 2026, 1:34 p.m. UTC | #2 
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.
Andrea Calabrese May 21, 2026, 2:35 p.m. UTC | #3 
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:
'Abel Vesa' via Amarula Linux May 22, 2026, 3:24 p.m. UTC | #4 
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.
Andrea Calabrese May 22, 2026, 5:24 p.m. UTC | #5 
> 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 mbox series

Patch

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))