> Date: Sat, 6 Nov 2021 14:10:57 -0500 > From: Scott Cheloha <scottchel...@gmail.com> > > 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.
To me this doesn't feel like an improvement. You replace straightforward code that clerly indicates which bits of the codepath aren't mpsafe yet with a more convoluted path. If this really makes a noticable difference, maybe we can consider it. But the in the end we should just make these calls mpsafe. > > 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); > >