[PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Tim Chen]

During testing of FFSB benchmark (configured with 
128 threaded write to 128 files on 16 SSD), scsi_host lock was 
heavily contended, accounting for 23.7% of cpu cycles.  There
are 64 cores in our test system and the JBOD
is connected with a mptsas HBA.  Taking a similar approach
as the patch by Vasu (http://permalink.gmane.org/gmane.linux.scsi.open-fcoe.devel/10110)
for Fiber Channel adapter, the following patch on 2.6.35 kernel 
avoids taking the scsi host lock when queueing mptsas scsi command. We see
a big drop in the cpu cycles contending for the lock (from 23.7% to 1.8%).  
The number of IO per sec increase by 10.6% from 62.9K per sec to 69.6K per sec.

If there is no good reason to prevent mptsas_qcmd from being 
executed in parallel, we should remove this lock from the queue 
command code path.  Other adapters probably can
benefit in a similar manner.


                        %cpu cycles contending host lock
                        2.6.35		2.6.35+patch
-----------------------------------------------------
scsi_dispatch_cmd       5.5%		0.44%
scsi_device_unbusy      6.1%		0.66%
scsi_request_fn         6.6%		0.35%
scsi_run_queue          5.5%		0.35%


Tim Chen


Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index ac000e8..ab3aab9 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -4984,6 +4984,8 @@ mptsas_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	sh->max_lun = max_lun;
 	sh->transportt = mptsas_transport_template;
 
+	sh->unlocked_qcmds = 1;
+
 	/* Required entry.
 	 */
 	sh->unique_id = ioc->id;
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ad0ed21..3819d66 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -749,11 +749,16 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 	if (unlikely(host->shost_state == SHOST_DEL)) {
 		cmd->result = (DID_NO_CONNECT << 16);
 		scsi_done(cmd);
+		spin_unlock_irqrestore(host->host_lock, flags);
 	} else {
 		trace_scsi_dispatch_cmd_start(cmd);
+		if (host->unlocked_qcmds)
+			spin_unlock_irqrestore(host->host_lock, flags);
 		rtn = host->hostt->queuecommand(cmd, scsi_done);
+		if (!host->unlocked_qcmds)
+			spin_unlock_irqrestore(host->host_lock, flags);
 	}
-	spin_unlock_irqrestore(host->host_lock, flags);
+
 	if (rtn) {
 		trace_scsi_dispatch_cmd_error(cmd, rtn);
 		if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b7bdecb..1814c51 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -636,6 +636,9 @@ struct Scsi_Host {
 	/* Asynchronous scan in progress */
 	unsigned async_scan:1;
 
+	/* call queuecommand without Scsi_Host lock held */
+	unsigned unlocked_qcmds:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 12:44 -0700, Tim Chen wrote:
> During testing of FFSB benchmark (configured with 
> 128 threaded write to 128 files on 16 SSD), scsi_host lock was 
> heavily contended, accounting for 23.7% of cpu cycles.  There
> are 64 cores in our test system and the JBOD
> is connected with a mptsas HBA.  Taking a similar approach
> as the patch by Vasu (http://permalink.gmane.org/gmane.linux.scsi.open-fcoe.devel/10110)
> for Fiber Channel adapter, the following patch on 2.6.35 kernel 
> avoids taking the scsi host lock when queueing mptsas scsi command. We see
> a big drop in the cpu cycles contending for the lock (from 23.7% to 1.8%).  
> The number of IO per sec increase by 10.6% from 62.9K per sec to 69.6K per sec.
> 
> If there is no good reason to prevent mptsas_qcmd from being 
> executed in parallel, we should remove this lock from the queue 
> command code path.  Other adapters probably can
> benefit in a similar manner.
> 
> 
>                         %cpu cycles contending host lock
>                         2.6.35		2.6.35+patch
> -----------------------------------------------------
> scsi_dispatch_cmd       5.5%		0.44%
> scsi_device_unbusy      6.1%		0.66%
> scsi_request_fn         6.6%		0.35%
> scsi_run_queue          5.5%		0.35%
> 
> 

Hi Tim and Co,

Many Thanks for posting these very interesting numbers with
unlocked_qcmds=1 + mpt-fusion SCSI LLD on a 64-core system..  Wow.. 8-)

I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
convinced me that doing conditional locking while is very simple, is not
the proper way for getting this resolved in mainline code.  I think in
the end this will require a longer sit down to do a wholesale conversion
of all existing SCSI LLD drivers, and identifing the broken ones that
still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
whatever strange & legacy reasons.

While there are still some outstanding TCM items that need to be
resolved in the next days, I am very interested to help make the
wholesale host_lock + ->queuecomamnd() conversion happen.  I will get a
lio-core-2.6.git branch setup for this purpose on .36-rc4 soon and start
working on the main SCSI Mid-layer conversion pieces sometime next week.
I am very eager to accept patches on a per LLD basis for this work, and
will be starting with the open-fcoe initiator, TCM_Loop, mpt2sas, and
open-iscsi.

I think the wholesole conversion is going to be pretty straight-forward,
and at least with the main SCSI LLDs (that we really care about ;) there
appear to be no immediate issues with a full conversion.

Best,

--nab


> Tim Chen
> 
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
> index ac000e8..ab3aab9 100644
> --- a/drivers/message/fusion/mptsas.c
> +++ b/drivers/message/fusion/mptsas.c
> @@ -4984,6 +4984,8 @@ mptsas_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	sh->max_lun = max_lun;
>  	sh->transportt = mptsas_transport_template;
>  
> +	sh->unlocked_qcmds = 1;
> +
>  	/* Required entry.
>  	 */
>  	sh->unique_id = ioc->id;
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index ad0ed21..3819d66 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -749,11 +749,16 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
>  	if (unlikely(host->shost_state == SHOST_DEL)) {
>  		cmd->result = (DID_NO_CONNECT << 16);
>  		scsi_done(cmd);
> +		spin_unlock_irqrestore(host->host_lock, flags);
>  	} else {
>  		trace_scsi_dispatch_cmd_start(cmd);
> +		if (host->unlocked_qcmds)
> +			spin_unlock_irqrestore(host->host_lock, flags);
>  		rtn = host->hostt->queuecommand(cmd, scsi_done);
> +		if (!host->unlocked_qcmds)
> +			spin_unlock_irqrestore(host->host_lock, flags);
>  	}
> -	spin_unlock_irqrestore(host->host_lock, flags);
> +
>  	if (rtn) {
>  		trace_scsi_dispatch_cmd_error(cmd, rtn);
>  		if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index b7bdecb..1814c51 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -636,6 +636,9 @@ struct Scsi_Host {
>  	/* Asynchronous scan in progress */
>  	unsigned async_scan:1;
>  
> +	/* call queuecommand without Scsi_Host lock held */
> +	unsigned unlocked_qcmds:1;
> +
>  	/*
>  	 * Optional work queue to be utilized by the transport
>  	 */
> 
> 
> 
> --
> 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] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Tim Chen]

On Thu, 2010-09-16 at 13:48 -0700, Nicholas A. Bellinger wrote:

> 
> I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> convinced me that doing conditional locking while is very simple, is not
> the proper way for getting this resolved in mainline code.  I think in
> the end this will require a longer sit down to do a wholesale conversion
> of all existing SCSI LLD drivers, and identifing the broken ones that
> still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> whatever strange & legacy reasons.

That certainly sounds like the right way to go.

> While there are still some outstanding TCM items that need to be
> resolved in the next days, I am very interested to help make the
> wholesale host_lock + ->queuecomamnd() conversion happen.  I will get a
> lio-core-2.6.git branch setup for this purpose on .36-rc4 soon and start
> working on the main SCSI Mid-layer conversion pieces sometime next week.
> I am very eager to accept patches on a per LLD basis for this work, and
> will be starting with the open-fcoe initiator, TCM_Loop, mpt2sas, and
> open-iscsi.
> 
> I think the wholesole conversion is going to be pretty straight-forward,
> and at least with the main SCSI LLDs (that we really care about ;) there
> appear to be no immediate issues with a full conversion.
> 

Looking forward to it.  Thanks.

Tim

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[James Bottomley]

On Thu, 2010-09-16 at 23:25 +0200, Andi Kleen wrote:
> > I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> > convinced me that doing conditional locking while is very simple, is not
> > the proper way for getting this resolved in mainline code.  I think in
> > the end this will require a longer sit down to do a wholesale conversion
> > of all existing SCSI LLD drivers, and identifing the broken ones that
> > still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> > whatever strange & legacy reasons.
> 
> The standard way to do that would be to first move the lock down
> into the drivers (similar to how it has been done with the BKL).
> This would be a fairly mechanic mindless patch. Lots of typing,
> but not really a lot of real code review needed.
> 
> Then next step the drivers who know they don't want it can remove it.

Yes, that's basically what Christoph did when he moved the lock out of
the eh path.

James

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Andi Kleen]

> I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> convinced me that doing conditional locking while is very simple, is not
> the proper way for getting this resolved in mainline code.  I think in
> the end this will require a longer sit down to do a wholesale conversion
> of all existing SCSI LLD drivers, and identifing the broken ones that
> still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> whatever strange & legacy reasons.

The standard way to do that would be to first move the lock down
into the drivers (similar to how it has been done with the BKL).
This would be a fairly mechanic mindless patch. Lots of typing,
but not really a lot of real code review needed.

Then next step the drivers who know they don't want it can remove it.

-Andi

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Vasu Dev]

On Thu, 2010-09-16 at 13:48 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 12:44 -0700, Tim Chen wrote:
> > During testing of FFSB benchmark (configured with 
> > 128 threaded write to 128 files on 16 SSD), scsi_host lock was 
> > heavily contended, accounting for 23.7% of cpu cycles.  There
> > are 64 cores in our test system and the JBOD
> > is connected with a mptsas HBA.  Taking a similar approach
> > as the patch by Vasu (http://permalink.gmane.org/gmane.linux.scsi.open-fcoe.devel/10110)
> > for Fiber Channel adapter, the following patch on 2.6.35 kernel 
> > avoids taking the scsi host lock when queueing mptsas scsi command. We see
> > a big drop in the cpu cycles contending for the lock (from 23.7% to 1.8%).  
> > The number of IO per sec increase by 10.6% from 62.9K per sec to 69.6K per sec.
> > 
> > If there is no good reason to prevent mptsas_qcmd from being 
> > executed in parallel, we should remove this lock from the queue 
> > command code path.  Other adapters probably can
> > benefit in a similar manner.
> > 
> > 
> >                         %cpu cycles contending host lock
> >                         2.6.35		2.6.35+patch
> > -----------------------------------------------------
> > scsi_dispatch_cmd       5.5%		0.44%
> > scsi_device_unbusy      6.1%		0.66%
> > scsi_request_fn         6.6%		0.35%
> > scsi_run_queue          5.5%		0.35%
> > 
> > 
> 
> Hi Tim and Co,
> 
> Many Thanks for posting these very interesting numbers with
> unlocked_qcmds=1 + mpt-fusion SCSI LLD on a 64-core system..  Wow.. 8-)
> 

I echo same, thanks Tim for these detailed numbers.

> I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> convinced me that doing conditional locking while is very simple, is not
> the proper way for getting this resolved in mainline code.  I think in
> the end this will require a longer sit down to do a wholesale conversion
> of all existing SCSI LLD drivers, and identifing the broken ones that
> still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> whatever strange & legacy reasons.
> 

I think doing few LLDs first and resolving any new issues caused by no
host_lock in those LLD would have helped with wholesale conv, beside if
a simple change helps perf now then should be good to have till
wholesale change done. However I'm also fine jumping to wholesale
approach directly.

> While there are still some outstanding TCM items that need to be
> resolved in the next days, I am very interested to help make the
> wholesale host_lock + ->queuecomamnd() conversion happen.  I will get a
> lio-core-2.6.git branch setup for this purpose on .36-rc4 soon and start
> working on the main SCSI Mid-layer conversion pieces sometime next week.
> I am very eager to accept patches on a per LLD basis for this work, and
> will be starting with the open-fcoe initiator, TCM_Loop, mpt2sas, and
> open-iscsi.
> 
> I think the wholesole conversion is going to be pretty straight-forward,
> and at least with the main SCSI LLDs (that we really care about ;) there
> appear to be no immediate issues with a full conversion.
> 

Sounds good, I wish I could help with that now but won't be able to till
Dec since heading for sabbatical this week.

	Thanks 
	Vasu

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 23:25 +0200, Andi Kleen wrote:
> > I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> > convinced me that doing conditional locking while is very simple, is not
> > the proper way for getting this resolved in mainline code.  I think in
> > the end this will require a longer sit down to do a wholesale conversion
> > of all existing SCSI LLD drivers, and identifing the broken ones that
> > still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> > whatever strange & legacy reasons.
> 
> The standard way to do that would be to first move the lock down
> into the drivers (similar to how it has been done with the BKL).
> This would be a fairly mechanic mindless patch. Lots of typing,
> but not really a lot of real code review needed.
> 
> Then next step the drivers who know they don't want it can remove it.

Well, the main bit is finding those drives that actually call some form
of struct Scsi_Host->host_lock unlock() -> do_work() -> lock() from
within their own ->queuecomamnd() caller, and avoid running into the big
ugly deadlock that would happen here..

I know that Open-iSCSi and my own iSCSI Initiator do this
"optimization", but I am not sure which other LLDs also do this as
well..

Best,

--nab

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 14:34 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 23:25 +0200, Andi Kleen wrote:
> > > I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> > > convinced me that doing conditional locking while is very simple, is not
> > > the proper way for getting this resolved in mainline code.  I think in
> > > the end this will require a longer sit down to do a wholesale conversion
> > > of all existing SCSI LLD drivers, and identifing the broken ones that
> > > still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> > > whatever strange & legacy reasons.
> > 
> > The standard way to do that would be to first move the lock down
> > into the drivers (similar to how it has been done with the BKL).
> > This would be a fairly mechanic mindless patch. Lots of typing,
> > but not really a lot of real code review needed.
> > 
> > Then next step the drivers who know they don't want it can remove it.
> 
> Well, the main bit is finding those drives that actually call some form
> of struct Scsi_Host->host_lock unlock() -> do_work() -> lock() from
> within their own ->queuecomamnd() caller, and avoid running into the big
> ugly deadlock that would happen here..
> 
> I know that Open-iSCSi and my own iSCSI Initiator do this
> "optimization", but I am not sure which other LLDs also do this as
> well..
> 
> Best,
> 

Ok, spotted another one of these in drivers/scsi/lpfc/lpfc_scsi.c:lpfc_queuecommand():

	<SNIP>

        if (phba->cfg_poll & ENABLE_FCP_RING_POLLING) {
                spin_unlock(shost->host_lock);
                lpfc_sli_handle_fast_ring_event(phba,
                        &phba->sli.ring[LPFC_FCP_RING], HA_R0RE_REQ);

                spin_lock(shost->host_lock);
                if (phba->cfg_poll & DISABLE_FCP_RING_INT)
                        lpfc_poll_rearm_timer(phba);
        }

        return 0;

Best,

--nab

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 14:44 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 14:34 -0700, Nicholas A. Bellinger wrote:
> > On Thu, 2010-09-16 at 23:25 +0200, Andi Kleen wrote:
> > > > I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> > > > convinced me that doing conditional locking while is very simple, is not
> > > > the proper way for getting this resolved in mainline code.  I think in
> > > > the end this will require a longer sit down to do a wholesale conversion
> > > > of all existing SCSI LLD drivers, and identifing the broken ones that
> > > > still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> > > > whatever strange & legacy reasons.
> > > 
> > > The standard way to do that would be to first move the lock down
> > > into the drivers (similar to how it has been done with the BKL).
> > > This would be a fairly mechanic mindless patch. Lots of typing,
> > > but not really a lot of real code review needed.
> > > 
> > > Then next step the drivers who know they don't want it can remove it.
> > 
> > Well, the main bit is finding those drives that actually call some form
> > of struct Scsi_Host->host_lock unlock() -> do_work() -> lock() from
> > within their own ->queuecomamnd() caller, and avoid running into the big
> > ugly deadlock that would happen here..
> > 
> > I know that Open-iSCSi and my own iSCSI Initiator do this
> > "optimization", but I am not sure which other LLDs also do this as
> > well..
> > 
> > Best,
> > 
> 
> Ok, spotted another one of these in drivers/scsi/lpfc/lpfc_scsi.c:lpfc_queuecommand():
> 
> 	<SNIP>
> 
>         if (phba->cfg_poll & ENABLE_FCP_RING_POLLING) {
>                 spin_unlock(shost->host_lock);
>                 lpfc_sli_handle_fast_ring_event(phba,
>                         &phba->sli.ring[LPFC_FCP_RING], HA_R0RE_REQ);
> 
>                 spin_lock(shost->host_lock);
>                 if (phba->cfg_poll & DISABLE_FCP_RING_INT)
>                         lpfc_poll_rearm_timer(phba);
>         }
> 
>         return 0;
> 
> Best,

And ditto ->queuecommand() with qla2xxx and qla4xxx LLDs as well:

	<SNIP>

        spin_unlock_irq(ha->host->host_lock);

        srb = qla4xxx_get_new_srb(ha, ddb_entry, cmd, done);
        if (!srb)
                goto qc_host_busy_lock;

        rval = qla4xxx_send_command_to_isp(ha, srb);
        if (rval != QLA_SUCCESS)
                goto qc_host_busy_free_sp;

        spin_lock_irq(ha->host->host_lock);
        return 0;


So yeah, it looks like the high performance LLDs have adopted this trick
over the years..  :-)

Best,

--nab

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Joe Eykholt]

On 9/16/10 2:25 PM, Andi Kleen wrote:
>> I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
>> convinced me that doing conditional locking while is very simple, is not
>> the proper way for getting this resolved in mainline code.  I think in
>> the end this will require a longer sit down to do a wholesale conversion
>> of all existing SCSI LLD drivers, and identifing the broken ones that
>> still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
>> whatever strange & legacy reasons.
> 
> The standard way to do that would be to first move the lock down
> into the drivers (similar to how it has been done with the BKL).
> This would be a fairly mechanic mindless patch. Lots of typing,
> but not really a lot of real code review needed.
> 
> Then next step the drivers who know they don't want it can remove it.
> 
> -Andi

I see problems with this, but maybe I'm missing something.

It seems to me we can't completely move the host lock down into the
drivers since its a shared lock between SCSI and the drivers now.

If we just have SCSI drop the lock and have the LLD reacquire it,
that may open up a hole that some LLDs might not tolerate.
It also hurts performance for the LLDs that want to keep
the lock for the duration of queuecommand() by adding an extra
unlock/lock.

Obviously, some LLDs may depend on that shared lock being held,
without a drop just before the call.

So, I think the flag approach is OK, or wait until the wholesale
approach can be done.

	Regards,
	Joe

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[James Bottomley]

On Thu, 2010-09-16 at 15:00 -0700, Joe Eykholt wrote:
> On 9/16/10 2:25 PM, Andi Kleen wrote:
> >> I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> >> convinced me that doing conditional locking while is very simple, is not
> >> the proper way for getting this resolved in mainline code.  I think in
> >> the end this will require a longer sit down to do a wholesale conversion
> >> of all existing SCSI LLD drivers, and identifing the broken ones that
> >> still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> >> whatever strange & legacy reasons.
> > 
> > The standard way to do that would be to first move the lock down
> > into the drivers (similar to how it has been done with the BKL).
> > This would be a fairly mechanic mindless patch. Lots of typing,
> > but not really a lot of real code review needed.
> > 
> > Then next step the drivers who know they don't want it can remove it.
> > 
> > -Andi
> 
> I see problems with this, but maybe I'm missing something.
> 
> It seems to me we can't completely move the host lock down into the
> drivers since its a shared lock between SCSI and the drivers now.

Not really ... look at the code path (in scsi.c:scsi_dispatch_cmd()).
We take the lock, then get the serial number (that would likley have to
be replaced with an atomic), check the state, call trace, call
queuecommand and drop the lock.  That should be replaceable with a no
lock sequence.

For some of our lower latency drivers, we're also going to have to start
looking at the queue lock->host lock shuffle we do for I/O accounting.

James

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Tim Chen]

On Thu, 2010-09-16 at 15:00 -0700, Joe Eykholt wrote:

> 
> It seems to me we can't completely move the host lock down into the
> drivers since its a shared lock between SCSI and the drivers now.
> 
> If we just have SCSI drop the lock and have the LLD reacquire it,
> that may open up a hole that some LLDs might not tolerate.
> It also hurts performance for the LLDs that want to keep
> the lock for the duration of queuecommand() by adding an extra
> unlock/lock.


It may be even better if we can completely drop the lock in
scsi_dispatch_cmd.  The other reason host_lock was taken there was to
serialize the increment and assignment of a scsi command serial number.
So maybe we can make the serial number counter an atomic.

We can just let the driver decide if they need a lock to serialize their
queue command.

Tim

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Christoph Hellwig]

On Thu, Sep 16, 2010 at 05:24:47PM -0400, James Bottomley wrote:
> > into the drivers (similar to how it has been done with the BKL).
> > This would be a fairly mechanic mindless patch. Lots of typing,
> > but not really a lot of real code review needed.
> > 
> > Then next step the drivers who know they don't want it can remove it.
> 
> Yes, that's basically what Christoph did when he moved the lock out of
> the eh path.

And it is what we should do here as well.  I'm just wondering if we rely
on the fact that we hold the lock over the check for the device beeing
deleted and the queuecommand call.

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 19:25 -0400, Christoph Hellwig wrote:
> On Thu, Sep 16, 2010 at 05:24:47PM -0400, James Bottomley wrote:
> > > into the drivers (similar to how it has been done with the BKL).
> > > This would be a fairly mechanic mindless patch. Lots of typing,
> > > but not really a lot of real code review needed.
> > > 
> > > Then next step the drivers who know they don't want it can remove it.
> > 
> > Yes, that's basically what Christoph did when he moved the lock out of
> > the eh path.
> 
> And it is what we should do here as well.  I'm just wondering if we rely
> on the fact that we hold the lock over the check for the device beeing
> deleted and the queuecommand call.
> 

Ugh, I completely forgot the about the (host->shost_state == SHOST_DEL)
check in scsi_dispatch_cmd() in patch #1, and just fixed this in
lio-core-2.6.git/drop-host_lock here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=e222907d9baac56425f87c602acdd757dc5fde08

Here is what the updated scsi_dispatch_cmd() now looks like:

	<SNIP>

        spin_lock_irqsave(host->host_lock, flags);
        /*
         * AK: unlikely race here: for some reason the timer could
         * expire before the serial number is set up below.
         *
         * TODO: kill serial or move to blk layer
         */
        scsi_cmd_get_serial(host, cmd);

        if (unlikely(host->shost_state == SHOST_DEL)) {
                spin_unlock_irqrestore(host->host_lock, flags);
                cmd->result = (DID_NO_CONNECT << 16);
                scsi_done(cmd);
        } else {
                spin_unlock_irqrestore(host->host_lock, flags);
                trace_scsi_dispatch_cmd_start(cmd);
                rtn = host->hostt->queuecommand(cmd, scsi_done);
        }

	<SNIP>

Thanks!

--nab

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Vasu Dev]

On Thu, 2010-09-16 at 17:13 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 19:25 -0400, Christoph Hellwig wrote:
> > On Thu, Sep 16, 2010 at 05:24:47PM -0400, James Bottomley wrote:
> > > > into the drivers (similar to how it has been done with the BKL).
> > > > This would be a fairly mechanic mindless patch. Lots of typing,
> > > > but not really a lot of real code review needed.
> > > > 
> > > > Then next step the drivers who know they don't want it can remove it.
> > > 
> > > Yes, that's basically what Christoph did when he moved the lock out of
> > > the eh path.
> > 
> > And it is what we should do here as well.  I'm just wondering if we rely
> > on the fact that we hold the lock over the check for the device beeing
> > deleted and the queuecommand call.
> > 
> 
> Ugh, I completely forgot the about the (host->shost_state == SHOST_DEL)
> check in scsi_dispatch_cmd() in patch #1, and just fixed this in
> lio-core-2.6.git/drop-host_lock here:
> 
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=e222907d9baac56425f87c602acdd757dc5fde08
> 
> Here is what the updated scsi_dispatch_cmd() now looks like:
> 
> 	<SNIP>
> 
>         spin_lock_irqsave(host->host_lock, flags);
>         /*
>          * AK: unlikely race here: for some reason the timer could
>          * expire before the serial number is set up below.
>          *
>          * TODO: kill serial or move to blk layer
>          */
>         scsi_cmd_get_serial(host, cmd);
> 
>         if (unlikely(host->shost_state == SHOST_DEL)) {
>                 spin_unlock_irqrestore(host->host_lock, flags);
>                 cmd->result = (DID_NO_CONNECT << 16);
>                 scsi_done(cmd);
>         } else {
>                 spin_unlock_irqrestore(host->host_lock, flags);

This unconditional drop will require LLD still using host lock for their
queuecommand to re-acquire the lock and that would hurt their perf as
Joe mentioned in his response. Also this change requires all such LLD to
grab and drop lock again and that change should go along with this
change.

BTW above change is very similar to patch under discussion, just off by
tiny additional check :-)

   Vasu
>                 trace_scsi_dispatch_cmd_start(cmd);
>                 rtn = host->hostt->queuecommand(cmd, scsi_done);
>         }
> 
> 	<SNIP>
> 
> Thanks!
> 
> --nab
> 
> 
> --
> 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] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Andi Kleen]

> Not really ... look at the code path (in scsi.c:scsi_dispatch_cmd()).
> We take the lock, then get the serial number (that would likley have to
> be replaced with an atomic), check the state, call trace, call

An atomic unfortunately usually doesn't scale much better than a spinlock.
I suspect serials would need to be made optional, e.g. 
computing them lazily if really needed.

-andi

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[Bart Van Assche]

On Fri, Sep 17, 2010 at 9:16 AM, Andi Kleen <ak@linux.intel.com> wrote:
>
> > Not really ... look at the code path (in scsi.c:scsi_dispatch_cmd()).
> > We take the lock, then get the serial number (that would likley have to
> > be replaced with an atomic), check the state, call trace, call
>
> An atomic unfortunately usually doesn't scale much better than a spinlock.
> I suspect serials would need to be made optional, e.g.
> computing them lazily if really needed.

We should be careful that the command processing order for commands
issued by different threads is not altered by removing the host lock,
at least for those SCSI commands where in-order processing matters.
There might be better solutions than a serial number though.

Bart.

Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

[James Bottomley]

On Fri, 2010-09-17 at 12:32 +0200, Bart Van Assche wrote:
> On Fri, Sep 17, 2010 at 9:16 AM, Andi Kleen <ak@linux.intel.com> wrote:
> >
> > > Not really ... look at the code path (in scsi.c:scsi_dispatch_cmd()).
> > > We take the lock, then get the serial number (that would likley have to
> > > be replaced with an atomic), check the state, call trace, call
> >
> > An atomic unfortunately usually doesn't scale much better than a spinlock.
> > I suspect serials would need to be made optional, e.g.
> > computing them lazily if really needed.
> 
> We should be careful that the command processing order for commands
> issued by different threads is not altered by removing the host lock,
> at least for those SCSI commands where in-order processing matters.
> There might be better solutions than a serial number though.

We don't actually make any ordering guarantees at the top of the stack.
The block layer originally did need internal ordering guarantees for
barriers, but they were automatically preserved by the fact that the
exit from the ioscheduler is single threaded.

However, the barrier redo means that we no longer even need the single
threaded guarantee ... and I suspect Jens is already thinking about
multi threading the ioscheduler exit, which is also another good reason
for reducing the locking footprint.

James