[PATCH 0/4] tcm_vhost lock and flush fix

[PATCH 0/4] tcm_vhost lock and flush fix

[Asias He]

Asias He (4):
  tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
  tcm_vhost: Introduce tcm_vhost_check_endpoint()
  tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
  tcm_vhost: Flush vhost_work in vhost_scsi_flush()

 drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

-- 
1.8.1.4

[PATCH 1/4] tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()

[Asias He]

tv_tpg->tv_tpg_vhost_count should be protected by tv_tpg->tv_tpg_mutex.

Signed-off-by: Asias He <asias@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 9951297..b3e50d7 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -860,9 +860,11 @@ static int vhost_scsi_clear_endpoint(
 		if (!tv_tpg)
 			continue;
 
+		mutex_lock(&tv_tpg->tv_tpg_mutex);
 		tv_tport = tv_tpg->tport;
 		if (!tv_tport) {
 			ret = -ENODEV;
+			mutex_unlock(&tv_tpg->tv_tpg_mutex);
 			goto err;
 		}
 
@@ -872,11 +874,13 @@ static int vhost_scsi_clear_endpoint(
 				tv_tport->tport_name, tv_tpg->tport_tpgt,
 				t->vhost_wwpn, t->vhost_tpgt);
 			ret = -EINVAL;
+			mutex_unlock(&tv_tpg->tv_tpg_mutex);
 			goto err;
 		}
 		tv_tpg->tv_tpg_vhost_count--;
 		vs->vs_tpg[target] = NULL;
 		vs->vs_endpoint = false;
+		mutex_unlock(&tv_tpg->tv_tpg_mutex);
 	}
 	mutex_unlock(&vs->dev.mutex);
 	return 0;
-- 
1.8.1.4

[PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()

[Asias He]

This helper is useful to check if vs->vs_endpoint is setup by
vhost_scsi_set_endpoint()

Signed-off-by: Asias He <asias@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index b3e50d7..29612bc 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -91,6 +91,18 @@ 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_scsi *vs)
+{
+	bool ret = false;
+
+	mutex_lock(&vs->dev.mutex);
+	if (vs->vs_endpoint)
+		ret = true;
+	mutex_unlock(&vs->dev.mutex);
+
+	return ret;
+}
+
 static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
 {
 	return 1;
-- 
1.8.1.4

[PATCH 3/4] tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()

[Asias He]

vs->vs_endpoint is protected by the vs->dev.mutex. Use
tcm_vhost_check_endpoint() to do check. The helper does the needed
locking for us.

Signed-off-by: Asias He <asias@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 29612bc..61093d1 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -594,7 +594,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	u8 target;
 
 	/* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
-	if (unlikely(!vs->vs_endpoint))
+	if (!tcm_vhost_check_endpoint(vs))
 		return;
 
 	mutex_lock(&vq->mutex);
-- 
1.8.1.4

[PATCH 4/4] tcm_vhost: Flush vhost_work in vhost_scsi_flush()

[Asias He]

We also need to flush the vhost_works. One is the completion vhost_work, the
other is event vhost_work.

Signed-off-by: Asias He <asias@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 61093d1..c5157cd 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -953,6 +953,7 @@ static void vhost_scsi_flush(struct vhost_scsi *vs)
 
 	for (i = 0; i < VHOST_SCSI_MAX_VQ; i++)
 		vhost_scsi_flush_vq(vs, i);
+	vhost_work_flush(&vs->dev, &vs->vs_completion_work);
 }
 
 static int vhost_scsi_set_features(struct vhost_scsi *vs, u64 features)
-- 
1.8.1.4

Re: [PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()

[Paolo Bonzini]

Il 12/03/2013 03:42, Asias He ha scritto:
> This helper is useful to check if vs->vs_endpoint is setup by
> vhost_scsi_set_endpoint()
> 
> Signed-off-by: Asias He <asias@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  drivers/vhost/tcm_vhost.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index b3e50d7..29612bc 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -91,6 +91,18 @@ 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_scsi *vs)
> +{
> +	bool ret = false;
> +
> +	mutex_lock(&vs->dev.mutex);
> +	if (vs->vs_endpoint)
> +		ret = true;
> +	mutex_unlock(&vs->dev.mutex);

The return value is invalid as soon as mutex_unlock is called, i.e.
before tcm_vhost_check_endpoint returns.  Instead, check vs->vs_endpoint
in the caller while the mutex is taken.

Paolo

> +	return ret;
> +}
> +
>  static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
>  {
>  	return 1;
> 

Re: [PATCH 0/4] tcm_vhost lock and flush fix

[Paolo Bonzini]

Il 12/03/2013 03:42, Asias He ha scritto:
> Asias He (4):
>   tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
>   tcm_vhost: Introduce tcm_vhost_check_endpoint()
>   tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
>   tcm_vhost: Flush vhost_work in vhost_scsi_flush()
> 
>  drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 

Patch 1, 2, 4:

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 3/4] tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()

[Michael S. Tsirkin]

On Tue, Mar 12, 2013 at 10:42:50AM +0800, Asias He wrote:
> vs->vs_endpoint is protected by the vs->dev.mutex. Use
> tcm_vhost_check_endpoint() to do check. The helper does the needed
> locking for us.
> 
> Signed-off-by: Asias He <asias@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

This takes dev mutex on data path which will introduce
contention esp for multiqueue.
How about storing the endpoint as part of vq
private data and protecting with vq mutex?

> ---
>  drivers/vhost/tcm_vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index 29612bc..61093d1 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -594,7 +594,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  	u8 target;
>  
>  	/* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
> -	if (unlikely(!vs->vs_endpoint))
> +	if (!tcm_vhost_check_endpoint(vs))
>  		return;
>  
>  	mutex_lock(&vq->mutex);
> -- 
> 1.8.1.4

Re: [PATCH 0/4] tcm_vhost lock and flush fix

[Michael S. Tsirkin]

On Tue, Mar 12, 2013 at 09:26:42AM +0100, Paolo Bonzini wrote:
> Il 12/03/2013 03:42, Asias He ha scritto:
> > Asias He (4):
> >   tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
> >   tcm_vhost: Introduce tcm_vhost_check_endpoint()
> >   tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
> >   tcm_vhost: Flush vhost_work in vhost_scsi_flush()
> > 
> >  drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> 
> Patch 1, 2, 4:
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

How is 2 useful without 3?

Re: [PATCH 1/4] tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()

[Michael S. Tsirkin]

On Tue, Mar 12, 2013 at 10:42:48AM +0800, Asias He wrote:
> tv_tpg->tv_tpg_vhost_count should be protected by tv_tpg->tv_tpg_mutex.
> 
> Signed-off-by: Asias He <asias@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  drivers/vhost/tcm_vhost.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index 9951297..b3e50d7 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -860,9 +860,11 @@ static int vhost_scsi_clear_endpoint(
>  		if (!tv_tpg)
>  			continue;
>  
> +		mutex_lock(&tv_tpg->tv_tpg_mutex);
>  		tv_tport = tv_tpg->tport;
>  		if (!tv_tport) {
>  			ret = -ENODEV;
> +			mutex_unlock(&tv_tpg->tv_tpg_mutex);
>  			goto err;
>  		}
>  
> @@ -872,11 +874,13 @@ static int vhost_scsi_clear_endpoint(
>  				tv_tport->tport_name, tv_tpg->tport_tpgt,
>  				t->vhost_wwpn, t->vhost_tpgt);
>  			ret = -EINVAL;
> +			mutex_unlock(&tv_tpg->tv_tpg_mutex);
>  			goto err;
>  		}
>  		tv_tpg->tv_tpg_vhost_count--;
>  		vs->vs_tpg[target] = NULL;
>  		vs->vs_endpoint = false;
> +		mutex_unlock(&tv_tpg->tv_tpg_mutex);
>  	}
>  	mutex_unlock(&vs->dev.mutex);
>  	return 0;

Does the error handling have to be so messy?
How about another label which does unlock and goto there?

> -- 
> 1.8.1.4

Re: [PATCH 0/4] tcm_vhost lock and flush fix

[Paolo Bonzini]

Il 12/03/2013 12:12, Michael S. Tsirkin ha scritto:
> On Tue, Mar 12, 2013 at 09:26:42AM +0100, Paolo Bonzini wrote:
>> Il 12/03/2013 03:42, Asias He ha scritto:
>>> Asias He (4):
>>>   tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
>>>   tcm_vhost: Introduce tcm_vhost_check_endpoint()
>>>   tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
>>>   tcm_vhost: Flush vhost_work in vhost_scsi_flush()
>>>
>>>  drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> Patch 1, 2, 4:
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> How is 2 useful without 3?

I can still say I reviewed it and Asias can include the tag in the next
respin.  But I screwed up, patch 2 is exactly the one that I said makes
little sense and should be folded into patch 3.

Paolo

Re: [PATCH 1/4] tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()

[Asias He]

On Tue, Mar 12, 2013 at 01:13:44PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 12, 2013 at 10:42:48AM +0800, Asias He wrote:
> > tv_tpg->tv_tpg_vhost_count should be protected by tv_tpg->tv_tpg_mutex.
> > 
> > Signed-off-by: Asias He <asias@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  drivers/vhost/tcm_vhost.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > index 9951297..b3e50d7 100644
> > --- a/drivers/vhost/tcm_vhost.c
> > +++ b/drivers/vhost/tcm_vhost.c
> > @@ -860,9 +860,11 @@ static int vhost_scsi_clear_endpoint(
> >  		if (!tv_tpg)
> >  			continue;
> >  
> > +		mutex_lock(&tv_tpg->tv_tpg_mutex);
> >  		tv_tport = tv_tpg->tport;
> >  		if (!tv_tport) {
> >  			ret = -ENODEV;
> > +			mutex_unlock(&tv_tpg->tv_tpg_mutex);
> >  			goto err;
> >  		}
> >  
> > @@ -872,11 +874,13 @@ static int vhost_scsi_clear_endpoint(
> >  				tv_tport->tport_name, tv_tpg->tport_tpgt,
> >  				t->vhost_wwpn, t->vhost_tpgt);
> >  			ret = -EINVAL;
> > +			mutex_unlock(&tv_tpg->tv_tpg_mutex);
> >  			goto err;
> >  		}
> >  		tv_tpg->tv_tpg_vhost_count--;
> >  		vs->vs_tpg[target] = NULL;
> >  		vs->vs_endpoint = false;
> > +		mutex_unlock(&tv_tpg->tv_tpg_mutex);
> >  	}
> >  	mutex_unlock(&vs->dev.mutex);
> >  	return 0;
> 
> Does the error handling have to be so messy?
> How about another label which does unlock and goto there?

Well, I will do it.

> > -- 
> > 1.8.1.4

-- 
Asias

Re: [PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()

[Asias He]

On Tue, Mar 12, 2013 at 09:26:18AM +0100, Paolo Bonzini wrote:
> Il 12/03/2013 03:42, Asias He ha scritto:
> > This helper is useful to check if vs->vs_endpoint is setup by
> > vhost_scsi_set_endpoint()
> > 
> > Signed-off-by: Asias He <asias@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  drivers/vhost/tcm_vhost.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > index b3e50d7..29612bc 100644
> > --- a/drivers/vhost/tcm_vhost.c
> > +++ b/drivers/vhost/tcm_vhost.c
> > @@ -91,6 +91,18 @@ 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_scsi *vs)
> > +{
> > +	bool ret = false;
> > +
> > +	mutex_lock(&vs->dev.mutex);
> > +	if (vs->vs_endpoint)
> > +		ret = true;
> > +	mutex_unlock(&vs->dev.mutex);
> 
> The return value is invalid as soon as mutex_unlock is called, i.e.
> before tcm_vhost_check_endpoint returns.  Instead, check vs->vs_endpoint
> in the caller while the mutex is taken.

Do you mean 1) or 2)?

   1)
   vhost_scsi_handle_vq()
   {
   
      mutex_lock(&vs->dev.mutex);
      check vs->vs_endpoint
      mutex_unlock(&vs->dev.mutex);
   
      handle vq
   }
   
   2)
   vhost_scsi_handle_vq()
   {
   
      lock vs->dev.mutex
      check vs->vs_endpoint
      handle vq
      unlock vs->dev.mutex
   }

1) does not make any difference with the original
one right?

2) would be too heavy. This might not be a problem in current 1 thread
per vhost model. But if we want concurrent multiqueue, this will be
killing us.

Anyway, the current one is not good. Need to think.

> Paolo
> 
> > +	return ret;
> > +}
> > +
> >  static int tcm_vhost_check_true(struct se_portal_group *se_tpg)
> >  {
> >  	return 1;
> > 
> 

-- 
Asias

Re: [PATCH 3/4] tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()

[Asias He]

On Tue, Mar 12, 2013 at 01:11:19PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 12, 2013 at 10:42:50AM +0800, Asias He wrote:
> > vs->vs_endpoint is protected by the vs->dev.mutex. Use
> > tcm_vhost_check_endpoint() to do check. The helper does the needed
> > locking for us.
> > 
> > Signed-off-by: Asias He <asias@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> This takes dev mutex on data path which will introduce
> contention esp for multiqueue.

Yes, for now it is okay, but for concurrent execution of multiqueue it is
really bad.

By the way, what is the overhead of taking and releasing the
vs->dev.mutex even if no one contents for it? Is this overhead gnorable.

> How about storing the endpoint as part of vq
> private data and protecting with vq mutex?

Hmm, this makes sense, let's see how well it works.

> > ---
> >  drivers/vhost/tcm_vhost.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > index 29612bc..61093d1 100644
> > --- a/drivers/vhost/tcm_vhost.c
> > +++ b/drivers/vhost/tcm_vhost.c
> > @@ -594,7 +594,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  	u8 target;
> >  
> >  	/* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
> > -	if (unlikely(!vs->vs_endpoint))
> > +	if (!tcm_vhost_check_endpoint(vs))
> >  		return;
> >  
> >  	mutex_lock(&vq->mutex);
> > -- 
> > 1.8.1.4

-- 
Asias

Re: [PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()

[Paolo Bonzini]

Il 13/03/2013 04:02, Asias He ha scritto:
> On Tue, Mar 12, 2013 at 09:26:18AM +0100, Paolo Bonzini wrote:
>> Il 12/03/2013 03:42, Asias He ha scritto:
>>> This helper is useful to check if vs->vs_endpoint is setup by
>>> vhost_scsi_set_endpoint()
>>>
>>> Signed-off-by: Asias He <asias@redhat.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  drivers/vhost/tcm_vhost.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
>>> index b3e50d7..29612bc 100644
>>> --- a/drivers/vhost/tcm_vhost.c
>>> +++ b/drivers/vhost/tcm_vhost.c
>>> @@ -91,6 +91,18 @@ 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_scsi *vs)
>>> +{
>>> +	bool ret = false;
>>> +
>>> +	mutex_lock(&vs->dev.mutex);
>>> +	if (vs->vs_endpoint)
>>> +		ret = true;
>>> +	mutex_unlock(&vs->dev.mutex);
>>
>> The return value is invalid as soon as mutex_unlock is called, i.e.
>> before tcm_vhost_check_endpoint returns.  Instead, check vs->vs_endpoint
>> in the caller while the mutex is taken.
> 
> Do you mean 1) or 2)?
> 
>    1)
>    vhost_scsi_handle_vq()
>    {
>    
>       mutex_lock(&vs->dev.mutex);
>       check vs->vs_endpoint
>       mutex_unlock(&vs->dev.mutex);
>    
>       handle vq
>    }
>    
>    2)
>    vhost_scsi_handle_vq()
>    {
>    
>       lock vs->dev.mutex
>       check vs->vs_endpoint
>       handle vq
>       unlock vs->dev.mutex
>    }
> 
> 1) does not make any difference with the original
> one right?

Yes, it's just what you have with tcm_vhost_check_endpoint inlined.

> 2) would be too heavy. This might not be a problem in current 1 thread
> per vhost model. But if we want concurrent multiqueue, this will be
> killing us.

I mean (2).  You could use an rwlock to enable more concurrency.

Paolo

Re: [PATCH 3/4] tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()

[Paolo Bonzini]

Il 13/03/2013 04:13, Asias He ha scritto:
>> > This takes dev mutex on data path which will introduce
>> > contention esp for multiqueue.
> Yes, for now it is okay, but for concurrent execution of multiqueue it is
> really bad.
> 
> By the way, what is the overhead of taking and releasing the
> vs->dev.mutex even if no one contents for it? Is this overhead gnorable.

There is a possibility of cacheline ping-pong, but apart from that it's
ignorable.

>> > How about storing the endpoint as part of vq
>> > private data and protecting with vq mutex?
> 
> Hmm, this makes sense, let's see how well it works.

Then VHOST_SCSI_SET_ENDPOINT would have to go through all vqs, no?  A
rwlock seems simpler.

Paolo

Re: [PATCH 3/4] tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()

[Asias He]

On Wed, Mar 13, 2013 at 09:02:38AM +0100, Paolo Bonzini wrote:
> Il 13/03/2013 04:13, Asias He ha scritto:
> >> > This takes dev mutex on data path which will introduce
> >> > contention esp for multiqueue.
> > Yes, for now it is okay, but for concurrent execution of multiqueue it is
> > really bad.
> > 
> > By the way, what is the overhead of taking and releasing the
> > vs->dev.mutex even if no one contents for it? Is this overhead gnorable.
> 
> There is a possibility of cacheline ping-pong, but apart from that it's
> ignorable.

Ah, thanks!

> >> > How about storing the endpoint as part of vq
> >> > private data and protecting with vq mutex?
> > 
> > Hmm, this makes sense, let's see how well it works.
> 
> Then VHOST_SCSI_SET_ENDPOINT would have to go through all vqs, no?  A
> rwlock seems simpler.

VHOST_SCSI_SET_ENDPOINT operation is not on the data path, it
is fine to go through all vqs, it's just a loop.

For the rwlock thing, let's discuss it on the other thread.

> Paolo

-- 
Asias

Re: [PATCH 2/4] tcm_vhost: Introduce tcm_vhost_check_endpoint()

[Asias He]

On Wed, Mar 13, 2013 at 09:00:43AM +0100, Paolo Bonzini wrote:
> Il 13/03/2013 04:02, Asias He ha scritto:
> > On Tue, Mar 12, 2013 at 09:26:18AM +0100, Paolo Bonzini wrote:
> >> Il 12/03/2013 03:42, Asias He ha scritto:
> >>> This helper is useful to check if vs->vs_endpoint is setup by
> >>> vhost_scsi_set_endpoint()
> >>>
> >>> Signed-off-by: Asias He <asias@redhat.com>
> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> ---
> >>>  drivers/vhost/tcm_vhost.c | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> >>> index b3e50d7..29612bc 100644
> >>> --- a/drivers/vhost/tcm_vhost.c
> >>> +++ b/drivers/vhost/tcm_vhost.c
> >>> @@ -91,6 +91,18 @@ 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_scsi *vs)
> >>> +{
> >>> +	bool ret = false;
> >>> +
> >>> +	mutex_lock(&vs->dev.mutex);
> >>> +	if (vs->vs_endpoint)
> >>> +		ret = true;
> >>> +	mutex_unlock(&vs->dev.mutex);
> >>
> >> The return value is invalid as soon as mutex_unlock is called, i.e.
> >> before tcm_vhost_check_endpoint returns.  Instead, check vs->vs_endpoint
> >> in the caller while the mutex is taken.
> > 
> > Do you mean 1) or 2)?
> > 
> >    1)
> >    vhost_scsi_handle_vq()
> >    {
> >    
> >       mutex_lock(&vs->dev.mutex);
> >       check vs->vs_endpoint
> >       mutex_unlock(&vs->dev.mutex);
> >    
> >       handle vq
> >    }
> >    
> >    2)
> >    vhost_scsi_handle_vq()
> >    {
> >    
> >       lock vs->dev.mutex
> >       check vs->vs_endpoint
> >       handle vq
> >       unlock vs->dev.mutex
> >    }
> > 
> > 1) does not make any difference with the original
> > one right?
> 
> Yes, it's just what you have with tcm_vhost_check_endpoint inlined.

okay.

> > 2) would be too heavy. This might not be a problem in current 1 thread
> > per vhost model. But if we want concurrent multiqueue, this will be
> > killing us.
> 
> I mean (2).  You could use an rwlock to enable more concurrency.

-- 
Asias

Re: [PATCH 0/4] tcm_vhost lock and flush fix

[Asias He]

On Tue, Mar 12, 2013 at 12:14:56PM +0100, Paolo Bonzini wrote:
> Il 12/03/2013 12:12, Michael S. Tsirkin ha scritto:
> > On Tue, Mar 12, 2013 at 09:26:42AM +0100, Paolo Bonzini wrote:
> >> Il 12/03/2013 03:42, Asias He ha scritto:
> >>> Asias He (4):
> >>>   tcm_vhost: Add missed lock in vhost_scsi_clear_endpoint()
> >>>   tcm_vhost: Introduce tcm_vhost_check_endpoint()
> >>>   tcm_vhost: Fix vs->vs_endpoint checking in vhost_scsi_handle_vq()
> >>>   tcm_vhost: Flush vhost_work in vhost_scsi_flush()
> >>>
> >>>  drivers/vhost/tcm_vhost.c | 19 ++++++++++++++++++-
> >>>  1 file changed, 18 insertions(+), 1 deletion(-)
> >>
> >> Patch 1, 2, 4:
> >>
> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > How is 2 useful without 3?
> 
> I can still say I reviewed it and Asias can include the tag in the next
> respin.  But I screwed up, patch 2 is exactly the one that I said makes
> little sense and should be folded into patch 3.

Thanks, forgot to include your Reviewed-by tag in v2. Including it. And
2 and 3 are folded.

-- 
Asias