lib/sock: provide a hint to picking optimal poll group

The process of matching qpair to poll group is split into
two distinct parts that occur on different threads.
See spdk_nvmf_tgt_new_qpair().

This results in a race condition for TCP between spdk_sock_map_lookup()
and spdk_sock_map_insert(), which are called in spdk_nvmf_get_optimal_poll_group()
and spdk_nvmf_poll_group_add() respectively.

Fixes #2113

This patch picks a hint from nvmf_tcp for next poll group,
which is then passed down to spdk_sock_map_lookup().

When matching placement_id exists, but does not have
a poll group assigned - the hint will be used.

Signed-off-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Change-Id: I4abde2bc9c39225c9f5dd7c3654fa2639bb0a27f
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10271
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>

CHANGELOG.md

@@ -199,6 +199,11 @@ individual traces.
 Added `spdk_ioviter_first` and `spdk_ioviter_next` to iterate over two iovecs and
 yield pointers to common length segments.
 
+### sock
+
+A new parameter `hint` is added to `spdk_sock_get_optimal_sock_group`. It allows to suggest
+a poll group when no optimal poll group is found.
+
 ## v21.10
 
 Structure `spdk_nvmf_target_opts` has been extended with new member `discovery_filter` which allows to specify

include/spdk/sock.h

@@ -467,10 +467,12 @@ int spdk_sock_group_close(struct spdk_sock_group **group);
  *
  * \param sock The socket
  * \param group Returns the optimal sock group. If there is no optimal sock group, returns NULL.
+ * \param hint When return is 0 and group is set to NULL, hint is used to set optimal sock group for the socket.
  *
  * \return 0 on success. Negated errno on failure.
  */
-int spdk_sock_get_optimal_sock_group(struct spdk_sock *sock, struct spdk_sock_group **group);
+int spdk_sock_get_optimal_sock_group(struct spdk_sock *sock, struct spdk_sock_group **group,
+				     struct spdk_sock_group *hint);
 
 /**
  * Get current socket implementation options.

include/spdk_internal/sock.h

@@ -114,7 +114,8 @@ struct spdk_net_impl {
 	bool (*is_ipv4)(struct spdk_sock *sock);
 	bool (*is_connected)(struct spdk_sock *sock);
 
-	struct spdk_sock_group_impl *(*group_impl_get_optimal)(struct spdk_sock *sock);
+	struct spdk_sock_group_impl *(*group_impl_get_optimal)(struct spdk_sock *sock,
+			struct spdk_sock_group_impl *hint);
 	struct spdk_sock_group_impl *(*group_impl_create)(void);
 	int (*group_impl_add_sock)(struct spdk_sock_group_impl *group, struct spdk_sock *sock);
 	int (*group_impl_remove_sock)(struct spdk_sock_group_impl *group, struct spdk_sock *sock);
@@ -323,7 +324,7 @@ void spdk_sock_map_release(struct spdk_sock_map *map, int placement_id);
  * Look up the group for the given placement_id.
  */
 int spdk_sock_map_lookup(struct spdk_sock_map *map, int placement_id,
-			 struct spdk_sock_group_impl **group_impl);
+			 struct spdk_sock_group_impl **group_impl, struct spdk_sock_group_impl *hint);
 
 /**
  * Find a placement id with no associated group

lib/nvme/nvme_tcp.c

@@ -2235,7 +2235,7 @@ nvme_tcp_qpair_get_optimal_poll_group(struct spdk_nvme_qpair *qpair)
 	struct spdk_sock_group *group = NULL;
 	int rc;
 
-	rc = spdk_sock_get_optimal_sock_group(tqpair->sock, &group);
+	rc = spdk_sock_get_optimal_sock_group(tqpair->sock, &group, NULL);
 	if (!rc && group != NULL) {
 		return spdk_sock_group_get_ctx(group);
 	}

lib/nvmf/tcp.c

@@ -1290,18 +1290,11 @@ static struct spdk_nvmf_transport_poll_group *
 nvmf_tcp_get_optimal_poll_group(struct spdk_nvmf_qpair *qpair)
 {
 	struct spdk_nvmf_tcp_transport *ttransport;
-	struct spdk_nvmf_transport_poll_group *result;
 	struct spdk_nvmf_tcp_poll_group **pg;
 	struct spdk_nvmf_tcp_qpair *tqpair;
-	struct spdk_sock_group *group = NULL;
+	struct spdk_sock_group *group = NULL, *hint = NULL;
 	int rc;
 
-	tqpair = SPDK_CONTAINEROF(qpair, struct spdk_nvmf_tcp_qpair, qpair);
-	rc = spdk_sock_get_optimal_sock_group(tqpair->sock, &group);
-	if (!rc && group != NULL) {
-		return spdk_sock_group_get_ctx(group);
-	}
-
 	ttransport = SPDK_CONTAINEROF(qpair->transport, struct spdk_nvmf_tcp_transport, transport);
 
 	pthread_mutex_lock(&ttransport->lock);
@@ -1313,16 +1306,27 @@ nvmf_tcp_get_optimal_poll_group(struct spdk_nvmf_qpair *qpair)
 
 	pg = &ttransport->next_pg;
 	assert(*pg != NULL);
+	hint = (*pg)->sock_group;
 
-	result = &(*pg)->group;
+	tqpair = SPDK_CONTAINEROF(qpair, struct spdk_nvmf_tcp_qpair, qpair);
+	rc = spdk_sock_get_optimal_sock_group(tqpair->sock, &group, hint);
+	if (rc != 0) {
+		pthread_mutex_unlock(&ttransport->lock);
+		return NULL;
+	} else if (group != NULL) {
+		/* Optimal poll group was found */
+		pthread_mutex_unlock(&ttransport->lock);
+		return spdk_sock_group_get_ctx(group);
+	}
 
+	/* The hint was used for optimal poll group, advance next_pg. */
 	*pg = TAILQ_NEXT(*pg, link);
 	if (*pg == NULL) {
 		*pg = TAILQ_FIRST(&ttransport->poll_groups);
 	}
 
 	pthread_mutex_unlock(&ttransport->lock);
-	return result;
+	return spdk_sock_group_get_ctx(hint);
 }
 
 static void

lib/sock/sock.c

@@ -38,6 +38,7 @@
 #include "spdk_internal/sock.h"
 #include "spdk/log.h"
 #include "spdk/env.h"
+#include "spdk/util.h"
 
 #define SPDK_SOCK_DEFAULT_PRIORITY 0
 #define SPDK_SOCK_DEFAULT_ZCOPY true
@@ -151,10 +152,9 @@ spdk_sock_map_release(struct spdk_sock_map *map, int placement_id)
 
 int
 spdk_sock_map_lookup(struct spdk_sock_map *map, int placement_id,
-		     struct spdk_sock_group_impl **group)
+		     struct spdk_sock_group_impl **group, struct spdk_sock_group_impl *hint)
 {
 	struct spdk_sock_placement_id_entry *entry;
-	int rc = -EINVAL;
 
 	*group = NULL;
 	pthread_mutex_lock(&map->mtx);
@@ -162,14 +162,33 @@ spdk_sock_map_lookup(struct spdk_sock_map *map, int placement_id,
 		if (placement_id == entry->placement_id) {
 			*group = entry->group;
 			if (*group != NULL) {
-				rc = 0;
+				/* Return previously assigned sock_group */
+				pthread_mutex_unlock(&map->mtx);
+				return 0;
 			}
 			break;
 		}
 	}
+
+	/* No entry with assigned sock_group, nor hint to use */
+	if (hint == NULL) {
+		pthread_mutex_unlock(&map->mtx);
+		return -EINVAL;
+	}
+
+	/* Create new entry if there is none with matching placement_id */
+	if (entry == NULL) {
+		entry = _sock_map_entry_alloc(map, placement_id);
+		if (entry == NULL) {
+			pthread_mutex_unlock(&map->mtx);
+			return -ENOMEM;
+		}
+	}
+
+	entry->group = hint;
 	pthread_mutex_unlock(&map->mtx);
 
-	return rc;
+	return 0;
 }
 
 void
@@ -205,13 +224,22 @@ spdk_sock_map_find_free(struct spdk_sock_map *map)
 }
 
 int
-spdk_sock_get_optimal_sock_group(struct spdk_sock *sock, struct spdk_sock_group **group)
+spdk_sock_get_optimal_sock_group(struct spdk_sock *sock, struct spdk_sock_group **group,
+				 struct spdk_sock_group *hint)
 {
 	struct spdk_sock_group_impl *group_impl;
+	struct spdk_sock_group_impl *hint_group_impl = NULL;
 
 	assert(group != NULL);
 
-	group_impl = sock->net_impl->group_impl_get_optimal(sock);
+	if (hint != NULL) {
+		hint_group_impl = sock_get_group_impl_from_group(sock, hint);
+		if (hint_group_impl == NULL) {
+			return -EINVAL;
+		}
+	}
+
+	group_impl = sock->net_impl->group_impl_get_optimal(sock, hint_group_impl);
 
 	if (group_impl) {
 		*group = group_impl->group;

module/sock/posix/posix.c

@@ -1083,13 +1083,13 @@ posix_sock_is_connected(struct spdk_sock *_sock)
 }
 
 static struct spdk_sock_group_impl *
-posix_sock_group_impl_get_optimal(struct spdk_sock *_sock)
+posix_sock_group_impl_get_optimal(struct spdk_sock *_sock, struct spdk_sock_group_impl *hint)
 {
 	struct spdk_posix_sock *sock = __posix_sock(_sock);
 	struct spdk_sock_group_impl *group_impl;
 
 	if (sock->placement_id != -1) {
-		spdk_sock_map_lookup(&g_map, sock->placement_id, &group_impl);
+		spdk_sock_map_lookup(&g_map, sock->placement_id, &group_impl, hint);
 		return group_impl;
 	}
 

module/sock/uring/uring.c

@@ -1283,13 +1283,13 @@ uring_sock_is_connected(struct spdk_sock *_sock)
 }
 
 static struct spdk_sock_group_impl *
-uring_sock_group_impl_get_optimal(struct spdk_sock *_sock)
+uring_sock_group_impl_get_optimal(struct spdk_sock *_sock, struct spdk_sock_group_impl *hint)
 {
 	struct spdk_uring_sock *sock = __uring_sock(_sock);
 	struct spdk_sock_group_impl *group;
 
 	if (sock->placement_id != -1) {
-		spdk_sock_map_lookup(&g_map, sock->placement_id, &group);
+		spdk_sock_map_lookup(&g_map, sock->placement_id, &group, hint);
 		return group;
 	}
 

test/unit/lib/nvme/nvme_tcp.c/nvme_tcp_ut.c

@@ -53,7 +53,7 @@ DEFINE_STUB(spdk_nvme_poll_group_remove, int, (struct spdk_nvme_poll_group *grou
 		struct spdk_nvme_qpair *qpair), 0);
 DEFINE_STUB(spdk_sock_get_optimal_sock_group,
 	    int,
-	    (struct spdk_sock *sock, struct spdk_sock_group **group),
+	    (struct spdk_sock *sock, struct spdk_sock_group **group, struct spdk_sock_group *hint),
 	    0);
 
 DEFINE_STUB(spdk_sock_group_get_ctx,

test/unit/lib/nvmf/tcp.c/tcp_ut.c

@@ -206,7 +206,7 @@ DEFINE_STUB_V(spdk_nvmf_request_free_buffers,
 
 DEFINE_STUB(spdk_sock_get_optimal_sock_group,
 	    int,
-	    (struct spdk_sock *sock, struct spdk_sock_group **group),
+	    (struct spdk_sock *sock, struct spdk_sock_group **group, struct spdk_sock_group *hint),
 	    0);
 
 DEFINE_STUB(spdk_sock_group_get_ctx,

test/unit/lib/sock/posix.c/posix_ut.c

@@ -45,7 +45,7 @@ DEFINE_STUB(spdk_sock_map_insert, int, (struct spdk_sock_map *map, int placement
 					struct spdk_sock_group_impl *group), 0);
 DEFINE_STUB_V(spdk_sock_map_release, (struct spdk_sock_map *map, int placement_id));
 DEFINE_STUB(spdk_sock_map_lookup, int, (struct spdk_sock_map *map, int placement_id,
-					struct spdk_sock_group_impl **group), 0);
+					struct spdk_sock_group_impl **group, struct spdk_sock_group_impl *hint), 0);
 DEFINE_STUB(spdk_sock_map_find_free, int, (struct spdk_sock_map *map), -1);
 DEFINE_STUB_V(spdk_sock_map_cleanup, (struct spdk_sock_map *map));
 

test/unit/lib/sock/sock.c/sock_ut.c

@@ -267,7 +267,7 @@ spdk_ut_sock_is_connected(struct spdk_sock *_sock)
 }
 
 static struct spdk_sock_group_impl *
-spdk_ut_sock_group_impl_get_optimal(struct spdk_sock *_sock)
+spdk_ut_sock_group_impl_get_optimal(struct spdk_sock *_sock, struct spdk_sock_group_impl *hint)
 {
 	return NULL;
 }
@@ -976,7 +976,7 @@ ut_sock_map(void)
 	CU_ASSERT(test_id == -1);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == -EINVAL);
 	CU_ASSERT(test_group == NULL);
 
@@ -986,7 +986,7 @@ ut_sock_map(void)
 	CU_ASSERT(rc == 0);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == 0);
 	CU_ASSERT(test_group == group_1);
 
@@ -1013,14 +1013,14 @@ ut_sock_map(void)
 	/* Release entry once and see that it still exists. */
 	spdk_sock_map_release(&map, 1);
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == 0);
 	CU_ASSERT(test_group == group_1);
 
 	/* Release entry second and final time. */
 	spdk_sock_map_release(&map, 1);
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == -EINVAL);
 	CU_ASSERT(test_group == NULL);
 
@@ -1032,7 +1032,7 @@ ut_sock_map(void)
 	CU_ASSERT(rc == 0);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == 0);
 	CU_ASSERT(test_group == group_1);
 
@@ -1040,7 +1040,7 @@ ut_sock_map(void)
 	CU_ASSERT(rc == 0);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 2, &test_group);
+	rc = spdk_sock_map_lookup(&map, 2, &test_group, NULL);
 	CU_ASSERT(rc == 0);
 	CU_ASSERT(test_group == group_2);
 
@@ -1052,7 +1052,7 @@ ut_sock_map(void)
 	CU_ASSERT(rc == 0);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == 0);
 	CU_ASSERT(test_group == group_1);
 
@@ -1060,7 +1060,7 @@ ut_sock_map(void)
 	CU_ASSERT(rc == -EINVAL);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == 0);
 	CU_ASSERT(test_group == group_1);
 
@@ -1072,7 +1072,7 @@ ut_sock_map(void)
 	CU_ASSERT(rc == 0);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, 1, &test_group);
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
 	CU_ASSERT(rc == -EINVAL);
 	CU_ASSERT(test_group == NULL);
 
@@ -1083,12 +1083,35 @@ ut_sock_map(void)
 	CU_ASSERT(rc == 0);
 
 	test_group = NULL;
-	rc = spdk_sock_map_lookup(&map, test_id, &test_group);
+	rc = spdk_sock_map_lookup(&map, test_id, &test_group, NULL);
 	CU_ASSERT(rc == 0);
 	CU_ASSERT(test_group == group_1);
 
 	spdk_sock_map_cleanup(&map);
 
+	/* Test 6
+	 * Use hint sock_group for for placement_id */
+	test_group = NULL;
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, group_1);
+	CU_ASSERT(rc == 0);
+	CU_ASSERT(test_group == NULL);
+
+	test_group = NULL;
+	rc = spdk_sock_map_lookup(&map, 1, &test_group, NULL);
+	CU_ASSERT(rc == 0);
+	CU_ASSERT(test_group == group_1);
+
+	test_id = spdk_sock_map_find_free(&map);
+	CU_ASSERT(test_id == -1);
+
+	rc = spdk_sock_map_insert(&map, 1, group_2);
+	CU_ASSERT(rc == -EINVAL);
+
+	rc = spdk_sock_map_insert(&map, 1, group_1);
+	CU_ASSERT(rc == 0);
+
+	spdk_sock_map_cleanup(&map);
+
 	spdk_ut_sock_group_impl_close(group_2);
 	spdk_ut_sock_group_impl_close(group_1);
 }

test/unit/lib/sock/uring.c/uring_ut.c

@@ -45,7 +45,7 @@ DEFINE_STUB(spdk_sock_map_insert, int, (struct spdk_sock_map *map, int placement
 					struct spdk_sock_group_impl *group), 0);
 DEFINE_STUB_V(spdk_sock_map_release, (struct spdk_sock_map *map, int placement_id));
 DEFINE_STUB(spdk_sock_map_lookup, int, (struct spdk_sock_map *map, int placement_id,
-					struct spdk_sock_group_impl **group), 0);
+					struct spdk_sock_group_impl **group, struct spdk_sock_group_impl *hint), 0);
 DEFINE_STUB(spdk_sock_map_find_free, int, (struct spdk_sock_map *map), -1);
 DEFINE_STUB_V(spdk_sock_map_cleanup, (struct spdk_sock_map *map));