[PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Nicholas A. Bellinger]

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

This patch converts the mpt2sas driver to run in host_lock less mode
with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
calling ->queuecommand() dispatch

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

diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 1a96a00..e564fe7 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
 }
 
 /**
- * _scsih_qcmd - main scsi request entry point
+ * _scsih_qcmd_irq_disable - main scsi request entry point
  * @scmd: pointer to scsi command object
  * @done: function pointer to be invoked on completion
  *
@@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
  * 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_irq_disable(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 {
 	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
 	struct MPT2SAS_DEVICE *sas_device_priv_data;
@@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 	return SCSI_MLQUEUE_HOST_BUSY;
 }
 
-static DEF_SCSI_QCMD(_scsih_qcmd)
+static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
 
 /**
  * _scsih_normalize_sense - normalize descriptor and fixed format sense data
-- 
1.7.2.3

[PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Nicholas A. Bellinger]

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

This patch converts the mpt2sas driver to run in host_lock less mode
with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
calling ->queuecommand() dispatch

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

diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 1a96a00..e564fe7 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
 }
 
 /**
- * _scsih_qcmd - main scsi request entry point
+ * _scsih_qcmd_irq_disable - main scsi request entry point
  * @scmd: pointer to scsi command object
  * @done: function pointer to be invoked on completion
  *
@@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
  * 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_irq_disable(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 {
 	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
 	struct MPT2SAS_DEVICE *sas_device_priv_data;
@@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 	return SCSI_MLQUEUE_HOST_BUSY;
 }
 
-static DEF_SCSI_QCMD(_scsih_qcmd)
+static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
 
 /**
  * _scsih_normalize_sense - normalize descriptor and fixed format sense data
-- 
1.7.2.3

Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Boaz Harrosh]

On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch converts the mpt2sas driver to run in host_lock less mode
> with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
> calling ->queuecommand() dispatch
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> index 1a96a00..e564fe7 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
>  }
>  
>  /**
> - * _scsih_qcmd - main scsi request entry point
> + * _scsih_qcmd_irq_disable - main scsi request entry point
>   * @scmd: pointer to scsi command object
>   * @done: function pointer to be invoked on completion
>   *
> @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
>   * 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_irq_disable(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
>  {
>  	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
>  	struct MPT2SAS_DEVICE *sas_device_priv_data;
> @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
>  	return SCSI_MLQUEUE_HOST_BUSY;
>  }
>  
> -static DEF_SCSI_QCMD(_scsih_qcmd)
> +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
>  

How can this (and other in the patchset) can be correct? I mean I expect
that if you remove the lock,xx_qcmd_lck,unlock then inside the xx_qcmd_lck
there was an unlock,do123,lock and that driver was effectively running lockless
before. (like in iscsi). But here this is new behaviour. If it is correct
I would like to see a statement from you that:
	"I have audited this driver, and all shared resources are protected by
         XYZ so ..."

Otherwise how can I know this is correct? I have never audited this driver myself

Thanks
Boaz

>  /**
>   * _scsih_normalize_sense - normalize descriptor and fixed format sense data

Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Nicholas A. Bellinger]

<Trimming CC list>

On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
> On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch converts the mpt2sas driver to run in host_lock less mode
> > with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
> > calling ->queuecommand() dispatch
> > 
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > index 1a96a00..e564fe7 100644
> > --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
> >  }
> >  
> >  /**
> > - * _scsih_qcmd - main scsi request entry point
> > + * _scsih_qcmd_irq_disable - main scsi request entry point
> >   * @scmd: pointer to scsi command object
> >   * @done: function pointer to be invoked on completion
> >   *
> > @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
> >   * 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_irq_disable(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
> >  {
> >  	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
> >  	struct MPT2SAS_DEVICE *sas_device_priv_data;
> > @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
> >  	return SCSI_MLQUEUE_HOST_BUSY;
> >  }
> >  
> > -static DEF_SCSI_QCMD(_scsih_qcmd)
> > +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
> >  
> 
> How can this (and other in the patchset) can be correct? I mean I expect
> that if you remove the lock,xx_qcmd_lck,unlock then inside the xx_qcmd_lck
> there was an unlock,do123,lock and that driver was effectively running lockless
> before. (like in iscsi). But here this is new behaviour. If it is correct
> I would like to see a statement from you that:
> 	"I have audited this driver, and all shared resources are protected by
>          XYZ so ..."
> 
> Otherwise how can I know this is correct? I have never audited this driver myself
> 

So for this specific mpt2sas case, Vasu Dev had been testing the
lock_less case w/o disabling interrupts for his original
SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable
during his JBOD lock_less small block IOP performance test.

This patch currently includes IRQ_DISABLE_SCSI_QCMD() usage which I am
not even sure is needed (according to Vasu's original findings), but I
decided to err on the conservative side until we can get some feedback
from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).

--nab

Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Jeff Garzik]

On 11/18/2010 05:57 PM, Nicholas A. Bellinger wrote:
> <Trimming CC list>
>
> On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
>> On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger<nab@linux-iscsi.org>
>>>
>>> This patch converts the mpt2sas driver to run in host_lock less mode
>>> with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
>>> calling ->queuecommand() dispatch
>>>
>>> Signed-off-by: Nicholas A. Bellinger<nab@linux-iscsi.org>
>>> ---
>>>   drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
>>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>>> index 1a96a00..e564fe7 100644
>>> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>>> @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
>>>   }
>>>
>>>   /**
>>> - * _scsih_qcmd - main scsi request entry point
>>> + * _scsih_qcmd_irq_disable - main scsi request entry point
>>>    * @scmd: pointer to scsi command object
>>>    * @done: function pointer to be invoked on completion
>>>    *
>>> @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
>>>    * 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_irq_disable(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
>>>   {
>>>   	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
>>>   	struct MPT2SAS_DEVICE *sas_device_priv_data;
>>> @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
>>>   	return SCSI_MLQUEUE_HOST_BUSY;
>>>   }
>>>
>>> -static DEF_SCSI_QCMD(_scsih_qcmd)
>>> +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
>>>
>>
>> How can this (and other in the patchset) can be correct? I mean I expect
>> that if you remove the lock,xx_qcmd_lck,unlock then inside the xx_qcmd_lck
>> there was an unlock,do123,lock and that driver was effectively running lockless
>> before. (like in iscsi). But here this is new behaviour. If it is correct
>> I would like to see a statement from you that:
>> 	"I have audited this driver, and all shared resources are protected by
>>           XYZ so ..."
>>
>> Otherwise how can I know this is correct? I have never audited this driver myself
>>
>
> So for this specific mpt2sas case, Vasu Dev had been testing the
> lock_less case w/o disabling interrupts for his original
> SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable
> during his JBOD lock_less small block IOP performance test.

Test results are not the same as reviewing and understanding the locking 
in the driver, and adjusting the code accordingly...

_scsih_qcmd_lck() seems to rely on per-device and per-host data not 
changing, which might seem to imply that the driver is secretly using 
the SCSI host_lock to guarantee access to members such as 
ioc->ioc_link_reset_in_progress or sas_target_priv_data->tm_busy.

These accesses appear racy in both lock-free and local_irq_save() 
configurations from my naive reading, though I do see additional locking 
inside mpt2sas_base_get_smid_scsiio()

	Jeff

Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Nicholas A. Bellinger]

On Thu, 2010-11-18 at 18:13 -0500, Jeff Garzik wrote:
> On 11/18/2010 05:57 PM, Nicholas A. Bellinger wrote:
> > <Trimming CC list>
> >
> > On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
> >> On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger<nab@linux-iscsi.org>
> >>>
> >>> This patch converts the mpt2sas driver to run in host_lock less mode
> >>> with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
> >>> calling ->queuecommand() dispatch
> >>>
> >>> Signed-off-by: Nicholas A. Bellinger<nab@linux-iscsi.org>
> >>> ---
> >>>   drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
> >>>   1 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>> index 1a96a00..e564fe7 100644
> >>> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>> @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
> >>>   }
> >>>
> >>>   /**
> >>> - * _scsih_qcmd - main scsi request entry point
> >>> + * _scsih_qcmd_irq_disable - main scsi request entry point
> >>>    * @scmd: pointer to scsi command object
> >>>    * @done: function pointer to be invoked on completion
> >>>    *
> >>> @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
> >>>    * 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_irq_disable(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
> >>>   {
> >>>   	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
> >>>   	struct MPT2SAS_DEVICE *sas_device_priv_data;
> >>> @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
> >>>   	return SCSI_MLQUEUE_HOST_BUSY;
> >>>   }
> >>>
> >>> -static DEF_SCSI_QCMD(_scsih_qcmd)
> >>> +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
> >>>
> >>
> >> How can this (and other in the patchset) can be correct? I mean I expect
> >> that if you remove the lock,xx_qcmd_lck,unlock then inside the xx_qcmd_lck
> >> there was an unlock,do123,lock and that driver was effectively running lockless
> >> before. (like in iscsi). But here this is new behaviour. If it is correct
> >> I would like to see a statement from you that:
> >> 	"I have audited this driver, and all shared resources are protected by
> >>           XYZ so ..."
> >>
> >> Otherwise how can I know this is correct? I have never audited this driver myself
> >>
> >
> > So for this specific mpt2sas case, Vasu Dev had been testing the
> > lock_less case w/o disabling interrupts for his original
> > SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable
> > during his JBOD lock_less small block IOP performance test.
> 
> Test results are not the same as reviewing and understanding the locking 
> in the driver, and adjusting the code accordingly...

Fair enough..

> 
> _scsih_qcmd_lck() seems to rely on per-device and per-host data not 
> changing, which might seem to imply that the driver is secretly using 
> the SCSI host_lock to guarantee access to members such as 
> ioc->ioc_link_reset_in_progress or sas_target_priv_data->tm_busy.
> 
> These accesses appear racy in both lock-free and local_irq_save() 
> configurations from my naive reading, though I do see additional locking 
> inside mpt2sas_base_get_smid_scsiio()
> 

Mmmm, so in that case I will go ahead and revert this mpt2sas patch in
lock_less-LLDs-for-38-v2 until this can be addressed by the LSI folks,
or by other folks who have an specific interest in mpt2sas being able to
run in lock_less mode for .38 code (Tim and Vasu..?)

Also, mpt2sas and message/fusion transport handlers are similar enough
that there are some likely issues there as well.  Thus far this other
patch to drivers/message/fusion/mptscsih.c has been stable in VMW guest
in my development testing, but this will need to be audited for
lock_less as well.

--nab

RE: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Desai, Kashyap]

> -----Original Message-----
> From: Nicholas A. Bellinger [mailto:nab@linux-iscsi.org]
> Sent: Friday, November 19, 2010 4:28 AM
> To: Boaz Harrosh
> Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike
> Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
> Subject: Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
> interrupts disabled externally
> 
> <Trimming CC list>
> 
> On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
> > On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > >
> > > This patch converts the mpt2sas driver to run in host_lock less
> mode
> > > with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
> > > calling ->queuecommand() dispatch
> > >
> > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > > ---
> > >  drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > index 1a96a00..e564fe7 100644
> > > --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> *scmd, u16 ioc_status)
> > >  }
> > >
> > >  /**
> > > - * _scsih_qcmd - main scsi request entry point
> > > + * _scsih_qcmd_irq_disable - main scsi request entry point
> > >   * @scmd: pointer to scsi command object
> > >   * @done: function pointer to be invoked on completion
> > >   *
> > > @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> *scmd, u16 ioc_status)
> > >   * 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_irq_disable(struct scsi_cmnd *scmd, void
> (*done)(struct scsi_cmnd *))
> > >  {
> > >  	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
> > >  	struct MPT2SAS_DEVICE *sas_device_priv_data;
> > > @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
> (*done)(struct scsi_cmnd *))
> > >  	return SCSI_MLQUEUE_HOST_BUSY;
> > >  }
> > >
> > > -static DEF_SCSI_QCMD(_scsih_qcmd)
> > > +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
> > >
> >
> > How can this (and other in the patchset) can be correct? I mean I
> expect
> > that if you remove the lock,xx_qcmd_lck,unlock then inside the
> xx_qcmd_lck
> > there was an unlock,do123,lock and that driver was effectively
> running lockless
> > before. (like in iscsi). But here this is new behaviour. If it is
> correct
> > I would like to see a statement from you that:
> > 	"I have audited this driver, and all shared resources are
> protected by
> >          XYZ so ..."
> >
> > Otherwise how can I know this is correct? I have never audited this
> driver myself
> >
> 
> So for this specific mpt2sas case, Vasu Dev had been testing the
> lock_less case w/o disabling interrupts for his original
> SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable
> during his JBOD lock_less small block IOP performance test.
> 
Nicholas, mpt2sas driver is safe even if in lock less condition.
I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen host lock push down code is available for mpt2sas driver 
I have seen static DEF_SCSI_QCMD(_scsih_qcmd) is available for mpt2sas driver. I also did testing removing that particular macro and making 
Mpt2sas driver in actual host lock less mode. Below observation is after removing static DEF_SCSI_QCMD(_scsih_qcmd) from mpt2sas (that is what I refer as actual host lock less mode)

I have tested stability of the driver and I/O integrity. Things were fine. I am not seeing any need for even going for IRQ_DISABLE_SCSI_QCMD() patch too.
Mpt2sas driver is able to handle host lock less mode seamlessly, since we have all required (racy) places under spinlocks.

I do not see any solid need for this patch. Please correct me if I am missing anything here.

~ Kashyap

> This patch currently includes IRQ_DISABLE_SCSI_QCMD() usage which I am
> not even sure is needed (according to Vasu's original findings), but I
> decided to err on the conservative side until we can get some feedback
> from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).
> 
> --nab
> 

Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Jeff Garzik]

On 11/19/2010 06:02 AM, Desai, Kashyap wrote:
>
>
>> -----Original Message-----
>> From: Nicholas A. Bellinger [mailto:nab@linux-iscsi.org]
>> Sent: Friday, November 19, 2010 4:28 AM
>> To: Boaz Harrosh
>> Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike
>> Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
>> Subject: Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
>> interrupts disabled externally
>>
>> <Trimming CC list>
>>
>> On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
>>> On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
>>>> From: Nicholas Bellinger<nab@linux-iscsi.org>
>>>>
>>>> This patch converts the mpt2sas driver to run in host_lock less
>> mode
>>>> with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
>>>> calling ->queuecommand() dispatch
>>>>
>>>> Signed-off-by: Nicholas A. Bellinger<nab@linux-iscsi.org>
>>>> ---
>>>>   drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
>>>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>> b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>>>> index 1a96a00..e564fe7 100644
>>>> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>>>> @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
>> *scmd, u16 ioc_status)
>>>>   }
>>>>
>>>>   /**
>>>> - * _scsih_qcmd - main scsi request entry point
>>>> + * _scsih_qcmd_irq_disable - main scsi request entry point
>>>>    * @scmd: pointer to scsi command object
>>>>    * @done: function pointer to be invoked on completion
>>>>    *
>>>> @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
>> *scmd, u16 ioc_status)
>>>>    * 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_irq_disable(struct scsi_cmnd *scmd, void
>> (*done)(struct scsi_cmnd *))
>>>>   {
>>>>   	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
>>>>   	struct MPT2SAS_DEVICE *sas_device_priv_data;
>>>> @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
>> (*done)(struct scsi_cmnd *))
>>>>   	return SCSI_MLQUEUE_HOST_BUSY;
>>>>   }
>>>>
>>>> -static DEF_SCSI_QCMD(_scsih_qcmd)
>>>> +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
>>>>
>>>
>>> How can this (and other in the patchset) can be correct? I mean I
>> expect
>>> that if you remove the lock,xx_qcmd_lck,unlock then inside the
>> xx_qcmd_lck
>>> there was an unlock,do123,lock and that driver was effectively
>> running lockless
>>> before. (like in iscsi). But here this is new behaviour. If it is
>> correct
>>> I would like to see a statement from you that:
>>> 	"I have audited this driver, and all shared resources are
>> protected by
>>>           XYZ so ..."
>>>
>>> Otherwise how can I know this is correct? I have never audited this
>> driver myself
>>>
>>
>> So for this specific mpt2sas case, Vasu Dev had been testing the
>> lock_less case w/o disabling interrupts for his original
>> SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable
>> during his JBOD lock_less small block IOP performance test.
>>
> Nicholas, mpt2sas driver is safe even if in lock less condition.
> I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen host lock push down code is available for mpt2sas driver
> I have seen static DEF_SCSI_QCMD(_scsih_qcmd) is available for mpt2sas driver. I also did testing removing that particular macro and making
> Mpt2sas driver in actual host lock less mode. Below observation is after removing static DEF_SCSI_QCMD(_scsih_qcmd) from mpt2sas (that is what I refer as actual host lock less mode)
>
> I have tested stability of the driver and I/O integrity. Things were fine. I am not seeing any need for even going for IRQ_DISABLE_SCSI_QCMD() patch too.
> Mpt2sas driver is able to handle host lock less mode seamlessly, since we have all required (racy) places under spinlocks.
>
> I do not see any solid need for this patch. Please correct me if I am missing anything here.

Which locks are held when setting or testing sas_target_priv_data->tm_busy?
ioc->ioc_link_reset_in_progress?
sas_target_priv_data->deleted?

Thanks,

	Jeff

RE: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Desai, Kashyap]

> -----Original Message-----
> From: Jeff Garzik [mailto:jgpobox@gmail.com] On Behalf Of Jeff Garzik
> Sent: Friday, November 19, 2010 10:09 PM
> To: Desai, Kashyap
> Cc: Nicholas A. Bellinger; Boaz Harrosh; linux-scsi; James Bottomley;
> Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion
> Linux
> Subject: Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
> interrupts disabled externally
> 
> On 11/19/2010 06:02 AM, Desai, Kashyap wrote:
> >
> >
> >> -----Original Message-----
> >> From: Nicholas A. Bellinger [mailto:nab@linux-iscsi.org]
> >> Sent: Friday, November 19, 2010 4:28 AM
> >> To: Boaz Harrosh
> >> Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig;
> Mike
> >> Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
> >> Subject: Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
> >> interrupts disabled externally
> >>
> >> <Trimming CC list>
> >>
> >> On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
> >>> On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> >>>> From: Nicholas Bellinger<nab@linux-iscsi.org>
> >>>>
> >>>> This patch converts the mpt2sas driver to run in host_lock less
> >> mode
> >>>> with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts
> while
> >>>> calling ->queuecommand() dispatch
> >>>>
> >>>> Signed-off-by: Nicholas A. Bellinger<nab@linux-iscsi.org>
> >>>> ---
> >>>>   drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
> >>>>   1 files changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >> b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>>> index 1a96a00..e564fe7 100644
> >>>> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>>> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>>> @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> >> *scmd, u16 ioc_status)
> >>>>   }
> >>>>
> >>>>   /**
> >>>> - * _scsih_qcmd - main scsi request entry point
> >>>> + * _scsih_qcmd_irq_disable - main scsi request entry point
> >>>>    * @scmd: pointer to scsi command object
> >>>>    * @done: function pointer to be invoked on completion
> >>>>    *
> >>>> @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> >> *scmd, u16 ioc_status)
> >>>>    * 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_irq_disable(struct scsi_cmnd *scmd, void
> >> (*done)(struct scsi_cmnd *))
> >>>>   {
> >>>>   	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device-
> >host);
> >>>>   	struct MPT2SAS_DEVICE *sas_device_priv_data;
> >>>> @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
> >> (*done)(struct scsi_cmnd *))
> >>>>   	return SCSI_MLQUEUE_HOST_BUSY;
> >>>>   }
> >>>>
> >>>> -static DEF_SCSI_QCMD(_scsih_qcmd)
> >>>> +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
> >>>>
> >>>
> >>> How can this (and other in the patchset) can be correct? I mean I
> >> expect
> >>> that if you remove the lock,xx_qcmd_lck,unlock then inside the
> >> xx_qcmd_lck
> >>> there was an unlock,do123,lock and that driver was effectively
> >> running lockless
> >>> before. (like in iscsi). But here this is new behaviour. If it is
> >> correct
> >>> I would like to see a statement from you that:
> >>> 	"I have audited this driver, and all shared resources are
> >> protected by
> >>>           XYZ so ..."
> >>>
> >>> Otherwise how can I know this is correct? I have never audited this
> >> driver myself
> >>>
> >>
> >> So for this specific mpt2sas case, Vasu Dev had been testing the
> >> lock_less case w/o disabling interrupts for his original
> >> SHT->unlocked_qcmd=1 patch, and from his comments this mode was
> stable
> >> during his JBOD lock_less small block IOP performance test.
> >>
> > Nicholas, mpt2sas driver is safe even if in lock less condition.
> > I have tested mpt2sas driver available in 2.6.37-rc2 where I have
> seen host lock push down code is available for mpt2sas driver
> > I have seen static DEF_SCSI_QCMD(_scsih_qcmd) is available for
> mpt2sas driver. I also did testing removing that particular macro and
> making
> > Mpt2sas driver in actual host lock less mode. Below observation is
> after removing static DEF_SCSI_QCMD(_scsih_qcmd) from mpt2sas (that is
> what I refer as actual host lock less mode)
> >
> > I have tested stability of the driver and I/O integrity. Things were
> fine. I am not seeing any need for even going for
> IRQ_DISABLE_SCSI_QCMD() patch too.
> > Mpt2sas driver is able to handle host lock less mode seamlessly,
> since we have all required (racy) places under spinlocks.
> >
> > I do not see any solid need for this patch. Please correct me if I am
> missing anything here.
> 
> Which locks are held when setting or testing sas_target_priv_data-
> >tm_busy?
> ioc->ioc_link_reset_in_progress?
> sas_target_priv_data->deleted?
> 
Jeff, tm_busy, ioc_link_reset_in_process and deleted field of respective data structure are not so critical. They are not exactly real-time, since they all are representing firmware state for the Host and Target. (It means there is always some open window when firmware state and driver data structure will not match)
e.a 
tm_busy is getting set to 1 when firmware sends "MPI2_EVENT_SAS_DEV_STAT_RC_INTERNAL_DEVICE_RESET". Even if we secure tm_busy read/write using some spinlock, it will not give any benefit, since there is already some delay when driver sets tm_busy and there is already some window in which few IOs will be sent to firmware.
But main reason for driver is to stop sending IOs to firmware when firmware is doing Task management. There is no harm if few commands go to firmware when task mangagement is running by firmware. (This is also unavoidable to not send single IO to firmware immediately after it sends MPI2_EVENT_SAS_DEV_STAT_RC_INTERNAL_DEVICE_RESET)

So my call is all three field of relative structure are not having any dependency with host_lock. They are additional sanity check before sending IOs to firmware and even if we sends few IOs in a small open windows (considering there is some open windows before setting variable at driver), it is fine. 


~ Kashyap


> Thanks,
> 
> 	Jeff
> 
> 

RE: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Desai, Kashyap]

Nicholas,

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.....)


So I would suggest to disable preemption also in your macro " IRQ_DISABLE_SCSI_QCMD " as below two level of prevention.
        local_irq_save(flags);
        preempt_disable();


mpt2sas parameters checks like ioc_link_reset_in_process,
tm_busy, scs_priv_target_data->deleted  are mainly to communicate device/IOC state of the firmware so there is no hard dependency w.r.t host_lock.


Thanks, Kashyap




> -----Original Message-----
> From: Desai, Kashyap
> Sent: Friday, November 19, 2010 4:33 PM
> To: Nicholas A. Bellinger; Boaz Harrosh
> Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike
> Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux
> Subject: RE: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
> interrupts disabled externally
> 
> 
> 
> > -----Original Message-----
> > From: Nicholas A. Bellinger [mailto:nab@linux-iscsi.org]
> > Sent: Friday, November 19, 2010 4:28 AM
> > To: Boaz Harrosh
> > Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike
> > Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
> > Subject: Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
> > interrupts disabled externally
> >
> > <Trimming CC list>
> >
> > On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
> > > On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > >
> > > > This patch converts the mpt2sas driver to run in host_lock less
> > mode
> > > > with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts
> while
> > > > calling ->queuecommand() dispatch
> > > >
> > > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > > > ---
> > > >  drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
> > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > > index 1a96a00..e564fe7 100644
> > > > --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > > +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > > @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> > *scmd, u16 ioc_status)
> > > >  }
> > > >
> > > >  /**
> > > > - * _scsih_qcmd - main scsi request entry point
> > > > + * _scsih_qcmd_irq_disable - main scsi request entry point
> > > >   * @scmd: pointer to scsi command object
> > > >   * @done: function pointer to be invoked on completion
> > > >   *
> > > > @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> > *scmd, u16 ioc_status)
> > > >   * 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_irq_disable(struct scsi_cmnd *scmd, void
> > (*done)(struct scsi_cmnd *))
> > > >  {
> > > >  	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device-
> >host);
> > > >  	struct MPT2SAS_DEVICE *sas_device_priv_data;
> > > > @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd,
> void
> > (*done)(struct scsi_cmnd *))
> > > >  	return SCSI_MLQUEUE_HOST_BUSY;
> > > >  }
> > > >
> > > > -static DEF_SCSI_QCMD(_scsih_qcmd)
> > > > +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
> > > >
> > >
> > > How can this (and other in the patchset) can be correct? I mean I
> > expect
> > > that if you remove the lock,xx_qcmd_lck,unlock then inside the
> > xx_qcmd_lck
> > > there was an unlock,do123,lock and that driver was effectively
> > running lockless
> > > before. (like in iscsi). But here this is new behaviour. If it is
> > correct
> > > I would like to see a statement from you that:
> > > 	"I have audited this driver, and all shared resources are
> > protected by
> > >          XYZ so ..."
> > >
> > > Otherwise how can I know this is correct? I have never audited this
> > driver myself
> > >
> >
> > So for this specific mpt2sas case, Vasu Dev had been testing the
> > lock_less case w/o disabling interrupts for his original
> > SHT->unlocked_qcmd=1 patch, and from his comments this mode was
> stable
> > during his JBOD lock_less small block IOP performance test.
> >
> Nicholas, mpt2sas driver is safe even if in lock less condition.
> I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen
> host lock push down code is available for mpt2sas driver
> I have seen static DEF_SCSI_QCMD(_scsih_qcmd) is available for mpt2sas
> driver. I also did testing removing that particular macro and making
> Mpt2sas driver in actual host lock less mode. Below observation is
> after removing static DEF_SCSI_QCMD(_scsih_qcmd) from mpt2sas (that is
> what I refer as actual host lock less mode)
> 
> I have tested stability of the driver and I/O integrity. Things were
> fine. I am not seeing any need for even going for
> IRQ_DISABLE_SCSI_QCMD() patch too.
> Mpt2sas driver is able to handle host lock less mode seamlessly, since
> we have all required (racy) places under spinlocks.
> 
> I do not see any solid need for this patch. Please correct me if I am
> missing anything here.
> 
> ~ Kashyap
> 
> > This patch currently includes IRQ_DISABLE_SCSI_QCMD() usage which I
> am
> > not even sure is needed (according to Vasu's original findings), but
> I
> > decided to err on the conservative side until we can get some
> feedback
> > from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).
> >
> > --nab
> >

RE: [PATCH 06/11] mpt2sas: Convert to host_lock less w/ interrupts disabled externally

[Nicholas A. Bellinger]

On Mon, 2010-12-20 at 20:34 +0530, Desai, Kashyap wrote:
> Nicholas,
> 

Hi Kashyap,

My apologies for the delayed response, as I have been AFK for the past
days..

> 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.
> 

Thank you for the clarification here.  

> 
> 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.....)
> 
> 

Hmmm, very good point.

> So I would suggest to disable preemption also in your macro " IRQ_DISABLE_SCSI_QCMD " as below two level of prevention.
>         local_irq_save(flags);
>         preempt_disable();
> 
> 

Ok, I have been asked to drop the IRQ_DISABLE_SCSI_QCMD() macro and open
code this for the handful of LLDs in the host_lock-less series.  I will
add the functional equivilent and disable preemption following your
recommendation in mpt2sas/mptfusion (and the other drivers using
IRQ_DISABLE_SCSI_QCMD) in the next round of lock_less-LLDs-for-38-v4.

> mpt2sas parameters checks like ioc_link_reset_in_process,
> tm_busy, scs_priv_target_data->deleted  are mainly to communicate device/IOC state of the firmware so there is no hard dependency w.r.t host_lock.
> 

Also understood.  Thanks for the feedback here and I will respin the
mpt2sas/mptfusion for your review next week.

Best Regards,

--nab

> 
> Thanks, Kashyap
> 
> 
> 
> 
> > -----Original Message-----
> > From: Desai, Kashyap
> > Sent: Friday, November 19, 2010 4:33 PM
> > To: Nicholas A. Bellinger; Boaz Harrosh
> > Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike
> > Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux
> > Subject: RE: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
> > interrupts disabled externally
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Nicholas A. Bellinger [mailto:nab@linux-iscsi.org]
> > > Sent: Friday, November 19, 2010 4:28 AM
> > > To: Boaz Harrosh
> > > Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike
> > > Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
> > > Subject: Re: [PATCH 06/11] mpt2sas: Convert to host_lock less w/
> > > interrupts disabled externally
> > >
> > > <Trimming CC list>
> > >
> > > On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
> > > > On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> > > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > >
> > > > > This patch converts the mpt2sas driver to run in host_lock less
> > > mode
> > > > > with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts
> > while
> > > > > calling ->queuecommand() dispatch
> > > > >
> > > > > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > > > > ---
> > > > >  drivers/scsi/mpt2sas/mpt2sas_scsih.c |    6 +++---
> > > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > > > index 1a96a00..e564fe7 100644
> > > > > --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > > > +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > > > > @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> > > *scmd, u16 ioc_status)
> > > > >  }
> > > > >
> > > > >  /**
> > > > > - * _scsih_qcmd - main scsi request entry point
> > > > > + * _scsih_qcmd_irq_disable - main scsi request entry point
> > > > >   * @scmd: pointer to scsi command object
> > > > >   * @done: function pointer to be invoked on completion
> > > > >   *
> > > > > @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd
> > > *scmd, u16 ioc_status)
> > > > >   * 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_irq_disable(struct scsi_cmnd *scmd, void
> > > (*done)(struct scsi_cmnd *))
> > > > >  {
> > > > >  	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device-
> > >host);
> > > > >  	struct MPT2SAS_DEVICE *sas_device_priv_data;
> > > > > @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd,
> > void
> > > (*done)(struct scsi_cmnd *))
> > > > >  	return SCSI_MLQUEUE_HOST_BUSY;
> > > > >  }
> > > > >
> > > > > -static DEF_SCSI_QCMD(_scsih_qcmd)
> > > > > +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
> > > > >
> > > >
> > > > How can this (and other in the patchset) can be correct? I mean I
> > > expect
> > > > that if you remove the lock,xx_qcmd_lck,unlock then inside the
> > > xx_qcmd_lck
> > > > there was an unlock,do123,lock and that driver was effectively
> > > running lockless
> > > > before. (like in iscsi). But here this is new behaviour. If it is
> > > correct
> > > > I would like to see a statement from you that:
> > > > 	"I have audited this driver, and all shared resources are
> > > protected by
> > > >          XYZ so ..."
> > > >
> > > > Otherwise how can I know this is correct? I have never audited this
> > > driver myself
> > > >
> > >
> > > So for this specific mpt2sas case, Vasu Dev had been testing the
> > > lock_less case w/o disabling interrupts for his original
> > > SHT->unlocked_qcmd=1 patch, and from his comments this mode was
> > stable
> > > during his JBOD lock_less small block IOP performance test.
> > >
> > Nicholas, mpt2sas driver is safe even if in lock less condition.
> > I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen
> > host lock push down code is available for mpt2sas driver
> > I have seen static DEF_SCSI_QCMD(_scsih_qcmd) is available for mpt2sas
> > driver. I also did testing removing that particular macro and making
> > Mpt2sas driver in actual host lock less mode. Below observation is
> > after removing static DEF_SCSI_QCMD(_scsih_qcmd) from mpt2sas (that is
> > what I refer as actual host lock less mode)
> > 
> > I have tested stability of the driver and I/O integrity. Things were
> > fine. I am not seeing any need for even going for
> > IRQ_DISABLE_SCSI_QCMD() patch too.
> > Mpt2sas driver is able to handle host lock less mode seamlessly, since
> > we have all required (racy) places under spinlocks.
> > 
> > I do not see any solid need for this patch. Please correct me if I am
> > missing anything here.
> > 
> > ~ Kashyap
> > 
> > > This patch currently includes IRQ_DISABLE_SCSI_QCMD() usage which I
> > am
> > > not even sure is needed (according to Vasu's original findings), but
> > I
> > > decided to err on the conservative side until we can get some
> > feedback
> > > from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).
> > >
> > > --nab
> > >
>