Commit 23d7ff31 authored by Darek Stojaczyk's avatar Darek Stojaczyk Committed by Jim Harris
Browse files

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: default avatarDarek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/448229


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
parent 2cddd571

lib/vhost/vhost.c

+12 −7
Original line number Diff line number Diff line
@@ -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;

lib/vhost/vhost_blk.c

+7 −16
Original line number Diff line number Diff line
@@ -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

lib/vhost/vhost_internal.h

+5 −3
Original line number Diff line number Diff line
@@ -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.

 *

lib/vhost/vhost_nvme.c

+7 −16
Original line number Diff line number Diff line
@@ -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

lib/vhost/vhost_scsi.c

+5 −9
Original line number Diff line number Diff line
@@ -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;

	}