sock: add sock_impl_opts to sock_opts

Some of the options in sock_impl_opts could be different for different
sockets (even if they're using the same impl).  However, outside of a
few selected options (recv_buf_size, send_buf_size), there was no
interface to change them.

This change will allow users to change impl_opts on a per-socket basis
when creating a socket.  Sockets created through accept() inherit
impl_opts from the listening socket.

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
Change-Id: I7628ae19def25cef6ffa62aa54bd34e446632579
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/13661
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot

include/spdk/sock.h

@@ -169,6 +169,17 @@ struct spdk_sock_opts {
 	 */
 	bool ktls;
 
+	/**
+	 * Socket implementation options.  If non-NULL, these will override those set by
+	 * spdk_sock_impl_set_opts().  The library copies this structure internally, so the user can
+	 * free it immediately after a spdk_sock_connect()/spdk_sock_listen() call.
+	 */
+	struct spdk_sock_impl_opts *impl_opts;
+
+	/**
+	 * Size of the impl_opts structure.
+	 */
+	size_t impl_opts_size;
 };
 
 /**

lib/sock/sock.c

@@ -255,6 +255,14 @@ spdk_sock_get_default_opts(struct spdk_sock_opts *opts)
 	if (SPDK_SOCK_OPTS_FIELD_OK(opts, ktls)) {
 		opts->ktls = SPDK_SOCK_DEFAULT_KTLS;
 	}
+
+	if (SPDK_SOCK_OPTS_FIELD_OK(opts, impl_opts)) {
+		opts->impl_opts = NULL;
+	}
+
+	if (SPDK_SOCK_OPTS_FIELD_OK(opts, impl_opts_size)) {
+		opts->impl_opts_size = 0;
+	}
 }
 
 /*
@@ -291,6 +299,14 @@ sock_init_opts(struct spdk_sock_opts *opts, struct spdk_sock_opts *opts_user)
 	if (SPDK_SOCK_OPTS_FIELD_OK(opts, ktls)) {
 		opts->ktls = opts_user->ktls;
 	}
+
+	if (SPDK_SOCK_OPTS_FIELD_OK(opts, impl_opts)) {
+		opts->impl_opts = opts_user->impl_opts;
+	}
+
+	if (SPDK_SOCK_OPTS_FIELD_OK(opts, impl_opts)) {
+		opts->impl_opts_size = opts_user->impl_opts_size;
+	}
 }
 
 struct spdk_sock *
@@ -333,6 +349,9 @@ spdk_sock_connect_ext(const char *ip, int port, char *_impl_name, struct spdk_so
 		if (sock != NULL) {
 			/* Copy the contents, both the two structures are the same ABI version */
 			memcpy(&sock->opts, &opts_local, sizeof(sock->opts));
+			/* Clear out impl_opts to make sure we don't keep reference to a dangling
+			 * pointer */
+			sock->opts.impl_opts = NULL;
 			sock->net_impl = impl;
 			TAILQ_INIT(&sock->queued_reqs);
 			TAILQ_INIT(&sock->pending_reqs);
@@ -384,6 +403,9 @@ spdk_sock_listen_ext(const char *ip, int port, char *_impl_name, struct spdk_soc
 		if (sock != NULL) {
 			/* Copy the contents, both the two structures are the same ABI version */
 			memcpy(&sock->opts, &opts_local, sizeof(sock->opts));
+			/* Clear out impl_opts to make sure we don't keep reference to a dangling
+			 * pointer */
+			sock->opts.impl_opts = NULL;
 			sock->net_impl = impl;
 			/* Don't need to initialize the request queues for listen
 			 * sockets. */

module/sock/posix/posix.c

@@ -147,6 +147,17 @@ posix_sock_impl_set_opts(const struct spdk_sock_impl_opts *opts, size_t len)
 	return 0;
 }
 
+static void
+posix_opts_get_impl_opts(const struct spdk_sock_opts *opts, struct spdk_sock_impl_opts *dest)
+{
+	/* Copy the default impl_opts first to cover cases when user's impl_opts is smaller */
+	memcpy(dest, &g_spdk_posix_sock_impl_opts, sizeof(*dest));
+
+	if (opts->impl_opts != NULL) {
+		posix_sock_copy_impl_opts(dest, opts->impl_opts, opts->impl_opts_size);
+	}
+}
+
 static int
 posix_sock_getaddr(struct spdk_sock *_sock, char *saddr, int slen, uint16_t *sport,
 		   char *caddr, int clen, uint16_t *cport)
@@ -401,7 +412,8 @@ posix_sock_alloc(int fd, struct spdk_sock_impl_opts *impl_opts, bool enable_zero
 }
 
 static int
-posix_fd_create(struct addrinfo *res, struct spdk_sock_opts *opts)
+posix_fd_create(struct addrinfo *res, struct spdk_sock_opts *opts,
+		struct spdk_sock_impl_opts *impl_opts)
 {
 	int fd;
 	int val = 1;
@@ -416,13 +428,13 @@ posix_fd_create(struct addrinfo *res, struct spdk_sock_opts *opts)
 		return -1;
 	}
 
-	sz = g_spdk_posix_sock_impl_opts.recv_buf_size;
+	sz = impl_opts->recv_buf_size;
 	rc = setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &sz, sizeof(sz));
 	if (rc) {
 		/* Not fatal */
 	}
 
-	sz = g_spdk_posix_sock_impl_opts.send_buf_size;
+	sz = impl_opts->send_buf_size;
 	rc = setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &sz, sizeof(sz));
 	if (rc) {
 		/* Not fatal */
@@ -806,6 +818,7 @@ posix_sock_create(const char *ip, int port,
 		  bool enable_ssl)
 {
 	struct spdk_posix_sock *sock;
+	struct spdk_sock_impl_opts impl_opts;
 	char buf[MAX_TMPBUF];
 	char portnum[PORTNUMLEN];
 	char *p;
@@ -818,6 +831,7 @@ posix_sock_create(const char *ip, int port,
 	SSL *ssl = 0;
 
 	assert(opts != NULL);
+	posix_opts_get_impl_opts(opts, &impl_opts);
 
 	if (ip == NULL) {
 		return NULL;
@@ -848,7 +862,7 @@ posix_sock_create(const char *ip, int port,
 	fd = -1;
 	for (res = res0; res != NULL; res = res->ai_next) {
 retry:
-		fd = posix_fd_create(res, opts);
+		fd = posix_fd_create(res, opts, &impl_opts);
 		if (fd < 0) {
 			continue;
 		}
@@ -891,7 +905,7 @@ retry:
 				fd = -1;
 				break;
 			}
-			enable_zcopy_impl_opts = g_spdk_posix_sock_impl_opts.enable_zerocopy_send_server;
+			enable_zcopy_impl_opts = impl_opts.enable_zerocopy_send_server;
 		} else if (type == SPDK_SOCK_CREATE_CONNECT) {
 			rc = connect(fd, res->ai_addr, res->ai_addrlen);
 			if (rc != 0) {
@@ -901,7 +915,7 @@ retry:
 				fd = -1;
 				continue;
 			}
-			enable_zcopy_impl_opts = g_spdk_posix_sock_impl_opts.enable_zerocopy_send_client;
+			enable_zcopy_impl_opts = impl_opts.enable_zerocopy_send_client;
 			if (enable_ssl) {
 				ctx = posix_sock_create_ssl_context(TLS_client_method(), opts);
 				if (!ctx) {
@@ -939,8 +953,7 @@ retry:
 	/* Only enable zero copy for non-loopback and non-ssl sockets. */
 	enable_zcopy_user_opts = opts->zcopy && !sock_is_loopback(fd) && !enable_ssl;
 
-	sock = posix_sock_alloc(fd, &g_spdk_posix_sock_impl_opts,
+	sock = posix_sock_alloc(fd, &impl_opts, enable_zcopy_user_opts && enable_zcopy_impl_opts);
-				enable_zcopy_user_opts && enable_zcopy_impl_opts);
 	if (sock == NULL) {
 		SPDK_ERRLOG("sock allocation failed\n");
 		close(fd);

module/sock/uring/uring.c

@@ -167,6 +167,17 @@ uring_sock_impl_set_opts(const struct spdk_sock_impl_opts *opts, size_t len)
 	return 0;
 }
 
+static void
+uring_opts_get_impl_opts(const struct spdk_sock_opts *opts, struct spdk_sock_impl_opts *dest)
+{
+	/* Copy the default impl_opts first to cover cases when user's impl_opts is smaller */
+	memcpy(dest, &g_spdk_uring_sock_impl_opts, sizeof(*dest));
+
+	if (opts->impl_opts != NULL) {
+		uring_sock_copy_impl_opts(dest, opts->impl_opts, opts->impl_opts_size);
+	}
+}
+
 static int
 uring_sock_getaddr(struct spdk_sock *_sock, char *saddr, int slen, uint16_t *sport,
 		   char *caddr, int clen, uint16_t *cport)
@@ -415,6 +426,7 @@ uring_sock_create(const char *ip, int port,
 		  struct spdk_sock_opts *opts)
 {
 	struct spdk_uring_sock *sock;
+	struct spdk_sock_impl_opts impl_opts;
 	char buf[MAX_TMPBUF];
 	char portnum[PORTNUMLEN];
 	char *p;
@@ -426,6 +438,7 @@ uring_sock_create(const char *ip, int port,
 	bool enable_zcopy_user_opts = true;
 
 	assert(opts != NULL);
+	uring_opts_get_impl_opts(opts, &impl_opts);
 
 	if (ip == NULL) {
 		return NULL;
@@ -462,13 +475,13 @@ retry:
 			continue;
 		}
 
-		val = g_spdk_uring_sock_impl_opts.recv_buf_size;
+		val = impl_opts.recv_buf_size;
 		rc = setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &val, sizeof val);
 		if (rc) {
 			/* Not fatal */
 		}
 
-		val = g_spdk_uring_sock_impl_opts.send_buf_size;
+		val = impl_opts.send_buf_size;
 		rc = setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &val, sizeof val);
 		if (rc) {
 			/* Not fatal */
@@ -566,7 +579,7 @@ retry:
 				break;
 			}
 
-			enable_zcopy_impl_opts = g_spdk_uring_sock_impl_opts.enable_zerocopy_send_server;
+			enable_zcopy_impl_opts = impl_opts.enable_zerocopy_send_server;
 		} else if (type == SPDK_SOCK_CREATE_CONNECT) {
 			rc = connect(fd, res->ai_addr, res->ai_addrlen);
 			if (rc != 0) {
@@ -585,7 +598,7 @@ retry:
 				break;
 			}
 
-			enable_zcopy_impl_opts = g_spdk_uring_sock_impl_opts.enable_zerocopy_send_client;
+			enable_zcopy_impl_opts = impl_opts.enable_zerocopy_send_client;
 		}
 		break;
 	}
@@ -596,8 +609,7 @@ retry:
 	}
 
 	enable_zcopy_user_opts = opts->zcopy && !sock_is_loopback(fd);
-	sock = uring_sock_alloc(fd, &g_spdk_uring_sock_impl_opts,
+	sock = uring_sock_alloc(fd, &impl_opts, enable_zcopy_user_opts && enable_zcopy_impl_opts);
-				enable_zcopy_user_opts && enable_zcopy_impl_opts);
 	if (sock == NULL) {
 		SPDK_ERRLOG("sock allocation failed\n");
 		close(fd);

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

@@ -1146,6 +1146,91 @@ ut_sock_map(void)
 	spdk_ut_sock_group_impl_close(group_1);
 }
 
+static void
+override_impl_opts(void)
+{
+	struct spdk_sock *lsock, *csock, *asock;
+	struct spdk_sock_opts opts;
+	struct spdk_sock_impl_opts impl_opts;
+	uint32_t send_buf_size;
+	size_t opts_size;
+	int rc;
+
+	opts_size = sizeof(impl_opts);
+	rc = spdk_sock_impl_get_opts("posix", &impl_opts, &opts_size);
+	CU_ASSERT_EQUAL(rc, 0);
+	opts.opts_size = sizeof(opts);
+	spdk_sock_get_default_opts(&opts);
+	opts.impl_opts = &impl_opts;
+	opts.impl_opts_size = sizeof(impl_opts);
+
+	/* Use send_buf_size to verify that impl_opts get overriden */
+	send_buf_size = impl_opts.send_buf_size;
+	impl_opts.send_buf_size = send_buf_size + 1;
+
+	lsock = spdk_sock_listen_ext("127.0.0.1", UT_PORT, "posix", &opts);
+	SPDK_CU_ASSERT_FATAL(lsock != NULL);
+	CU_ASSERT_EQUAL(lsock->impl_opts.send_buf_size, send_buf_size + 1);
+
+	/* Check the same for connect() */
+	opts_size = sizeof(impl_opts);
+	rc = spdk_sock_impl_get_opts("posix", &impl_opts, &opts_size);
+	CU_ASSERT_EQUAL(rc, 0);
+	opts.opts_size = sizeof(opts);
+	spdk_sock_get_default_opts(&opts);
+	opts.impl_opts = &impl_opts;
+	opts.impl_opts_size = sizeof(impl_opts);
+
+	impl_opts.send_buf_size = send_buf_size + 2;
+
+	csock = spdk_sock_connect_ext("127.0.0.1", UT_PORT, "posix", &opts);
+	SPDK_CU_ASSERT_FATAL(csock != NULL);
+	CU_ASSERT_EQUAL(csock->impl_opts.send_buf_size, send_buf_size + 2);
+
+	/* Check that accept() inherits impl_opts from listen socket */
+	asock = spdk_sock_accept(lsock);
+	SPDK_CU_ASSERT_FATAL(asock != NULL);
+	CU_ASSERT_EQUAL(asock->impl_opts.send_buf_size, send_buf_size + 1);
+
+	spdk_sock_close(&asock);
+	spdk_sock_close(&csock);
+	spdk_sock_close(&lsock);
+
+	/* Check that impl_opts_size is verified by setting it to the offset of send_buf_size  */
+	opts_size = sizeof(impl_opts);
+	rc = spdk_sock_impl_get_opts("posix", &impl_opts, &opts_size);
+	CU_ASSERT_EQUAL(rc, 0);
+	opts.opts_size = sizeof(opts);
+	spdk_sock_get_default_opts(&opts);
+	opts.impl_opts = &impl_opts;
+	opts.impl_opts_size = offsetof(struct spdk_sock_impl_opts, send_buf_size);
+
+	send_buf_size = impl_opts.send_buf_size;
+	impl_opts.send_buf_size = send_buf_size + 1;
+
+	lsock = spdk_sock_listen_ext("127.0.0.1", UT_PORT, "posix", &opts);
+	SPDK_CU_ASSERT_FATAL(lsock != NULL);
+	CU_ASSERT_EQUAL(lsock->impl_opts.send_buf_size, send_buf_size);
+
+	/* Check the same for connect() */
+	opts_size = sizeof(impl_opts);
+	rc = spdk_sock_impl_get_opts("posix", &impl_opts, &opts_size);
+	CU_ASSERT_EQUAL(rc, 0);
+	opts.opts_size = sizeof(opts);
+	spdk_sock_get_default_opts(&opts);
+	opts.impl_opts = &impl_opts;
+	opts.impl_opts_size = offsetof(struct spdk_sock_impl_opts, send_buf_size);
+
+	impl_opts.send_buf_size = send_buf_size + 2;
+
+	csock = spdk_sock_connect_ext("127.0.0.1", UT_PORT, "posix", &opts);
+	SPDK_CU_ASSERT_FATAL(csock != NULL);
+	CU_ASSERT_EQUAL(csock->impl_opts.send_buf_size, send_buf_size);
+
+	spdk_sock_close(&lsock);
+	spdk_sock_close(&csock);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1167,6 +1252,7 @@ main(int argc, char **argv)
 	CU_ADD_TEST(suite, ut_sock_impl_get_set_opts);
 	CU_ADD_TEST(suite, posix_sock_impl_get_set_opts);
 	CU_ADD_TEST(suite, ut_sock_map);
+	CU_ADD_TEST(suite, override_impl_opts);
 
 	CU_basic_set_mode(CU_BRM_VERBOSE);