Changeset View
Standalone View
sys/kern/kern_event.c
| Show First 20 Lines • Show All 819 Lines • ▼ Show 20 Lines | kqtimer_proc_continue(struct proc *p) | ||||
| PROC_LOCK_ASSERT(p, MA_OWNED); | PROC_LOCK_ASSERT(p, MA_OWNED); | ||||
| getboottimebin(&bt); | getboottimebin(&bt); | ||||
| now = bttosbt(bt); | now = bttosbt(bt); | ||||
| TAILQ_FOREACH_SAFE(kc, &p->p_kqtim_stop, link, kc1) { | TAILQ_FOREACH_SAFE(kc, &p->p_kqtim_stop, link, kc1) { | ||||
| TAILQ_REMOVE(&p->p_kqtim_stop, kc, link); | TAILQ_REMOVE(&p->p_kqtim_stop, kc, link); | ||||
| kc->flags &= ~KQ_TIMER_CB_ENQUEUED; | kc->flags &= ~KQ_TIMER_CB_ENQUEUED; | ||||
| if (kc->next <= now) | if (kc->next <= now) | ||||
markj: `now` is derived from getboottimebin(), isn't this bogus for non-NOTE_ABSTIME knotes? | |||||
Done Inline ActionsSorry, I do not follow. For any kind of timer (ABSTIME or not), kc->next records the sbintime (relative to the boottime) of the moment when the timer must be fired. At least this is my understanding, please see filt_timerstart(). kib: Sorry, I do not follow.
For any kind of timer (ABSTIME or not), kc->next records the sbintime… | |||||
Not Done Inline Actionsfilt_timerstart() records the sbintime relative to the uptime, the amount of time elapsed since the kernel booted. The boottime is the time that the kernel booted relative to the unix epoch. So here, the comparison kc->next <= now is always true in practice, for non-ABSTIME knotes. That is why the infinite loop happens: filt_timerexpire_l() should be setting kc->next to some time in the future, so kqtimer_proc_continue() should not visit the knote again, but it does anyway. I agree with your patch in any case, but this loop also looks wrong to me. markj: filt_timerstart() records the sbintime relative to the uptime, the amount of time elapsed since… | |||||
Done Inline ActionsDo you mean that now is incorrectly calculated there? Like this? diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c index 85b7b6c953af..a1cd409c53d9 100644 --- a/sys/kern/kern_event.c +++ b/sys/kern/kern_event.c @@ -814,14 +814,11 @@ void kqtimer_proc_continue(struct proc *p) { struct kq_timer_cb_data *kc, *kc1; - struct bintime bt; sbintime_t now; PROC_LOCK_ASSERT(p, MA_OWNED); - getboottimebin(&bt); - now = bttosbt(bt); - + now = sbinuptime(); TAILQ_FOREACH_SAFE(kc, &p->p_kqtim_stop, link, kc1) { TAILQ_REMOVE(&p->p_kqtim_stop, kc, link); kc->flags &= ~KQ_TIMER_CB_ENQUEUED; kib: Do you mean that now is incorrectly calculated there? Like this?
```
diff --git… | |||||
Done Inline ActionsI think so, yes. markj: I think so, yes. | |||||
Not Done Inline ActionsI verified that this change on its own fixes the loop triggered by the test program in PR 293141. markj: I verified that this change on its own fixes the loop triggered by the test program in PR… | |||||
Done Inline ActionsIn typical situation yes, but if the thread was descheduled for some time larger than the timer timeout, it might be not. kib: In typical situation yes, but if the thread was descheduled for some time larger than the timer… | |||||
| filt_timerexpire_l(kc->kn, true); | filt_timerexpire_l(kc->kn, true); | ||||
| else | else | ||||
| kqtimer_sched_callout(kc); | kqtimer_sched_callout(kc); | ||||
| } | } | ||||
| } | } | ||||
| static void | static void | ||||
| filt_timerexpire_l(struct knote *kn, bool proc_locked) | filt_timerexpire_l(struct knote *kn, bool proc_locked) | ||||
| { | { | ||||
| struct kq_timer_cb_data *kc; | struct kq_timer_cb_data *kc; | ||||
| struct proc *p; | struct proc *p; | ||||
| uint64_t delta; | uint64_t delta; | ||||
| sbintime_t now; | sbintime_t now; | ||||
| kc = kn->kn_ptr.p_v; | kc = kn->kn_ptr.p_v; | ||||
| if ((kn->kn_flags & EV_ONESHOT) != 0 || kc->to == 0) { | if ((kn->kn_flags & EV_ONESHOT) != 0 || kc->to == 0) { | ||||
| kn->kn_data++; | kn->kn_data++; | ||||
| KNOTE_ACTIVATE(kn, 0); | KNOTE_ACTIVATE(kn, 0); | ||||
| return; | return; | ||||
| } | } | ||||
| now = sbinuptime(); | now = sbinuptime(); | ||||
| if (now >= kc->next) { | if (now >= kc->next) { | ||||
Done Inline Actions... and isn't this wrong for NOTE_ABSTIME knotes? markj: ... and isn't this wrong for NOTE_ABSTIME knotes? | |||||
Done Inline ActionsMy comment here is wrong, I misunderstood the code. kc->next is always relative to the kernel's boot time. markj: My comment here is wrong, I misunderstood the code. `kc->next` is always relative to the… | |||||
| delta = (now - kc->next) / kc->to; | delta = (now - kc->next) / kc->to; | ||||
| if (delta == 0) | if (delta == 0) | ||||
| delta = 1; | delta = 1; | ||||
| kn->kn_data += delta; | kn->kn_data += delta; | ||||
| kc->next += delta * kc->to; | kc->next += delta * kc->to; | ||||
| if (now >= kc->next) /* overflow */ | if (now >= kc->next) /* overflow */ | ||||
| kc->next = now + kc->to; | kc->next = now + kc->to; | ||||
| KNOTE_ACTIVATE(kn, 0); /* XXX - handle locking */ | KNOTE_ACTIVATE(kn, 0); /* XXX - handle locking */ | ||||
| } | } | ||||
| /* | /* | ||||
| * Initial check for stopped kc->p is racy. It is fine to | * Initial check for stopped kc->p is racy. It is fine to | ||||
| * miss the set of the stop flags, at worst we would schedule | * miss the set of the stop flags, at worst we would schedule | ||||
| * one more callout. On the other hand, it is not fine to not | * one more callout. On the other hand, it is not fine to not | ||||
| * schedule when we we missed clearing of the flags, we | * schedule when we we missed clearing of the flags, we | ||||
| * recheck them under the lock and observe consistent state. | * recheck them under the lock and observe consistent state. | ||||
| */ | */ | ||||
| p = kc->p; | p = kc->p; | ||||
| if (P_SHOULDSTOP(p) || P_KILLED(p)) { | if (P_SHOULDSTOP(p) || P_KILLED(p)) { | ||||
| if (!proc_locked) | if (!proc_locked) | ||||
| PROC_LOCK(p); | PROC_LOCK(p); | ||||
| if (P_SHOULDSTOP(p) || P_KILLED(p)) { | if (P_SHOULDSTOP(p) || P_KILLED(p)) { | ||||
| if ((kc->flags & KQ_TIMER_CB_ENQUEUED) == 0) { | if ((kc->flags & KQ_TIMER_CB_ENQUEUED) == 0) { | ||||
| /* | |||||
| * Insert into head so that | |||||
| * kqtimer_proc_continue() does not | |||||
| * iterate into us again. | |||||
| */ | |||||
| kc->flags |= KQ_TIMER_CB_ENQUEUED; | kc->flags |= KQ_TIMER_CB_ENQUEUED; | ||||
| TAILQ_INSERT_TAIL(&p->p_kqtim_stop, kc, link); | TAILQ_INSERT_HEAD(&p->p_kqtim_stop, kc, link); | ||||
| } | } | ||||
| if (!proc_locked) | if (!proc_locked) | ||||
| PROC_UNLOCK(p); | PROC_UNLOCK(p); | ||||
| return; | return; | ||||
| } | } | ||||
| if (!proc_locked) | if (!proc_locked) | ||||
| PROC_UNLOCK(p); | PROC_UNLOCK(p); | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 2,492 Lines • Show Last 20 Lines | |||||
now is derived from getboottimebin(), isn't this bogus for non-NOTE_ABSTIME knotes?