[PATCH] signal: Skip the altstack update when not needed

[PATCH] signal: Skip the altstack update when not needed

[Chang S. Bae]

New x86 FPU features require a large signal stack for their large states.
Instead of requiring a large stack for every process, make sure enough
altstack both at sys_sigaltstack() and when enabling the feature in each
process.

The optional size check was added. It helps to reject a too-small altstack
when the large feature is enabled. Also, the architecture code examines
each thread's altstack large enough before enabling the feature.

But threads can be racy without a lock. So, this enforcement mechanism
accompanies a lock to serialize altstack updates and the size check.

On the signal return path, the altstack is restored via do_sigaltstack().
In fact, the threads without altstack ensure it is disabled there. While no
altstack change is needed in this case, this call ends up obtaining the
lock.

When multiple signal returns hit the lock at the same time, this
unnecessarily increases the lock contention.

Add a new check to avoid this. Check if an altstack update is needed. If
not, skip the lock and the update. This may help sys_sigaltstack() in
general. So place it in the function.

Reported-by: kernel test robot <oliver.sang@intel.com>
Fixes: 3aac3ebea08f ("x86/signal: Implement sigaltstack size validation")
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
---
 kernel/signal.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/signal.c b/kernel/signal.c
index a629b11bf3e0..eeb634f954cd 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -4185,6 +4185,11 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
 				ss_mode != 0))
 			return -EINVAL;
 
+		if (t->sas_ss_sp == (unsigned long)ss_sp &&
+		    t->sas_ss_size == ss_size &&
+		    t->sas_ss_flags == ss_flags)
+			return 0;
+
 		sigaltstack_lock();
 		if (ss_mode == SS_DISABLE) {
 			ss_size = 0;
-- 
2.17.1

Re: [PATCH] signal: Skip the altstack update when not needed

[Dave Hansen]

On 12/9/21 3:24 PM, Chang S. Bae wrote:
> New x86 FPU features require a large signal stack for their large states.
> Instead of requiring a large stack for every process, make sure enough
> altstack both at sys_sigaltstack() and when enabling the feature in each
> process.

This is jumping into the imperative voice when describing the
background.  It's rather jarring

> The optional size check was added. It helps to reject a too-small altstack
> when the large feature is enabled. Also, the architecture code examines
> each thread's altstack large enough before enabling the feature.
> 
> But threads can be racy without a lock. So, this enforcement mechanism
> accompanies a lock to serialize altstack updates and the size check.
> 
> On the signal return path, the altstack is restored via do_sigaltstack().
> In fact, the threads without altstack ensure it is disabled there. While no
> altstack change is needed in this case, this call ends up obtaining the
> lock.
> 
> When multiple signal returns hit the lock at the same time, this
> unnecessarily increases the lock contention.
> 
> Add a new check to avoid this. Check if an altstack update is needed. If
> not, skip the lock and the update. This may help sys_sigaltstack() in
> general. So place it in the function.

How about:

== Background ==

Support for large, "dynamic" fpstates was recently merged.  This
included code to ensure that sigaltstacks are sufficiently sized for
these large states.  A new lock was added to remove races between
enabling large features and setting up sigaltstacks.

== Problem ==

The new lock (sigaltstack_lock()) is acquired in the sigreturn path
before restoring the old sigaltstack.  Unfortunately, contention on the
new lock causes a measurable signal handling performance regression[link
here].  However, the common case is that no *changes* are made to the
sigaltstack state at sigreturn.

== Solution ==

do_sigaltstack() acquires sigaltstack_lock() and is used for both
sys_sigaltstack() and restoring the sigaltstack in sys_sigreturn().
Check for changes to the sigaltstack before taking the lock.  If no
changes were made, return before acquiring the lock.

This removes lock contention from the common-case sigreturn path.


> diff --git a/kernel/signal.c b/kernel/signal.c
> index a629b11bf3e0..eeb634f954cd 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -4185,6 +4185,11 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp,
>  				ss_mode != 0))
>  			return -EINVAL;
>  
> +		if (t->sas_ss_sp == (unsigned long)ss_sp &&
> +		    t->sas_ss_size == ss_size &&
> +		    t->sas_ss_flags == ss_flags)
> +			return 0;

This needs something like:

		/*
		 * Return before taking any locks if no actual
		 * sigaltstack changes were requested.
		 */

>  		sigaltstack_lock();
>  		if (ss_mode == SS_DISABLE) {
>  			ss_size = 0;
>