From 2e394521a6713a5a6ad9e7a8c1990db9049d0b5e Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Thu, 15 Jul 2021 08:53:29 -0400 Subject: [PATCH] lib/event: refactor _spdk_governor_set() By default g_governor is now NULL. It can be set either by event framework or schedulers directly. Dynamic_scheduler and gscheduler specifically want to use the dpdk_governor, so their initialization now sets it explicitly. To unset and deinitialize current governor, _spdk_governor_set(NULL) has to be called. This results in moving governor deinitalization to that call too. The "default" governor has been removed. Every spdk_governor callback is now mandatory. Signed-off-by: Tomasz Zawadzki Change-Id: Ibf76bd28bfbb159416026996fa217bb3325a3d31 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8810 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 | 6 +++- lib/event/app_rpc.c | 7 ++-- lib/event/gscheduler.c | 9 ++--- lib/event/reactor.c | 38 +++++++++++----------- lib/event/scheduler_dynamic.c | 9 ++--- test/unit/lib/event/reactor.c/reactor_ut.c | 14 +++++--- 6 files changed, 44 insertions(+), 39 deletions(-) diff --git a/include/spdk_internal/event.h b/include/spdk_internal/event.h index de9c489fdd..3989f995f8 100644 --- a/include/spdk_internal/event.h +++ b/include/spdk_internal/event.h @@ -198,7 +198,11 @@ struct spdk_governor { void _spdk_governor_list_add(struct spdk_governor *governor); /** - * Change current governor. + * Set the current governor. + * + * Always deinitalizes previously set governor. + * No governor will be set if name parameter is NULL. + * This function should be invoked on scheduling reactor. * * \param name Name of the governor to be used. * diff --git a/lib/event/app_rpc.c b/lib/event/app_rpc.c index 020395f637..df01c5cb21 100644 --- a/lib/event/app_rpc.c +++ b/lib/event/app_rpc.c @@ -400,8 +400,7 @@ _rpc_framework_get_reactors(void *arg1, void *arg2) spdk_json_write_named_uint64(ctx->w, "idle", reactor->idle_tsc); spdk_json_write_named_bool(ctx->w, "in_interrupt", reactor->in_interrupt); governor = _spdk_governor_get(); - /* We need to check whether governor can return current core frequency. */ - if (governor->get_core_curr_freq != NULL) { + if (governor != NULL) { /* Governor returns core freqs in kHz, we want MHz. */ curr_core_freq = governor->get_core_curr_freq(current_core) / 1000; spdk_json_write_named_uint32(ctx->w, "core_freq", curr_core_freq); @@ -528,7 +527,9 @@ rpc_framework_get_scheduler(struct spdk_jsonrpc_request *request, spdk_json_write_object_begin(w); spdk_json_write_named_string(w, "scheduler_name", scheduler->name); spdk_json_write_named_uint64(w, "scheduler_period", scheduler_period); - spdk_json_write_named_string(w, "governor_name", governor->name); + if (governor != NULL) { + spdk_json_write_named_string(w, "governor_name", governor->name); + } spdk_json_write_object_end(w); spdk_jsonrpc_end_result(request, w); } diff --git a/lib/event/gscheduler.c b/lib/event/gscheduler.c index 68a902e7f8..1401339256 100644 --- a/lib/event/gscheduler.c +++ b/lib/event/gscheduler.c @@ -49,12 +49,7 @@ init(void) static void deinit(void) { - struct spdk_governor *governor; - - governor = _spdk_governor_get(); - if (governor->deinit) { - governor->deinit(); - } + _spdk_governor_set(NULL); } static void @@ -67,6 +62,8 @@ balance(struct spdk_scheduler_core_info *cores, int core_count) int rc; governor = _spdk_governor_get(); + assert(governor != NULL); + /* 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 a796c361c7..4fa1d94215 100644 --- a/lib/event/reactor.c +++ b/lib/event/reactor.c @@ -77,13 +77,7 @@ static struct spdk_scheduler_core_info *g_core_infos = NULL; TAILQ_HEAD(, spdk_governor) g_governor_list = TAILQ_HEAD_INITIALIZER(g_governor_list); -static int _governor_get_capabilities(uint32_t lcore_id, - struct spdk_governor_capabilities *capabilities); - -static struct spdk_governor g_governor = { - .name = "default", - .get_core_capabilities = _governor_get_capabilities, -}; +static struct spdk_governor *g_governor; static int reactor_interrupt_init(struct spdk_reactor *reactor); static void reactor_interrupt_fini(struct spdk_reactor *reactor); @@ -1477,14 +1471,6 @@ reactor_interrupt_fini(struct spdk_reactor *reactor) reactor->fgrp = NULL; } -static int -_governor_get_capabilities(uint32_t lcore_id, struct spdk_governor_capabilities *capabilities) -{ - capabilities->priority = false; - - return 0; -} - static struct spdk_governor * _governor_find(char *name) { @@ -1505,25 +1491,39 @@ _spdk_governor_set(char *name) struct spdk_governor *governor; int rc = 0; + /* NULL governor was specifically requested */ + if (name == NULL) { + if (g_governor) { + g_governor->deinit(); + } + g_governor = NULL; + return 0; + } + governor = _governor_find(name); if (governor == NULL) { return -EINVAL; } - if (governor->init) { - rc = governor->init(); + if (g_governor == governor) { + return 0; } + rc = governor->init(); if (rc == 0) { - g_governor = *governor; + if (g_governor) { + g_governor->deinit(); + } + g_governor = governor; } + return rc; } struct spdk_governor * _spdk_governor_get(void) { - return &g_governor; + return g_governor; } void diff --git a/lib/event/scheduler_dynamic.c b/lib/event/scheduler_dynamic.c index 881da20e89..ab1f81a0b0 100644 --- a/lib/event/scheduler_dynamic.c +++ b/lib/event/scheduler_dynamic.c @@ -242,15 +242,10 @@ init(void) static void deinit(void) { - struct spdk_governor *governor; - free(g_cores); g_cores = NULL; - governor = _spdk_governor_get(); - if (governor->deinit) { - governor->deinit(); - } + _spdk_governor_set(NULL); } static void @@ -324,6 +319,8 @@ balance(struct spdk_scheduler_core_info *cores_info, int cores_count) } governor = _spdk_governor_get(); + assert(governor != NULL); + /* 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 a5c8940fd9..5295ab4277 100644 --- a/test/unit/lib/event/reactor.c/reactor_ut.c +++ b/test/unit/lib/event/reactor.c/reactor_ut.c @@ -762,16 +762,22 @@ core_freq_max(uint32_t lcore) return 0; } +DEFINE_STUB(core_freq_min, int, (uint32_t lcore_id), 0); +DEFINE_STUB(core_caps, int, + (uint32_t lcore_id, struct spdk_governor_capabilities *capabilities), 0); +DEFINE_STUB(governor_init, int, (void), 0); +DEFINE_STUB_V(governor_deinit, (void)); + static struct spdk_governor governor = { .name = "dpdk_governor", .get_core_curr_freq = NULL, .core_freq_up = core_freq_up, .core_freq_down = core_freq_down, .set_core_freq_max = core_freq_max, - .set_core_freq_min = NULL, - .get_core_capabilities = NULL, - .init = NULL, - .deinit = NULL, + .set_core_freq_min = core_freq_min, + .get_core_capabilities = core_caps, + .init = governor_init, + .deinit = governor_deinit, }; static void