From 03aa8995e9b9440a192f36435fc0aaae7a17f98e Mon Sep 17 00:00:00 2001 From: Ziye Yang Date: Tue, 1 Sep 2020 00:47:15 +0800 Subject: [PATCH] lib/sock: Fix the coredump issue in sock_map_realese When tested on Linux 5.8 kernel and configure spdk with debug mode (--enable-debug), and test SPDK NVMe-oF tcp transport, and we see the coredump in sock_map_release with the following statements: assert(entry->ref > 0); After debug, I can confirm that the placement_id value got from the following function (sock->net_impl->get_placement_id) changes. It means that: When the sock is added into the poll group (spdk_sock_group_add_sock), we get the placement_id (named as Value(begin)); and when the sock is removed from the poll group (spdk_sock_group_remove_sock), we get the plaemednt_id on the same sock (named as Vaule(end)). I found that Value(begin) ! = Value(end). So our solution is for a socket, we will get placement_id once, then we can solve this issue. Signed-off-by: Ziye Yang Change-Id: Ia1d0cf39247b53410260561aca5af38130cc0abb Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/3983 Community-CI: Mellanox Build Bot Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins Reviewed-by: Tomasz Zawadzki Reviewed-by: Ben Walker Reviewed-by: Aleksey Marchuk --- include/spdk_internal/sock.h | 1 + lib/sock/sock.c | 30 +++++++++++++++++++++++------- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/include/spdk_internal/sock.h b/include/spdk_internal/sock.h index d88d6bd03..7b17a781d 100644 --- a/include/spdk_internal/sock.h +++ b/include/spdk_internal/sock.h @@ -63,6 +63,7 @@ struct spdk_sock { TAILQ_HEAD(, spdk_sock_request) queued_reqs; TAILQ_HEAD(, spdk_sock_request) pending_reqs; int queued_iovcnt; + int placement_id; struct { uint8_t closed : 1; diff --git a/lib/sock/sock.c b/lib/sock/sock.c index 5ea90385c..6c407c71e 100644 --- a/lib/sock/sock.c +++ b/lib/sock/sock.c @@ -146,13 +146,29 @@ sock_remove_sock_group_from_map_table(struct spdk_sock_group *group) } +static int +sock_get_placement_id(struct spdk_sock *sock) +{ + int rc; + int placement_id; + + if (!sock->placement_id) { + rc = sock->net_impl->get_placement_id(sock, &placement_id); + if (!rc && (placement_id != 0)) { + sock->placement_id = placement_id; + } + } + + return sock->placement_id; +} + int spdk_sock_get_optimal_sock_group(struct spdk_sock *sock, struct spdk_sock_group **group) { - int placement_id = 0, rc; + int placement_id; - rc = sock->net_impl->get_placement_id(sock, &placement_id); - if (!rc && (placement_id != 0)) { + placement_id = sock_get_placement_id(sock); + if (placement_id != 0) { sock_map_lookup(placement_id, group); return 0; } else { @@ -508,8 +524,8 @@ spdk_sock_group_add_sock(struct spdk_sock_group *group, struct spdk_sock *sock, return -1; } - rc = sock->net_impl->get_placement_id(sock, &placement_id); - if (!rc && (placement_id != 0)) { + placement_id = sock_get_placement_id(sock); + if (placement_id != 0) { rc = sock_map_insert(placement_id, group); if (rc < 0) { return -1; @@ -557,8 +573,8 @@ spdk_sock_group_remove_sock(struct spdk_sock_group *group, struct spdk_sock *soc assert(group_impl == sock->group_impl); - rc = sock->net_impl->get_placement_id(sock, &placement_id); - if (!rc && (placement_id != 0)) { + placement_id = sock_get_placement_id(sock); + if (placement_id != 0) { sock_map_release(placement_id); }