From f52d74a9fd940b1e49845308f4621d759fdb310c Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Thu, 26 Oct 2023 15:40:48 -0600 Subject: [PATCH] Delete the validation thread pool My original intent was "deprecate thread-pool.validation.max," but it turns out it was just a symptom of a (mostly inoffensive) overcomplication. thread-pool.validation.max has proven confusing to users, because it doesn't make sense for it to be configurable. The thread count should always equal the number of RPKI trees, which in turn equals the number of TALs. There's no reason why Fort should offload this decision to the user. As for the thread pool, the validation cycle is not really a fitting problem for such an ellaborate solution, because the former involves a very small amount (typically 5) of long-lived threads that start at the same time, once every hour or so. So instead of pooling a configured amount of threads in the beginning, spawn raw threads as needed. The RTR server still employs a thread pool. --- docs/usage.md | 26 ++++++++++++-------------- src/config.c | 11 +++-------- src/config.h | 1 - src/object/tal.c | 31 +++++++++++++++++++++---------- src/object/tal.h | 3 +-- src/rtr/db/vrps.c | 15 +-------------- 6 files changed, 38 insertions(+), 49 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 02ff1a09..a0a16615 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -62,7 +62,6 @@ description: Guide to use arguments of FORT Validator. 46. [`--asn1-decode-max-stack`](#--asn1-decode-max-stack) 47. [`--stale-repository-period`](#--stale-repository-period) 48. [`--thread-pool.server.max`](#--thread-poolservermax) - 49. [`--thread-pool.validation.max`](#--thread-poolvalidationmax) 50. [`--rsync.enabled`](#--rsyncenabled) 51. [`--rsync.priority`](#--rsyncpriority) 52. [`--rsync.strategy`](#--rsyncstrategy) @@ -84,6 +83,7 @@ description: Guide to use arguments of FORT Validator. 5. [`--rrdp.retry.interval`](#--rrdpretryinterval) 60. [`init-locations`](#init-locations) 41. [`--http.idle-timeout`](#--httpidle-timeout) + 49. [`--thread-pool.validation.max`](#--thread-poolvalidationmax) ## Syntax @@ -912,19 +912,6 @@ Number of threads the RTR server will reserve for RTR client (router) request ha > Before Fort 1.5.1, this value used to represent the maximum number of client _connections_ the server would be able to hold at any given time. It scales better now. -### `--thread-pool.validation.max` - -- **Type:** Integer -- **Availability:** `argv` and JSON -- **Default:** 5 -- **Range:** 1--100 - -> ![Warning!](img/warn.svg) Deprecated. This value should always equal the number of TALs you have, and will therefore be automated and retired in the future. - -Number of threads in the validation thread pool. - -During every validation cycle, one thread is borrowed from this pool per TAL, to validate the RPKI tree of the corresponding TAL. - ### `--rsync.enabled` - **Type:** Boolean (`true`, `false`) @@ -1298,3 +1285,14 @@ rsync synchronization strategy. Commands the way rsync URLs are approached durin - **Range:** 0--[`UINT_MAX`](http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html) Deprecated alias for [`--http.low-speed-time`](#--httplow-speed-time). + +### `--thread-pool.validation.max` + +- **Type:** Integer +- **Availability:** `argv` and JSON +- **Default:** 5 +- **Range:** 1--100 + +> ![img/warn.svg](img/warn.svg) This argument **is DEPRECATED**. + +Does nothing as of Fort 1.6.0. \ No newline at end of file diff --git a/src/config.c b/src/config.c index 46206d47..18b7bec8 100644 --- a/src/config.c +++ b/src/config.c @@ -187,7 +187,7 @@ struct rpki_config { } server; /* Threads related to validation cycles */ struct { - unsigned int max; + unsigned int max; /* Deprecated */ } validation; } thread_pool; }; @@ -739,7 +739,8 @@ static const struct option_field options[] = { .type = >_uint, .offset = offsetof(struct rpki_config, thread_pool.validation.max), - .doc = "Number of threads in the validation thread pool. (Each thread handles one TAL tree.)", + .doc = "Deprecated; does nothing.", + .deprecated = true, .min = 0, .max = 100, }, @@ -1429,12 +1430,6 @@ config_get_thread_pool_server_max(void) return rpki_config.thread_pool.server.max; } -unsigned int -config_get_thread_pool_validation_max(void) -{ - return rpki_config.thread_pool.validation.max; -} - void config_set_rsync_enabled(bool value) { diff --git a/src/config.h b/src/config.h index c7d353db..0ef1c65d 100644 --- a/src/config.h +++ b/src/config.h @@ -54,7 +54,6 @@ char const *config_get_output_bgpsec(void); enum output_format config_get_output_format(void); unsigned int config_get_asn1_decode_max_stack(void); unsigned int config_get_thread_pool_server_max(void); -unsigned int config_get_thread_pool_validation_max(void); /* Logging getters */ bool config_get_op_log_enabled(void); diff --git a/src/object/tal.c b/src/object/tal.c index f0256adf..1a8fd312 100644 --- a/src/object/tal.c +++ b/src/object/tal.c @@ -37,6 +37,8 @@ struct tal { }; struct validation_thread { + pthread_t pid; + /* TAL file name */ char *tal_file; /* @@ -56,7 +58,6 @@ struct validation_thread { SLIST_HEAD(threads_list, validation_thread); struct tal_param { - struct thread_pool *pool; struct db_table *db; struct threads_list threads; }; @@ -539,10 +540,10 @@ end: validation_destroy(state); return error; } -static void -do_file_validation(void *thread_arg) +static void * +do_file_validation(void *arg) { - struct validation_thread *thread = thread_arg; + struct validation_thread *thread = arg; struct tal *tal; int error; @@ -582,6 +583,7 @@ destroy_tal: end: fnstack_cleanup(); thread->exit_status = error; + return NULL; } static void @@ -597,6 +599,7 @@ __do_file_validation(char const *tal_file, void *arg) { struct tal_param *t_param = arg; struct validation_thread *thread; + int error; thread = pmalloc(sizeof(struct validation_thread)); @@ -606,21 +609,27 @@ __do_file_validation(char const *tal_file, void *arg) thread->retry_local = true; thread->sync_files = true; - thread_pool_push(t_param->pool, thread->tal_file, do_file_validation, - thread); + error = pthread_create(&thread->pid, NULL, do_file_validation, thread); + if (error) { + pr_op_err("Could not spawn the file validation thread: %s", + strerror(error)); + free(thread->tal_file); + free(thread); + return error; + } + SLIST_INSERT_HEAD(&t_param->threads, thread, next); return 0; } int -perform_standalone_validation(struct thread_pool *pool, struct db_table *table) +perform_standalone_validation(struct db_table *table) { struct tal_param param; struct validation_thread *thread; int error; - param.pool = pool; param.db = table; SLIST_INIT(¶m.threads); @@ -637,10 +646,12 @@ perform_standalone_validation(struct thread_pool *pool, struct db_table *table) } /* Wait for all */ - thread_pool_wait(pool); - while (!SLIST_EMPTY(¶m.threads)) { thread = SLIST_FIRST(¶m.threads); + error = pthread_join(thread->pid, NULL); + if (error) + pr_crit("pthread_join() threw %d on the '%s' thread.", + error, thread->tal_file); SLIST_REMOVE_HEAD(¶m.threads, next); if (thread->exit_status) { error = thread->exit_status; diff --git a/src/object/tal.h b/src/object/tal.h index f0f7b628..8f4d72b0 100644 --- a/src/object/tal.h +++ b/src/object/tal.h @@ -5,7 +5,6 @@ #include "types/uri.h" #include "rtr/db/db_table.h" -#include "thread/thread_pool.h" struct tal; @@ -15,6 +14,6 @@ void tal_destroy(struct tal *); char const *tal_get_file_name(struct tal *); void tal_get_spki(struct tal *, unsigned char const **, size_t *); -int perform_standalone_validation(struct thread_pool *, struct db_table *); +int perform_standalone_validation(struct db_table *); #endif /* TAL_OBJECT_H_ */ diff --git a/src/rtr/db/vrps.c b/src/rtr/db/vrps.c index 57abc26a..65a39f9f 100644 --- a/src/rtr/db/vrps.c +++ b/src/rtr/db/vrps.c @@ -14,7 +14,6 @@ #include "rtr/rtr.h" #include "rtr/db/db_table.h" #include "slurm/slurm_loader.h" -#include "thread/thread_pool.h" #include "cache/local_cache.h" struct vrp_node { @@ -78,9 +77,6 @@ struct state { static struct state state; -/* Thread pool to use when the TALs will be processed */ -static struct thread_pool *pool; - /** Protects @state.base, @state.deltas and @state.serial. */ static pthread_rwlock_t state_lock; @@ -108,12 +104,6 @@ vrps_init(void) time_t now; int error; - pool = NULL; - error = thread_pool_create("Validation", - config_get_thread_pool_validation_max(), &pool); - if (error) - return error; - state.base = NULL; state.deltas = darray_create(); @@ -149,15 +139,12 @@ vrps_init(void) revert_deltas: darray_destroy(state.deltas); - thread_pool_destroy(pool); return error; } void vrps_destroy(void) { - thread_pool_destroy(pool); - pthread_rwlock_destroy(&state_lock); if (state.slurm != NULL) @@ -225,7 +212,7 @@ __perform_standalone_validation(struct db_table **result) db = db_table_create(); - error = perform_standalone_validation(pool, db); + error = perform_standalone_validation(db); if (error) { db_table_destroy(db); return error; -- 2.47.2