From 23d7ff31fc1faf5828fb00b40c4c93ff2a6aad0a Mon Sep 17 00:00:00 2001 From: Darek Stojaczyk Date: Sun, 17 Mar 2019 12:13:01 +0100 Subject: [PATCH] vhost: change vsession->lcore only within that lcore There is currently a small window after we stop session's pollers and before we mark the session as stopped (by setting vsession->lcore to -1). If spdk_vhost_dev_foreach_session() is called within this window, its callback could assume the session is still running and for example in vhost scsi target hotremove case, could destroy an io_channel for the second time - as it'd first done when the session was stopped. That's a bug. A similar case exists for session start. We fix the above by setting vsession->lcore directly after starting or stopping the session, hence eliminating the possible window for data races. This has a few implications: * spdk_vhost_session_send_event() called before session start can't operate on vsession->lcore, so it needs to be provided with the lcore as an additional parameter now. * the vsession->lcore can't be accessed until spdk_vhost_session_start_done() is called, so its existing usages were replaced with spdk_env_get_current_core() * active_session_num is decremented right after spdk_vhost_session_stop_done() is called and before spdk_vhost_session_send_event() returns, so some active_session_num == 1 checks meaning "the last session gets stopped now" needed to be changed to check against == 0, as if "the last session has been just stopped" Change-Id: I5781bb0ce247425130c9672e0df27d06b6234317 Signed-off-by: Darek Stojaczyk Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/448229 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Changpeng Liu --- lib/vhost/vhost.c | 19 ++++++++++++------- lib/vhost/vhost_blk.c | 23 +++++++---------------- lib/vhost/vhost_internal.h | 8 +++++--- lib/vhost/vhost_nvme.c | 23 +++++++---------------- lib/vhost/vhost_scsi.c | 14 +++++--------- 5 files changed, 36 insertions(+), 51 deletions(-) diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index 5b538f2ba..432ed2e0f 100644 --- a/lib/vhost/vhost.c +++ b/lib/vhost/vhost.c @@ -884,12 +884,22 @@ complete_session_event(struct spdk_vhost_session *vsession, int response) void spdk_vhost_session_start_done(struct spdk_vhost_session *vsession, int response) { + if (response == 0) { + vsession->lcore = spdk_env_get_current_core(); + assert(vsession->vdev->active_session_num < UINT32_MAX); + vsession->vdev->active_session_num++; + } complete_session_event(vsession, response); } void spdk_vhost_session_stop_done(struct spdk_vhost_session *vsession, int response) { + if (response == 0) { + vsession->lcore = -1; + assert(vsession->vdev->active_session_num > 0); + vsession->vdev->active_session_num--; + } complete_session_event(vsession, response); } @@ -968,7 +978,7 @@ out_unlock: } int -spdk_vhost_session_send_event(struct spdk_vhost_session *vsession, +spdk_vhost_session_send_event(int32_t lcore, struct spdk_vhost_session *vsession, spdk_vhost_session_fn cb_fn, unsigned timeout_sec, const char *errmsg) { @@ -988,7 +998,7 @@ spdk_vhost_session_send_event(struct spdk_vhost_session *vsession, ev_ctx.cb_fn = cb_fn; vsession->event_ctx = &ev_ctx; - ev = spdk_event_allocate(vsession->lcore, spdk_vhost_event_cb, &ev_ctx, NULL); + ev = spdk_event_allocate(lcore, spdk_vhost_event_cb, &ev_ctx, NULL); assert(ev); spdk_event_call(ev); pthread_mutex_unlock(&g_spdk_vhost_mutex); @@ -1060,9 +1070,6 @@ _stop_session(struct spdk_vhost_session *vsession) spdk_vhost_session_mem_unregister(vsession); free(vsession->mem); - vsession->lcore = -1; - assert(vdev->active_session_num > 0); - vdev->active_session_num--; } static void @@ -1183,8 +1190,6 @@ start_device(int vid) goto out; } - assert(vdev->active_session_num < UINT32_MAX); - vdev->active_session_num++; out: pthread_mutex_unlock(&g_spdk_vhost_mutex); return rc; diff --git a/lib/vhost/vhost_blk.c b/lib/vhost/vhost_blk.c index 89c830159..dec3fc854 100644 --- a/lib/vhost/vhost_blk.c +++ b/lib/vhost/vhost_blk.c @@ -720,7 +720,7 @@ spdk_vhost_blk_start_cb(struct spdk_vhost_dev *vdev, bvsession->requestq_poller = spdk_poller_register(bvdev->bdev ? vdev_worker : no_bdev_vdev_worker, bvsession, 0); SPDK_INFOLOG(SPDK_LOG_VHOST, "Started poller for vhost controller %s on lcore %d\n", - vdev->name, vsession->lcore); + vdev->name, spdk_env_get_current_core()); out: spdk_vhost_session_start_done(vsession, rc); return rc; @@ -729,15 +729,15 @@ out: static int spdk_vhost_blk_start(struct spdk_vhost_session *vsession) { + int32_t lcore; int rc; - vsession->lcore = spdk_vhost_allocate_reactor(vsession->vdev->cpumask); - rc = spdk_vhost_session_send_event(vsession, spdk_vhost_blk_start_cb, + lcore = spdk_vhost_allocate_reactor(vsession->vdev->cpumask); + rc = spdk_vhost_session_send_event(lcore, vsession, spdk_vhost_blk_start_cb, 3, "start session"); if (rc != 0) { - spdk_vhost_free_reactor(vsession->lcore); - vsession->lcore = -1; + spdk_vhost_free_reactor(lcore); } return rc; @@ -803,17 +803,8 @@ err: static int spdk_vhost_blk_stop(struct spdk_vhost_session *vsession) { - int rc; - - rc = spdk_vhost_session_send_event(vsession, spdk_vhost_blk_stop_cb, - 3, "stop session"); - if (rc != 0) { - return rc; - } - - spdk_vhost_free_reactor(vsession->lcore); - vsession->lcore = -1; - return 0; + return spdk_vhost_session_send_event(vsession->lcore, vsession, + spdk_vhost_blk_stop_cb, 3, "stop session"); } static void diff --git a/lib/vhost/vhost_internal.h b/lib/vhost/vhost_internal.h index 46d5f3f65..1ba27ab09 100644 --- a/lib/vhost/vhost_internal.h +++ b/lib/vhost/vhost_internal.h @@ -318,7 +318,7 @@ void spdk_vhost_dev_foreach_session(struct spdk_vhost_dev *dev, spdk_vhost_session_fn fn, void *arg); /** - * Call a function on the provided session's lcore and block until either + * Call a function on the provided lcore and block until either * spdk_vhost_session_start_done() or spdk_vhost_session_stop_done() * is called. * @@ -326,6 +326,7 @@ void spdk_vhost_dev_foreach_session(struct spdk_vhost_dev *dev, * will unlock for the time it's waiting. It's meant to be called only * from start/stop session callbacks. * + * \param lcore target session's lcore * \param vsession vhost session * \param cb_fn the function to call. The void *arg parameter in cb_fn * is always NULL. @@ -334,7 +335,7 @@ void spdk_vhost_dev_foreach_session(struct spdk_vhost_dev *dev, * \param errmsg error message to print once the timeout expires * \return return the code passed to spdk_vhost_session_event_done(). */ -int spdk_vhost_session_send_event(struct spdk_vhost_session *vsession, +int spdk_vhost_session_send_event(int32_t lcore, struct spdk_vhost_session *vsession, spdk_vhost_session_fn cb_fn, unsigned timeout_sec, const char *errmsg); @@ -355,7 +356,8 @@ void spdk_vhost_session_start_done(struct spdk_vhost_session *vsession, int resp * Finish a blocking spdk_vhost_session_send_event() call and finally * stop the session. This must be called on the session's lcore which * used to receive all session-related messages (e.g. from - * spdk_vhost_dev_foreach_session()). + * spdk_vhost_dev_foreach_session()). After this call, the session- + * related messages will be once again processed by any arbitrary thread. * * Must be called under the global vhost lock. * diff --git a/lib/vhost/vhost_nvme.c b/lib/vhost/vhost_nvme.c index fa519de5d..597c04479 100644 --- a/lib/vhost/vhost_nvme.c +++ b/lib/vhost/vhost_nvme.c @@ -1092,7 +1092,7 @@ spdk_vhost_nvme_start_cb(struct spdk_vhost_dev *vdev, } SPDK_NOTICELOG("Start Device %u, Path %s, lcore %d\n", vsession->vid, - vdev->path, vsession->lcore); + vdev->path, spdk_env_get_current_core()); for (i = 0; i < nvme->num_ns; i++) { ns_dev = &nvme->ns[i]; @@ -1113,6 +1113,7 @@ spdk_vhost_nvme_start_cb(struct spdk_vhost_dev *vdev, static int spdk_vhost_nvme_start(struct spdk_vhost_session *vsession) { + int32_t lcore; int rc; if (vsession->vdev->active_session_num > 0) { @@ -1121,13 +1122,12 @@ spdk_vhost_nvme_start(struct spdk_vhost_session *vsession) return -1; } - vsession->lcore = spdk_vhost_allocate_reactor(vsession->vdev->cpumask); - rc = spdk_vhost_session_send_event(vsession, spdk_vhost_nvme_start_cb, + lcore = spdk_vhost_allocate_reactor(vsession->vdev->cpumask); + rc = spdk_vhost_session_send_event(lcore, vsession, spdk_vhost_nvme_start_cb, 3, "start session"); if (rc != 0) { - spdk_vhost_free_reactor(vsession->lcore); - vsession->lcore = -1; + spdk_vhost_free_reactor(lcore); } return rc; @@ -1215,17 +1215,8 @@ spdk_vhost_nvme_stop_cb(struct spdk_vhost_dev *vdev, static int spdk_vhost_nvme_stop(struct spdk_vhost_session *vsession) { - int rc; - - rc = spdk_vhost_session_send_event(vsession, spdk_vhost_nvme_stop_cb, - 3, "start session"); - if (rc != 0) { - return rc; - } - - spdk_vhost_free_reactor(vsession->lcore); - vsession->lcore = -1; - return 0; + return spdk_vhost_session_send_event(vsession->lcore, vsession, + spdk_vhost_nvme_stop_cb, 3, "start session"); } static void diff --git a/lib/vhost/vhost_scsi.c b/lib/vhost/vhost_scsi.c index 515dbce05..8557c34b1 100644 --- a/lib/vhost/vhost_scsi.c +++ b/lib/vhost/vhost_scsi.c @@ -1305,7 +1305,7 @@ spdk_vhost_scsi_start_cb(struct spdk_vhost_dev *vdev, } } SPDK_INFOLOG(SPDK_LOG_VHOST, "Started poller for vhost controller %s on lcore %d\n", - vdev->name, vsession->lcore); + vdev->name, spdk_env_get_current_core()); svsession->requestq_poller = spdk_poller_register(vdev_worker, svsession, 0); if (vsession->virtqueue[VIRTIO_SCSI_CONTROLQ].vring.desc && @@ -1339,12 +1339,9 @@ spdk_vhost_scsi_start(struct spdk_vhost_session *vsession) svdev->lcore = spdk_vhost_allocate_reactor(svdev->vdev.cpumask); } - vsession->lcore = svdev->lcore; - rc = spdk_vhost_session_send_event(vsession, spdk_vhost_scsi_start_cb, + rc = spdk_vhost_session_send_event(svdev->lcore, vsession, spdk_vhost_scsi_start_cb, 3, "start session"); if (rc != 0) { - vsession->lcore = -1; - if (svdev->vdev.active_session_num == 0) { spdk_vhost_free_reactor(svdev->lcore); svdev->lcore = -1; @@ -1420,14 +1417,13 @@ spdk_vhost_scsi_stop(struct spdk_vhost_session *vsession) SPDK_ERRLOG("Trying to stop non-scsi session as a scsi one.\n"); return -1; } - rc = spdk_vhost_session_send_event(vsession, spdk_vhost_scsi_stop_cb, - 3, "stop session"); + rc = spdk_vhost_session_send_event(vsession->lcore, vsession, + spdk_vhost_scsi_stop_cb, 3, "stop session"); if (rc != 0) { return rc; } - vsession->lcore = -1; - if (vsession->vdev->active_session_num == 1) { + if (vsession->vdev->active_session_num == 0) { spdk_vhost_free_reactor(svsession->svdev->lcore); svsession->svdev->lcore = -1; }