On 29/03/16(Tue) 22:36, Alexander Bluhm wrote:
> Hi,
>
> from a customer's system I got this panic:
>
> kernel diagnostic assertion "(kn->kn_status & KN_QUEUED) == 0" failed: file
> "..
> /../../../kern/kern_event.c", line 1071
>
> panic() at panic+0xfe
> __assert() at __assert+0x25
> knote_enqueue() at knote_enqueue+0x8c
> knote() at knote+0x47
> selwakeup() at selwakeup+0x1b
> logwakeup() at logwakeup+0x20
> log() at log+0xfc
> ...
> softclock() at softclock+0x315
> softintr_dispatch() at softintr_dispatch+0x7f
>
> When looking at the condition in KNOTE_ACTIVATE()
> if ((kn->kn_status & (KN_QUEUED | KN_DISABLED)) == 0)
> knote_enqueue(kn);
> and the assertion in knote_enqueue()
> KASSERT((kn->kn_status & KN_QUEUED) == 0);
> it is quite obvious that interrupts must be blocked between those.
>
> So put the splhigh() around KNOTE_ACTIVATE() and use a splassert()
> within knote_enqueue(). This is more or less where FreeBSD puts
> its KQ_LOCK().
Don't you want to raise the SPL around all the places where kn_status
is checked?
Is knote_dequeue() safe? Even if it is do we want to put a splassert()
in there to keep it in sync with knote_enqueue()?
I'm also wondering, don't you want to move the content of
KNOTE_ACTIVATE() to the function of the same name and also put an
splassert() there?
Are the splhigh() around kn_status in kqueue_register() safe?
> @@ -964,10 +971,14 @@ void
> knote(struct klist *list, long hint)
> {
> struct knote *kn, *kn0;
> + int s;
>
> - SLIST_FOREACH_SAFE(kn, list, kn_selnext, kn0)
> + SLIST_FOREACH_SAFE(kn, list, kn_selnext, kn0) {
> + s = splhigh();
> if (kn->kn_fop->f_event(kn, hint))
> KNOTE_ACTIVATE(kn);
> + splx(s);
> + }
> }
Here I'd simply take splhigh() for the whole loop.
BTW why are we using splhigh() and not softclock()?
