On Sat, Feb 04, 2023 at 01:24:58AM +0300, Vitaliy Makkoveev wrote:
> Hi,
>
> kevent(2) system call is ulocked more than year ago. Since select(2)
> and pselect(2) are kqueue(2)/kevent(2) wrappers, it makes sense to
> unlock them too. select(2) does the temporary kernel event queue
> initialization for this call only and does queue scan with events
> conversion between kevent(2) and select(2) data formats.
>
> pselect(2) is the same, but it also sets provided signal mask. sigset_t
> is unsigned int and only current thread does `p_sigmask' and `p_oldmask'
> modification, so dosigsuspend() call outside kernel lock is safe.
> claudio@, is this true? If so, I want to mark `p_sigmask' and
> `p_oldmask' as atomic in proc structure description.
p_sigmask is already marked atomic (and is accessed by other processes
concurrently in ptsignal()). p_oldmask is only accessed by curproc which
should probably be marked differently. Using atomic for a pure local
variable is wrong. Not sure if we have a special letter for this.
Now dosigsuspend() changes two states, p_flag and the p_sigmask.
p->p_oldmask = p->p_sigmask;
atomic_setbits_int(&p->p_flag, P_SIGSUSPEND);
p->p_sigmask = newmask;
and ptsignal() does this:
TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
...
/* skip threads that have the signal blocked */
if ((q->p_sigmask & mask) != 0)
continue;
...
if (q->p_flag & P_SIGSUSPEND)
break;
}
Since this would run concurrently with your change I wonder if the order
in dosigsupend() is the wrong way around. I think p->p_sigmask needs to be
set first. Currently the order does not matter since both ptsignal() and
dosigsuspend() use the KERNEL_LOCK.
If we change the order do we need some additional memory barriers to
ensure the compiler/cpu reorder the store order? It this enough to solve
the data dependency between dosigsuspend() and ptsignal()?
--
:wq Claudio
