Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 23, 2015, 9:57 a.m., Matt Jordan wrote: Ship It! As an addendum to this, this patch does not merge cleanly on Asterisk 11. As such, I'll be merging this to Asterisk 13+. Alas, as I don't run a system that can compile kqueue, it's kind of hard for me to backport that. If someone would like a backport of this patch to 11, a review should be put up for that branch. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14766 --- On March 18, 2015, 11:44 a.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 18, 2015, 11:44 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 27, 2015, 9:41 a.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433574 Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14766 --- Ship it! Ship It! - Matt Jordan On March 18, 2015, 11:44 a.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 18, 2015, 11:44 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 18, 2015, 4:44 p.m.) Review request for Asterisk Developers. Changes --- Address review comments. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs (updated) - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 16, 2015, 3:18 a.m., Matt Jordan wrote:
/trunk/res/res_timing_kqueue.c, lines 76-83
https://reviewboard.asterisk.org/r/4465/diff/2/?file=72098#file72098line76
For structure packing, you may want to declare this as:
struct kqueue_timer {
int handle;
intptr_t period;
#ifndef EVFILT_USER
int continuous_fd;
unsigned int contuous_fd_valid:1;
#endif
unsigned int is_continuous:1;
}
I suppose the spec may allow it to be smaller, but sizeof(intptr_t) is
typically = sizeof(int).
- Justin
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4465/#review14696
---
On March 13, 2015, 2:07 a.m., Justin T. Gibbs wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4465/
---
(Updated March 13, 2015, 2:07 a.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24857
https://issues.asterisk.org/jira/browse/ASTERISK-24857
Repository: Asterisk
Description
---
Update the kqueue timing module to conform to current timer API.
This fixes issues with using the kqueue timing source on Asterisk 13
on FreeBSD 10.
res_timing_kqueue.c:
Remove support for kevent64(). The values used to support Asterisk
timers fit within 32bits and so can be handled on all platforms via
kevent().
Provide debug logging for, but do not track, unacked events. This
matches the behavior of all other timer implementations.
Implement continuous mode by triggering and leaving active, a user
event. This ensures that the file descriptor for the timer returns
immediately from poll(), without placing the load of a high speed
timer on the kernel.
In kqueue_timer_get_max_rate(), don't overstate the capability of
the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX,
the largest integer type kqueue supports for timers.
In kqueue_timer_get_event(), assume the caller woke up from poll()
and just return the mode the timer is currently in. This matches
all other timer implementations.
Adjust the test code now that unacked events are not tracked.
Diffs
-
/trunk/res/res_timing_kqueue.c 432637
Diff: https://reviewboard.asterisk.org/r/4465/diff/
Testing
---
Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing
calls, voicemail prompts and recordings. All of the above failed without
these changes.
Thanks,
Justin T. Gibbs
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4465/#review14696
---
Thanks for the patch - there's a few other kqueue related issues floating
around out there that this patch may help address.
I didn't review this for correctness, merely for coding guideline violations.
Assuming someone else who can run res_timing_kqueue can confirm that it works
and that the tests pass, I'll be happy to Ship It! once the guidelines are
cleaned up.
/trunk/res/res_timing_kqueue.c
https://reviewboard.asterisk.org/r/4465/#comment25245
For structure packing, you may want to declare this as:
struct kqueue_timer {
int handle;
intptr_t period;
#ifndef EVFILT_USER
int continuous_fd;
unsigned int contuous_fd_valid:1;
#endif
unsigned int is_continuous:1;
}
/trunk/res/res_timing_kqueue.c
https://reviewboard.asterisk.org/r/4465/#comment25244
Nitpick: in the Asterisk project, we don't place the return type of the
function or its scope on a separate line.
You can check out the coding guidelines here:
https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines
/trunk/res/res_timing_kqueue.c
https://reviewboard.asterisk.org/r/4465/#comment25246
Just use ast_debug(X, ...) here.
Since this appears to be a pretty 'low level' debug message, a debug level
of 5 is probably appropriate.
/trunk/res/res_timing_kqueue.c
https://reviewboard.asterisk.org/r/4465/#comment25247
And ast_debug(5, ...); here as well
/trunk/res/res_timing_kqueue.c
https://reviewboard.asterisk.org/r/4465/#comment25248
ast_debug(5, ...); here as well
/trunk/res/res_timing_kqueue.c
https://reviewboard.asterisk.org/r/4465/#comment25249
ast_debug(5, ...); here
- Matt Jordan
On March 12, 2015, 9:07 p.m., Justin T. Gibbs wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4465/
---
(Updated March 12, 2015, 9:07 p.m.)
Review request for Asterisk Developers.
Bugs: ASTERISK-24857
https://issues.asterisk.org/jira/browse/ASTERISK-24857
Repository: Asterisk
Description
---
Update the kqueue timing module to conform to current timer API.
This fixes issues with using the kqueue timing source on Asterisk 13
on FreeBSD 10.
res_timing_kqueue.c:
Remove support for kevent64(). The values used to support Asterisk
timers fit within 32bits and so can be handled on all platforms via
kevent().
Provide debug logging for, but do not track, unacked events. This
matches the behavior of all other timer implementations.
Implement continuous mode by triggering and leaving active, a user
event. This ensures that the file descriptor for the timer returns
immediately from poll(), without placing the load of a high speed
timer on the kernel.
In kqueue_timer_get_max_rate(), don't overstate the capability of
the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX,
the largest integer type kqueue supports for timers.
In kqueue_timer_get_event(), assume the caller woke up from poll()
and just return the mode the timer is currently in. This matches
all other timer implementations.
Adjust the test code now that unacked events are not tracked.
Diffs
-
/trunk/res/res_timing_kqueue.c 432637
Diff: https://reviewboard.asterisk.org/r/4465/diff/
Testing
---
Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing
calls, voicemail prompts and recordings. All of the above failed without
these changes.
Thanks,
Justin T. Gibbs
--
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 10, 2015, 1:26 p.m., Ed Hynan wrote: /trunk/res/res_timing_kqueue.c, line 240 https://reviewboard.asterisk.org/r/4465/diff/1/?file=71888#file71888line240 Portability: at least EVFILT_USER and NOTE_TRIGGER are not defined on all kqueue(2) systems. Justin T. Gibbs wrote: Which platforms are you referring to? OS-X added support in 10.6. Why they haven't updated their man pages is anyones guess: http://www.opensource.apple.com/source/xnu/xnu-1699.24.23/tools/tests/xnu_quick_test/kqueue_tests.c Ed Hynan wrote: OpenBSD and NetBSD. These do not have the user event filter. Ed Hynan wrote: I adapted a test program so see if uncollected EVFILT_TIMER events can make poll() return -- reliably. After testing on FreeBSD 9.0 and OpenBSD 5.5 (leaving NetBSD alone unless something seemed promising), I find that both will fail to return from poll initially; also both can be 'kicked' with a signal or a EVFILT_READ event -- but the result differs on the two systems, and this is undocumented and almost certainly undefined behavior and a side effect (and I assume the EVFILT_USER event was just a similar 'kick'). I haven't studied this timing code, so I shouldn't say much, but *if* res_timing_kqueue.c expects to return a kqueue fd for use with poll, and that poll will wake for EVFILT_TIMER expire events, then that won't work (judging by the test program trying to do just that). The technique in res_timing_pthread.c looks promising: return the read end of a pipe, signal poll by writing a byte, unsignal by consuming the byte (works in the test), and just using EVFILT_TIMER for the ticks. Justin T. Gibbs wrote: I'm not sure what you mean by 'kick'. I believe that EVFILT_TIMER events are reliable. If the application is slow to consume timer events, the kevent will accumulate them and the file descriptor for the kqueue() cause poll to return immediately. The goal of this change is to ensure that continuous mode is cheap and activates the file descriptor immediately. Setting the timer to run as quickly as possible just burns CPU cycles and means continuous mode blocks until the timer goes off at least once. Depending on the timer resolution of the system, this could be a long time. I need to do some more testing with the original driver to quantify this, but I believe this was the cause of my failures on FreeBSD. I coded up a version to use EVFILT_READ on a file descriptor of a closed pipe. This worked on FreeBSD. But since it consumes another fd, I think its still better to use EVFLT_USER if it is available. Ed Hynan wrote: Yes, EVFILT_TIMER events are reliable. I mean the interaction of kqueue with a timer and poll -- the expectation that poll will return for a kqueue timer expiration. kqueue_timer_fd(), assigned to ast_timing_interface kqueue_timing.timer_fd in svn trunk source, returns the kqueue descriptor (timer-handle) -- this code was not present in 11.* sources, so I haven't had the possible problems, but *if* the descriptor returned from ast_timing_interface kqueue_timing.timer_fd() is used with poll (e.g. main/timing.c), expectations might/will fail to be met. By kick I mean that in tests after poll initially fails to return for kqueue timer expiration, some other events e.g. a signal might be followed by poll suddenly starting to return for kqueue timer expiration -- like something stuck was kicked loose. My guess is that the user event you added was doing just that; but it seems to be unexpected side effect behavior. Regardless of all that, the idea that it's better to use EVFLT_USER if it is available would lead to substantially different code for the kqueue systems. Of course, it's up to asterisk-devs whether that is acceptable (I'm a contributor). I was wrong that poll does not return for EVFILT_TIMER, and of course that EVFILT_USER was some unexpected trigger. I had quickly added code to an existing test program, and failed to make sure the kqueue fd was assigned the kqueue() return when passed to poll -- I was observing poll on the wrong descriptor. Reduce my statements to the issue of portability, and pardon the noise. - Ed --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14623 --- On March 13, 2015, 2:07 a.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 13, 2015, 2:07 a.m.) Review request for Asterisk
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 10, 2015, 1:26 p.m., Ed Hynan wrote: /trunk/res/res_timing_kqueue.c, line 240 https://reviewboard.asterisk.org/r/4465/diff/1/?file=71888#file71888line240 Portability: at least EVFILT_USER and NOTE_TRIGGER are not defined on all kqueue(2) systems. Justin T. Gibbs wrote: Which platforms are you referring to? OS-X added support in 10.6. Why they haven't updated their man pages is anyones guess: http://www.opensource.apple.com/source/xnu/xnu-1699.24.23/tools/tests/xnu_quick_test/kqueue_tests.c Ed Hynan wrote: OpenBSD and NetBSD. These do not have the user event filter. Ed Hynan wrote: I adapted a test program so see if uncollected EVFILT_TIMER events can make poll() return -- reliably. After testing on FreeBSD 9.0 and OpenBSD 5.5 (leaving NetBSD alone unless something seemed promising), I find that both will fail to return from poll initially; also both can be 'kicked' with a signal or a EVFILT_READ event -- but the result differs on the two systems, and this is undocumented and almost certainly undefined behavior and a side effect (and I assume the EVFILT_USER event was just a similar 'kick'). I haven't studied this timing code, so I shouldn't say much, but *if* res_timing_kqueue.c expects to return a kqueue fd for use with poll, and that poll will wake for EVFILT_TIMER expire events, then that won't work (judging by the test program trying to do just that). The technique in res_timing_pthread.c looks promising: return the read end of a pipe, signal poll by writing a byte, unsignal by consuming the byte (works in the test), and just using EVFILT_TIMER for the ticks. Justin T. Gibbs wrote: I'm not sure what you mean by 'kick'. I believe that EVFILT_TIMER events are reliable. If the application is slow to consume timer events, the kevent will accumulate them and the file descriptor for the kqueue() cause poll to return immediately. The goal of this change is to ensure that continuous mode is cheap and activates the file descriptor immediately. Setting the timer to run as quickly as possible just burns CPU cycles and means continuous mode blocks until the timer goes off at least once. Depending on the timer resolution of the system, this could be a long time. I need to do some more testing with the original driver to quantify this, but I believe this was the cause of my failures on FreeBSD. I coded up a version to use EVFILT_READ on a file descriptor of a closed pipe. This worked on FreeBSD. But since it consumes another fd, I think its still better to use EVFLT_USER if it is available. Yes, EVFILT_TIMER events are reliable. I mean the interaction of kqueue with a timer and poll -- the expectation that poll will return for a kqueue timer expiration. kqueue_timer_fd(), assigned to ast_timing_interface kqueue_timing.timer_fd in svn trunk source, returns the kqueue descriptor (timer-handle) -- this code was not present in 11.* sources, so I haven't had the possible problems, but *if* the descriptor returned from ast_timing_interface kqueue_timing.timer_fd() is used with poll (e.g. main/timing.c), expectations might/will fail to be met. By kick I mean that in tests after poll initially fails to return for kqueue timer expiration, some other events e.g. a signal might be followed by poll suddenly starting to return for kqueue timer expiration -- like something stuck was kicked loose. My guess is that the user event you added was doing just that; but it seems to be unexpected side effect behavior. Regardless of all that, the idea that it's better to use EVFLT_USER if it is available would lead to substantially different code for the kqueue systems. Of course, it's up to asterisk-devs whether that is acceptable (I'm a contributor). - Ed --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14623 --- On March 13, 2015, 2:07 a.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 13, 2015, 2:07 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent().
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 13, 2015, 2:07 a.m.) Review request for Asterisk Developers. Changes --- If EVFLT_USER isn't available, fallback to EVFLT_READ against a file descriptor from a closed pipe to keep a kqueue timer continuously active. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs (updated) - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 10, 2015, 1:26 p.m., Ed Hynan wrote: /trunk/res/res_timing_kqueue.c, line 240 https://reviewboard.asterisk.org/r/4465/diff/1/?file=71888#file71888line240 Portability: at least EVFILT_USER and NOTE_TRIGGER are not defined on all kqueue(2) systems. Justin T. Gibbs wrote: Which platforms are you referring to? OS-X added support in 10.6. Why they haven't updated their man pages is anyones guess: http://www.opensource.apple.com/source/xnu/xnu-1699.24.23/tools/tests/xnu_quick_test/kqueue_tests.c Ed Hynan wrote: OpenBSD and NetBSD. These do not have the user event filter. Ed Hynan wrote: I adapted a test program so see if uncollected EVFILT_TIMER events can make poll() return -- reliably. After testing on FreeBSD 9.0 and OpenBSD 5.5 (leaving NetBSD alone unless something seemed promising), I find that both will fail to return from poll initially; also both can be 'kicked' with a signal or a EVFILT_READ event -- but the result differs on the two systems, and this is undocumented and almost certainly undefined behavior and a side effect (and I assume the EVFILT_USER event was just a similar 'kick'). I haven't studied this timing code, so I shouldn't say much, but *if* res_timing_kqueue.c expects to return a kqueue fd for use with poll, and that poll will wake for EVFILT_TIMER expire events, then that won't work (judging by the test program trying to do just that). The technique in res_timing_pthread.c looks promising: return the read end of a pipe, signal poll by writing a byte, unsignal by consuming the byte (works in the test), and just using EVFILT_TIMER for the ticks. I'm not sure what you mean by 'kick'. I believe that EVFILT_TIMER events are reliable. If the application is slow to consume timer events, the kevent will accumulate them and the file descriptor for the kqueue() cause poll to return immediately. The goal of this change is to ensure that continuous mode is cheap and activates the file descriptor immediately. Setting the timer to run as quickly as possible just burns CPU cycles and means continuous mode blocks until the timer goes off at least once. Depending on the timer resolution of the system, this could be a long time. I need to do some more testing with the original driver to quantify this, but I believe this was the cause of my failures on FreeBSD. I coded up a version to use EVFILT_READ on a file descriptor of a closed pipe. This worked on FreeBSD. But since it consumes another fd, I think its still better to use EVFLT_USER if it is available. - Justin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14623 --- On March 13, 2015, 2:07 a.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 13, 2015, 2:07 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs --
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 10, 2015, 1:26 p.m., Ed Hynan wrote: /trunk/res/res_timing_kqueue.c, line 240 https://reviewboard.asterisk.org/r/4465/diff/1/?file=71888#file71888line240 Portability: at least EVFILT_USER and NOTE_TRIGGER are not defined on all kqueue(2) systems. Justin T. Gibbs wrote: Which platforms are you referring to? OS-X added support in 10.6. Why they haven't updated their man pages is anyones guess: http://www.opensource.apple.com/source/xnu/xnu-1699.24.23/tools/tests/xnu_quick_test/kqueue_tests.c OpenBSD and NetBSD. These do not have the user event filter. - Ed --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14623 --- On March 9, 2015, 6:21 p.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 9, 2015, 6:21 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14620 --- My immediate question for this is: How does this impact OSX? The timing module was originally written for both, do these changes work for both? - Joshua Colp On March 9, 2015, 6:21 p.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 9, 2015, 6:21 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14623 --- /trunk/res/res_timing_kqueue.c https://reviewboard.asterisk.org/r/4465/#comment25180 Portability: at least EVFILT_USER and NOTE_TRIGGER are not defined on all kqueue(2) systems. /trunk/res/res_timing_kqueue.c https://reviewboard.asterisk.org/r/4465/#comment25181 See above. /trunk/res/res_timing_kqueue.c https://reviewboard.asterisk.org/r/4465/#comment25182 See above. - Ed Hynan On March 9, 2015, 6:21 p.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 9, 2015, 6:21 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 10, 2015, 1:26 p.m., Ed Hynan wrote: /trunk/res/res_timing_kqueue.c, line 240 https://reviewboard.asterisk.org/r/4465/diff/1/?file=71888#file71888line240 Portability: at least EVFILT_USER and NOTE_TRIGGER are not defined on all kqueue(2) systems. Which platforms are you referring to? OS-X added support in 10.6. Why they haven't updated their man pages is anyones guess: http://www.opensource.apple.com/source/xnu/xnu-1699.24.23/tools/tests/xnu_quick_test/kqueue_tests.c - Justin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14623 --- On March 9, 2015, 6:21 p.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 9, 2015, 6:21 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4465: Update the kqueue timing module to conform to current timer API.
On March 10, 2015, 1:08 p.m., Joshua Colp wrote: My immediate question for this is: How does this impact OSX? The timing module was originally written for both, do these changes work for both? I haven't tested against OS-X, but it has had EVFILT_USER support since 10.6. - Justin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/#review14620 --- On March 9, 2015, 6:21 p.m., Justin T. Gibbs wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4465/ --- (Updated March 9, 2015, 6:21 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24857 https://issues.asterisk.org/jira/browse/ASTERISK-24857 Repository: Asterisk Description --- Update the kqueue timing module to conform to current timer API. This fixes issues with using the kqueue timing source on Asterisk 13 on FreeBSD 10. res_timing_kqueue.c: Remove support for kevent64(). The values used to support Asterisk timers fit within 32bits and so can be handled on all platforms via kevent(). Provide debug logging for, but do not track, unacked events. This matches the behavior of all other timer implementations. Implement continuous mode by triggering and leaving active, a user event. This ensures that the file descriptor for the timer returns immediately from poll(), without placing the load of a high speed timer on the kernel. In kqueue_timer_get_max_rate(), don't overstate the capability of the timer. On some platforms, UINT_MAX is greater than INTPTR_MAX, the largest integer type kqueue supports for timers. In kqueue_timer_get_event(), assume the caller woke up from poll() and just return the mode the timer is currently in. This matches all other timer implementations. Adjust the test code now that unacked events are not tracked. Diffs - /trunk/res/res_timing_kqueue.c 432637 Diff: https://reviewboard.asterisk.org/r/4465/diff/ Testing --- Asterisk 13.2.0 on FreeBSD 10-stable: timing test, pjsip incoming/outgoing calls, voicemail prompts and recordings. All of the above failed without these changes. Thanks, Justin T. Gibbs -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
