From b75841029f6e813f0efaf011f3f6ea86b92ab9c0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Apr 2025 01:01:41 +0900 Subject: [PATCH 1/4] udev: split out two functions from on_post() No functional change, just refactoring and preparation for later change. --- src/udev/udev-manager.c | 106 ++++++++++++++++++++++++---------------- 1 file changed, 63 insertions(+), 43 deletions(-) diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index f053c67c8b..94a6d75848 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -227,6 +227,44 @@ void manager_kill_workers(Manager *manager, bool force) { } } +static int on_kill_workers_event(sd_event_source *s, uint64_t usec, void *userdata) { + Manager *manager = ASSERT_PTR(userdata); + + log_debug("Cleaning up idle workers."); + manager_kill_workers(manager, SIGTERM); + + return 0; +} + +static int manager_reset_kill_workers_timer(Manager *manager) { + int r; + + assert(manager); + + if (hashmap_isempty(manager->workers)) { + /* There are no workers. Disabling unnecessary timer event source. */ + r = sd_event_source_set_enabled(manager->kill_workers_event, SD_EVENT_OFF); + if (r < 0) + return log_warning_errno(r, "Failed to disable timer event source for cleaning up workers: %m"); + } else { + r = event_reset_time_relative( + manager->event, + &manager->kill_workers_event, + CLOCK_MONOTONIC, + 3 * USEC_PER_SEC, + USEC_PER_SEC, + on_kill_workers_event, + manager, + SD_EVENT_PRIORITY_NORMAL, + "kill-workers-event", + /* force_reset = */ false); + if (r < 0) + return log_warning_errno(r, "Failed to enable timer event source for cleaning up workers: %m"); + } + + return 0; +} + void manager_exit(Manager *manager) { assert(manager); @@ -309,15 +347,6 @@ void manager_reload(Manager *manager, bool force) { notify_ready(manager); } -static int on_kill_workers_event(sd_event_source *s, uint64_t usec, void *userdata) { - Manager *manager = ASSERT_PTR(userdata); - - log_debug("Cleaning up idle workers."); - manager_kill_workers(manager, false); - - return 1; -} - static int on_event_timeout(sd_event_source *s, uint64_t usec, void *userdata) { Event *event = ASSERT_PTR(userdata); @@ -887,9 +916,26 @@ static int on_sigchld(sd_event_source *s, const siginfo_t *si, void *userdata) { return 0; } +static int manager_unlink_queue_file(Manager *manager) { + assert(manager); + + if (manager->events) + return 0; /* There are queued events. */ + + /* There are no queued events. Let's remove /run/udev/queue and clean up the idle processes. */ + if (unlink("/run/udev/queue") < 0) { + if (errno == ENOENT) + return 0; + + return log_warning_errno(errno, "Failed to unlink /run/udev/queue: %m"); + } + + log_debug("No events are queued, removed /run/udev/queue."); + return 0; +} + static int on_post(sd_event_source *s, void *userdata) { Manager *manager = ASSERT_PTR(userdata); - int r; if (manager->events) { /* Try to process pending events if idle workers exist. Why is this necessary? @@ -897,41 +943,15 @@ static int on_post(sd_event_source *s, void *userdata) { * the corresponding device might have been locked and the processing of the event * delayed for a while, preventing the worker from processing the event immediately. * Now, the device may be unlocked. Let's try again! */ - event_queue_start(manager); - return 1; + (void) event_queue_start(manager); + return 0; } - /* There are no queued events. Let's remove /run/udev/queue and clean up the idle processes. */ + (void) manager_unlink_queue_file(manager); + (void) manager_reset_kill_workers_timer(manager); - if (unlink("/run/udev/queue") < 0) { - if (errno != ENOENT) - log_warning_errno(errno, "Failed to unlink /run/udev/queue, ignoring: %m"); - } else - log_debug("No events are queued, removed /run/udev/queue."); - - if (!hashmap_isempty(manager->workers)) { - /* There are idle workers */ - r = event_reset_time_relative( - manager->event, - &manager->kill_workers_event, - CLOCK_MONOTONIC, - 3 * USEC_PER_SEC, - USEC_PER_SEC, - on_kill_workers_event, - manager, - SD_EVENT_PRIORITY_NORMAL, - "kill-workers-event", - /* force_reset = */ false); - if (r < 0) - log_warning_errno(r, "Failed to enable timer event source for cleaning up idle workers, ignoring: %m"); - - return 1; - } - - /* There are no idle workers. */ - r = sd_event_source_set_enabled(manager->kill_workers_event, SD_EVENT_OFF); - if (r < 0) - log_warning_errno(r, "Failed to disable timer event source for cleaning up idle workers, ignoring: %m"); + if (!hashmap_isempty(manager->workers)) + return 0; /* There still exist idle workers. */ if (manager->exit) { udev_watch_dump(); @@ -942,7 +962,7 @@ static int on_post(sd_event_source *s, void *userdata) { /* cleanup possible left-over processes in our cgroup */ (void) cg_kill(manager->cgroup, SIGKILL, CGROUP_IGNORE_SELF, /* set=*/ NULL, /* kill_log= */ NULL, /* userdata= */ NULL); - return 1; + return 0; } Manager* manager_new(void) { From b6ede8e2756ebaf5049f9975f4cde81445578fd2 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Apr 2025 01:49:30 +0900 Subject: [PATCH 2/4] udev: unconditionally send signals to workers It is not necessary to wait for a worker processing an event before sending SIGTERM. Workers will handle SIGTERM after they finish events that they are currently processing. Let's send SIGTERM whenever it necessary. --- src/udev/udev-config.c | 6 +++--- src/udev/udev-manager.c | 25 ++++++------------------- src/udev/udev-manager.h | 2 +- 3 files changed, 10 insertions(+), 23 deletions(-) diff --git a/src/udev/udev-config.c b/src/udev/udev-config.c index feca748f9e..31611ece64 100644 --- a/src/udev/udev-config.c +++ b/src/udev/udev-config.c @@ -342,7 +342,7 @@ void manager_set_log_level(Manager *manager, int log_level) { return; log_set_max_level(manager->config.log_level); - manager_kill_workers(manager, /* force = */ false); + manager_kill_workers(manager, SIGTERM); } void manager_set_trace(Manager *manager, bool enable) { @@ -357,7 +357,7 @@ void manager_set_trace(Manager *manager, bool enable) { return; log_set_max_level(manager->config.log_level); - manager_kill_workers(manager, /* force = */ false); + manager_kill_workers(manager, SIGTERM); } static void manager_adjust_config(UdevConfig *config) { @@ -433,7 +433,7 @@ void manager_set_environment(Manager *manager, char * const *v) { } if (changed) - manager_kill_workers(manager, /* force = */ false); + manager_kill_workers(manager, SIGTERM); } int manager_load(Manager *manager, int argc, char *argv[]) { diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 94a6d75848..b1faeedab7 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -77,7 +77,6 @@ typedef enum WorkerState { WORKER_RUNNING, WORKER_IDLE, WORKER_KILLED, - WORKER_KILLING, } WorkerState; typedef struct Worker { @@ -208,22 +207,13 @@ static int worker_new(Worker **ret, Manager *manager, sd_device_monitor *worker_ return 0; } -void manager_kill_workers(Manager *manager, bool force) { - Worker *worker; - +void manager_kill_workers(Manager *manager, int signo) { assert(manager); + Worker *worker; HASHMAP_FOREACH(worker, manager->workers) { - if (worker->state == WORKER_KILLED) - continue; - - if (worker->state == WORKER_RUNNING && !force) { - worker->state = WORKER_KILLING; - continue; - } - worker->state = WORKER_KILLED; - (void) pidref_kill(&worker->pidref, SIGTERM); + (void) pidref_kill(&worker->pidref, signo); } } @@ -286,7 +276,7 @@ void manager_exit(Manager *manager) { /* discard queued events and kill workers */ event_queue_cleanup(manager, EVENT_QUEUED); - manager_kill_workers(manager, true); + manager_kill_workers(manager, SIGTERM); } void notify_ready(Manager *manager) { @@ -332,7 +322,7 @@ void manager_reload(Manager *manager, bool force) { flags |= manager_reload_config(manager); if (FLAGS_SET(flags, UDEV_RELOAD_KILL_WORKERS)) - manager_kill_workers(manager, false); + manager_kill_workers(manager, SIGTERM); udev_builtin_reload(flags); @@ -844,10 +834,7 @@ static int on_worker_notify(sd_event_source *s, int fd, uint32_t revents, void * event_free(worker->event); /* Update the state of the worker. */ - if (worker->state == WORKER_KILLING) { - worker->state = WORKER_KILLED; - (void) pidref_kill(&worker->pidref, SIGTERM); - } else if (worker->state != WORKER_KILLED) + if (worker->state != WORKER_KILLED) worker->state = WORKER_IDLE; return 0; diff --git a/src/udev/udev-manager.h b/src/udev/udev-manager.h index 3ed393c1f0..16a558e7b8 100644 --- a/src/udev/udev-manager.h +++ b/src/udev/udev-manager.h @@ -63,7 +63,7 @@ void manager_exit(Manager *manager); void notify_ready(Manager *manager); -void manager_kill_workers(Manager *manager, bool force); +void manager_kill_workers(Manager *manager, int signo); bool devpath_conflict(const char *a, const char *b); From 45e8c283d87614cddc7ff23371b3e2701bd386cd Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Apr 2025 02:04:06 +0900 Subject: [PATCH 3/4] udev: do not wait for event queue being empty on exit When the manager process is requested to terminate, if a worker process try to lock a block device and failed, then the worker returns a TRY_AGAIN notification and the event is requeued. Hence, the event queue may have pending events even after manager_exit() is called. In such situation, sd_event_exit() will never called, and udevd will stuck. This makes, after termination is requested, it only checks whether there are any events currently being processed. --- src/udev/udev-manager.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index b1faeedab7..375bcd55e8 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -588,8 +588,9 @@ static int event_queue_start(Manager *manager) { int r; assert(manager); + assert(!manager->exit); - if (!manager->events || manager->exit || manager->stop_exec_queue) + if (!manager->events || manager->stop_exec_queue) return 0; r = event_source_disable(manager->kill_workers_event); @@ -921,9 +922,29 @@ static int manager_unlink_queue_file(Manager *manager) { return 0; } +static int on_post_exit(Manager *manager) { + assert(manager); + assert(manager->exit); + + LIST_FOREACH(event, event, manager->events) + if (event->state == EVENT_RUNNING) + return 0; /* There still exist events being processed. */ + + (void) manager_unlink_queue_file(manager); + + if (!hashmap_isempty(manager->workers)) + return 0; /* There still exist running workers. */ + + udev_watch_dump(); + return sd_event_exit(manager->event, 0); +} + static int on_post(sd_event_source *s, void *userdata) { Manager *manager = ASSERT_PTR(userdata); + if (manager->exit) + return on_post_exit(manager); + if (manager->events) { /* Try to process pending events if idle workers exist. Why is this necessary? * When a worker finished an event and became idle, even if there was a pending event, @@ -940,11 +961,6 @@ static int on_post(sd_event_source *s, void *userdata) { if (!hashmap_isempty(manager->workers)) return 0; /* There still exist idle workers. */ - if (manager->exit) { - udev_watch_dump(); - return sd_event_exit(manager->event, 0); - } - if (manager->cgroup && set_isempty(manager->synthesize_change_child_event_sources)) /* cleanup possible left-over processes in our cgroup */ (void) cg_kill(manager->cgroup, SIGKILL, CGROUP_IGNORE_SELF, /* set=*/ NULL, /* kill_log= */ NULL, /* userdata= */ NULL); From b135cb321a0f41d59a666bcbd8ead4cb2337c78f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 6 Apr 2025 02:06:12 +0900 Subject: [PATCH 4/4] udev: drop unnecessary discardment of queued events With the previous commit, now on_post_exit() checks only events currently being processed. Hence, it is not necessary to discard queued events in manager_exit(). Also, as already SIGTERM is sent to all workers, kill workers timer is not necessary anymore after manager_exit(), hence disable it. This mostly does not change any behavior. Just refactoring and preparation for later change. --- src/udev/udev-manager.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 375bcd55e8..883b27a232 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -107,15 +107,6 @@ static Event* event_free(Event *event) { return mfree(event); } -static void event_queue_cleanup(Manager *manager, EventState match_state) { - LIST_FOREACH(event, event, manager->events) { - if (match_state != EVENT_UNDEF && match_state != event->state) - continue; - - event_free(event); - } -} - static Worker* worker_free(Worker *worker) { if (!worker) return NULL; @@ -149,7 +140,8 @@ Manager* manager_free(Manager *manager) { udev_rules_free(manager->rules); hashmap_free(manager->workers); - event_queue_cleanup(manager, EVENT_UNDEF); + while (manager->events) + event_free(manager->events); safe_close(manager->inotify_fd); @@ -274,9 +266,9 @@ void manager_exit(Manager *manager) { (void) sd_event_source_set_enabled(sd_device_monitor_get_event_source(manager->monitor), SD_EVENT_OFF); (void) sd_device_monitor_detach_event(manager->monitor); - /* discard queued events and kill workers */ - event_queue_cleanup(manager, EVENT_QUEUED); + /* Kill all workers with SIGTERM, and disable unnecessary timer event source. */ manager_kill_workers(manager, SIGTERM); + manager->kill_workers_event = sd_event_source_disable_unref(manager->kill_workers_event); } void notify_ready(Manager *manager) {