From 376d893a20debb524002f0646a5b93d9ab666938 Mon Sep 17 00:00:00 2001 From: Darek Stojaczyk Date: Mon, 29 Apr 2019 09:26:10 +0200 Subject: [PATCH] vhost: introduce vsession->started We used to rely on lcore >= 0 for sessions that are started (have their pollers running) and in order to prevent data races, that lcore field had to be set from the same thread that runs the pollers, directly after registering/unregistering them. The lcore was always set to spdk_env_get_current_core(), but we won't be able to use an equivalent get_current_poll_group() function after we switch to poll groups. We will have a poll group object only inside spdk_vhost_session_send_event() that's called from the DPDK rte_vhost thread. In order to change the lcore field (or a poll group one) from spdk_vhost_session_send_event(), we'll need a separate field to maintain the started/stopped status that's only going to be modified from the session's thread. Change-Id: Idb09cae3c4715eebb20282aad203987b26be707b Signed-off-by: Darek Stojaczyk Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452394 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Jim Harris Reviewed-by: Shuhei Matsumoto --- lib/vhost/rte_vhost_compat.c | 8 ++++---- lib/vhost/vhost.c | 13 ++++++++----- lib/vhost/vhost_internal.h | 1 + lib/vhost/vhost_scsi.c | 4 ++-- test/unit/lib/vhost/vhost.c/vhost_ut.c | 1 + 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/lib/vhost/rte_vhost_compat.c b/lib/vhost/rte_vhost_compat.c index df71775b7..abebf68f8 100644 --- a/lib/vhost/rte_vhost_compat.c +++ b/lib/vhost/rte_vhost_compat.c @@ -66,7 +66,7 @@ spdk_extern_vhost_pre_msg_handler(int vid, void *_msg) switch (msg->request) { case VHOST_USER_GET_VRING_BASE: - if (vsession->forced_polling && vsession->lcore != -1) { + if (vsession->forced_polling && vsession->started) { /* Our queue is stopped for whatever reason, but we may still * need to poll it after it's initialized again. */ @@ -77,7 +77,7 @@ spdk_extern_vhost_pre_msg_handler(int vid, void *_msg) case VHOST_USER_SET_VRING_ADDR: case VHOST_USER_SET_VRING_NUM: case VHOST_USER_SET_VRING_KICK: - if (vsession->forced_polling && vsession->lcore != -1) { + if (vsession->forced_polling && vsession->started) { /* Additional queues are being initialized, so we either processed * enough I/Os and are switching from SeaBIOS to the OS now, or * we were never in SeaBIOS in the first place. Either way, we @@ -105,7 +105,7 @@ spdk_extern_vhost_pre_msg_handler(int vid, void *_msg) * We will start the device again from the post-processing * message handler. */ - if (vsession->lcore != -1) { + if (vsession->started) { g_spdk_vhost_ops.destroy_device(vid); vsession->needs_restart = true; } @@ -187,7 +187,7 @@ spdk_extern_vhost_post_msg_handler(int vid, void *_msg) /* vhost-user spec tells us to start polling a queue after receiving * its SET_VRING_KICK message. Let's do it! */ - if (vsession->forced_polling && vsession->lcore == -1) { + if (vsession->forced_polling && !vsession->started) { g_spdk_vhost_ops.new_device(vid); } break; diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index ec8d2659d..5a71308a4 100644 --- a/lib/vhost/vhost.c +++ b/lib/vhost/vhost.c @@ -899,6 +899,7 @@ spdk_vhost_session_start_done(struct spdk_vhost_session *vsession, int response) { if (response == 0) { vsession->lcore = spdk_env_get_current_core(); + vsession->started = true; assert(vsession->vdev->active_session_num < UINT32_MAX); vsession->vdev->active_session_num++; } @@ -910,6 +911,7 @@ spdk_vhost_session_stop_done(struct spdk_vhost_session *vsession, int response) { if (response == 0) { vsession->lcore = -1; + vsession->started = false; assert(vsession->vdev->active_session_num > 0); vsession->vdev->active_session_num--; } @@ -963,7 +965,7 @@ spdk_vhost_event_async_foreach_fn(void *arg1, void *arg2) goto out_unlock_continue; } - if (vsession->lcore >= 0 && + if (vsession->started && (uint32_t)vsession->lcore != spdk_env_get_current_core()) { /* if session has been relocated to other core, it is no longer thread-safe * to access its contents here. Even though we're running under the global @@ -1099,7 +1101,7 @@ stop_device(int vid) return; } - if (vsession->lcore == -1) { + if (!vsession->started) { /* already stopped, nothing to do */ pthread_mutex_unlock(&g_spdk_vhost_mutex); return; @@ -1126,7 +1128,7 @@ start_device(int vid) } vdev = vsession->vdev; - if (vsession->lcore != -1) { + if (vsession->started) { /* already started, nothing to do */ rc = 0; goto out; @@ -1345,6 +1347,7 @@ new_connection(int vid) vsession->id = vdev->vsessions_num++; vsession->vid = vid; vsession->lcore = -1; + vsession->started = false; vsession->initialized = false; vsession->next_stats_check_time = 0; vsession->stats_check_interval = SPDK_VHOST_STATS_CHECK_INTERVAL_MS * @@ -1369,7 +1372,7 @@ destroy_connection(int vid) return; } - if (vsession->lcore != -1) { + if (vsession->started) { _stop_session(vsession); } @@ -1389,7 +1392,7 @@ spdk_vhost_external_event_foreach_continue(struct spdk_vhost_dev *vdev, goto out_finish_foreach; } - while (vsession->lcore == -1) { + while (!vsession->started) { if (vsession->initialized) { rc = fn(vdev, vsession, arg); if (rc < 0) { diff --git a/lib/vhost/vhost_internal.h b/lib/vhost/vhost_internal.h index 242e51b03..0e926d951 100644 --- a/lib/vhost/vhost_internal.h +++ b/lib/vhost/vhost_internal.h @@ -127,6 +127,7 @@ struct spdk_vhost_session { int32_t lcore; bool initialized; + bool started; bool needs_restart; bool forced_polling; diff --git a/lib/vhost/vhost_scsi.c b/lib/vhost/vhost_scsi.c index 99498184c..7c5b43ad2 100644 --- a/lib/vhost/vhost_scsi.c +++ b/lib/vhost/vhost_scsi.c @@ -947,7 +947,7 @@ spdk_vhost_scsi_session_add_tgt(struct spdk_vhost_dev *vdev, svsession = (struct spdk_vhost_scsi_session *)vsession; session_sdev = &svsession->scsi_dev_state[scsi_tgt_num]; - if (vsession->lcore == -1 || session_sdev->dev != NULL) { + if (!vsession->started || session_sdev->dev != NULL) { /* Nothing to do. */ return 0; } @@ -1092,7 +1092,7 @@ spdk_vhost_scsi_session_remove_tgt(struct spdk_vhost_dev *vdev, svsession = (struct spdk_vhost_scsi_session *)vsession; state = &svsession->scsi_dev_state[scsi_tgt_num]; - if (vsession->lcore == -1 || state->dev == NULL) { + if (!vsession->started || state->dev == NULL) { /* Nothing to do */ return 0; } diff --git a/test/unit/lib/vhost/vhost.c/vhost_ut.c b/test/unit/lib/vhost/vhost.c/vhost_ut.c index c4200ce30..5f8cdd357 100644 --- a/test/unit/lib/vhost/vhost.c/vhost_ut.c +++ b/test/unit/lib/vhost/vhost.c/vhost_ut.c @@ -164,6 +164,7 @@ start_vdev(struct spdk_vhost_dev *vdev) CU_ASSERT(rc == 0); SPDK_CU_ASSERT_FATAL(vsession != NULL); vsession->lcore = 0; + vsession->started = true; vsession->vid = 0; vsession->mem = mem; TAILQ_INSERT_TAIL(&vdev->vsessions, vsession, tailq);