ivampiresp/Spdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

env/memory: fix unregistration of memory after memory registration issue

Browse Source 
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 <aviv.bendavid@vastdata.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11219 (master)

(cherry picked from commit c883771123)
Change-Id: I65b667f2e84533f234a2e330b20e9ad9eef32854
Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12476
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Konrad Sztyber <konrad.sztyber@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
This commit is contained in:
Aviv Ben-David 2022-01-04 17:35:14 +02:00 committed by Keith Lucas
parent 56d5ded6f2
commit 90a5c8c547
2 changed files with 19 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
lib/env_dpdk/memory.c
View File

@ -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;
}

10
test/env/memory/memory_ut.c vendored
View File

@ -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, &reg_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.