Re: userret: take/release kernel lock once

[Scott Cheloha]

On Fri, Nov 05, 2021 at 03:15:26PM +0100, Christian Ludwig wrote:
> 
> comments inline.
> 
> [...]
> 
> Unlocking if (!need_lock) below looks odd. I think it would make more
> sense to reverse the logic:
> 
>       have_lock = 0;
> 
>       if (flags) {
>               if (!have_lock) {
>                       have_lock = 1;
>                       KERNEL_LOCK()
>               }
>       }
> 
>       if (have_lock)
>               KERNEL_UNLOCK();

... sure, but at that point we may as well use the more idiomatic
adjective "locked".

Updated patch attached.

Do you (or does anyone else) know whether there is a technical reason
for having discrete kernel lock sections here?  I am running with this
now without apparent issue.

Index: kern_sig.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.287
diff -u -p -r1.287 kern_sig.c
--- kern_sig.c  24 Oct 2021 00:02:25 -0000      1.287
+++ kern_sig.c  6 Nov 2021 19:05:58 -0000
@@ -1897,27 +1897,35 @@ filt_signal(struct knote *kn, long hint)
 void
 userret(struct proc *p)
 {
-       int signum;
+       int locked, signum;
+
+       locked = 0;
 
        /* send SIGPROF or SIGVTALRM if their timers interrupted this thread */
        if (p->p_flag & P_PROFPEND) {
                atomic_clearbits_int(&p->p_flag, P_PROFPEND);
-               KERNEL_LOCK();
+               if (!locked) {
+                       locked = 1;
+                       KERNEL_LOCK();
+               }
                psignal(p, SIGPROF);
-               KERNEL_UNLOCK();
        }
        if (p->p_flag & P_ALRMPEND) {
                atomic_clearbits_int(&p->p_flag, P_ALRMPEND);
-               KERNEL_LOCK();
+               if (!locked) {
+                       locked = 1;
+                       KERNEL_LOCK();
+               }
                psignal(p, SIGVTALRM);
-               KERNEL_UNLOCK();
        }
 
        if (SIGPENDING(p) != 0) {
-               KERNEL_LOCK();
+               if (!locked) {
+                       locked = 1;
+                       KERNEL_LOCK();
+               }
                while ((signum = cursig(p)) != 0)
                        postsig(p, signum);
-               KERNEL_UNLOCK();
        }
 
        /*
@@ -1929,12 +1937,16 @@ userret(struct proc *p)
        if (p->p_flag & P_SIGSUSPEND) {
                atomic_clearbits_int(&p->p_flag, P_SIGSUSPEND);
                p->p_sigmask = p->p_oldmask;
-
-               KERNEL_LOCK();
+               if (!locked) {
+                       locked = 1;
+                       KERNEL_LOCK();
+               }
                while ((signum = cursig(p)) != 0)
                        postsig(p, signum);
-               KERNEL_UNLOCK();
        }
+
+       if (locked)
+               KERNEL_UNLOCK();
 
        if (p->p_flag & P_SUSPSINGLE)
                single_thread_check(p, 0);