Re: kq_count earlier!
On Mon, Oct 09, 2017 at 12:15:46PM +0200, Martin Pieuchot wrote:
> To prevent an infinite loop, threads looking for events inside
> kqueue_scan(), insert a `marker' in the list. Such markers are
> not accounted and they are removed from the list as soon as the
> thread is finished or goes to sleep.
>
> Diff below is a small cleanup to keep the accounting of events in
> sync with the number of events on the list. This is a noop for the
> moment, but it's small & easy part to review of my upcoming diff.
>
> ok?
OK bluhm@
>
> Index: kern/kern_event.c
> ===
> RCS file: /cvs/src/sys/kern/kern_event.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 kern_event.c
> --- kern/kern_event.c 31 May 2017 14:52:05 - 1.79
> +++ kern/kern_event.c 9 Oct 2017 09:57:54 -
> @@ -760,22 +760,24 @@ start:
> TAILQ_INSERT_TAIL(>kq_head, , kn_tqe);
> while (count) {
> kn = TAILQ_FIRST(>kq_head);
> - TAILQ_REMOVE(>kq_head, kn, kn_tqe);
> if (kn == ) {
> + TAILQ_REMOVE(>kq_head, kn, kn_tqe);
> splx(s);
> if (count == maxevents)
> goto retry;
> goto done;
> }
> +
> + TAILQ_REMOVE(>kq_head, kn, kn_tqe);
> + kq->kq_count--;
> +
> if (kn->kn_status & KN_DISABLED) {
> kn->kn_status &= ~KN_QUEUED;
> - kq->kq_count--;
> continue;
> }
> if ((kn->kn_flags & EV_ONESHOT) == 0 &&
> kn->kn_fop->f_event(kn, 0) == 0) {
> kn->kn_status &= ~(KN_QUEUED | KN_ACTIVE);
> - kq->kq_count--;
> continue;
> }
> *kevp = kn->kn_kevent;
> @@ -783,7 +785,6 @@ start:
> nkev++;
> if (kn->kn_flags & EV_ONESHOT) {
> kn->kn_status &= ~KN_QUEUED;
> - kq->kq_count--;
> splx(s);
> kn->kn_fop->f_detach(kn);
> knote_drop(kn, p, p->p_fd);
> @@ -796,9 +797,9 @@ start:
> if (kn->kn_flags & EV_DISPATCH)
> kn->kn_status |= KN_DISABLED;
> kn->kn_status &= ~(KN_QUEUED | KN_ACTIVE);
> - kq->kq_count--;
> } else {
> TAILQ_INSERT_TAIL(>kq_head, kn, kn_tqe);
> + kq->kq_count++;
> }
> count--;
> if (nkev == KQ_NEVENTS) {
Re: kq_count earlier!
On 09/10/17(Mon) 14:35, Joerg Sonnenberger wrote: > On Mon, Oct 09, 2017 at 12:27:50PM +0200, Martin Pieuchot wrote: > > On 09/10/17(Mon) 12:22, Joerg Sonnenberger wrote: > > > On Mon, Oct 09, 2017 at 12:15:46PM +0200, Martin Pieuchot wrote: > > > > Diff below is a small cleanup to keep the accounting of events in > > > > sync with the number of events on the list. This is a noop for the > > > > moment, but it's small & easy part to review of my upcoming diff. > > > > > > Well, not counting the markers allows skipping the scan when no real > > > content is on the list. That's a non-trivial condition if the list is > > > scanned by multiple threads and started non-empty. > > > > I don't understand if you're suggesting something or not. Did you spot > > something wrong in the diff I sent? > > You are removing optimisation potential. Counting only real events is > more useful than counting the entries on the list. You clearly didn't read my diff. I'm not changing anything.
Re: kq_count earlier!
On Mon, Oct 09, 2017 at 12:27:50PM +0200, Martin Pieuchot wrote: > On 09/10/17(Mon) 12:22, Joerg Sonnenberger wrote: > > On Mon, Oct 09, 2017 at 12:15:46PM +0200, Martin Pieuchot wrote: > > > Diff below is a small cleanup to keep the accounting of events in > > > sync with the number of events on the list. This is a noop for the > > > moment, but it's small & easy part to review of my upcoming diff. > > > > Well, not counting the markers allows skipping the scan when no real > > content is on the list. That's a non-trivial condition if the list is > > scanned by multiple threads and started non-empty. > > I don't understand if you're suggesting something or not. Did you spot > something wrong in the diff I sent? You are removing optimisation potential. Counting only real events is more useful than counting the entries on the list. Joerg
Re: kq_count earlier!
On 09/10/17(Mon) 12:22, Joerg Sonnenberger wrote: > On Mon, Oct 09, 2017 at 12:15:46PM +0200, Martin Pieuchot wrote: > > Diff below is a small cleanup to keep the accounting of events in > > sync with the number of events on the list. This is a noop for the > > moment, but it's small & easy part to review of my upcoming diff. > > Well, not counting the markers allows skipping the scan when no real > content is on the list. That's a non-trivial condition if the list is > scanned by multiple threads and started non-empty. I don't understand if you're suggesting something or not. Did you spot something wrong in the diff I sent?
Re: kq_count earlier!
On Mon, Oct 09, 2017 at 12:15:46PM +0200, Martin Pieuchot wrote: > Diff below is a small cleanup to keep the accounting of events in > sync with the number of events on the list. This is a noop for the > moment, but it's small & easy part to review of my upcoming diff. Well, not counting the markers allows skipping the scan when no real content is on the list. That's a non-trivial condition if the list is scanned by multiple threads and started non-empty. Joerg
kq_count earlier!
To prevent an infinite loop, threads looking for events inside
kqueue_scan(), insert a `marker' in the list. Such markers are
not accounted and they are removed from the list as soon as the
thread is finished or goes to sleep.
Diff below is a small cleanup to keep the accounting of events in
sync with the number of events on the list. This is a noop for the
moment, but it's small & easy part to review of my upcoming diff.
ok?
Index: kern/kern_event.c
===
RCS file: /cvs/src/sys/kern/kern_event.c,v
retrieving revision 1.79
diff -u -p -r1.79 kern_event.c
--- kern/kern_event.c 31 May 2017 14:52:05 - 1.79
+++ kern/kern_event.c 9 Oct 2017 09:57:54 -
@@ -760,22 +760,24 @@ start:
TAILQ_INSERT_TAIL(>kq_head, , kn_tqe);
while (count) {
kn = TAILQ_FIRST(>kq_head);
- TAILQ_REMOVE(>kq_head, kn, kn_tqe);
if (kn == ) {
+ TAILQ_REMOVE(>kq_head, kn, kn_tqe);
splx(s);
if (count == maxevents)
goto retry;
goto done;
}
+
+ TAILQ_REMOVE(>kq_head, kn, kn_tqe);
+ kq->kq_count--;
+
if (kn->kn_status & KN_DISABLED) {
kn->kn_status &= ~KN_QUEUED;
- kq->kq_count--;
continue;
}
if ((kn->kn_flags & EV_ONESHOT) == 0 &&
kn->kn_fop->f_event(kn, 0) == 0) {
kn->kn_status &= ~(KN_QUEUED | KN_ACTIVE);
- kq->kq_count--;
continue;
}
*kevp = kn->kn_kevent;
@@ -783,7 +785,6 @@ start:
nkev++;
if (kn->kn_flags & EV_ONESHOT) {
kn->kn_status &= ~KN_QUEUED;
- kq->kq_count--;
splx(s);
kn->kn_fop->f_detach(kn);
knote_drop(kn, p, p->p_fd);
@@ -796,9 +797,9 @@ start:
if (kn->kn_flags & EV_DISPATCH)
kn->kn_status |= KN_DISABLED;
kn->kn_status &= ~(KN_QUEUED | KN_ACTIVE);
- kq->kq_count--;
} else {
TAILQ_INSERT_TAIL(>kq_head, kn, kn_tqe);
+ kq->kq_count++;
}
count--;
if (nkev == KQ_NEVENTS) {
