From 7f833615536bc14dbc1f9ccf56de6cbabb21752a Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Wed, 6 Jul 2022 17:59:45 +0200 Subject: [PATCH] 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 Change-Id: I7628ae19def25cef6ffa62aa54bd34e446632579 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/13661 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Jim Harris Community-CI: Broadcom CI Community-CI: Mellanox Build Bot --- include/spdk/sock.h | 11 ++++ lib/sock/sock.c | 22 ++++++++ module/sock/posix/posix.c | 29 +++++++--- module/sock/uring/uring.c | 24 ++++++-- test/unit/lib/sock/sock.c/sock_ut.c | 86 +++++++++++++++++++++++++++++ 5 files changed, 158 insertions(+), 14 deletions(-) diff --git a/include/spdk/sock.h b/include/spdk/sock.h index 44aa0e750..e6026348e 100644 --- a/include/spdk/sock.h +++ b/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; }; /** diff --git a/lib/sock/sock.c b/lib/sock/sock.c index ac9d4d65a..9691a1d36 100644 --- a/lib/sock/sock.c +++ b/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. */ diff --git a/module/sock/posix/posix.c b/module/sock/posix/posix.c index be148e881..e0c642b54 100644 --- a/module/sock/posix/posix.c +++ b/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, - enable_zcopy_user_opts && enable_zcopy_impl_opts); + sock = posix_sock_alloc(fd, &impl_opts, enable_zcopy_user_opts && enable_zcopy_impl_opts); if (sock == NULL) { SPDK_ERRLOG("sock allocation failed\n"); close(fd); diff --git a/module/sock/uring/uring.c b/module/sock/uring/uring.c index 8fc216fb1..0eecd9a2d 100644 --- a/module/sock/uring/uring.c +++ b/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, - enable_zcopy_user_opts && enable_zcopy_impl_opts); + sock = uring_sock_alloc(fd, &impl_opts, enable_zcopy_user_opts && enable_zcopy_impl_opts); if (sock == NULL) { SPDK_ERRLOG("sock allocation failed\n"); close(fd); diff --git a/test/unit/lib/sock/sock.c/sock_ut.c b/test/unit/lib/sock/sock.c/sock_ut.c index 2a8fcfbbe..fc32b39ab 100644 --- a/test/unit/lib/sock/sock.c/sock_ut.c +++ b/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);