From e8e87ec8460ea83a95efc9f781c3b90b4f8ad579 Mon Sep 17 00:00:00 2001 From: Maxim Cournoyer Date: Sat, 23 Dec 2023 00:23:31 -0500 Subject: [PATCH 1/3] monitor: Fix bug using callback procedure in place of error callback. * modules/udev/monitor.scm (make-udev-monitor): Apply udev-monitor-set-error-callback! to the error-callback default procedure, no the callback one. --- modules/udev/monitor.scm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/udev/monitor.scm b/modules/udev/monitor.scm index 245627d..75fadc3 100644 --- a/modules/udev/monitor.scm +++ b/modules/udev/monitor.scm @@ -61,7 +61,7 @@ or #f to match any type." (let ((monitor (%make-udev-monitor udev))) (udev-monitor-set-timeout! monitor timeout-sec timeout-usec) (udev-monitor-set-callback! monitor callback) - (udev-monitor-set-error-callback! monitor callback) + (udev-monitor-set-error-callback! monitor error-callback) (when filter (let ((subsystem (car filter)) (devtype (cadr filter))) From 3187ba7879c4182c8d6c8f87fa9271f6ba2e5c21 Mon Sep 17 00:00:00 2001 From: Maxim Cournoyer Date: Sat, 23 Dec 2023 00:26:58 -0500 Subject: [PATCH 2/3] Fix typo in error message of the monitor scanner. * libguile-udev/udev-monitor-func.c (udev_monitor_scanner): Fix typo. --- libguile-udev/udev-monitor-func.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libguile-udev/udev-monitor-func.c b/libguile-udev/udev-monitor-func.c index 6de9fb2..5106061 100644 --- a/libguile-udev/udev-monitor-func.c +++ b/libguile-udev/udev-monitor-func.c @@ -200,7 +200,7 @@ void* udev_monitor_scanner(void* arg) monitor_fd = udev_monitor_get_fd(umd->udev_monitor); if (monitor_fd < 0) { - char msg[] = "Could not udev monitor file descriptor."; + char msg[] = "Could not retrieve udev monitor file descriptor."; scm_call_2(error_callback, udev_monitor, scm_from_locale_string(msg)); umd->is_scanning = 0; From 980fe4055a24c1be57ab8fcb9835089e256f637d Mon Sep 17 00:00:00 2001 From: Maxim Cournoyer Date: Fri, 22 Dec 2023 23:37:29 -0500 Subject: [PATCH 3/3] Do not poll when omitting timeout values for the monitor scanner. Previously, it would default to use a 0 s timeout, which for select means that it doesn't block (it would poll continuously and waste CPU). This change unifies the make-udev-monitor/udev-monitor-set-timeout! interface with that of Guile's select procedure. * NEWS: Update news. * doc/guile-udev.texi (API Reference): Update doc. * examples/device-listener.scm (main) : Remove timeout arguments. * libguile-udev/udev-monitor-func.c (udev-monitor-set-timeout!): Make timeout arguments optional. Update doc. Streamline definition, removing all arguments validation, which is now handled by scm_select. * libguile-udev/udev-monitor-func.c (select_args_data): New struct. (call_select, false_on_exception): New procedures. (udev_monitor_scanner): Replace C select call with scm_select call, adjusting the rest accordingly. * libguile-udev/udev-monitor-type.h: Replace timeout member with new 'secs' and 'usecs' ones. * libguile-udev/udev-monitor-type.c (gudev_monitor_to_scm): Set default values of secs and usecs to SCM_BOOL_F. * modules/udev/monitor.scm (make-udev-monitor): Do not specify default values for filter, timeout-sec and timeout-usec keyword arguments. Update doc. Fixes: https://github.com/artyom-poptsov/guile-udev/issues/5 --- NEWS | 5 ++ doc/guile-udev.texi | 47 ++++++++++------ examples/device-listener.scm | 7 +-- libguile-udev/udev-monitor-func.c | 90 +++++++++++++++++++------------ libguile-udev/udev-monitor-type.c | 4 +- libguile-udev/udev-monitor-type.h | 3 +- modules/udev/monitor.scm | 12 +++-- 7 files changed, 105 insertions(+), 63 deletions(-) diff --git a/NEWS b/NEWS index cc3833c..21d5a3b 100644 --- a/NEWS +++ b/NEWS @@ -43,6 +43,11 @@ Targets are: - Document Guile-Udev types. Thanks to Maxim Cournoyer . +** Change the default the timeout values of `make-udev-monitor' to #f +The TIMEOUT-SEC and TIMEOUT-USEC arguments now default to #f instead of 0, +which means that the `select' call no longer polls by default, which would +consume 100% of a CPU core (see: +https://github.com/artyom-poptsov/guile-udev/issues/5). * Changes in version 0.2.4 (2022-12-27) ** Bugfix: Check every string that comes from the udev for NULL diff --git a/doc/guile-udev.texi b/doc/guile-udev.texi index 8f02170..cdbb46f 100644 --- a/doc/guile-udev.texi +++ b/doc/guile-udev.texi @@ -182,8 +182,15 @@ Monitor object allows to scan for device events. When an event occurs, the monitor calls a specified callback. Inside the scanner loop, the monitor calls @code{select} procedure to check if any events available. The @code{select} timeout can be set by @code{udev-monitor-set-timeout!} -procedure. The timeout seconds and milliseconds parts are set to zero by -default. +procedure. By default, no timeout is used, which means the monitor will wait +forever for an event to be triggered. + +@quotation note +The monitor scanner loops infinitely; that is, even when a timeout value is +used, the scanner will keep monitoring events, but will poll at the requested +period. For maximum efficiency, it is recommended to not use a timeout and +let the underlying @code{select} system call indefinitely wait for an event. +@end quotation These procedures are available from the module @code{(udev monitor)}. @@ -233,28 +240,34 @@ Example: (make-udev-monitor udev #:filter (list "usb" "usb_device")) @end lisp @item timeout-sec -Timeout for 'select' call in the event listener. This part of the timeout is -specified in seconds. +Optional timeout for 'select' call in the event listener. This part of the +timeout is specified in seconds. Expected type: Non-negative integer. -Default value: 0 +Default value: @code{#f} + +Examples: + +@lisp +(make-udev-monitor udev) ;no timeout, most efficient +@end lisp -Example: @lisp -(make-udev-monitor udev #:timeout-sec 1) +(make-udev-monitor udev #:timeout-sec 3.33) ;3.33 s poll period @end lisp + @item timeout-usec -Timeout for 'select' call in the event listener. This part of the timeout is -specified in milliseconds. +Optional timeout for 'select' call in the event listener, in microseconds. +The @var{timeout-sec} argument must be specified as well otherwise +@var{timeout-usec} is ignored. Expected type: Non-negative integer. -Default value: 0 +Default value: @code{#f} -Example: @lisp -(make-udev-monitor udev #:timeout-usec 1) +(make-udev-monitor udev #:timeout-sec 0 #:timeout-usec 250) ;250 us poll period @end lisp @end table @@ -278,10 +291,12 @@ Remove all the filters that was set for @var{udev-monitor}. Throws @code{guile-udev-error} on errors. @end deffn -@deffn {Scheme Procedure} udev-monitor-set-timeout! udev-monitor seconds milliseconds -Set the @var{udev-monitor} udev event polling timeout. The @var{seconds} and -@var{milliseconds} parameters must be integer numbers greater or equal to -zero. +@deffn {Scheme Procedure} udev-monitor-set-timeout! udev-monitor [secs] [usecs] +Set the @var{udev-monitor} udev event polling timeout. The @var{secs} +parameter must be a non-negative integer or float representing seconds, while +@var{usecs} must be a non-negative integer representing microseconds. To +clear the timeout value, @var{secs} may be set to @code{#f} or omitted +entirely. @end deffn @deffn {Scheme Procedure} udev-monitor-set-callback! udev-monitor callback diff --git a/examples/device-listener.scm b/examples/device-listener.scm index 6ceaf23..2070a72 100755 --- a/examples/device-listener.scm +++ b/examples/device-listener.scm @@ -13,11 +13,8 @@ (define (main args) (let* ((udev (make-udev)) (udev-monitor (make-udev-monitor udev - #:timeout-sec 1 - #:timeout-usec 0 - #:callback callback - #:filter (list "usb" - "usb_device")))) + #:callback callback + #:filter (list "usb" "usb_device")))) (udev-monitor-start-scanning! udev-monitor) (while #t (sleep 1)))) diff --git a/libguile-udev/udev-monitor-func.c b/libguile-udev/udev-monitor-func.c index 5106061..6050c1a 100644 --- a/libguile-udev/udev-monitor-func.c +++ b/libguile-udev/udev-monitor-func.c @@ -140,34 +140,23 @@ SCM_DEFINE_N(gudev_monitor_set_error_callback_x, "udev-monitor-set-error-callbac } #undef FUNC_NAME -SCM_DEFINE_N(gudev_monitor_set_timeout_x, "udev-monitor-set-timeout!", 3, - (SCM udev_monitor, SCM seconds, SCM milliseconds), - "Set monitor event poll timeout. \ -Throws 'guile-udev-error' on errors.") +SCM_DEFINE(gudev_monitor_set_timeout_x, "udev-monitor-set-timeout!", 1, 2, 0, + (SCM udev_monitor, SCM secs, SCM usecs), + "Set monitor event poll @var{seconds} and @var{microseconds}\n" + "timeout non-negative numbers in seconds and microseconds,\n" + "respectively.\n\n" + + "@var{secs} and @var{usecs} are optional;\n" + "They share the same semantic as the corresponding arguments of\n" + "Guile's `select' abstraction. When left unspecified, the timeout\n" + "is cleared.\n\n" + + "Throws 'guile-udev-error' on errors.") #define FUNC_NAME s_gudev_monitor_set_timeout_x { gudev_monitor_t* umd = gudev_monitor_from_scm(udev_monitor); - SCM_ASSERT(scm_number_p(seconds), seconds, SCM_ARG2, FUNC_NAME); - SCM_ASSERT(scm_number_p(milliseconds), milliseconds, SCM_ARG3, FUNC_NAME); - long c_seconds = scm_to_long(seconds); - long c_milliseconds = scm_to_long(milliseconds); - - if (c_seconds < 0) { - guile_udev_error1( - FUNC_NAME, - "'Seconds' part of the timeout must be >= 0.", - seconds); - } - - if (c_milliseconds < 0) { - guile_udev_error1( - FUNC_NAME, - "'Milliseconds' part of the timeout must be >= 0.", - milliseconds); - } - - umd->timeout.tv_sec = c_seconds; - umd->timeout.tv_usec = c_milliseconds; + umd->secs = secs; + umd->usecs = usecs; scm_remember_upto_here_1(udev_monitor); @@ -175,15 +164,32 @@ Throws 'guile-udev-error' on errors.") } #undef FUNC_NAME +struct select_args_data { + SCM reads; + SCM secs; + SCM usecs; +}; + +/* A procedure for calling scm_select via scm_internal_catch. */ +SCM call_select(void *data) { + struct select_args_data* args = data; + return scm_select(args->reads, SCM_EOL, SCM_EOL, args->secs, args->usecs); +} + +/* Dummy handler returning false. */ +SCM false_on_exception(void *data, SCM key, SCM args) { + return SCM_BOOL_F; +} + void* udev_monitor_scanner(void* arg) #define FUNC_NAME "udev_monitor_scanner" { SCM udev_monitor = (SCM) arg; gudev_monitor_t* umd = gudev_monitor_from_scm(udev_monitor); - fd_set fd_set_; int result; - int select_result; - int monitor_fd; + SCM select_result; + int c_monitor_fd; + SCM monitor_fd; struct udev_device *dev; SCM error_callback = umd->error_callback; @@ -198,7 +204,9 @@ void* udev_monitor_scanner(void* arg) return NULL; } - monitor_fd = udev_monitor_get_fd(umd->udev_monitor); + c_monitor_fd = udev_monitor_get_fd(umd->udev_monitor); + monitor_fd = scm_from_int(c_monitor_fd); + if (monitor_fd < 0) { char msg[] = "Could not retrieve udev monitor file descriptor."; scm_call_2(error_callback, udev_monitor, @@ -207,21 +215,30 @@ void* udev_monitor_scanner(void* arg) return NULL; } + struct select_args_data select_args; + select_args.reads = scm_list_1(monitor_fd); + select_args.secs = umd->secs; + select_args.usecs = umd->usecs; + SCM callback = umd->scanner_callback; SCM device; + SCM read_fdes; while (1) { - struct timeval timeout = umd->timeout; pthread_mutex_lock(&umd->lock); if (! umd->is_scanning) { break; } pthread_mutex_unlock(&umd->lock); - FD_ZERO(&fd_set_); - FD_SET(monitor_fd, &fd_set_); - select_result = select(monitor_fd + 1, &fd_set_, NULL, NULL, &timeout); - if (select_result == -1) { + /* We use scm_select here instead of C select so as to benefit from + * the 'secs' and 'usecs' arguments validation/behavior that it + * provides. */ + select_result = scm_internal_catch(scm_from_utf8_symbol("system-error"), + call_select, &select_args, + false_on_exception, NULL); + // Actual error handling is done here. + if (scm_is_false(select_result)) { char msg[] = "Error during 'select' call."; scm_call_2(error_callback, udev_monitor, scm_from_locale_string(msg)); @@ -229,7 +246,10 @@ void* udev_monitor_scanner(void* arg) pthread_cancel(pthread_self()); break; } - if (FD_ISSET(monitor_fd, &fd_set_)) { + + read_fdes = scm_car(select_result); + + if (scm_member(monitor_fd, read_fdes)) { dev = udev_monitor_receive_device(umd->udev_monitor); device = udev_device_to_scm(umd->udev, dev); scm_call_1(callback, device); diff --git a/libguile-udev/udev-monitor-type.c b/libguile-udev/udev-monitor-type.c index 68aaa83..0400540 100644 --- a/libguile-udev/udev-monitor-type.c +++ b/libguile-udev/udev-monitor-type.c @@ -81,8 +81,8 @@ SCM gudev_monitor_to_scm(SCM udev, struct udev_monitor *udev_monitor) gudev_monitor_t* umd = make_udev_monitor(); umd->udev = udev; umd->udev_monitor = udev_monitor; - umd->timeout.tv_sec = 0; - umd->timeout.tv_usec = 0; + umd->secs = SCM_BOOL_F; + umd->usecs = SCM_BOOL_F; umd->is_scanning = 0; umd->scanner_callback = SCM_BOOL_F; pthread_mutex_init(&umd->lock, NULL); diff --git a/libguile-udev/udev-monitor-type.h b/libguile-udev/udev-monitor-type.h index 6d49e67..92c8346 100644 --- a/libguile-udev/udev-monitor-type.h +++ b/libguile-udev/udev-monitor-type.h @@ -45,7 +45,8 @@ struct gudev_monitor { /** * @brief timeout -- 'select' timeout for the device monitor. */ - struct timeval timeout; + SCM secs; + SCM usecs; pthread_mutex_t lock; pthread_t scanner_thread; diff --git a/modules/udev/monitor.scm b/modules/udev/monitor.scm index 75fadc3..a97a7de 100644 --- a/modules/udev/monitor.scm +++ b/modules/udev/monitor.scm @@ -50,14 +50,18 @@ (format (current-error-port) "ERROR: in ~a: ~a~%" monitor error-message))) - (filter #f) - (timeout-usec 0) - (timeout-sec 0)) + filter + timeout-sec + timeout-usec) "Create a new 'udev-monitor' object configured with the specified parameters. CALLBACK is a one argument procedure (receiving a 'udev-device' object) called when an event matching the specified FILTER. FILTER is a list whose first element is the device subsystem, and whose second argument is the device type, -or #f to match any type." +or #f to match any type. If a timeout is desired, TIMEOUT-SEC or both +TIMEOUT-SEC and TIMEOUT-USEC may be provided as non-negative numbers of +seconds and microseconds, respectively. If TIMEOUT-USEC, is used, TIMEOUT-SEC +must also have a value, else it is ignored, as for the 'secs' and 'usecs' +argument of Guile's 'select' procedure." (let ((monitor (%make-udev-monitor udev))) (udev-monitor-set-timeout! monitor timeout-sec timeout-usec) (udev-monitor-set-callback! monitor callback)