Commit c8837711 authored by Aviv Ben-David's avatar Aviv Ben-David Committed by Tomasz Zawadzki
Browse files

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: default avatarAviv Ben-David <aviv.bendavid@vastdata.com>
Change-Id: I65b667f2e84533f234a2e330b20e9ad9eef32854
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11219


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent ba4ffda6

lib/env_dpdk/memory.c

+9 −3
Original line number Diff line number Diff line
@@ -252,10 +252,10 @@ err_unregister: 
		}



		for (; idx_1gb < UINT64_MAX; idx_1gb--) {

			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 ((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)) {



				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;

		}

test/env/memory/memory_ut.c

+10 −0
Original line number Diff line number Diff line
@@ -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.