[PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts scsi_dispatch_cmd() to only hold struct Scsi_Host->host_lock
for scsi_cmd_get_serial(), and drops the lock because the call into
the LLD with host->hostt->queuecommand().

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/scsi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ad0ed21..06e5b3a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -745,6 +745,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 	 * TODO: kill serial or move to blk layer
 	 */
 	scsi_cmd_get_serial(host, cmd); 
+	spin_unlock_irqrestore(host->host_lock, flags);
 
 	if (unlikely(host->shost_state == SHOST_DEL)) {
 		cmd->result = (DID_NO_CONNECT << 16);
@@ -753,7 +754,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 		trace_scsi_dispatch_cmd_start(cmd);
 		rtn = host->hostt->queuecommand(cmd, scsi_done);
 	}
-	spin_unlock_irqrestore(host->host_lock, flags);
+
 	if (rtn) {
 		trace_scsi_dispatch_cmd_error(cmd, rtn);
 		if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
-- 
1.7.2.3

[PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts scsi_dispatch_cmd() to only hold struct Scsi_Host->host_lock
for scsi_cmd_get_serial(), and drops the lock because the call into
the LLD with host->hostt->queuecommand().

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/scsi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ad0ed21..06e5b3a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -745,6 +745,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 	 * TODO: kill serial or move to blk layer
 	 */
 	scsi_cmd_get_serial(host, cmd); 
+	spin_unlock_irqrestore(host->host_lock, flags);
 
 	if (unlikely(host->shost_state == SHOST_DEL)) {
 		cmd->result = (DID_NO_CONNECT << 16);
@@ -753,7 +754,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 		trace_scsi_dispatch_cmd_start(cmd);
 		rtn = host->hostt->queuecommand(cmd, scsi_done);
 	}
-	spin_unlock_irqrestore(host->host_lock, flags);
+
 	if (rtn) {
 		trace_scsi_dispatch_cmd_error(cmd, rtn);
 		if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
-- 
1.7.2.3

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[James Bottomley]

On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch converts scsi_dispatch_cmd() to only hold struct Scsi_Host->host_lock
> for scsi_cmd_get_serial(), and drops the lock because the call into
> the LLD with host->hostt->queuecommand().
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/scsi/scsi.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index ad0ed21..06e5b3a 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -745,6 +745,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
>  	 * TODO: kill serial or move to blk layer
>  	 */
>  	scsi_cmd_get_serial(host, cmd); 
> +	spin_unlock_irqrestore(host->host_lock, flags);

So at least from where I stand, my object is to reduce the number of
times we take and release the lock, which this doesn't do.  As I said
before: we need to figure out the rest, which likely includes an atomic
for the serial number (which is almost unused).  I think the check
against SHOST_DEL is fine unlocked.

James

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 22:46 -0400, James Bottomley wrote:
> On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch converts scsi_dispatch_cmd() to only hold struct Scsi_Host->host_lock
> > for scsi_cmd_get_serial(), and drops the lock because the call into
> > the LLD with host->hostt->queuecommand().
> > 
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/scsi/scsi.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index ad0ed21..06e5b3a 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -745,6 +745,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
> >  	 * TODO: kill serial or move to blk layer
> >  	 */
> >  	scsi_cmd_get_serial(host, cmd); 
> > +	spin_unlock_irqrestore(host->host_lock, flags);
> 
> So at least from where I stand, my object is to reduce the number of
> times we take and release the lock, which this doesn't do.  As I said
> before: we need to figure out the rest, which likely includes an atomic
> for the serial number (which is almost unused).  I think the check
> against SHOST_DEL is fine unlocked.
> 

Ok, this makes sense if the shost_state == SHOST_DEL check really is OK
to be done w/o host_lock held (I really don't know on this one..)

So if that is the case, then Tim and Vasu can you guys have a look at
dropping the host_lock around serial number usage and post this for
review..?  I will plan to include the host_lock'less serial number patch
into lio-core-2.6.git/drop-host_lock with the rest of the LLD driver
conversion patches from this afternoon and respin a new branch for James
and Co. to review.

Thanks!

--nab

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Christoph Hellwig]

On Thu, Sep 16, 2010 at 10:46:11PM -0400, James Bottomley wrote:
> So at least from where I stand, my object is to reduce the number of
> times we take and release the lock, which this doesn't do.  As I said
> before: we need to figure out the rest, which likely includes an atomic
> for the serial number (which is almost unused).  I think the check
> against SHOST_DEL is fine unlocked.


The check by itself for sure is.  But I wonder whether we make any
assumptions about it not changing while we are in ->queuecommand, which
isn't nessecarily the case after this patch.

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Andi Kleen]

> So at least from where I stand, my object is to reduce the number of
> times we take and release the lock, which this doesn't do.  As I said
> before: we need to figure out the rest, which likely includes an atomic
> for the serial number (which is almost unused).  I think the check

If it's unused it should be removed, make optional.
Atomics are a scalability problem too and not much cheaper than spinlocks.

-Andi

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[James Bottomley]

On Fri, 2010-09-17 at 09:20 +0200, Andi Kleen wrote:
> > So at least from where I stand, my object is to reduce the number of
> > times we take and release the lock, which this doesn't do.  As I said
> > before: we need to figure out the rest, which likely includes an atomic
> > for the serial number (which is almost unused).  I think the check
> 
> If it's unused it should be removed, make optional.
> Atomics are a scalability problem too and not much cheaper than spinlocks.

I don't disagree with the idea of removing it, especially as it has so
few users, but replacing the host lock with an atomic here would still
vastly reduce the contention, which is the initial complaint.  The
contention occurs because the host lock is so widely used for other
things.  The way to reduce that contention is firstly to reduce the
length of code covered by the lock and also reduce the actual number of
places where the lock is taken.  Compared with host lock's current vast
footprint, and atomic here is tiny.

James

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Andi Kleen]

> I don't disagree with the idea of removing it, especially as it has so
> few users, but replacing the host lock with an atomic here would still
> vastly reduce the contention, which is the initial complaint.  The

Actually the complaint is the overhead of the spin lock. This can be 
either caused
by contention or by cache line bounce time.

> contention occurs because the host lock is so widely used for other
> things.  The way to reduce that contention is firstly to reduce the
> length of code covered by the lock and also reduce the actual number of
> places where the lock is taken.  Compared with host lock's current vast
> footprint, and atomic here is tiny.

That assumes that it's contention that is the problem and not simply 
bounce time.

-Andi

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[James Bottomley]

On Fri, 2010-09-17 at 16:22 +0200, Andi Kleen wrote:
> > I don't disagree with the idea of removing it, especially as it has so
> > few users, but replacing the host lock with an atomic here would still
> > vastly reduce the contention, which is the initial complaint.  The
> 
> Actually the complaint is the overhead of the spin lock. This can be 
> either caused
> by contention or by cache line bounce time.

The original complaint was contention.  My desire is to reduce the
locked path coverage, so I saw an opportunity.

What I was actually thinking of for the atomic is that we'd let it range
[1..INT_MAX] so a zero was an indicator of no use of this.  Then the
actual code could become

if (atomic_read(x)) {
	do {
		y = atomic_add_return(1, x);
	} while (y == 0);
}

So "fast" cards not using the serial number set a zero there (we'd
default initialise to one), the line is shared so no bouncing (because
it's never updated).  This should satisfy everyone.

> > contention occurs because the host lock is so widely used for other
> > things.  The way to reduce that contention is firstly to reduce the
> > length of code covered by the lock and also reduce the actual number of
> > places where the lock is taken.  Compared with host lock's current vast
> > footprint, and atomic here is tiny.
> 
> That assumes that it's contention that is the problem and not simply 
> bounce time.

That's what the patch and data that started this whole thread showed,
yes ... but I think actual bounce in the spinlock is also a problem ...
we just don't have data to show it.

James

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Nicholas A. Bellinger]

On Fri, 2010-09-17 at 10:57 -0400, James Bottomley wrote:
> On Fri, 2010-09-17 at 16:22 +0200, Andi Kleen wrote:
> > > I don't disagree with the idea of removing it, especially as it has so
> > > few users, but replacing the host lock with an atomic here would still
> > > vastly reduce the contention, which is the initial complaint.  The
> > 
> > Actually the complaint is the overhead of the spin lock. This can be 
> > either caused
> > by contention or by cache line bounce time.
> 
> The original complaint was contention.  My desire is to reduce the
> locked path coverage, so I saw an opportunity.
> 
> What I was actually thinking of for the atomic is that we'd let it range
> [1..INT_MAX] so a zero was an indicator of no use of this.  Then the
> actual code could become
> 
> if (atomic_read(x)) {
> 	do {
> 		y = atomic_add_return(1, x);
> 	} while (y == 0);
> }

The conversion of struct scsi_cmnd->serial_number to atomic_t and the
above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.

I will take a look at this conversion and respin a complete set of
patches for review a bit later today.

Thanks!

--nab

> 
> So "fast" cards not using the serial number set a zero there (we'd
> default initialise to one), the line is shared so no bouncing (because
> it's never updated).  This should satisfy everyone.
> 
> > > contention occurs because the host lock is so widely used for other
> > > things.  The way to reduce that contention is firstly to reduce the
> > > length of code covered by the lock and also reduce the actual number of
> > > places where the lock is taken.  Compared with host lock's current vast
> > > footprint, and atomic here is tiny.
> > 
> > That assumes that it's contention that is the problem and not simply 
> > bounce time.
> 
> That's what the patch and data that started this whole thread showed,
> yes ... but I think actual bounce in the spinlock is also a problem ...
> we just don't have data to show it.
> 
> James
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Nicholas A. Bellinger]

On Fri, 2010-09-17 at 09:37 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2010-09-17 at 10:57 -0400, James Bottomley wrote:
> > On Fri, 2010-09-17 at 16:22 +0200, Andi Kleen wrote:
> > > > I don't disagree with the idea of removing it, especially as it has so
> > > > few users, but replacing the host lock with an atomic here would still
> > > > vastly reduce the contention, which is the initial complaint.  The
> > > 
> > > Actually the complaint is the overhead of the spin lock. This can be 
> > > either caused
> > > by contention or by cache line bounce time.
> > 
> > The original complaint was contention.  My desire is to reduce the
> > locked path coverage, so I saw an opportunity.
> > 
> > What I was actually thinking of for the atomic is that we'd let it range
> > [1..INT_MAX] so a zero was an indicator of no use of this.  Then the
> > actual code could become
> > 
> > if (atomic_read(x)) {
> > 	do {
> > 		y = atomic_add_return(1, x);
> > 	} while (y == 0);
> > }
> 
> The conversion of struct scsi_cmnd->serial_number to atomic_t and the
> above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.
> 

Actually, that should be the conversion of struct
Scsi_Host->cmd_serial_number to an atomic_t.   AFAICT there is no reason
for struct scsi_cmnd->serial_number needing to be an atomic_t.

Best,

--nab

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Joe Eykholt]

On 9/17/10 9:37 AM, Nicholas A. Bellinger wrote:
> On Fri, 2010-09-17 at 10:57 -0400, James Bottomley wrote:
>> On Fri, 2010-09-17 at 16:22 +0200, Andi Kleen wrote:
>>>> I don't disagree with the idea of removing it, especially as it has so
>>>> few users, but replacing the host lock with an atomic here would still
>>>> vastly reduce the contention, which is the initial complaint.  The
>>>
>>> Actually the complaint is the overhead of the spin lock. This can be 
>>> either caused
>>> by contention or by cache line bounce time.
>>
>> The original complaint was contention.  My desire is to reduce the
>> locked path coverage, so I saw an opportunity.
>>
>> What I was actually thinking of for the atomic is that we'd let it range
>> [1..INT_MAX] so a zero was an indicator of no use of this.  Then the
>> actual code could become
>>
>> if (atomic_read(x)) {
>> 	do {
>> 		y = atomic_add_return(1, x);
>> 	} while (y == 0);
>> }

A tiny trick I like to use is to start a serial number at 1 and
increment by 2 so its always odd and then never wraps to 0.
That eliminates the check for 0 (and the curly brackets).

> The conversion of struct scsi_cmnd->serial_number to atomic_t and the
> above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.
> 
> I will take a look at this conversion and respin a complete set of
> patches for review a bit later today.
> 
> Thanks!
> 
> --nab
> 
>>
>> So "fast" cards not using the serial number set a zero there (we'd
>> default initialise to one), the line is shared so no bouncing (because
>> it's never updated).  This should satisfy everyone.
>>
>>>> contention occurs because the host lock is so widely used for other
>>>> things.  The way to reduce that contention is firstly to reduce the
>>>> length of code covered by the lock and also reduce the actual number of
>>>> places where the lock is taken.  Compared with host lock's current vast
>>>> footprint, and atomic here is tiny.
>>>
>>> That assumes that it's contention that is the problem and not simply 
>>> bounce time.
>>
>> That's what the patch and data that started this whole thread showed,
>> yes ... but I think actual bounce in the spinlock is also a problem ...
>> we just don't have data to show it.
>>
>> James
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Tim Chen]

On Fri, 2010-09-17 at 09:41 -0700, Nicholas A. Bellinger wrote:

> > > 
> > > What I was actually thinking of for the atomic is that we'd let it range
> > > [1..INT_MAX] so a zero was an indicator of no use of this.  Then the
> > > actual code could become
> > > 
> > > if (atomic_read(x)) {
> > > 	do {
> > > 		y = atomic_add_return(1, x);
> > > 	} while (y == 0);
> > > }
> > 
> > The conversion of struct scsi_cmnd->serial_number to atomic_t and the
> > above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.
> > 
> 
> Actually, that should be the conversion of struct
> Scsi_Host->cmd_serial_number to an atomic_t.   AFAICT there is no reason
> for struct scsi_cmnd->serial_number needing to be an atomic_t.

Just want to verify the hidden assumption we have here when the atomic
int Scsi_Host->cmd_serial_number counter overflow after increment. The
counter itself then becomes negative.  We are assuming that when we do
type conversion back to unsigned long scsi_cmnd->serial_number, we will
get the right thing.  

So for 32-bit int, we expect if we start with 0x7fffffff in hex and the
expected sequence will be

	2147483647 (int) -> 2147483647 (unsigned long)  [0x7fffffff]
	+1
	-2147483648 (int) -> 2147483648 (unsigned long) [0x80000000]
	+1
	-2147483647 (int) -> 2147483649 (unsigned long) [0x80000001]
	
If there is architecture where the above assumption is not true (which
I'm not aware of but just checking), then we should manually wrap the
atomic counter to 1 when counter overflow.  

Tim
		
		

Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()

[Nicholas A. Bellinger]

On Fri, 2010-09-17 at 10:49 -0700, Tim Chen wrote:
> On Fri, 2010-09-17 at 09:41 -0700, Nicholas A. Bellinger wrote:
> 
> > > > 
> > > > What I was actually thinking of for the atomic is that we'd let it range
> > > > [1..INT_MAX] so a zero was an indicator of no use of this.  Then the
> > > > actual code could become
> > > > 
> > > > if (atomic_read(x)) {
> > > > 	do {
> > > > 		y = atomic_add_return(1, x);
> > > > 	} while (y == 0);
> > > > }
> > > 
> > > The conversion of struct scsi_cmnd->serial_number to atomic_t and the
> > > above code for scsi_cmd_get_serial() sounds perfectly reasonable to me.
> > > 
> > 
> > Actually, that should be the conversion of struct
> > Scsi_Host->cmd_serial_number to an atomic_t.   AFAICT there is no reason
> > for struct scsi_cmnd->serial_number needing to be an atomic_t.
> 
> Just want to verify the hidden assumption we have here when the atomic
> int Scsi_Host->cmd_serial_number counter overflow after increment. The
> counter itself then becomes negative.  We are assuming that when we do
> type conversion back to unsigned long scsi_cmnd->serial_number, we will
> get the right thing.  
> 
> So for 32-bit int, we expect if we start with 0x7fffffff in hex and the
> expected sequence will be
> 
> 	2147483647 (int) -> 2147483647 (unsigned long)  [0x7fffffff]
> 	+1
> 	-2147483648 (int) -> 2147483648 (unsigned long) [0x80000000]
> 	+1
> 	-2147483647 (int) -> 2147483649 (unsigned long) [0x80000001]
> 	
> If there is architecture where the above assumption is not true (which
> I'm not aware of but just checking), then we should manually wrap the
> atomic counter to 1 when counter overflow.  
> 

I was thinking about this as well, but I figured since jejb recommended
it's usage for scsi_cmd_get_serial() that it would not be a problem.
However I am not sure if he had keeping the struct
scsi_cmnd->serial_number a 'unsigned long' that is done in the new v2
patches.

James, is there any other consideration here wrt to atomic_t wrapping
for scsi_cmd_get_serial() using a astruct Scsi_Host->cmd_serial_number
assignment to an 'unsigned long' that I need to include in a v3
series..?

--nab