From aebbce25206db5ce247d69dbf72d97b1965e37c7 Mon Sep 17 00:00:00 2001 From: Evgeniy Kochetov Date: Thu, 28 Jan 2021 16:33:07 +0200 Subject: [PATCH] nvme: Refactor active namespace list retrieval Previous implementation allocated memory just once at the beginning of active NS list retrieval procedure. It allocated memory for maximum possible number of active namespaces, i.e. 'cdata.nn'. This patch changes allocation logic. One page is allocated at the beginning. If more is needed, reallocation is done with one more page. This patch also removes SPDK_MALLOC_DMA flag from allocation since we don't do RDMA directly into this buffer. Signed-off-by: Evgeniy Kochetov Change-Id: Iaa80c4d70c54daaf71dcbf755c63a01a1d83b772 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/6502 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- lib/nvme/nvme_ctrlr.c | 68 ++++++--- lib/nvme/nvme_internal.h | 1 + .../lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c | 143 ++++++++++++++++-- 3 files changed, 180 insertions(+), 32 deletions(-) diff --git a/lib/nvme/nvme_ctrlr.c b/lib/nvme/nvme_ctrlr.c index 2684d2d70..5a9a3a3ab 100644 --- a/lib/nvme/nvme_ctrlr.c +++ b/lib/nvme/nvme_ctrlr.c @@ -1784,7 +1784,6 @@ typedef void (*nvme_active_ns_ctx_deleter)(struct nvme_active_ns_ctx *); struct nvme_active_ns_ctx { struct spdk_nvme_ctrlr *ctrlr; uint32_t page; - uint32_t num_pages; uint32_t next_nsid; uint32_t *new_ns_list; nvme_active_ns_ctx_deleter deleter; @@ -1796,7 +1795,6 @@ static struct nvme_active_ns_ctx * nvme_active_ns_ctx_create(struct spdk_nvme_ctrlr *ctrlr, nvme_active_ns_ctx_deleter deleter) { struct nvme_active_ns_ctx *ctx; - uint32_t num_pages = 0; uint32_t *new_ns_list = NULL; ctx = calloc(1, sizeof(*ctx)); @@ -1805,19 +1803,14 @@ nvme_active_ns_ctx_create(struct spdk_nvme_ctrlr *ctrlr, nvme_active_ns_ctx_dele return NULL; } - if (ctrlr->num_ns) { - /* The allocated size must be a multiple of sizeof(struct spdk_nvme_ns_list) */ - num_pages = (ctrlr->num_ns * sizeof(new_ns_list[0]) - 1) / sizeof(struct spdk_nvme_ns_list) + 1; - new_ns_list = spdk_zmalloc(num_pages * sizeof(struct spdk_nvme_ns_list), ctrlr->page_size, - NULL, SPDK_ENV_LCORE_ID_ANY, SPDK_MALLOC_DMA | SPDK_MALLOC_SHARE); - if (!new_ns_list) { - NVME_CTRLR_ERRLOG(ctrlr, "Failed to allocate active_ns_list!\n"); - free(ctx); - return NULL; - } + new_ns_list = spdk_zmalloc(sizeof(struct spdk_nvme_ns_list), ctrlr->page_size, + NULL, SPDK_ENV_LCORE_ID_ANY, SPDK_MALLOC_SHARE); + if (!new_ns_list) { + NVME_CTRLR_ERRLOG(ctrlr, "Failed to allocate active_ns_list!\n"); + free(ctx); + return NULL; } - ctx->num_pages = num_pages; ctx->new_ns_list = new_ns_list; ctx->ctrlr = ctrlr; ctx->deleter = deleter; @@ -1835,8 +1828,12 @@ nvme_active_ns_ctx_destroy(struct nvme_active_ns_ctx *ctx) static void nvme_ctrlr_identify_active_ns_swap(struct spdk_nvme_ctrlr *ctrlr, uint32_t **new_ns_list) { + uint32_t max_active_ns_idx = 0; + + while ((*new_ns_list)[max_active_ns_idx++]); spdk_free(ctrlr->active_ns_list); ctrlr->active_ns_list = *new_ns_list; + ctrlr->max_active_ns_idx = max_active_ns_idx; *new_ns_list = NULL; } @@ -1844,6 +1841,7 @@ static void nvme_ctrlr_identify_active_ns_async_done(void *arg, const struct spdk_nvme_cpl *cpl) { struct nvme_active_ns_ctx *ctx = arg; + uint32_t *new_ns_list = NULL; if (spdk_nvme_cpl_is_error(cpl)) { ctx->state = NVME_ACTIVE_NS_STATE_ERROR; @@ -1851,11 +1849,22 @@ nvme_ctrlr_identify_active_ns_async_done(void *arg, const struct spdk_nvme_cpl * } ctx->next_nsid = ctx->new_ns_list[1024 * ctx->page + 1023]; - if (ctx->next_nsid == 0 || ++ctx->page == ctx->num_pages) { + if (ctx->next_nsid == 0) { ctx->state = NVME_ACTIVE_NS_STATE_DONE; goto out; } + ctx->page++; + new_ns_list = spdk_realloc(ctx->new_ns_list, + (ctx->page + 1) * sizeof(struct spdk_nvme_ns_list), + ctx->ctrlr->page_size); + if (!new_ns_list) { + SPDK_ERRLOG("Failed to reallocate active_ns_list!\n"); + ctx->state = NVME_ACTIVE_NS_STATE_ERROR; + goto out; + } + + ctx->new_ns_list = new_ns_list; nvme_ctrlr_identify_active_ns_async(ctx); return; @@ -1872,7 +1881,7 @@ nvme_ctrlr_identify_active_ns_async(struct nvme_active_ns_ctx *ctx) uint32_t i; int rc; - if (ctrlr->num_ns == 0) { + if (ctrlr->cdata.nn == 0) { ctx->state = NVME_ACTIVE_NS_STATE_DONE; goto out; } @@ -1884,7 +1893,27 @@ nvme_ctrlr_identify_active_ns_async(struct nvme_active_ns_ctx *ctx) * an active ns list, i.e. all namespaces report as active */ if (ctrlr->vs.raw < SPDK_NVME_VERSION(1, 1, 0) || ctrlr->quirks & NVME_QUIRK_IDENTIFY_CNS) { - for (i = 0; i < ctrlr->num_ns; i++) { + uint32_t *new_ns_list; + uint32_t num_pages; + + /* + * Active NS list must always end with zero element. + * So, we allocate for cdata.nn+1. + */ + num_pages = spdk_divide_round_up(ctrlr->cdata.nn + 1, + sizeof(struct spdk_nvme_ns_list) / sizeof(new_ns_list[0])); + new_ns_list = spdk_realloc(ctx->new_ns_list, + num_pages * sizeof(struct spdk_nvme_ns_list), + ctx->ctrlr->page_size); + if (!new_ns_list) { + SPDK_ERRLOG("Failed to reallocate active_ns_list!\n"); + ctx->state = NVME_ACTIVE_NS_STATE_ERROR; + goto out; + } + + ctx->new_ns_list = new_ns_list; + ctx->new_ns_list[ctrlr->cdata.nn] = 0; + for (i = 0; i < ctrlr->cdata.nn; i++) { ctx->new_ns_list[i] = i + 1; } @@ -2498,6 +2527,7 @@ nvme_ctrlr_destruct_namespaces(struct spdk_nvme_ctrlr *ctrlr) spdk_free(ctrlr->active_ns_list); ctrlr->active_ns_list = NULL; + ctrlr->max_active_ns_idx = 0; } static void @@ -3631,12 +3661,12 @@ nvme_ctrlr_active_ns_idx(struct spdk_nvme_ctrlr *ctrlr, uint32_t nsid) { int32_t result = -1; - if (ctrlr->active_ns_list == NULL || nsid == 0 || nsid > ctrlr->num_ns) { + if (ctrlr->active_ns_list == NULL || nsid == 0 || nsid > ctrlr->cdata.nn) { return result; } int32_t lower = 0; - int32_t upper = ctrlr->num_ns - 1; + int32_t upper = ctrlr->max_active_ns_idx; int32_t mid; while (lower <= upper) { @@ -3673,7 +3703,7 @@ uint32_t spdk_nvme_ctrlr_get_next_active_ns(struct spdk_nvme_ctrlr *ctrlr, uint32_t prev_nsid) { int32_t nsid_idx = nvme_ctrlr_active_ns_idx(ctrlr, prev_nsid); - if (ctrlr->active_ns_list && nsid_idx >= 0 && (uint32_t)nsid_idx < ctrlr->num_ns - 1) { + if (nsid_idx >= 0 && (uint32_t)nsid_idx < ctrlr->max_active_ns_idx) { return ctrlr->active_ns_list[nsid_idx + 1]; } return 0; diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index 115f33ec1..21397623a 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -826,6 +826,7 @@ struct spdk_nvme_ctrlr { /** * Keep track of active namespaces */ + uint32_t max_active_ns_idx; uint32_t *active_ns_list; struct spdk_bit_array *free_io_qids; diff --git a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c index 22e65434c..8378b2f2c 100644 --- a/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c +++ b/test/unit/lib/nvme/nvme_ctrlr.c/nvme_ctrlr_ut.c @@ -2,7 +2,7 @@ * BSD LICENSE * * Copyright (c) Intel Corporation. All rights reserved. - * Copyright (c) 2020 Mellanox Technologies LTD. All rights reserved. + * Copyright (c) 2020, 2021 Mellanox Technologies LTD. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -387,6 +387,9 @@ nvme_ctrlr_cmd_set_async_event_config(struct spdk_nvme_ctrlr *ctrlr, return 0; } +static uint32_t *g_active_ns_list = NULL; +static uint32_t g_active_ns_list_length = 0; + int nvme_ctrlr_cmd_identify(struct spdk_nvme_ctrlr *ctrlr, uint8_t cns, uint16_t cntid, uint32_t nsid, uint8_t csi, void *payload, size_t payload_size, @@ -397,14 +400,29 @@ nvme_ctrlr_cmd_identify(struct spdk_nvme_ctrlr *ctrlr, uint8_t cns, uint16_t cnt uint32_t i = 0; struct spdk_nvme_ns_list *ns_list = (struct spdk_nvme_ns_list *)payload; - for (i = 1; i <= ctrlr->num_ns; i++) { - if (i <= nsid) { - continue; - } + memset(payload, 0, payload_size); + if (g_active_ns_list == NULL) { + for (i = 1; i <= ctrlr->num_ns; i++) { + if (i <= nsid) { + continue; + } - ns_list->ns_list[count++] = i; - if (count == SPDK_COUNTOF(ns_list->ns_list)) { - break; + ns_list->ns_list[count++] = i; + if (count == SPDK_COUNTOF(ns_list->ns_list)) { + break; + } + } + } else { + for (i = 0; i < g_active_ns_list_length; i++) { + uint32_t cur_nsid = g_active_ns_list[i]; + if (cur_nsid <= nsid) { + continue; + } + + ns_list->ns_list[count++] = cur_nsid; + if (count == SPDK_COUNTOF(ns_list->ns_list)) { + break; + } } } @@ -1882,17 +1900,17 @@ test_nvme_ctrlr_test_active_ns(void) ctrlr.vs.bits.mjr = 1; ctrlr.vs.bits.mnr = minor; ctrlr.vs.bits.ter = 0; - ctrlr.num_ns = 1531; + ctrlr.num_ns = ctrlr.cdata.nn = 1531; nvme_ctrlr_identify_active_ns(&ctrlr); for (nsid = 1; nsid <= ctrlr.num_ns; nsid++) { CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, nsid) == true); } - ctrlr.num_ns = 1559; - for (; nsid <= ctrlr.num_ns; nsid++) { + + for (; nsid <= 1559; nsid++) { CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, nsid) == false); } - ctrlr.num_ns = 1531; + for (nsid = 0; nsid < ctrlr.num_ns; nsid++) { ctrlr.active_ns_list[nsid] = 0; } @@ -1940,7 +1958,7 @@ test_nvme_ctrlr_test_active_ns_error_case(void) ctrlr.vs.bits.mjr = 1; ctrlr.vs.bits.mnr = 2; ctrlr.vs.bits.ter = 0; - ctrlr.num_ns = 2; + ctrlr.cdata.nn = 2; set_status_code = SPDK_NVME_SC_INVALID_FIELD; rc = nvme_ctrlr_identify_active_ns(&ctrlr); @@ -2474,6 +2492,103 @@ test_nvme_ctrlr_set_state(void) MOCK_CLEAR(spdk_get_ticks); } +static void +test_nvme_ctrlr_active_ns_list_v0(void) +{ + DECLARE_AND_CONSTRUCT_CTRLR(); + + ctrlr.vs.bits.mjr = 1; + ctrlr.vs.bits.mnr = 0; + ctrlr.vs.bits.ter = 0; + ctrlr.cdata.nn = 1024; + + ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); + CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024)); + CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1025)); + CU_ASSERT(spdk_nvme_ctrlr_get_first_active_ns(&ctrlr) == 1); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1023) == 1024); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1024) == 0); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1025) == 0); + + nvme_ctrlr_destruct(&ctrlr); +} + +static void +test_nvme_ctrlr_active_ns_list_v2(void) +{ + uint32_t i; + uint32_t active_ns_list[1024]; + DECLARE_AND_CONSTRUCT_CTRLR(); + + ctrlr.vs.bits.mjr = 1; + ctrlr.vs.bits.mnr = 2; + ctrlr.vs.bits.ter = 0; + ctrlr.cdata.nn = 4096; + + g_active_ns_list = active_ns_list; + g_active_ns_list_length = SPDK_COUNTOF(active_ns_list); + + /* No active namespaces */ + memset(active_ns_list, 0, sizeof(active_ns_list)); + ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); + CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024)); + CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1025)); + CU_ASSERT(spdk_nvme_ctrlr_get_first_active_ns(&ctrlr) == 0); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1024) == 0); + + nvme_ctrlr_destruct(&ctrlr); + + /* 1024 active namespaces - one full page */ + memset(active_ns_list, 0, sizeof(active_ns_list)); + for (i = 0; i < 1024; ++i) { + active_ns_list[i] = i + 1; + } + + ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; + g_active_ns_list = active_ns_list; + g_active_ns_list_length = SPDK_COUNTOF(active_ns_list); + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); + CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024)); + CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1025)); + CU_ASSERT(spdk_nvme_ctrlr_get_first_active_ns(&ctrlr) == 1); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1023) == 1024); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1024) == 0); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1025) == 0); + + nvme_ctrlr_destruct(&ctrlr); + + /* 1023 active namespaces - full page minus one */ + memset(active_ns_list, 0, sizeof(active_ns_list)); + for (i = 0; i < 1023; ++i) { + active_ns_list[i] = i + 1; + } + + ctrlr.state = NVME_CTRLR_STATE_IDENTIFY_ACTIVE_NS; + SPDK_CU_ASSERT_FATAL(nvme_ctrlr_process_init(&ctrlr) == 0); /* -> IDENTIFY_NS */ + SPDK_CU_ASSERT_FATAL(ctrlr.state == NVME_CTRLR_STATE_IDENTIFY_NS); + CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1)); + CU_ASSERT(spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1023)); + CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1024)); + CU_ASSERT(!spdk_nvme_ctrlr_is_active_ns(&ctrlr, 1025)); + CU_ASSERT(spdk_nvme_ctrlr_get_first_active_ns(&ctrlr) == 1); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1023) == 0); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1024) == 0); + CU_ASSERT(spdk_nvme_ctrlr_get_next_active_ns(&ctrlr, 1025) == 0); + + nvme_ctrlr_destruct(&ctrlr); + + g_active_ns_list = NULL; + g_active_ns_list_length = 0; +} + int main(int argc, char **argv) { CU_pSuite suite = NULL; @@ -2518,6 +2633,8 @@ int main(int argc, char **argv) CU_ADD_TEST(suite, test_nvme_cmd_map_sgls); CU_ADD_TEST(suite, test_nvme_ctrlr_set_arbitration_feature); CU_ADD_TEST(suite, test_nvme_ctrlr_set_state); + CU_ADD_TEST(suite, test_nvme_ctrlr_active_ns_list_v0); + CU_ADD_TEST(suite, test_nvme_ctrlr_active_ns_list_v2); CU_basic_set_mode(CU_BRM_VERBOSE); CU_basic_run_tests();