Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Tue, Apr 02, 2013 at 09:27:57AM +1030, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: Rusty's currently doing some reorgs of -net let's delay cleanups there to avoid stepping on each other's toys. Let's focus on scsi here. E.g. any chance framing assumptions can be fixed in 3.10? I am waiting for your removal of the dma-compelete ordering stuff in vhost-net. Cheers, Rusty. Now, it looks like it's actually a smart datastructure. It allows signalling consumptions from multiple without any locks, with multiple consumers, and just a single kref counter. Nothing simpler than a producer/consumer does this. Yes it can in theory delay some tx completions a bit but normally no one is waiting for them. We can refactor it to save some memory, and cleanup the code, playing with this now. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Tue, Apr 02, 2013 at 06:18:33PM +0300, Michael S. Tsirkin wrote:
On Tue, Apr 02, 2013 at 11:10:02PM +0800, Asias He wrote:
On Tue, Apr 02, 2013 at 03:15:31PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 01, 2013 at 10:13:47AM +0800, Asias He wrote:
On Sun, Mar 31, 2013 at 11:20:24AM +0300, Michael S. Tsirkin wrote:
On Fri, Mar 29, 2013 at 02:22:52PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:18:22AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin
wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint
is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock.
However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex
lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only
take the
vq-mutex lock which is per queue and make the concurrent
multiqueue
process having less lock contention. Further, in the read
side of
vq-private_data, we can even do not take only lock if it
is accessed in
the vhost worker thread, because it is protected by vhost
rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c
b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec
*iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct
vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is
setup by calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker
which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group
*se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from
vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the
vq-mutex, and thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Mon, Apr 01, 2013 at 10:13:47AM +0800, Asias He wrote:
On Sun, Mar 31, 2013 at 11:20:24AM +0300, Michael S. Tsirkin wrote:
On Fri, Mar 29, 2013 at 02:22:52PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:18:22AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is
setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock.
However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent
multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is
accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by
calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and
thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock =
rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If
we want to
use rcu, use it explicitly and avoid the vhost rcu completely.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Tue, Apr 02, 2013 at 09:27:57AM +1030, Rusty Russell wrote: Michael S. Tsirkin m...@redhat.com writes: Rusty's currently doing some reorgs of -net let's delay cleanups there to avoid stepping on each other's toys. Let's focus on scsi here. E.g. any chance framing assumptions can be fixed in 3.10? I am waiting for your removal of the dma-compelete ordering stuff in vhost-net. Cheers, Rusty. Sure. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Tue, Apr 02, 2013 at 03:15:31PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 01, 2013 at 10:13:47AM +0800, Asias He wrote:
On Sun, Mar 31, 2013 at 11:20:24AM +0300, Michael S. Tsirkin wrote:
On Fri, Mar 29, 2013 at 02:22:52PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:18:22AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is
setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock.
However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock
to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take
the
vq-mutex lock which is per queue and make the concurrent
multiqueue
process having less lock contention. Further, in the read side
of
vq-private_data, we can even do not take only lock if it is
accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c
b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue
*vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is
setup by calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker
which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex,
and thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock =
rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Tue, Apr 02, 2013 at 11:10:02PM +0800, Asias He wrote:
On Tue, Apr 02, 2013 at 03:15:31PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 01, 2013 at 10:13:47AM +0800, Asias He wrote:
On Sun, Mar 31, 2013 at 11:20:24AM +0300, Michael S. Tsirkin wrote:
On Fri, Mar 29, 2013 at 02:22:52PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:18:22AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin
wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint
is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock.
However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex
lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only
take the
vq-mutex lock which is per queue and make the concurrent
multiqueue
process having less lock contention. Further, in the read
side of
vq-private_data, we can even do not take only lock if it is
accessed in
the vhost worker thread, because it is protected by vhost
rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c
b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue
*vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is
setup by calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker
which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group
*se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker?
*/
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex,
and thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker?
*/
struct socket *sock =
rcu_dereference_check(vq-private_data, 1);
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
Michael S. Tsirkin m...@redhat.com writes: Rusty's currently doing some reorgs of -net let's delay cleanups there to avoid stepping on each other's toys. Let's focus on scsi here. E.g. any chance framing assumptions can be fixed in 3.10? I am waiting for your removal of the dma-compelete ordering stuff in vhost-net. Cheers, Rusty. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Fri, Mar 29, 2013 at 02:22:52PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:18:22AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup
or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However,
when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed
in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by
calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and
thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock = rcu_dereference_check(vq-private_data,
1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we
want to
use rcu, use it explicitly and avoid the vhost rcu completely.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as
synchronize_rcu */
mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, vs);
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Fri, Mar 29, 2013 at 02:27:50PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:06:15AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:47:15PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 10:33:30AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is
setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock.
However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent
multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is
accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by
calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and
thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock =
rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If
we want to
use rcu, use it explicitly and avoid the vhost rcu completely.
The point is to make spurios wakeups as lightweight as possible.
The seemed to happen a lot with -net.
Should not happen with -scsi at all.
I am wondering:
1. Why there is a lot of spurios wakeups
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Sun, Mar 31, 2013 at 11:20:24AM +0300, Michael S. Tsirkin wrote:
On Fri, Mar 29, 2013 at 02:22:52PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:18:22AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is
setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However,
when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is
accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is setup by
calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and
thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock =
rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we
want to
use rcu, use it explicitly and avoid the vhost rcu completely.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as
synchronize_rcu */
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Sun, Mar 31, 2013 at 11:23:12AM +0300, Michael S. Tsirkin wrote:
On Fri, Mar 29, 2013 at 02:27:50PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 11:06:15AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:47:15PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 10:33:30AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is
setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock.
However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock
to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take
the
vq-mutex lock which is per queue and make the concurrent
multiqueue
process having less lock contention. Further, in the read side
of
vq-private_data, we can even do not take only lock if it is
accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c
b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue
*vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is
setup by calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker
which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex,
and thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock =
rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 11:18:22AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is setup by
calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi
*vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and thus
do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we
want to
use rcu, use it explicitly and avoid the vhost rcu completely.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as
synchronize_rcu */
mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, vs);
vhost_init_used(vq);
mutex_unlock(vq-mutex);
}
- vs-vs_endpoint = true;
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 11:06:15AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:47:15PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 10:33:30AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is
setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However,
when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is
accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38
+-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is setup by
calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and
thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock =
rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we
want to
use rcu, use it explicitly and avoid the vhost rcu completely.
The point is to make spurios wakeups as lightweight as possible.
The seemed to happen a lot with -net.
Should not happen with -scsi at all.
I am wondering:
1. Why there is a lot of spurios wakeups
2. What performance impact it would give if we take the lock to check
vq-private_data. Sinc, at any time, either handle_tx or
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK)) PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, vs);
vhost_init_used(vq);
mutex_unlock(vq-mutex);
}
- vs-vs_endpoint = true;
ret = 0;
} else {
ret = -EEXIST;
There's also some weird smp_mb__after_atomic_inc() with no
atomic in sight just above ... Nicholas what was the point there?
@@ -849,6 +866,8 @@ static int vhost_scsi_clear_endpoint(
{
struct tcm_vhost_tport *tv_tport;
struct tcm_vhost_tpg *tv_tpg;
+ struct vhost_virtqueue *vq;
+ bool match = false;
int index, ret, i;
u8 target;
@@ -884,9 +903,18 @@ static int vhost_scsi_clear_endpoint(
}
tv_tpg-tv_tpg_vhost_count--;
vs-vs_tpg[target] = NULL;
- vs-vs_endpoint = false;
+ match = true;
mutex_unlock(tv_tpg-tv_tpg_mutex);
}
+ if (match) {
+ for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
+ vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
+ mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, NULL);
+ mutex_unlock(vq-mutex);
+ }
+ }
I'm trying to understand what's going on here.
Does vhost_scsi only have a single target?
Because the moment you clear one target you
also set private_data to NULL ...
mutex_unlock(vs-dev.mutex);
return 0;
--
1.8.1.4
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK)) PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is setup by calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we want to
use rcu, use it explicitly and avoid the vhost rcu completely.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, vs);
vhost_init_used(vq);
mutex_unlock(vq-mutex);
}
- vs-vs_endpoint = true;
ret = 0;
} else {
ret = -EEXIST;
There's also some weird smp_mb__after_atomic_inc() with no
atomic in sight just above ... Nicholas what was the point there?
@@ -849,6 +866,8 @@ static int vhost_scsi_clear_endpoint(
{
struct tcm_vhost_tport *tv_tport;
struct tcm_vhost_tpg *tv_tpg;
+ struct vhost_virtqueue *vq;
+ bool match = false;
int index, ret, i;
u8 target;
@@ -884,9 +903,18 @@ static int vhost_scsi_clear_endpoint(
}
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK)) PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi
*vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we want
to
use rcu, use it explicitly and avoid the vhost rcu completely.
The point is to make spurios wakeups as lightweight as possible.
The seemed to happen a lot with -net.
Should not happen with -scsi at all.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, vs);
vhost_init_used(vq);
mutex_unlock(vq-mutex);
}
- vs-vs_endpoint = true;
ret = 0;
} else {
ret = -EEXIST;
There's also some weird smp_mb__after_atomic_inc() with no
atomic in sight just above ... Nicholas what was the point there?
@@ -849,6 +866,8 @@ static int vhost_scsi_clear_endpoint(
{
struct tcm_vhost_tport *tv_tport;
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 10:33:30AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is setup by
calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi
*vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and thus
do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we
want to
use rcu, use it explicitly and avoid the vhost rcu completely.
The point is to make spurios wakeups as lightweight as possible.
The seemed to happen a lot with -net.
Should not happen with -scsi at all.
I am wondering:
1. Why there is a lot of spurios wakeups
2. What performance impact it would give if we take the lock to check
vq-private_data. Sinc, at any time, either handle_tx or handle_rx
can be running. So we can almost always take the vq-mutex mutex.
Did you managed to measure real perf difference?
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0;
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 04:47:15PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 10:33:30AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup
or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However,
when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed
in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK))
PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by
calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in
drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct
vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and
thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock = rcu_dereference_check(vq-private_data,
1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we
want to
use rcu, use it explicitly and avoid the vhost rcu completely.
The point is to make spurios wakeups as lightweight as possible.
The seemed to happen a lot with -net.
Should not happen with -scsi at all.
I am wondering:
1. Why there is a lot of spurios wakeups
2. What performance impact it would give if we take the lock to check
vq-private_data. Sinc, at any time, either handle_tx or handle_rx
can be running. So we can almost always take the vq-mutex mutex.
Did you managed to measure real perf difference?
At some point when this was
Re: [PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
On Thu, Mar 28, 2013 at 04:10:02PM +0800, Asias He wrote:
On Thu, Mar 28, 2013 at 08:16:59AM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 28, 2013 at 10:17:28AM +0800, Asias He wrote:
Currently, vs-vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK)) PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+ * We can handle the vq only after the endpoint is setup by calling the
+ * VHOST_SCSI_SET_ENDPOINT ioctl.
+ *
+ * TODO: Check that we are running from vhost_worker which acts
+ * as read-side critical section for vhost kind of RCU.
+ * See the comments in struct vhost_virtqueue in drivers/vhost/vhost.h
+ */
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi
*vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
I would just move the check to under vq mutex,
and avoid rcu completely. In vhost-net we are using
private data outside lock so we can't do this,
no such issue here.
Are you talking about:
handle_tx:
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
wmem = atomic_read(sock-sk-sk_wmem_alloc);
if (wmem = sock-sk-sk_sndbuf) {
mutex_lock(vq-mutex);
tx_poll_start(net, sock);
mutex_unlock(vq-mutex);
return;
}
mutex_lock(vq-mutex);
Why not do the atomic_read and tx_poll_start under the vq-mutex, and thus do
the check under the lock as well.
handle_rx:
mutex_lock(vq-mutex);
/* TODO: check that we are running from vhost_worker? */
struct socket *sock = rcu_dereference_check(vq-private_data, 1);
if (!sock)
return;
mutex_lock(vq-mutex);
Can't we can do the check under the vq-mutex here?
The rcu is still there but it makes the code easier to read. IMO, If we want
to
use rcu, use it explicitly and avoid the vhost rcu completely.
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, vs);
vhost_init_used(vq);
mutex_unlock(vq-mutex);
}
- vs-vs_endpoint = true;
ret = 0;
} else {
ret = -EEXIST;
There's also some weird smp_mb__after_atomic_inc() with no
atomic in sight just above ... Nicholas what was the point there?
@@ -849,6 +866,8 @@ static int vhost_scsi_clear_endpoint(
{
struct tcm_vhost_tport *tv_tport;
struct tcm_vhost_tpg *tv_tpg;
+ struct vhost_virtqueue *vq;
+ bool match = false;
int index, ret, i;
u8 target;
@@
[PATCH V2 2/2] tcm_vhost: Use vq-private_data to indicate if the endpoint is setup
Currently, vs-vs_endpoint is used indicate if the endpoint is setup or
not. It is set or cleared in vhost_scsi_set_endpoint() or
vhost_scsi_clear_endpoint() under the vs-dev.mutex lock. However, when
we check it in vhost_scsi_handle_vq(), we ignored the lock.
Instead of using the vs-vs_endpoint and the vs-dev.mutex lock to
indicate the status of the endpoint, we use per virtqueue
vq-private_data to indicate it. In this way, we can only take the
vq-mutex lock which is per queue and make the concurrent multiqueue
process having less lock contention. Further, in the read side of
vq-private_data, we can even do not take only lock if it is accessed in
the vhost worker thread, because it is protected by vhost rcu.
Signed-off-by: Asias He as...@redhat.com
---
drivers/vhost/tcm_vhost.c | 38 +-
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 5e3d4487..0524267 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -67,7 +67,6 @@ struct vhost_scsi {
/* Protected by vhost_scsi-dev.mutex */
struct tcm_vhost_tpg *vs_tpg[VHOST_SCSI_MAX_TARGET];
char vs_vhost_wwpn[TRANSPORT_IQN_LEN];
- bool vs_endpoint;
struct vhost_dev dev;
struct vhost_virtqueue vqs[VHOST_SCSI_MAX_VQ];
@@ -91,6 +90,24 @@ static int iov_num_pages(struct iovec *iov)
((unsigned long)iov-iov_base PAGE_MASK)) PAGE_SHIFT;
}
+static bool tcm_vhost_check_endpoint(struct vhost_virtqueue *vq)
+{
+ bool ret = false;
+
+ /*
+* We can handle the vq only after the endpoint is setup by calling the
+* VHOST_SCSI_SET_ENDPOINT ioctl.
+*
+* TODO: Check that we are running from vhost_worker which acts
+* as read-side critical section for vhost kind of RCU.
+* See the comments in struct vhost_virtqueue in drivers/vhost/vhost.h
+*/
+ if (rcu_dereference_check(vq-private_data, 1))
+ ret = true;
+
+ return ret;
+}
+
static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
{
return 1;
@@ -581,8 +598,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
int head, ret;
u8 target;
- /* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
- if (unlikely(!vs-vs_endpoint))
+ if (!tcm_vhost_check_endpoint(vq))
return;
mutex_lock(vq-mutex);
@@ -829,11 +845,12 @@ static int vhost_scsi_set_endpoint(
sizeof(vs-vs_vhost_wwpn));
for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, vs);
vhost_init_used(vq);
mutex_unlock(vq-mutex);
}
- vs-vs_endpoint = true;
ret = 0;
} else {
ret = -EEXIST;
@@ -849,6 +866,8 @@ static int vhost_scsi_clear_endpoint(
{
struct tcm_vhost_tport *tv_tport;
struct tcm_vhost_tpg *tv_tpg;
+ struct vhost_virtqueue *vq;
+ bool match = false;
int index, ret, i;
u8 target;
@@ -884,9 +903,18 @@ static int vhost_scsi_clear_endpoint(
}
tv_tpg-tv_tpg_vhost_count--;
vs-vs_tpg[target] = NULL;
- vs-vs_endpoint = false;
+ match = true;
mutex_unlock(tv_tpg-tv_tpg_mutex);
}
+ if (match) {
+ for (i = 0; i VHOST_SCSI_MAX_VQ; i++) {
+ vq = vs-vqs[i];
+ /* Flushing the vhost_work acts as synchronize_rcu */
+ mutex_lock(vq-mutex);
+ rcu_assign_pointer(vq-private_data, NULL);
+ mutex_unlock(vq-mutex);
+ }
+ }
mutex_unlock(vs-dev.mutex);
return 0;
--
1.8.1.4
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
