Details

Reviewers

Commits

rG2fd1ffefaa4d: Stop arming kqueue timers on knote owner suspend or terminate
rG533e5057ed25: Add helper for kqueue timers callout scheduling
rG4d27d8d2f3b8: Stop arming realtime posix process timers on suspend or terminate
rGdc47fdf1319f: Stop arming periodic process timers on suspend or terminate

Summary

This way, even if the process specified very tight reschedule intervals, it should be stoppable/killable.

This patch requires at least one improvement. I calculate the count of skipped events for kq timers, but not for posix for timer_getoverrun(). I suspect that this can benefit from your other change, than can be refactored to be used there as well.

Per-commit view is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/timers

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib requested review of this revision.Mar 6 2021, 3:25 AM

kib created this revision.

I find it strange to disable periodic timers when the process is stopped. Applications may require information about the passage of time even if they're stopped. There is even an example in the Linux timerfd man page which demonstrates that their periodic timer continues firing when the owning process is stopped. Is the intent to ensure that a debugger can be attached?

Is Linux timerfd passable? If yes, this is different from kqueue.

In D29106#652008, @kib wrote:

Is Linux timerfd passable? If yes, this is different from kqueue.

I can't find an answer without looking at the sources. But consider an application with an event loop that uses EVFILT_TIMER to periodically fetch some statistics. Such a program could easily detect and normalize missing events due to SIGSTOP, but now will behave incorrectly. At least this should be documented, but IMO the new behaviour is surprising and will probably break some applications.

In D29106#652011, @markj wrote:

In D29106#652008, @kib wrote:

Is Linux timerfd passable? If yes, this is different from kqueue.

I can't find an answer without looking at the sources. But consider an application with an event loop that uses EVFILT_TIMER to periodically fetch some statistics. Such a program could easily detect and normalize missing events due to SIGSTOP, but now will behave incorrectly. At least this should be documented, but IMO the new behaviour is surprising and will probably break some applications.

What do you mean by 'behaving incorrectly'? How would it detect that events are missed due to SIGSTOP? Note that I increment kn_data by the number of the schedule intervals between prev and current moments, which is exactly the number of missed knote_activate() calls.

https://gist.github.com/12332ccd0876d9d95264e6e434549585
https://gist.github.com/ac6cba0501246a1c96ba4420a690dc3c
are the test programs I used

Start the program, then ^Z it, then continue after some time.

In D29106#652012, @kib wrote:

In D29106#652011, @markj wrote:

In D29106#652008, @kib wrote:

Is Linux timerfd passable? If yes, this is different from kqueue.

I can't find an answer without looking at the sources. But consider an application with an event loop that uses EVFILT_TIMER to periodically fetch some statistics. Such a program could easily detect and normalize missing events due to SIGSTOP, but now will behave incorrectly. At least this should be documented, but IMO the new behaviour is surprising and will probably break some applications.

What do you mean by 'behaving incorrectly'? How would it detect that events are missed due to SIGSTOP? Note that I increment kn_data by the number of the schedule intervals between prev and current moments, which is exactly the number of missed knote_activate() calls.

Sorry, I misunderstood part of the patch. If kn_data is updated as you say then my comment does not apply.

markj added inline comments.Mar 8 2021, 10:39 PM

sys/kern/kern_event.c
695	I would use "continue" instead of the past tense "continued".
704	Why not sbinuptime()?
750	Consider using a local variable for `kc->p`.
sys/kern/kern_time.c
908	This will use `it_interval` to set the precision, but suppose it_interval is not set, and the process was stopped and started again before the timeout expires. Then we will use a zero-valued it_interval to calculate the precision for the callout. Really I think we should not be scheduling a callout at all if one is still pending at this point.

kib marked 4 inline comments as done.Mar 8 2021, 11:39 PM

kib added inline comments.

sys/kern/kern_event.c
704	sbinuptime() returns relative time since boot, while getboottimebin() provides absolute time. Timers are set in abs time, and I need it there.
sys/kern/kern_time.c
908	If it_interval is not set, then P2_ITSTOPPED is not set, and itimer_proc_continue() is not called. I only stop periodic (rescheduling) timers, one-shot timers are left alone for stopped process. And for periodic, I do not cancel scheduled callout, I let it execute and observe the process state, then decide if it needs to reschedule. Cancellation is simply not reliable, esp. while network people do not even consider other users and make incompatible KBI changes without handling all callers.

continued->continue
use local p for kc->p

I still owe the calculation of itimer overrun when stopped, but lets agree on the approach first.

emaste added a subscriber: emaste.Mar 9 2021, 3:08 PM

Also stop posix realtime timers

I'm sorry for the delay.

In D29106#652487, @kib wrote:

I still owe the calculation of itimer overrun when stopped, but lets agree on the approach first.

The overall approach? I reread the patch again and I'm ok with it.

sys/kern/kern_time.c
913	I think we need to verify that `p->p_itimers != NULL` first.

In D29106#664294, @markj wrote:

I'm sorry for the delay.

In D29106#652487, @kib wrote:

I still owe the calculation of itimer overrun when stopped, but lets agree on the approach first.

The overall approach? I reread the patch again and I'm ok with it.