On 01/05/19(Wed) 16:35, David Gwynne wrote:
> i originally came at this from the other side, where i wanted to run
> kqueue_enqueue and _dequeue without the KERNEL_LOCK, but that implied
> making kqueue_scan use the mutex too, which allowed the syscall to
> become less locked.
>
> it assumes that the existing locking in kqueue_scan is in the right
> place, it just turns it into a mutex instead of KERNEL_LOCK with
> splhigh. it leaves the kqueue_register code under KERNEL_LOCK, but if
> you're not making changes with kevent then this should be a win.
>
> there's an extra rwlock around the kqueue_scan call. this protects the
> kq_head list from having multiple marker structs attached to it. that is
> an extremely rare situation, ie, you'd have to have two threads execute
> kevent on the same kq fd concurrently, but that never happens. right?
If you look at the archives on bugs@ that can happen. The diff that has
been backed out exposed an issue possibly related to multiple threads
playing with the queue.
> it seems to work ok, but i havent tested it extensively.
>
> thoughts?
I believe removing the KERNEL_LOCK() from the syscall should be done in a
second step, see below.
> @@ -796,10 +810,12 @@ start:
> nkev++;
> if (kn->kn_flags & EV_ONESHOT) {
> kn->kn_status &= ~KN_QUEUED;
> - splx(s);
> + mtx_leave(&kq->kq_mtx);
> + KERNEL_LOCK();
> kn->kn_fop->f_detach(kn);
> + KERNEL_UNLOCK();
> knote_drop(kn, p);
> - s = splhigh();
> + mtx_enter(&kq->kq_mtx);
By releasing the mutex here aren't we allowing multiple callers to play
with the queue? Is it safe? Does that mean that another thread might
ends up removing the existing marker?
Please look at my previous change "Make it possible for multiple
threads to enter kqueue_scan() in parallel" which is based on the
approach taken by Dfly. I believe there's many more dragons in there
:o)
