> Date: Sat, 6 Nov 2021 14:10:57 -0500
> From: Scott Cheloha <[email protected]>
>
> 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);
>
>
