From 5bf2973e9f1b40e261ae6061bf04901f551a1db7 Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Tue, 13 Jul 2021 07:22:12 -0400 Subject: [PATCH] lib/event: remove per core init/deinit API from governor There is no explicit need for the spdk governors initialization to occur on per core basis. This implementation detail for dpdk_governor is now hidden in the init/deinit calls. There is no recourse when failing deinit for a certain core, so ignore the return code. Changed return type for deinit in governor and scheduler to void. While here modified the callbacks for scheduler to no longer require passing currently selected governor as an argument. Signed-off-by: Tomasz Zawadzki Change-Id: I7f0b7a09aa7f5d12ae47fca25186faeedac31a95 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8791 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Community-CI: Broadcom CI Reviewed-by: Ben Walker Reviewed-by: Jim Harris Reviewed-by: Konrad Sztyber --- include/spdk_internal/event.h | 11 ++---- lib/event/dpdk_governor.c | 46 +++++++++++++++++----- lib/event/gscheduler.c | 27 +++++-------- lib/event/reactor.c | 34 ++++++---------- lib/event/scheduler_dynamic.c | 32 +++++---------- test/unit/lib/event/reactor.c/reactor_ut.c | 2 - 6 files changed, 70 insertions(+), 82 deletions(-) diff --git a/include/spdk_internal/event.h b/include/spdk_internal/event.h index e80b9f53d..de9c489fd 100644 --- a/include/spdk_internal/event.h +++ b/include/spdk_internal/event.h @@ -180,10 +180,8 @@ struct spdk_governor { int (*set_core_freq_min)(uint32_t lcore_id); int (*get_core_capabilities)(uint32_t lcore_id, struct spdk_governor_capabilities *capabilities); - int (*init_core)(uint32_t lcore_id); - int (*deinit_core)(uint32_t lcore_id); int (*init)(void); - int (*deinit)(void); + void (*deinit)(void); TAILQ_ENTRY(spdk_governor) link; }; @@ -256,20 +254,19 @@ struct spdk_scheduler_core_info { * Scheduler balance function type. * Accepts array of core_info which is of size 'count' and returns updated array. */ -typedef void (*spdk_scheduler_balance_fn)(struct spdk_scheduler_core_info *core_info, int count, - struct spdk_governor *governor); +typedef void (*spdk_scheduler_balance_fn)(struct spdk_scheduler_core_info *core_info, int count); /** * Scheduler init function type. * Called on scheduler module initialization. */ -typedef int (*spdk_scheduler_init_fn)(struct spdk_governor *governor); +typedef int (*spdk_scheduler_init_fn)(void); /** * Scheduler deinitialization function type. * Called on reactor fini. */ -typedef int (*spdk_scheduler_deinit_fn)(struct spdk_governor *governor); +typedef void (*spdk_scheduler_deinit_fn)(void); /** Thread scheduler */ struct spdk_scheduler { diff --git a/lib/event/dpdk_governor.c b/lib/event/dpdk_governor.c index 4a4902943..e6384d746 100644 --- a/lib/event/dpdk_governor.c +++ b/lib/event/dpdk_governor.c @@ -33,6 +33,7 @@ #include "spdk/stdinc.h" #include "spdk/log.h" +#include "spdk/env.h" #include "spdk/event.h" #include "spdk_internal/event.h" @@ -133,18 +134,47 @@ _init_core(uint32_t lcore_id) } static int -_deinit_core(uint32_t lcore_id) +_init(void) { - int rc; + uint32_t i, j; + int rc = 0; - rc = rte_power_exit(lcore_id); - if (rc) { - SPDK_ERRLOG("DPDK Power management library deinitialization failed on core%d\n", lcore_id); + SPDK_ENV_FOREACH_CORE(i) { + rc = _init_core(i); + if (rc != 0) { + SPDK_ERRLOG("Failed to initialize on core%d\n", i); + break; + } } + if (rc == 0) { + return rc; + } + + /* When initalization of a core failed, deinitalize prior cores. */ + SPDK_ENV_FOREACH_CORE(j) { + if (j >= i) { + break; + } + if (rte_power_exit(j) != 0) { + SPDK_ERRLOG("Failed to deinitialize on core%d\n", j); + } + } return rc; } +static void +_deinit(void) +{ + uint32_t i; + + SPDK_ENV_FOREACH_CORE(i) { + if (rte_power_exit(i) != 0) { + SPDK_ERRLOG("Failed to deinitialize on core%d\n", i); + } + } +} + static struct spdk_governor dpdk_governor = { .name = "dpdk_governor", .get_core_curr_freq = _get_core_curr_freq, @@ -153,10 +183,8 @@ static struct spdk_governor dpdk_governor = { .set_core_freq_max = _set_core_freq_max, .set_core_freq_min = _set_core_freq_min, .get_core_capabilities = _get_core_capabilities, - .init_core = _init_core, - .deinit_core = _deinit_core, - .init = NULL, - .deinit = NULL, + .init = _init, + .deinit = _deinit, }; SPDK_GOVERNOR_REGISTER(&dpdk_governor); diff --git a/lib/event/gscheduler.c b/lib/event/gscheduler.c index 2da19a4cc..68a902e7f 100644 --- a/lib/event/gscheduler.c +++ b/lib/event/gscheduler.c @@ -41,41 +41,32 @@ #include "spdk/env.h" static int -init(struct spdk_governor *governor) +init(void) { return _spdk_governor_set("dpdk_governor"); } -static int -deinit(struct spdk_governor *governor) +static void +deinit(void) { - uint32_t i; - int rc = 0; - - SPDK_ENV_FOREACH_CORE(i) { - if (governor->deinit_core) { - rc = governor->deinit_core(i); - if (rc != 0) { - return rc; - } - } - } + struct spdk_governor *governor; + governor = _spdk_governor_get(); if (governor->deinit) { - rc = governor->deinit(); + governor->deinit(); } - - return rc; } static void -balance(struct spdk_scheduler_core_info *cores, int core_count, struct spdk_governor *governor) +balance(struct spdk_scheduler_core_info *cores, int core_count) { + struct spdk_governor *governor; struct spdk_scheduler_core_info *core; struct spdk_governor_capabilities capabilities; uint32_t i; int rc; + governor = _spdk_governor_get(); /* Gather active/idle statistics */ SPDK_ENV_FOREACH_CORE(i) { core = &cores[i]; diff --git a/lib/event/reactor.c b/lib/event/reactor.c index 1dc025a11..a796c361c 100644 --- a/lib/event/reactor.c +++ b/lib/event/reactor.c @@ -120,7 +120,7 @@ _spdk_scheduler_set(char *name) } } else { if (g_scheduler != NULL && g_scheduler->deinit != NULL) { - g_scheduler->deinit(&g_governor); + g_scheduler->deinit(); } g_scheduler = scheduler; } @@ -128,7 +128,7 @@ _spdk_scheduler_set(char *name) g_new_scheduler = scheduler; if (scheduler->init != NULL) { - scheduler->init(&g_governor); + scheduler->init(); } return 0; @@ -306,7 +306,7 @@ spdk_reactors_fini(void) } if (g_scheduler->deinit != NULL) { - g_scheduler->deinit(&g_governor); + g_scheduler->deinit(); } spdk_thread_lib_fini(); @@ -757,7 +757,7 @@ static void _reactors_scheduler_balance(void *arg1, void *arg2) { if (g_reactor_state == SPDK_REACTOR_STATE_RUNNING) { - g_scheduler->balance(g_core_infos, g_reactor_count, &g_governor); + g_scheduler->balance(g_core_infos, g_reactor_count); g_scheduler_core_number = spdk_env_get_first_core(); _reactors_scheduler_update_core_mode(NULL); @@ -964,7 +964,7 @@ reactor_run(void *arg) !g_scheduling_in_progress)) { if (spdk_unlikely(g_scheduler != g_new_scheduler)) { if (g_scheduler->deinit != NULL) { - g_scheduler->deinit(&g_governor); + g_scheduler->deinit(); } g_scheduler = g_new_scheduler; } @@ -1503,33 +1503,21 @@ int _spdk_governor_set(char *name) { struct spdk_governor *governor; - uint32_t i; - int rc; + int rc = 0; governor = _governor_find(name); if (governor == NULL) { return -EINVAL; } - g_governor = *governor; - - if (g_governor.init) { - rc = g_governor.init(); - if (rc != 0) { - return rc; - } + if (governor->init) { + rc = governor->init(); } - SPDK_ENV_FOREACH_CORE(i) { - if (g_governor.init_core) { - rc = g_governor.init_core(i); - if (rc != 0) { - return rc; - } - } + if (rc == 0) { + g_governor = *governor; } - - return 0; + return rc; } struct spdk_governor * diff --git a/lib/event/scheduler_dynamic.c b/lib/event/scheduler_dynamic.c index 37fd5c3e1..881da20e8 100644 --- a/lib/event/scheduler_dynamic.c +++ b/lib/event/scheduler_dynamic.c @@ -221,7 +221,7 @@ _find_optimal_core(struct spdk_scheduler_thread_info *thread_info) } static int -init(struct spdk_governor *governor) +init(void) { int rc; @@ -239,33 +239,18 @@ init(struct spdk_governor *governor) return 0; } -static int -deinit(struct spdk_governor *governor) +static void +deinit(void) { - uint32_t i; - int rc = 0; + struct spdk_governor *governor; free(g_cores); g_cores = NULL; - if (!g_core_mngmnt_available) { - return 0; - } - - if (governor->deinit_core) { - SPDK_ENV_FOREACH_CORE(i) { - rc = governor->deinit_core(i); - if (rc != 0) { - SPDK_ERRLOG("Failed to deinitialize governor for core %d\n", i); - } - } - } - + governor = _spdk_governor_get(); if (governor->deinit) { - rc = governor->deinit(); + governor->deinit(); } - - return rc; } static void @@ -293,10 +278,10 @@ _balance_active(struct spdk_scheduler_thread_info *thread_info) } static void -balance(struct spdk_scheduler_core_info *cores_info, int cores_count, - struct spdk_governor *governor) +balance(struct spdk_scheduler_core_info *cores_info, int cores_count) { struct spdk_reactor *reactor; + struct spdk_governor *governor; struct spdk_scheduler_core_info *core; struct core_stats *main_core; uint32_t i; @@ -338,6 +323,7 @@ balance(struct spdk_scheduler_core_info *cores_info, int cores_count, return; } + governor = _spdk_governor_get(); /* Change main core frequency if needed */ if (busy_threads_present) { rc = governor->set_core_freq_max(g_main_lcore); diff --git a/test/unit/lib/event/reactor.c/reactor_ut.c b/test/unit/lib/event/reactor.c/reactor_ut.c index 5447ab37a..a5c8940fd 100644 --- a/test/unit/lib/event/reactor.c/reactor_ut.c +++ b/test/unit/lib/event/reactor.c/reactor_ut.c @@ -770,8 +770,6 @@ static struct spdk_governor governor = { .set_core_freq_max = core_freq_max, .set_core_freq_min = NULL, .get_core_capabilities = NULL, - .init_core = NULL, - .deinit_core = NULL, .init = NULL, .deinit = NULL, };