Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] signal: Skip the altstack update when not needed | From | Dave Hansen <> | Date | Thu, 9 Dec 2021 15:54:11 -0800 |
| |
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; >
| ![\](/images/icornerr.gif) |