[PATCH] mpt2sas: Remove queuecommand wrapper

[PATCH] mpt2sas: Remove queuecommand wrapper

[Matthew Wilcox]

Now that mpt2sas no longer uses the serial_number, we can remove the
wrapper that acquires the host_lock around queuecommand.  This is the
most contended lock for one benchmark, and on a 8-socket machine this
patch leads to a 70% application-level improvement.

Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>

diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index a7dbc68..67708b3 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -3680,9 +3680,9 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd,
  * SCSI_MLQUEUE_HOST_BUSY if the entire host queue is full
  */
 static int
-_scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
+_scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
 {
-	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
+	struct MPT2SAS_ADAPTER *ioc = shost_priv(shost);
 	struct MPT2SAS_DEVICE *sas_device_priv_data;
 	struct MPT2SAS_TARGET *sas_target_priv_data;
 	struct _raid_device *raid_device;
@@ -3690,7 +3690,6 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 	u32 mpi_control;
 	u16 smid;
 
-	scmd->scsi_done = done;
 	sas_device_priv_data = scmd->device->hostdata;
 	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
 		scmd->result = DID_NO_CONNECT << 16;
@@ -3814,8 +3813,6 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 	return SCSI_MLQUEUE_HOST_BUSY;
 }
 
-static DEF_SCSI_QCMD(_scsih_qcmd)
-
 /**
  * _scsih_normalize_sense - normalize descriptor and fixed format sense data
  * @sense_buffer: sense data returned by target

RE: [PATCH] mpt2sas: Remove queuecommand wrapper

[Desai, Kashyap]

Matthew, I am fine with the patch, but just wanted to double check we do have included  changes related to below conversation.
Mpt2sas and mptsas driver wants IRQ and preemption disable when it enter in to qcmd callback.

http://140.211.166.79/mailarchive/linux-scsi/2010/12/23/6887919

~ Kashyap

> -----Original Message-----
> From: Matthew Wilcox [mailto:willy@linux.intel.com]
> Sent: Tuesday, July 05, 2011 12:57 AM
> To: linux-scsi@vger.kernel.org
> Cc: DL-MPT Fusion Linux
> Subject: [PATCH] mpt2sas: Remove queuecommand wrapper
> 
> 
> Now that mpt2sas no longer uses the serial_number, we can remove the
> wrapper that acquires the host_lock around queuecommand.  This is the
> most contended lock for one benchmark, and on a 8-socket machine this
> patch leads to a 70% application-level improvement.
> 
> Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>
> 
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> index a7dbc68..67708b3 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> @@ -3680,9 +3680,9 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER
> *ioc, struct scsi_cmnd *scmd,
>   * SCSI_MLQUEUE_HOST_BUSY if the entire host queue is full
>   */
>  static int
> -_scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd
> *))
> +_scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
>  {
> -	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
> +	struct MPT2SAS_ADAPTER *ioc = shost_priv(shost);
>  	struct MPT2SAS_DEVICE *sas_device_priv_data;
>  	struct MPT2SAS_TARGET *sas_target_priv_data;
>  	struct _raid_device *raid_device;
> @@ -3690,7 +3690,6 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
> (*done)(struct scsi_cmnd *))
>  	u32 mpi_control;
>  	u16 smid;
> 
> -	scmd->scsi_done = done;
>  	sas_device_priv_data = scmd->device->hostdata;
>  	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
>  		scmd->result = DID_NO_CONNECT << 16;
> @@ -3814,8 +3813,6 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
> (*done)(struct scsi_cmnd *))
>  	return SCSI_MLQUEUE_HOST_BUSY;
>  }
> 
> -static DEF_SCSI_QCMD(_scsih_qcmd)
> -
>  /**
>   * _scsih_normalize_sense - normalize descriptor and fixed format sense
> data
>   * @sense_buffer: sense data returned by target

Re: [PATCH] mpt2sas: Remove queuecommand wrapper

[Matthew Wilcox]

On Tue, Jul 05, 2011 at 10:16:25AM +0530, Desai, Kashyap wrote:
> Matthew, I am fine with the patch, but just wanted to double check we do have included  changes related to below conversation.
> Mpt2sas and mptsas driver wants IRQ and preemption disable when it enter in to qcmd callback.
> 
> http://140.211.166.79/mailarchive/linux-scsi/2010/12/23/6887919

The piece I believe you're referring to:

> I have gone through another round of code walkthrough (for mpt2sas and mptfusion) to \
> find out dependency w.r.t new host lock less mode. In my opinion, w/ interrupts \
> disable is a good ideal. (Earlier, I mentioned that mpt2sas is safe even if \
> interrupts are enabled)  In real scenario, *preemption* disable is more IMP for \
> mpt2sas/mptfusion driver than interrupts disable.

> e.a if scsih_qcmd_xxx has executed half of the code and (due to preemption is not \
> disable for the same CPU), Scheduler can execute any other process on the SAME cpu \
> (Though IRQ is disable). Consider Error handling is kicked off on the same CPU and as \
> part of EH, it executed HBA reset. As part of HBA reset Driver has return back all \
> pending Scsi command to mid-layer, and once control come back to original \
> scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data corruption/h/w hung \
> or anything critical.....)

I don't believe this scenario can happen.  Error handling will not
commence until all commands are returned to the midlayer, which requires
that queuecommand is no longer running.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

RE: [PATCH] mpt2sas: Remove queuecommand wrapper

[Desai, Kashyap]

________________________________________
From: Matthew Wilcox [matthew@wil.cx]
Sent: Tuesday, July 05, 2011 10:14 AM
To: Desai, Kashyap
Cc: Matthew Wilcox; linux-scsi@vger.kernel.org; DL-MPT Fusion Linux
Subject: Re: [PATCH] mpt2sas: Remove queuecommand wrapper

On Tue, Jul 05, 2011 at 10:16:25AM +0530, Desai, Kashyap wrote:
> Matthew, I am fine with the patch, but just wanted to double check we do have included  changes related to below conversation.
> Mpt2sas and mptsas driver wants IRQ and preemption disable when it enter in to qcmd callback.
>
> http://140.211.166.79/mailarchive/linux-scsi/2010/12/23/6887919

The piece I believe you're referring to:

YES. ! That is my concern..

> I have gone through another round of code walkthrough (for mpt2sas and mptfusion) to \
> find out dependency w.r.t new host lock less mode. In my opinion, w/ interrupts \
> disable is a good ideal. (Earlier, I mentioned that mpt2sas is safe even if \
> interrupts are enabled)  In real scenario, *preemption* disable is more IMP for \
> mpt2sas/mptfusion driver than interrupts disable.

> e.a if scsih_qcmd_xxx has executed half of the code and (due to preemption is not \
> disable for the same CPU), Scheduler can execute any other process on the SAME cpu \
> (Though IRQ is disable). Consider Error handling is kicked off on the same CPU and as \
> part of EH, it executed HBA reset. As part of HBA reset Driver has return back all \
> pending Scsi command to mid-layer, and once control come back to original \
> scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data corruption/h/w hung \
> or anything critical.....)

I don't believe this scenario can happen.  Error handling will not
commence until all commands are returned to the midlayer, which requires
that queuecommand is no longer running.

I was able to see this scenario when I intially started doing code review. Since host_lock is removed Error handling can remove code from queuecommand anytime ( I asssume preemption is not disable, only IRQ is disable).. I am almost sure if we are planning to remove host_lock from SML, we at least need irq and preemption disable for mpt2sas and mptfusion to work properly.

I am not sure we have done those changes in SML ? 



--
Matthew Wilcox                          Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] mpt2sas: Remove queuecommand wrapper

[Matthew Wilcox]

On Tue, Jul 05, 2011 at 08:25:43PM +0530, Desai, Kashyap wrote:
> > > e.a if scsih_qcmd_xxx has executed half of the code and (due to preemption is not \
> > > disable for the same CPU), Scheduler can execute any other process on the SAME cpu \
> > > (Though IRQ is disable). Consider Error handling is kicked off on the same CPU and as \
> > > part of EH, it executed HBA reset. As part of HBA reset Driver has return back all \
> > > pending Scsi command to mid-layer, and once control come back to original \
> > > scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data corruption/h/w hung \
> > > or anything critical.....)
> > 
> > I don't believe this scenario can happen.  Error handling will not
> > commence until all commands are returned to the midlayer, which requires
> > that queuecommand is no longer running.
> 
> I was able to see this scenario when I intially started doing code review. Since host_lock is removed Error handling can remove code from queuecommand anytime ( I asssume preemption is not disable, only IRQ is disable).. I am almost sure if we are planning to remove host_lock from SML, we at least need irq and preemption disable for mpt2sas and mptfusion to work properly.

I'm pretty sure that can't happen.  Look:

void scsi_eh_wakeup(struct Scsi_Host *shost)
{
        if (shost->host_busy == shost->host_failed) {
                trace_scsi_eh_wakeup(shost);
                wake_up_process(shost->ehandler);

So the SCSI error handler doesn't run until host_busy == host_failed.
host_busy is incremented in scsi_request_fn() (before scsi_dispatch_cmd
is called).  host_failed is incremented in scsi_eh_scmd_add().  So after
a command has been issued, it prevents the EH from running until it
too fails.  That means queuecommand cannot be running at the same time
as the EH.

That's documented in Documentation/scsi/scsi_eh.txt section [1-3].

RE: [PATCH] mpt2sas: Remove queuecommand wrapper

[Desai, Kashyap]

> -----Original Message-----
> From: Matthew Wilcox [mailto:willy@linux.intel.com]
> Sent: Tuesday, July 05, 2011 8:52 PM
> To: Desai, Kashyap
> Cc: Matthew Wilcox; linux-scsi@vger.kernel.org; DL-MPT Fusion Linux;
> nab@linux-iscsi.org
> Subject: Re: [PATCH] mpt2sas: Remove queuecommand wrapper
> 
> On Tue, Jul 05, 2011 at 08:25:43PM +0530, Desai, Kashyap wrote:
> > > > e.a if scsih_qcmd_xxx has executed half of the code and (due to
> preemption is not \
> > > > disable for the same CPU), Scheduler can execute any other process
> on the SAME cpu \
> > > > (Though IRQ is disable). Consider Error handling is kicked off on
> the same CPU and as \
> > > > part of EH, it executed HBA reset. As part of HBA reset Driver has
> return back all \
> > > > pending Scsi command to mid-layer, and once control come back to
> original \
> > > > scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data
> corruption/h/w hung \
> > > > or anything critical.....)
> > >
> > > I don't believe this scenario can happen.  Error handling will not
> > > commence until all commands are returned to the midlayer, which
> requires
> > > that queuecommand is no longer running.
> >
> > I was able to see this scenario when I intially started doing code
> review. Since host_lock is removed Error handling can remove code from
> queuecommand anytime ( I asssume preemption is not disable, only IRQ is
> disable).. I am almost sure if we are planning to remove host_lock from
> SML, we at least need irq and preemption disable for mpt2sas and
> mptfusion to work properly.
> 
> I'm pretty sure that can't happen.  Look:
> 
> void scsi_eh_wakeup(struct Scsi_Host *shost)
> {
>         if (shost->host_busy == shost->host_failed) {

I think you are right here. 

I did below experiment.  
Since now host_lock is removed from dispatch() callback.
I added sleep() inside scsih_qcmd().. Just to mimic preemption. And called Host reset using
"Sg_reset -h" at the sametime when driver is sleeping inside qcmd(). I see the kernel crash due to invalid scsi command pointer access after driver comes out from sleep().

 
As you said, due to actual Error handling will not wake up, but if user/customer do testing using sg_tools
They can kick off Host reset on demand.

What is your input on my above test case/observation ?

>                 trace_scsi_eh_wakeup(shost);
>                 wake_up_process(shost->ehandler);
> 
> So the SCSI error handler doesn't run until host_busy == host_failed.
> host_busy is incremented in scsi_request_fn() (before scsi_dispatch_cmd
> is called).  host_failed is incremented in scsi_eh_scmd_add().  So after
> a command has been issued, it prevents the EH from running until it
> too fails.  That means queuecommand cannot be running at the same time
> as the EH.
> 
> That's documented in Documentation/scsi/scsi_eh.txt section [1-3].

Re: [PATCH] mpt2sas: Remove queuecommand wrapper

[Matthew Wilcox]

On Tue, Jul 05, 2011 at 10:48:00PM +0530, Desai, Kashyap wrote:
> > -----Original Message-----
> > From: Matthew Wilcox [mailto:willy@linux.intel.com]
> > Sent: Tuesday, July 05, 2011 8:52 PM
> > To: Desai, Kashyap
> > Cc: Matthew Wilcox; linux-scsi@vger.kernel.org; DL-MPT Fusion Linux;
> > nab@linux-iscsi.org
> > Subject: Re: [PATCH] mpt2sas: Remove queuecommand wrapper

> > void scsi_eh_wakeup(struct Scsi_Host *shost)
> > {
> >         if (shost->host_busy == shost->host_failed) {
> 
> I think you are right here. 
> 
> I did below experiment.  
> Since now host_lock is removed from dispatch() callback.
> I added sleep() inside scsih_qcmd().. Just to mimic preemption. And called Host reset using
> "Sg_reset -h" at the sametime when driver is sleeping inside qcmd(). I see the kernel crash due to invalid scsi command pointer access after driver comes out from sleep().

OK, but I don't think we've introduced a new race here, just expanded
the window.

If you follow the path down from userspace, we get to
scsi_nonblockable_ioctl() (doesn't take the host_lock), then
scsi_reset_provider (which takes and releases the host_lock to
set tmf_in_progress), then scsi_try_host_reset() which calls
eh_host_reset_handler() without holding the host_lock.

So it's entirely possible to hit the same race on an SMP machine already
as there's nothing to synchronise the eh_host_reset_handler call with
queuecommand.

I think someone who uses sg_reset ought to be aware of the possibility
that they can trash their system.  Doug, what do you think?

> As you said, due to actual Error handling will not wake up, but if user/customer do testing using sg_tools
> They can kick off Host reset on demand.
> 
> What is your input on my above test case/observation ?
> 
> >                 trace_scsi_eh_wakeup(shost);
> >                 wake_up_process(shost->ehandler);
> > 
> > So the SCSI error handler doesn't run until host_busy == host_failed.
> > host_busy is incremented in scsi_request_fn() (before scsi_dispatch_cmd
> > is called).  host_failed is incremented in scsi_eh_scmd_add().  So after
> > a command has been issued, it prevents the EH from running until it
> > too fails.  That means queuecommand cannot be running at the same time
> > as the EH.
> > 
> > That's documented in Documentation/scsi/scsi_eh.txt section [1-3].

Re: [PATCH] mpt2sas: Remove queuecommand wrapper

[Dan Williams]

On Tue, Jul 5, 2011 at 1:24 PM, Matthew Wilcox <willy@linux.intel.com> wrote:
> On Tue, Jul 05, 2011 at 10:48:00PM +0530, Desai, Kashyap wrote:
>> I did below experiment.
>> Since now host_lock is removed from dispatch() callback.
>> I added sleep() inside scsih_qcmd().. Just to mimic preemption. And called Host reset using
>> "Sg_reset -h" at the sametime when driver is sleeping inside qcmd(). I see the kernel crash due to invalid scsi command pointer access after driver comes out from sleep().
>
> OK, but I don't think we've introduced a new race here, just expanded
> the window.
>
> If you follow the path down from userspace, we get to
> scsi_nonblockable_ioctl() (doesn't take the host_lock), then
> scsi_reset_provider (which takes and releases the host_lock to
> set tmf_in_progress), then scsi_try_host_reset() which calls
> eh_host_reset_handler() without holding the host_lock.
>
> So it's entirely possible to hit the same race on an SMP machine already
> as there's nothing to synchronise the eh_host_reset_handler call with
> queuecommand.
>
> I think someone who uses sg_reset ought to be aware of the possibility
> that they can trash their system.  Doug, what do you think?
>

Shouldn't the reset ioctl wait for all outstanding commands to return
to the midlayer before forcing the reset?  If all tmf routines are
written under scsi_eh (all outstanding commands quiesced) assumptions
it seems the ioctl path must make the same guarantees before
triggering a tmf action in a driver?
--
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