From 90a5c8c547b0232179112b7bee0ff8ec19450cb8 Mon Sep 17 00:00:00 2001 From: Aviv Ben-David Date: Tue, 4 Jan 2022 17:35:14 +0200 Subject: [PATCH] env/memory: fix unregistration of memory after memory registration issue The current error handling of `spdk_mem_map_notify_walk` has off by 2 issue. This issue can split one memory region to multiple smaller regions when calling the callback to unregister the memory region. Also, in case of failure, the 1 GB maps of the map failed to be freed. RDMA doesn't support this behavior and support calling the callback only once for each previously registered memory region. Signed-off-by: Aviv Ben-David Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11219 (master) (cherry picked from commit c8837711239bdf69654102a641b25d977bb78ab9) Change-Id: I65b667f2e84533f234a2e330b20e9ad9eef32854 Signed-off-by: Krzysztof Karas Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12476 Reviewed-by: Tomasz Zawadzki Reviewed-by: Konrad Sztyber Reviewed-by: Jim Harris Tested-by: SPDK CI Jenkins --- lib/env_dpdk/memory.c | 12 +++++++++--- test/env/memory/memory_ut.c | 10 ++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/env_dpdk/memory.c b/lib/env_dpdk/memory.c index 62c594227..36c08e23b 100644 --- a/lib/env_dpdk/memory.c +++ b/lib/env_dpdk/memory.c @@ -252,10 +252,10 @@ err_unregister: } for (; idx_1gb < UINT64_MAX; idx_1gb--) { + /* Rebuild the virtual address from the indexes */ + uint64_t vaddr = (idx_256tb << SHIFT_1GB) | (idx_1gb << SHIFT_2MB); if ((map_1gb->map[idx_1gb].translation_2mb & REG_MAP_REGISTERED) && (contig_end == UINT64_MAX || (map_1gb->map[idx_1gb].translation_2mb & REG_MAP_NOTIFY_START) == 0)) { - /* Rebuild the virtual address from the indexes */ - uint64_t vaddr = (idx_256tb << SHIFT_1GB) | (idx_1gb << SHIFT_2MB); if (contig_end == UINT64_MAX) { contig_end = vaddr; @@ -263,12 +263,14 @@ err_unregister: contig_start = vaddr; } else { if (contig_end != UINT64_MAX) { + if (map_1gb->map[idx_1gb].translation_2mb & REG_MAP_NOTIFY_START) { + contig_start = vaddr; + } /* End of of a virtually contiguous range */ map->ops.notify_cb(map->cb_ctx, map, SPDK_MEM_MAP_NOTIFY_UNREGISTER, (void *)contig_start, contig_end - contig_start + VALUE_2MB); - idx_1gb++; } contig_end = UINT64_MAX; } @@ -285,6 +287,7 @@ spdk_mem_map_alloc(uint64_t default_translation, const struct spdk_mem_map_ops * { struct spdk_mem_map *map; int rc; + size_t i; map = calloc(1, sizeof(*map)); if (map == NULL) { @@ -309,6 +312,9 @@ spdk_mem_map_alloc(uint64_t default_translation, const struct spdk_mem_map_ops * pthread_mutex_unlock(&g_spdk_mem_map_mutex); DEBUG_PRINT("Initial mem_map notify failed\n"); pthread_mutex_destroy(&map->mutex); + for (i = 0; i < sizeof(map->map_256tb.map) / sizeof(map->map_256tb.map[0]); i++) { + free(map->map_256tb.map[i]); + } free(map); return NULL; } diff --git a/test/env/memory/memory_ut.c b/test/env/memory/memory_ut.c index e0583375f..3fb5b7f18 100644 --- a/test/env/memory/memory_ut.c +++ b/test/env/memory/memory_ut.c @@ -100,6 +100,8 @@ test_mem_map_notify_fail(void *cb_ctx, struct spdk_mem_map *map, enum spdk_mem_map_notify_action action, void *vaddr, size_t size) { struct spdk_mem_map *reg_map = cb_ctx; + uint64_t reg_addr; + uint64_t reg_size = size; switch (action) { case SPDK_MEM_MAP_NOTIFY_REGISTER: @@ -107,8 +109,16 @@ test_mem_map_notify_fail(void *cb_ctx, struct spdk_mem_map *map, /* Test the error handling. */ return -1; } + + CU_ASSERT(spdk_mem_map_set_translation(map, (uint64_t)vaddr, (uint64_t)size, (uint64_t)vaddr) == 0); + break; case SPDK_MEM_MAP_NOTIFY_UNREGISTER: + /* validate the start address */ + reg_addr = spdk_mem_map_translate(map, (uint64_t)vaddr, ®_size); + CU_ASSERT(reg_addr == (uint64_t)vaddr); + spdk_mem_map_clear_translation(map, (uint64_t)vaddr, size); + /* Clear the same region in the other mem_map to be able to * verify that there was no memory left still registered after * the mem_map creation failure.