From: Nicholas Bellinger <[email protected]>
Greetings James and Co,
Attached are the current set of LLD host_lock-less conversions in the queue
for .38, that have now been pushed into lio-core-2.6.git/lock_less-LLDs-for-38-v3
New in this v3 series are the megaraid_sas LLD changes necessary to run in
host_lock-less mode, which have been tested thus far in legacy x86 interrupt
pin mode using 8708EM2 QEMU HBA emulation into KVM guest using TCM_Loop
virtual SCSI LUNs.
This series also drops the drivers/scsi patches that have been resolved with
the following (Thanks!)
commit 459dbf72e4d2b4aa13620e6b70d54f098547bf13
Author: James Bottomley <[email protected]>
Date: Wed Nov 17 10:10:57 2010 -0600
[SCSI] Eliminate error handler overload of the SCSI serial number
At this point the remaining code TODO items for the LLD conversion patches
include
*) libiscsi: NULL sc->scsi_done() callback in exception path in iscsi_queuecommand().
The last status on this from Mike Christie was:
"This will NULL pointer. See a couple lines above where we NULL it.
iscsi_free_task checks if the scsi_done pointer is set and if it is it
will call scsi_done.
It is a hack to prevent the normal completion path from calling
scsi_done. For the case where we return SCSI_MLQUEUE_TARGET_BUSY (the
prepd_reject case) we need something to prevent scsi_done from getting
called.
For the return 0/prepd_fault case we can just call sc->scsi_done, but we
have to move some code around."
mnc, have you been able to take another look at this..?
*) mpt2sas: Locking considerations for firmware specific data structures
A patch to mptsas was included originally to convert to host_lock less w/ interrupts
disabled externally here:
http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=87ba9071b46f37f1adab1732d41b87bf195d191f
but due to concerns from jgarzik this had been dropped from the lock_less-LLDs-for-38-v2
during the last round. The feedback from Kashyap @ LSI is that mpt2sas technically should be
able to run in host_lock-less, but there needs to be some explict clarification in the code
for the conversion to happen. More details are here:
http://marc.info/?l=linux-scsi&m=129022827729386&w=2
Kashyap, can you take another look at this and make sure this is acceptable..?
*) megaraid_sas: Running in host_lock-less mode with MSI-X interrupts
This has not been tested yet, but I would like to get some review from the LSI
megaraid_sas folks now that their patches to enable MSI-X interrupts has been sent
to linux-scsi.
AdamR, any thoughts or comments to add here..?
----------------------------------------------------------------------------------
So at this point the next steps will be to shaking out breakage in corner cases for
struct scsi_device hotplug removal, more testing+validation, and more LLD code review.
For the CC'ed LLD maintainers, please have another look at the changes to enable
host_lock-less operation within your code and send the necessary ACKs to linux-scsi
at your earliest convience. Many thanks to everyone who has contributed!
If anyone has any other patches, comments or concerns please let me know and they
will be queued up for the next v4 for-38 round.
Best Regards,
Signed-off-by: Nicholas A. Bellinger <[email protected]>
Nicholas Bellinger (12):
libiscsi: Convert to host_lock less w/ interrupts disabled internally
scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
libsas: Convert to host_lock less w/ interrupts disabled externally
message: Convert to host_lock less w/ interrupts disabled externally
fnic: Convert to host_lock less w/ interrupts disabled externally
lpfc: Convert to host_lock less w/ interrupts disabled externally
qla2xxx: Convert to host_lock less w/ interrupts disabled externally
qla4xxx: Convert to host_lock less w/ interrupts disabled externally
scsi_debug: Convert to host_lock less
megaraid_sas: Add smp_mb__after_atomic_*() for
instance->fw_outstanding
megaraid_sas: Convert instance->issuepend_done to atomic_t
megaraid_sas: Convert SHT->queuecommand() to run host_lock-less
drivers/message/fusion/mptfc.c | 2 +-
drivers/message/fusion/mptsas.c | 6 +-
drivers/message/fusion/mptscsih.c | 8 ++-
drivers/message/fusion/mptscsih.h | 2 +-
drivers/message/fusion/mptspi.c | 6 +-
drivers/scsi/fnic/fnic_scsi.c | 14 +----
drivers/scsi/libiscsi.c | 10 +---
drivers/scsi/libsas/sas_scsi_host.c | 12 +----
drivers/scsi/lpfc/lpfc_scsi.c | 9 ++--
drivers/scsi/megaraid/megaraid_sas.c | 85 ++++++++++++++++++++++------------
drivers/scsi/megaraid/megaraid_sas.h | 2 +-
drivers/scsi/qla2xxx/qla_os.c | 11 +---
drivers/scsi/qla4xxx/ql4_os.c | 10 +---
drivers/scsi/scsi_debug.c | 16 +++----
include/scsi/scsi_host.h | 14 ++++++
15 files changed, 107 insertions(+), 100 deletions(-)
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts the libsas queuecommand to run in host_lock less mode
w/ the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling
->queuecommand() dispatch.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/libsas/sas_scsi_host.c | 12 ++----------
1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 29251fa..011580f 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -185,24 +185,17 @@ int sas_queue_up(struct sas_task *task)
/**
* sas_queuecommand -- Enqueue a command for processing
* @parameters: See SCSI Core documentation
- *
- * Note: XXX: Remove the host unlock/lock pair when SCSI Core can
- * call us without holding an IRQ spinlock...
*/
-static int sas_queuecommand_lck(struct scsi_cmnd *cmd,
+static int sas_queuecommand_irq_disable(struct scsi_cmnd *cmd,
void (*scsi_done)(struct scsi_cmnd *))
- __releases(host->host_lock)
__acquires(dev->sata_dev.ap->lock)
__releases(dev->sata_dev.ap->lock)
- __acquires(host->host_lock)
{
int res = 0;
struct domain_device *dev = cmd_to_domain_dev(cmd);
struct Scsi_Host *host = cmd->device->host;
struct sas_internal *i = to_sas_internal(host->transportt);
- spin_unlock_irq(host->host_lock);
-
{
struct sas_ha_struct *sas_ha = dev->port->ha;
struct sas_task *task;
@@ -250,11 +243,10 @@ static int sas_queuecommand_lck(struct scsi_cmnd *cmd,
}
}
out:
- spin_lock_irq(host->host_lock);
return res;
}
-DEF_SCSI_QCMD(sas_queuecommand)
+IRQ_DISABLE_SCSI_QCMD(sas_queuecommand)
static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
{
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch adds a IRQ_DISABLE_SCSI_QCMD() wrapper macro used by LLDs
that can now run in host_lock less mode, but still need interrupts disabled
using local_irq_save() before calling their lld_queuecommand() dispatcher.
jgarzik says this method is in fact slower than doing a spin_lock_irqsave() on
internal lib_lld_queuecommand() callers (as is done in libiscsi and libata)
but is still needed by the majority of lock_less LLDs.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
include/scsi/scsi_host.h | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e7e3858..321b114 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -521,6 +521,20 @@ struct scsi_host_template {
return rc; \
}
+/*
+ * Used for LLDs running in lock-less mode, but that still require having
+ * interrupts disable during their lld_queuecommand() dispatch.
+ */
+#define IRQ_DISABLE_SCSI_QCMD(func_name) \
+ int func_name(struct Scsi_Host *shost, struct scsi_cmnd *cmd) \
+ { \
+ unsigned long irq_flags; \
+ int rc; \
+ local_irq_save(irq_flags); \
+ rc = func_name##_irq_disable(cmd, cmd->scsi_done); \
+ local_irq_restore(irq_flags); \
+ return rc; \
+ }
/*
* shost state: If you alter this, you also need to alter scsi_sysfs.c
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts the message/fusion mptsas, mptscsih and mptspi
to run in host_lock less mode with the new IRQ_DISABLE_SCSI_QCMD()
that disables interrupts while calling ->queuecommand() dispatch.
Note that mptfc.c is left out as it checks libfc rport state, which
for the moment still needs the host lock.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/message/fusion/mptfc.c | 2 +-
drivers/message/fusion/mptsas.c | 6 +++---
drivers/message/fusion/mptscsih.c | 8 +++++---
drivers/message/fusion/mptscsih.h | 2 +-
drivers/message/fusion/mptspi.c | 6 +++---
5 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/message/fusion/mptfc.c b/drivers/message/fusion/mptfc.c
index d784c36..349b3c7 100644
--- a/drivers/message/fusion/mptfc.c
+++ b/drivers/message/fusion/mptfc.c
@@ -677,7 +677,7 @@ mptfc_qcmd_lck(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
return 0;
}
- return mptscsih_qcmd(SCpnt,done);
+ return mptscsih_qcmd(SCpnt->device->host, SCpnt);
}
static DEF_SCSI_QCMD(mptfc_qcmd)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index d48c2c6..f0d205e 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -1889,7 +1889,7 @@ mptsas_slave_alloc(struct scsi_device *sdev)
}
static int
-mptsas_qcmd_lck(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
+mptsas_qcmd_irq_disable(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
{
MPT_SCSI_HOST *hd;
MPT_ADAPTER *ioc;
@@ -1910,10 +1910,10 @@ mptsas_qcmd_lck(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
if (ioc->debug_level & MPT_DEBUG_SCSI)
scsi_print_command(SCpnt);
- return mptscsih_qcmd(SCpnt,done);
+ return mptscsih_qcmd(SCpnt->device->host, SCpnt);
}
-static DEF_SCSI_QCMD(mptsas_qcmd)
+static IRQ_DISABLE_SCSI_QCMD(mptsas_qcmd)
/**
* mptsas_mptsas_eh_timed_out - resets the scsi_cmnd timeout
diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
index 59b8f53..4ff996e 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -1383,7 +1383,7 @@ mptscsih_proc_info(struct Scsi_Host *host, char *buffer, char **start, off_t off
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
/**
- * mptscsih_qcmd - Primary Fusion MPT SCSI initiator IO start routine.
+ * mptscsih_qcmd_irq_disable - Primary Fusion MPT SCSI initiator IO start routine.
* @SCpnt: Pointer to scsi_cmnd structure
* @done: Pointer SCSI mid-layer IO completion function
*
@@ -1393,8 +1393,8 @@ mptscsih_proc_info(struct Scsi_Host *host, char *buffer, char **start, off_t off
*
* Returns 0. (rtn value discarded by linux scsi mid-layer)
*/
-int
-mptscsih_qcmd(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
+static int
+mptscsih_qcmd_irq_disable(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
{
MPT_SCSI_HOST *hd;
MPT_FRAME_HDR *mf;
@@ -1529,6 +1529,8 @@ mptscsih_qcmd(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
return SCSI_MLQUEUE_HOST_BUSY;
}
+IRQ_DISABLE_SCSI_QCMD(mptscsih_qcmd);
+
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
/*
* mptscsih_freeChainBuffers - Function to free chain buffers associated
diff --git a/drivers/message/fusion/mptscsih.h b/drivers/message/fusion/mptscsih.h
index 45a5ff3..1815aef 100644
--- a/drivers/message/fusion/mptscsih.h
+++ b/drivers/message/fusion/mptscsih.h
@@ -113,7 +113,7 @@ extern int mptscsih_resume(struct pci_dev *pdev);
#endif
extern int mptscsih_proc_info(struct Scsi_Host *host, char *buffer, char **start, off_t offset, int length, int func);
extern const char * mptscsih_info(struct Scsi_Host *SChost);
-extern int mptscsih_qcmd(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *));
+extern int mptscsih_qcmd(struct Scsi_Host *sh, struct scsi_cmnd *SCpnt);
extern int mptscsih_IssueTaskMgmt(MPT_SCSI_HOST *hd, u8 type, u8 channel,
u8 id, int lun, int ctx2abort, ulong timeout);
extern void mptscsih_slave_destroy(struct scsi_device *device);
diff --git a/drivers/message/fusion/mptspi.c b/drivers/message/fusion/mptspi.c
index 6d9568d..53eec57 100644
--- a/drivers/message/fusion/mptspi.c
+++ b/drivers/message/fusion/mptspi.c
@@ -780,7 +780,7 @@ static int mptspi_slave_configure(struct scsi_device *sdev)
}
static int
-mptspi_qcmd_lck(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
+mptspi_qcmd_irq_disable(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
{
struct _MPT_SCSI_HOST *hd = shost_priv(SCpnt->device->host);
VirtDevice *vdevice = SCpnt->device->hostdata;
@@ -802,10 +802,10 @@ mptspi_qcmd_lck(struct scsi_cmnd *SCpnt, void (*done)(struct scsi_cmnd *))
if (spi_dv_pending(scsi_target(SCpnt->device)))
ddvprintk(ioc, scsi_print_command(SCpnt));
- return mptscsih_qcmd(SCpnt,done);
+ return mptscsih_qcmd(SCpnt->device->host, SCpnt);
}
-static DEF_SCSI_QCMD(mptspi_qcmd)
+static IRQ_DISABLE_SCSI_QCMD(mptspi_qcmd)
static void mptspi_slave_destroy(struct scsi_device *sdev)
{
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts lpfc to run in host_lock less mode with the new
IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
dispatch. It also drops the legacy host_lock unlock optimization around
lpfc_sli_handle_fast_ring_event().
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/lpfc/lpfc_scsi.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index 581837b..62a62e9 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -2886,7 +2886,7 @@ void lpfc_poll_timeout(unsigned long ptr)
}
/**
- * lpfc_queuecommand - scsi_host_template queuecommand entry point
+ * lpfc_queuecommand_irq_disable - scsi_host_template queuecommand entry point
* @cmnd: Pointer to scsi_cmnd data structure.
* @done: Pointer to done routine.
*
@@ -2899,7 +2899,8 @@ void lpfc_poll_timeout(unsigned long ptr)
* SCSI_MLQUEUE_HOST_BUSY - Block all devices served by this host temporarily.
**/
static int
-lpfc_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done) (struct scsi_cmnd *))
+lpfc_queuecommand_irq_disable(struct scsi_cmnd *cmnd,
+ void (*done) (struct scsi_cmnd *))
{
struct Scsi_Host *shost = cmnd->device->host;
struct lpfc_vport *vport = (struct lpfc_vport *) shost->hostdata;
@@ -3038,11 +3039,9 @@ lpfc_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done) (struct scsi_cmnd *))
goto out_host_busy_free_buf;
}
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);
}
@@ -3060,7 +3059,7 @@ lpfc_queuecommand_lck(struct scsi_cmnd *cmnd, void (*done) (struct scsi_cmnd *))
return 0;
}
-static DEF_SCSI_QCMD(lpfc_queuecommand)
+static IRQ_DISABLE_SCSI_QCMD(lpfc_queuecommand)
/**
* lpfc_abort_handler - scsi_host_template eh_abort_handler entry point
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts scsi_debug to run in host_lock less mode with interrupts
enabled, as we don't have a underly HW reason to use IRQ_DISABLE_SCSI_QCMD().
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/scsi_debug.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index c499eb3..71cd1fa 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3538,7 +3538,7 @@ static void sdebug_remove_adapter(void)
}
static
-int scsi_debug_queuecommand_lck(struct scsi_cmnd *SCpnt, done_funct_t done)
+int scsi_debug_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *SCpnt)
{
unsigned char *cmd = (unsigned char *) SCpnt->cmnd;
int len, k;
@@ -3566,17 +3566,17 @@ int scsi_debug_queuecommand_lck(struct scsi_cmnd *SCpnt, done_funct_t done)
if (target == SCpnt->device->host->hostt->this_id) {
printk(KERN_INFO "scsi_debug: initiator's id used as "
"target!\n");
- return schedule_resp(SCpnt, NULL, done,
+ return schedule_resp(SCpnt, NULL, SCpnt->scsi_done,
DID_NO_CONNECT << 16, 0);
}
if ((SCpnt->device->lun >= scsi_debug_max_luns) &&
(SCpnt->device->lun != SAM2_WLUN_REPORT_LUNS))
- return schedule_resp(SCpnt, NULL, done,
+ return schedule_resp(SCpnt, NULL, SCpnt->scsi_done,
DID_NO_CONNECT << 16, 0);
devip = devInfoReg(SCpnt->device);
if (NULL == devip)
- return schedule_resp(SCpnt, NULL, done,
+ return schedule_resp(SCpnt, NULL, SCpnt->scsi_done,
DID_NO_CONNECT << 16, 0);
if ((scsi_debug_every_nth != 0) &&
@@ -3610,8 +3610,8 @@ int scsi_debug_queuecommand_lck(struct scsi_cmnd *SCpnt, done_funct_t done)
mk_sense_buffer(devip, ILLEGAL_REQUEST,
INVALID_OPCODE, 0);
errsts = check_condition_result;
- return schedule_resp(SCpnt, devip, done, errsts,
- 0);
+ return schedule_resp(SCpnt, devip, SCpnt->scsi_done,
+ errsts, 0);
}
}
@@ -3885,12 +3885,10 @@ xdwrite_read:
errsts = check_condition_result;
break;
}
- return schedule_resp(SCpnt, devip, done, errsts,
+ return schedule_resp(SCpnt, devip, SCpnt->scsi_done, errsts,
(delay_override ? 0 : scsi_debug_delay));
}
-static DEF_SCSI_QCMD(scsi_debug_queuecommand)
-
static struct scsi_host_template sdebug_driver_template = {
.proc_info = scsi_debug_proc_info,
.proc_name = sdebug_proc_name,
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch adds four missing smp_mb__after_atomic_[inc,dec] barriers
for atomic_inc() and atomic_dec() usage with instance->fw_outstanding
within the main I/O dispatcher megasas_queue_command_lck(), completion
callback megasas_complete_cmd() and struct megasas_instance HW reset
queue drain in megasas_issue_pending_cmds_again().
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/megaraid/megaraid_sas.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c
index 7451bc0..31c5419 100644
--- a/drivers/scsi/megaraid/megaraid_sas.c
+++ b/drivers/scsi/megaraid/megaraid_sas.c
@@ -1398,6 +1398,7 @@ megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd
* Issue the command to the FW
*/
atomic_inc(&instance->fw_outstanding);
+ smp_mb__after_atomic_inc();
instance->instancet->fire_cmd(instance, cmd->frame_phys_addr,
cmd->frame_count-1, instance->reg_set);
@@ -2068,6 +2069,7 @@ megasas_complete_cmd(struct megasas_instance *instance, struct megasas_cmd *cmd,
if (exception) {
atomic_dec(&instance->fw_outstanding);
+ smp_mb__after_atomic_dec();
scsi_dma_unmap(cmd->scmd);
cmd->scmd->scsi_done(cmd->scmd);
@@ -2116,6 +2118,7 @@ megasas_complete_cmd(struct megasas_instance *instance, struct megasas_cmd *cmd,
}
atomic_dec(&instance->fw_outstanding);
+ smp_mb__after_atomic_dec();
scsi_dma_unmap(cmd->scmd);
cmd->scmd->scsi_done(cmd->scmd);
@@ -2219,6 +2222,7 @@ megasas_issue_pending_cmds_again(struct megasas_instance *instance)
cmd, cmd->scmd->cmnd[0], cmd->scmd->serial_number);
atomic_inc(&instance->fw_outstanding);
+ smp_mb__after_atomic_inc();
instance->instancet->fire_cmd(instance,
cmd->frame_phys_addr,
cmd->frame_count-1, instance->reg_set);
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts fnic to run in host_lock less mode with the new
IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
dispatch. It also drops the legacy host_lock unlock optimization.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/fnic/fnic_scsi.c | 14 +++-----------
1 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 22d0240..9f51beb 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -349,7 +349,8 @@ static inline int fnic_queue_wq_copy_desc(struct fnic *fnic,
* Routine to send a scsi cdb
* Called with host_lock held and interrupts disabled.
*/
-static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
+static int fnic_queuecommand_irq_disable(struct scsi_cmnd *sc,
+ void (*done)(struct scsi_cmnd *))
{
struct fc_lport *lp;
struct fc_rport *rport;
@@ -373,13 +374,6 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
if (lp->state != LPORT_ST_READY || !(lp->link_up))
return SCSI_MLQUEUE_HOST_BUSY;
- /*
- * Release host lock, use driver resource specific locks from here.
- * Don't re-enable interrupts in case they were disabled prior to the
- * caller disabling them.
- */
- spin_unlock(lp->host->host_lock);
-
/* Get a new io_req for this SCSI IO */
fnic = lport_priv(lp);
@@ -452,12 +446,10 @@ static int fnic_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_
}
}
out:
- /* acquire host lock before returning to SCSI */
- spin_lock(lp->host->host_lock);
return ret;
}
-DEF_SCSI_QCMD(fnic_queuecommand)
+IRQ_DISABLE_SCSI_QCMD(fnic_queuecommand)
/*
* fnic_fcpio_fw_reset_cmpl_handler
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts qla2xxx to run in host_lock less mode with the new
IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
dispatch. It also drops the legacy host_lock unlock optimization.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/qla2xxx/qla_os.c | 11 +++--------
1 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 2c0876c..c6cdfeb 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -534,7 +534,8 @@ qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
}
static int
-qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
+qla2xxx_queuecommand_irq_disable(struct scsi_cmnd *cmd,
+ void (*done)(struct scsi_cmnd *))
{
scsi_qla_host_t *vha = shost_priv(cmd->device->host);
fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
@@ -544,7 +545,6 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
srb_t *sp;
int rval;
- spin_unlock_irq(vha->host->host_lock);
if (ha->flags.eeh_busy) {
if (ha->flags.pci_channel_io_perm_failure)
cmd->result = DID_NO_CONNECT << 16;
@@ -585,8 +585,6 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
if (rval != QLA_SUCCESS)
goto qc24_host_busy_free_sp;
- spin_lock_irq(vha->host->host_lock);
-
return 0;
qc24_host_busy_free_sp:
@@ -594,21 +592,18 @@ qc24_host_busy_free_sp:
mempool_free(sp, ha->srb_mempool);
qc24_host_busy_lock:
- spin_lock_irq(vha->host->host_lock);
return SCSI_MLQUEUE_HOST_BUSY;
qc24_target_busy:
- spin_lock_irq(vha->host->host_lock);
return SCSI_MLQUEUE_TARGET_BUSY;
qc24_fail_command:
- spin_lock_irq(vha->host->host_lock);
done(cmd);
return 0;
}
-static DEF_SCSI_QCMD(qla2xxx_queuecommand)
+static IRQ_DISABLE_SCSI_QCMD(qla2xxx_queuecommand)
/*
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts megasas_queue_command_lck -> to megasas_queue_command
running in modern host_lock-less mode. This includes running using
struct megasas_instance->hba_lock to disable interrupts around existing
callers in megasas_queue_command(). In the existing code the
megasas_instance->hba_lock is taken to disable interrupts for
*) Setup of megasas_cmd for scsi_cmnd descriptor in megasas_get_cmd()
*) Issuing megasas_cmd to firmware via instance->instancet->fire_cmd()
Along with commit 1cb8d64a19c and 0fbb4eb3b to make the following
megasas_instance members use proper atomic_t values:
*) fw_issuepend_done: used to signal SCSI_MLQUEUE_HOST_BUSY during
->queuecommand(), and used during shutdown)
*) fw_outstanding: used to track number outstanding megasas_cmd
descriptors
The one part of the code not originaly held, but has now been converted
to hold instance->hba_lock w/ interrupts disabled megasas_build_ldio()
and megasas_build_dcdb(). This part of code was originally protected
by Scsi_Host->host_lock disabling interrupts.
This allows for megaraid_sas to run in host_lock less dispatch
w/ disabling interrupts around internal megasas_instance->hba_lock.
So far this is functioning with QEMU Megasas 8708EM2 HBA emulation
with SG_IO into KVM Guest from TCM_Loop backstores on a .37-rc3 KVM
x86_64 Host.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/megaraid/megaraid_sas.c | 68 ++++++++++++++++++++++-----------
1 files changed, 45 insertions(+), 23 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c
index da0d3e3..db3075d 100644
--- a/drivers/scsi/megaraid/megaraid_sas.c
+++ b/drivers/scsi/megaraid/megaraid_sas.c
@@ -122,19 +122,17 @@ megasas_complete_cmd(struct megasas_instance *instance, struct megasas_cmd *cmd,
u8 alt_status);
/**
- * megasas_get_cmd - Get a command from the free pool
+ * __megasas_get_cmd - Get a command from the free pool
* @instance: Adapter soft state
*
* Returns a free command from the pool
+ * Locking: Called with instance->cmd_pool_lock w/ spin_lock_irqsave
*/
-static struct megasas_cmd *megasas_get_cmd(struct megasas_instance
- *instance)
+static struct megasas_cmd *
+__megasas_get_cmd(struct megasas_instance *instance)
{
- unsigned long flags;
struct megasas_cmd *cmd = NULL;
- spin_lock_irqsave(&instance->cmd_pool_lock, flags);
-
if (!list_empty(&instance->cmd_pool)) {
cmd = list_entry((&instance->cmd_pool)->next,
struct megasas_cmd, list);
@@ -143,7 +141,25 @@ static struct megasas_cmd *megasas_get_cmd(struct megasas_instance
printk(KERN_ERR "megasas: Command pool empty!\n");
}
+ return cmd;
+}
+
+/**
+ * megasas_get_cmd - Get a command from the free pool
+ * @instance: Adapter soft state
+ *
+ * Returns a free command from the pool
+ */
+static struct megasas_cmd *
+megasas_get_cmd(struct megasas_instance *instance)
+{
+ struct megasas_cmd *cmd;
+ unsigned long flags;
+
+ spin_lock_irqsave(&instance->cmd_pool_lock, flags);
+ cmd = __megasas_get_cmd(instance);
spin_unlock_irqrestore(&instance->cmd_pool_lock, flags);
+
return cmd;
}
@@ -1332,9 +1348,18 @@ megasas_dump_pending_frames(struct megasas_instance *instance)
* megasas_queue_command - Queue entry point
* @scmd: SCSI command to be queued
* @done: Callback entry point
+ *
+ * This is the main Linux/SCSI per I/O cmd dispatcher that is running
+ * in 'host_lock-less' mode w/o the legacy Scsi_Host->host_Lock held,
+ * and with IRQs enabled.
+ *
+ * This means that interaction with FW requires the instance->hba_lock
+ * be held with interrupts disabled, or that other megasas_instance
+ * reference must be done in an atomic fashion in modern host_lock-less
+ * mode.
*/
static int
-megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd *))
+megasas_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *scmd)
{
u32 frame_count;
struct megasas_cmd *cmd;
@@ -1347,23 +1372,18 @@ megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd
if (atomic_read(&instance->fw_issuepend_done) == 0)
return SCSI_MLQUEUE_HOST_BUSY;
- spin_lock_irqsave(&instance->hba_lock, flags);
- if (instance->adprecovery != MEGASAS_HBA_OPERATIONAL) {
- spin_unlock_irqrestore(&instance->hba_lock, flags);
- return SCSI_MLQUEUE_HOST_BUSY;
- }
-
- spin_unlock_irqrestore(&instance->hba_lock, flags);
-
- scmd->scsi_done = done;
- scmd->result = 0;
-
if (MEGASAS_IS_LOGICAL(scmd) &&
(scmd->device->id >= MEGASAS_MAX_LD || scmd->device->lun)) {
scmd->result = DID_BAD_TARGET << 16;
goto out_done;
}
+ spin_lock_irqsave(&instance->hba_lock, flags);
+ if (instance->adprecovery != MEGASAS_HBA_OPERATIONAL) {
+ spin_unlock_irqrestore(&instance->hba_lock, flags);
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
+
switch (scmd->cmnd[0]) {
case SYNCHRONIZE_CACHE:
/*
@@ -1371,14 +1391,17 @@ megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd
* No need to send it down
*/
scmd->result = DID_OK << 16;
+ spin_unlock_irqrestore(&instance->hba_lock, flags);
goto out_done;
default:
break;
}
- cmd = megasas_get_cmd(instance);
- if (!cmd)
+ cmd = __megasas_get_cmd(instance);
+ if (!cmd) {
+ spin_unlock_irqrestore(&instance->hba_lock, flags);
return SCSI_MLQUEUE_HOST_BUSY;
+ }
/*
* Logical drive command
@@ -1387,6 +1410,7 @@ megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd
frame_count = megasas_build_ldio(instance, scmd, cmd);
else
frame_count = megasas_build_dcdb(instance, scmd, cmd);
+ spin_unlock_irqrestore(&instance->hba_lock, flags);
if (!frame_count)
goto out_return_cmd;
@@ -1414,12 +1438,10 @@ megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd
out_return_cmd:
megasas_return_cmd(instance, cmd);
out_done:
- done(scmd);
+ scmd->scsi_done(scmd);
return 0;
}
-static DEF_SCSI_QCMD(megasas_queue_command)
-
static struct megasas_instance *megasas_lookup_instance(u16 host_no)
{
int i;
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch changes iscsi_queuecommand_lck() to a host_lock less
iscsi_queuecommand() that will internally disable interrupts using
session->lock and drop the now legacy host_lock unlock.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/libiscsi.c | 10 ++--------
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index c15fde8..5535782 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1599,7 +1599,7 @@ enum {
FAILURE_SESSION_NOT_READY,
};
-static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
+int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
{
struct iscsi_cls_session *cls_session;
struct Scsi_Host *host;
@@ -1609,13 +1609,11 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
struct iscsi_conn *conn;
struct iscsi_task *task = NULL;
- sc->scsi_done = done;
sc->result = 0;
sc->SCp.ptr = NULL;
host = sc->device->host;
ihost = shost_priv(host);
- spin_unlock(host->host_lock);
cls_session = starget_to_session(scsi_target(sc->device));
session = cls_session->dd_data;
@@ -1706,7 +1704,6 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
session->queued_cmdsn++;
spin_unlock(&session->lock);
- spin_lock(host->host_lock);
return 0;
prepd_reject:
@@ -1716,7 +1713,6 @@ reject:
spin_unlock(&session->lock);
ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
sc->cmnd[0], reason);
- spin_lock(host->host_lock);
return SCSI_MLQUEUE_TARGET_BUSY;
prepd_fault:
@@ -1732,12 +1728,10 @@ fault:
scsi_out(sc)->resid = scsi_out(sc)->length;
scsi_in(sc)->resid = scsi_in(sc)->length;
}
- done(sc);
- spin_lock(host->host_lock);
+ sc->scsi_done(sc);
return 0;
}
-DEF_SCSI_QCMD(iscsi_queuecommand)
EXPORT_SYMBOL_GPL(iscsi_queuecommand);
int iscsi_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts qla4xxx to run in host_lock less mode with the new
IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
dispatch. It also drops the legacy host_lock unlock optimization.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/qla4xxx/ql4_os.c | 10 +++-------
1 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 0d48fb4..d21eba7 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -450,7 +450,7 @@ void qla4xxx_srb_compl(struct kref *ref)
}
/**
- * qla4xxx_queuecommand - scsi layer issues scsi command to driver.
+ * qla4xxx_queuecommand_irq_disable - scsi layer issues scsi command to driver.
* @cmd: Pointer to Linux's SCSI command structure
* @done_fn: Function that the driver calls to notify the SCSI mid-layer
* that the command has been processed.
@@ -463,7 +463,7 @@ void qla4xxx_srb_compl(struct kref *ref)
* completion handling). Unfortunely, it sometimes calls the scheduler
* in interrupt context which is a big NO! NO!.
**/
-static int qla4xxx_queuecommand_lck(struct scsi_cmnd *cmd,
+static int qla4xxx_queuecommand_irq_disable(struct scsi_cmnd *cmd,
void (*done)(struct scsi_cmnd *))
{
struct scsi_qla_host *ha = to_qla_host(cmd->device->host);
@@ -508,8 +508,6 @@ static int qla4xxx_queuecommand_lck(struct scsi_cmnd *cmd,
test_bit(DPC_RESET_HA_FW_CONTEXT, &ha->dpc_flags))
goto qc_host_busy;
- spin_unlock_irq(ha->host->host_lock);
-
srb = qla4xxx_get_new_srb(ha, ddb_entry, cmd, done);
if (!srb)
goto qc_host_busy_lock;
@@ -518,7 +516,6 @@ static int qla4xxx_queuecommand_lck(struct scsi_cmnd *cmd,
if (rval != QLA_SUCCESS)
goto qc_host_busy_free_sp;
- spin_lock_irq(ha->host->host_lock);
return 0;
qc_host_busy_free_sp:
@@ -526,7 +523,6 @@ qc_host_busy_free_sp:
mempool_free(srb, ha->srb_mempool);
qc_host_busy_lock:
- spin_lock_irq(ha->host->host_lock);
qc_host_busy:
return SCSI_MLQUEUE_HOST_BUSY;
@@ -537,7 +533,7 @@ qc_fail_command:
return 0;
}
-static DEF_SCSI_QCMD(qla4xxx_queuecommand)
+static IRQ_DISABLE_SCSI_QCMD(qla4xxx_queuecommand)
/**
* qla4xxx_mem_free - frees memory allocated to adapter
--
1.7.3.4
From: Nicholas Bellinger <[email protected]>
This patch converts struct megasas_instance->issuepend_done to
an atomic_t fw_issuepend_done. This is because ->issuepend_done is
being used as a check to determine if SCSI_MLQUEUE_HOST_BUSY should
be returned during megasas_queue_command_lck(). So in order for a
conversion to lock-less operation to occur, this bit needs to be
handled in an atomic manner.
Signed-off-by: Nicholas A. Bellinger <[email protected]>
---
drivers/scsi/megaraid/megaraid_sas.c | 13 ++++++-------
drivers/scsi/megaraid/megaraid_sas.h | 2 +-
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c
index 31c5419..da0d3e3 100644
--- a/drivers/scsi/megaraid/megaraid_sas.c
+++ b/drivers/scsi/megaraid/megaraid_sas.c
@@ -1344,7 +1344,7 @@ megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd
instance = (struct megasas_instance *)
scmd->device->host->hostdata;
- if (instance->issuepend_done == 0)
+ if (atomic_read(&instance->fw_issuepend_done) == 0)
return SCSI_MLQUEUE_HOST_BUSY;
spin_lock_irqsave(&instance->hba_lock, flags);
@@ -1590,8 +1590,7 @@ void megasas_do_ocr(struct megasas_instance *instance)
}
instance->instancet->disable_intr(instance->reg_set);
instance->adprecovery = MEGASAS_ADPRESET_SM_INFAULT;
- instance->issuepend_done = 0;
-
+ atomic_set(&instance->fw_issuepend_done, 0);
atomic_set(&instance->fw_outstanding, 0);
megasas_internal_reset_defer_cmds(instance);
process_fw_state_change_wq(&instance->work_init);
@@ -1941,7 +1940,7 @@ megasas_service_aen(struct megasas_instance *instance, struct megasas_cmd *cmd)
megasas_return_cmd(instance, cmd);
if ((instance->unload == 0) &&
- ((instance->issuepend_done == 1))) {
+ ((atomic_read(&instance->fw_issuepend_done) == 1))) {
struct megasas_aen_event *ev;
ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
if (!ev) {
@@ -2357,7 +2356,7 @@ process_fw_state_change_wq(struct work_struct *work)
instance->instancet->enable_intr(instance->reg_set);
megasas_issue_pending_cmds_again(instance);
- instance->issuepend_done = 1;
+ atomic_set(&instance->fw_issuepend_done, 1);
}
return ;
}
@@ -2417,8 +2416,8 @@ megasas_deplete_reply_queue(struct megasas_instance *instance,
instance->instancet->disable_intr(instance->reg_set);
instance->adprecovery = MEGASAS_ADPRESET_SM_INFAULT;
- instance->issuepend_done = 0;
+ atomic_set(&instance->fw_issuepend_done, 0);
atomic_set(&instance->fw_outstanding, 0);
megasas_internal_reset_defer_cmds(instance);
@@ -3798,7 +3797,7 @@ megasas_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
megasas_poll_wait_aen = 0;
instance->flag_ieee = 0;
instance->ev = NULL;
- instance->issuepend_done = 1;
+ atomic_set(&instance->fw_issuepend_done, 1);
instance->adprecovery = MEGASAS_HBA_OPERATIONAL;
megasas_poll_wait_aen = 0;
diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index ad16f5e..eaeb873 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -1303,6 +1303,7 @@ struct megasas_instance {
atomic_t fw_outstanding;
atomic_t fw_reset_no_pci_access;
+ atomic_t fw_issuepend_done;
struct megasas_instance_template *instancet;
struct tasklet_struct isr_tasklet;
@@ -1311,7 +1312,6 @@ struct megasas_instance {
u8 flag;
u8 unload;
u8 flag_ieee;
- u8 issuepend_done;
u8 disableOnlineCtrlReset;
u8 adprecovery;
unsigned long last_time;
--
1.7.3.4
On Sun, Dec 19, 2010 at 01:22:02PM -0800, Nicholas A. Bellinger wrote:
> This patch converts qla2xxx to run in host_lock less mode with the new
> IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
> dispatch. It also drops the legacy host_lock unlock optimization.
I'm not sure this is the right direction to go. Now that Jeff's done
the pushdown and put in the compatibility macros, I don't think it makes
sense to do another partial transition on each driver. Much better to
take our time, analyse each driver thoroughly, and kill the DEF_SCSI_QCMD
in each driver without introducing IRQ_DISABLE_SCSI_QCMD.
In particular for this driver, it explicitly re-enables interrupts,
so it's pretty easy to do a full conversion. Compile-tested only.
Convert qla2xxx driver to run without the shost lock
Signed-off-by: Matthew Wilcox <[email protected]>
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 2c0876c..b44d986 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -513,7 +513,7 @@ qla24xx_fw_version_str(struct scsi_qla_host *vha, char *str)
static inline srb_t *
qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
- struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
+ struct scsi_cmnd *cmd)
{
srb_t *sp;
struct qla_hw_data *ha = vha->hw;
@@ -527,16 +527,15 @@ qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
sp->cmd = cmd;
sp->flags = 0;
CMD_SP(cmd) = (void *)sp;
- cmd->scsi_done = done;
sp->ctx = NULL;
return sp;
}
static int
-qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
+qla2xxx_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
{
- scsi_qla_host_t *vha = shost_priv(cmd->device->host);
+ scsi_qla_host_t *vha = shost_priv(shost);
fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
struct fc_rport *rport = starget_to_rport(scsi_target(cmd->device));
struct qla_hw_data *ha = vha->hw;
@@ -544,7 +543,6 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
srb_t *sp;
int rval;
- spin_unlock_irq(vha->host->host_lock);
if (ha->flags.eeh_busy) {
if (ha->flags.pci_channel_io_perm_failure)
cmd->result = DID_NO_CONNECT << 16;
@@ -577,39 +575,32 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
goto qc24_target_busy;
}
- sp = qla2x00_get_new_sp(base_vha, fcport, cmd, done);
+ sp = qla2x00_get_new_sp(base_vha, fcport, cmd);
if (!sp)
- goto qc24_host_busy_lock;
+ goto qc24_host_busy;
rval = ha->isp_ops->start_scsi(sp);
if (rval != QLA_SUCCESS)
goto qc24_host_busy_free_sp;
- spin_lock_irq(vha->host->host_lock);
-
return 0;
qc24_host_busy_free_sp:
qla2x00_sp_free_dma(sp);
mempool_free(sp, ha->srb_mempool);
-qc24_host_busy_lock:
- spin_lock_irq(vha->host->host_lock);
+qc24_host_busy:
return SCSI_MLQUEUE_HOST_BUSY;
qc24_target_busy:
- spin_lock_irq(vha->host->host_lock);
return SCSI_MLQUEUE_TARGET_BUSY;
qc24_fail_command:
- spin_lock_irq(vha->host->host_lock);
- done(cmd);
+ cmd->scsi_done(cmd);
return 0;
}
-static DEF_SCSI_QCMD(qla2xxx_queuecommand)
-
/*
* qla2x00_eh_wait_on_command
--
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."
On Sun, Dec 19, 2010 at 01:21:56PM -0800, Nicholas A. Bellinger wrote:
> This patch changes iscsi_queuecommand_lck() to a host_lock less
> iscsi_queuecommand() that will internally disable interrupts using
> session->lock and drop the now legacy host_lock unlock.
I think this patch is buggy. Before, iscsi_queuecommand_lck is called with
interrupts disabled. Now, it's called with interrupts disabled. Elsewhere,
session->lock is acquired with bh's disabled:
void iscsi_put_task(struct iscsi_task *task)
{
struct iscsi_session *session = task->conn->session;
spin_lock_bh(&session->lock);
So I think you need to convert the
spin_lock(&session->lock);
in iscsi_queuecommand_lck to at least a spin_lock_bh, and possibly
a spin_lock_irq -- it's not clear to me whether it needs to exclude
against interrupt context, or only BH context. In some places, it's
taken with spin_lock_irqsave().
I'd play it safe and use _irq, but someone more confident with this code
might choose to only use _bh.
> Signed-off-by: Nicholas A. Bellinger <[email protected]>
> ---
> drivers/scsi/libiscsi.c | 10 ++--------
> 1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index c15fde8..5535782 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1599,7 +1599,7 @@ enum {
> FAILURE_SESSION_NOT_READY,
> };
>
> -static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
> +int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
> {
> struct iscsi_cls_session *cls_session;
> struct Scsi_Host *host;
> @@ -1609,13 +1609,11 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
> struct iscsi_conn *conn;
> struct iscsi_task *task = NULL;
>
> - sc->scsi_done = done;
> sc->result = 0;
> sc->SCp.ptr = NULL;
>
> host = sc->device->host;
> ihost = shost_priv(host);
> - spin_unlock(host->host_lock);
>
> cls_session = starget_to_session(scsi_target(sc->device));
> session = cls_session->dd_data;
> @@ -1706,7 +1704,6 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
>
> session->queued_cmdsn++;
> spin_unlock(&session->lock);
> - spin_lock(host->host_lock);
> return 0;
>
> prepd_reject:
> @@ -1716,7 +1713,6 @@ reject:
> spin_unlock(&session->lock);
> ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
> sc->cmnd[0], reason);
> - spin_lock(host->host_lock);
> return SCSI_MLQUEUE_TARGET_BUSY;
>
> prepd_fault:
> @@ -1732,12 +1728,10 @@ fault:
> scsi_out(sc)->resid = scsi_out(sc)->length;
> scsi_in(sc)->resid = scsi_in(sc)->length;
> }
> - done(sc);
> - spin_lock(host->host_lock);
> + sc->scsi_done(sc);
> return 0;
> }
>
> -DEF_SCSI_QCMD(iscsi_queuecommand)
> EXPORT_SYMBOL_GPL(iscsi_queuecommand);
>
> int iscsi_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
> --
> 1.7.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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."
On 12/19/2010 06:11 PM, Matthew Wilcox wrote:
> I'm not sure this is the right direction to go. Now that Jeff's done
> the pushdown and put in the compatibility macros, I don't think it makes
> sense to do another partial transition on each driver. Much better to
> take our time, analyse each driver thoroughly, and kill the DEF_SCSI_QCMD
> in each driver without introducing IRQ_DISABLE_SCSI_QCMD.
Agreed... DEF_SCSI_QCMD was intended, as you note, to encourage a
piecemeal, in-depth approach to cleaning up the rest of the drivers.
Hopefully DEF_SCSI_QCMD removal means someone actually figured out [or
already knew] the locking for a driver, and created the most appropriate
patch, rather than another half-step.
Jeff
On Sun, 2010-12-19 at 16:11 -0700, Matthew Wilcox wrote:
> On Sun, Dec 19, 2010 at 01:22:02PM -0800, Nicholas A. Bellinger wrote:
> > This patch converts qla2xxx to run in host_lock less mode with the new
> > IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
> > dispatch. It also drops the legacy host_lock unlock optimization.
>
> I'm not sure this is the right direction to go. Now that Jeff's done
> the pushdown and put in the compatibility macros, I don't think it makes
> sense to do another partial transition on each driver. Much better to
> take our time, analyse each driver thoroughly, and kill the DEF_SCSI_QCMD
> in each driver without introducing IRQ_DISABLE_SCSI_QCMD.
Yes, the LLDs using IRQ_DISABLE_SCSI_QCMD in this series are the ones
that we collectively know can be looked at for further optimization to
use spin_lock_irq() around a LLD dependent lock to realize the extra
benefit of not using local_irq_save(). The conversion of DEF_SCSI_QCMD
-> IRQ_DISABLE_SCSI_QCMD is to signal this explictly to the other LLD
folks that for the move to fully lock-less operation, that they want to
be looking at what libiscsi, megaraid_sas, scsi_debug, tcm_loop and your
qla2xxx patch is doing.. ;)
>
> In particular for this driver, it explicitly re-enables interrupts,
> so it's pretty easy to do a full conversion. Compile-tested only.
>
Great, I will give this a shot with ISP25xx series HW and get this added
as a incremental patch for -v3 branch, and ammended into the qla2xxx LLD
patch for v4.
Thanks Matthew!
--nab
> Convert qla2xxx driver to run without the shost lock
>
> Signed-off-by: Matthew Wilcox <[email protected]>
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 2c0876c..b44d986 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -513,7 +513,7 @@ qla24xx_fw_version_str(struct scsi_qla_host *vha, char *str)
>
> static inline srb_t *
> qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> - struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> + struct scsi_cmnd *cmd)
> {
> srb_t *sp;
> struct qla_hw_data *ha = vha->hw;
> @@ -527,16 +527,15 @@ qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> sp->cmd = cmd;
> sp->flags = 0;
> CMD_SP(cmd) = (void *)sp;
> - cmd->scsi_done = done;
> sp->ctx = NULL;
>
> return sp;
> }
>
> static int
> -qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> +qla2xxx_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> {
> - scsi_qla_host_t *vha = shost_priv(cmd->device->host);
> + scsi_qla_host_t *vha = shost_priv(shost);
> fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
> struct fc_rport *rport = starget_to_rport(scsi_target(cmd->device));
> struct qla_hw_data *ha = vha->hw;
> @@ -544,7 +543,6 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> srb_t *sp;
> int rval;
>
> - spin_unlock_irq(vha->host->host_lock);
> if (ha->flags.eeh_busy) {
> if (ha->flags.pci_channel_io_perm_failure)
> cmd->result = DID_NO_CONNECT << 16;
> @@ -577,39 +575,32 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> goto qc24_target_busy;
> }
>
> - sp = qla2x00_get_new_sp(base_vha, fcport, cmd, done);
> + sp = qla2x00_get_new_sp(base_vha, fcport, cmd);
> if (!sp)
> - goto qc24_host_busy_lock;
> + goto qc24_host_busy;
>
> rval = ha->isp_ops->start_scsi(sp);
> if (rval != QLA_SUCCESS)
> goto qc24_host_busy_free_sp;
>
> - spin_lock_irq(vha->host->host_lock);
> -
> return 0;
>
> qc24_host_busy_free_sp:
> qla2x00_sp_free_dma(sp);
> mempool_free(sp, ha->srb_mempool);
>
> -qc24_host_busy_lock:
> - spin_lock_irq(vha->host->host_lock);
> +qc24_host_busy:
> return SCSI_MLQUEUE_HOST_BUSY;
>
> qc24_target_busy:
> - spin_lock_irq(vha->host->host_lock);
> return SCSI_MLQUEUE_TARGET_BUSY;
>
> qc24_fail_command:
> - spin_lock_irq(vha->host->host_lock);
> - done(cmd);
> + cmd->scsi_done(cmd);
>
> return 0;
> }
>
> -static DEF_SCSI_QCMD(qla2xxx_queuecommand)
> -
>
> /*
> * qla2x00_eh_wait_on_command
>
On Sun, 2010-12-19 at 16:38 -0700, Matthew Wilcox wrote:
> On Sun, Dec 19, 2010 at 01:21:56PM -0800, Nicholas A. Bellinger wrote:
> > This patch changes iscsi_queuecommand_lck() to a host_lock less
> > iscsi_queuecommand() that will internally disable interrupts using
> > session->lock and drop the now legacy host_lock unlock.
>
> I think this patch is buggy. Before, iscsi_queuecommand_lck is called with
> interrupts disabled. Now, it's called with interrupts disabled. Elsewhere,
> session->lock is acquired with bh's disabled:
>
> void iscsi_put_task(struct iscsi_task *task)
> {
> struct iscsi_session *session = task->conn->session;
>
> spin_lock_bh(&session->lock);
>
> So I think you need to convert the
> spin_lock(&session->lock);
> in iscsi_queuecommand_lck to at least a spin_lock_bh, and possibly
> a spin_lock_irq -- it's not clear to me whether it needs to exclude
> against interrupt context, or only BH context. In some places, it's
> taken with spin_lock_irqsave().
>
> I'd play it safe and use _irq, but someone more confident with this code
> might choose to only use _bh.
>
Hmmm, indeed.. I must have dropped this recently as the last patch for
libiscsi posted below still has been converted to spin_lock_irq() for
session->lock here:
http://marc.info/?l=linux-scsi&m=128952081412461&w=2
Converting back to spin_lock_irq() usage for the moment.. Thanks alot
of noticing this..
MikeC and Hannes, do you think this is safe to use spin_lock_bh() as
well..?
--nab
> > Signed-off-by: Nicholas A. Bellinger <[email protected]>
> > ---
> > drivers/scsi/libiscsi.c | 10 ++--------
> > 1 files changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> > index c15fde8..5535782 100644
> > --- a/drivers/scsi/libiscsi.c
> > +++ b/drivers/scsi/libiscsi.c
> > @@ -1599,7 +1599,7 @@ enum {
> > FAILURE_SESSION_NOT_READY,
> > };
> >
> > -static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
> > +int iscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
> > {
> > struct iscsi_cls_session *cls_session;
> > struct Scsi_Host *host;
> > @@ -1609,13 +1609,11 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
> > struct iscsi_conn *conn;
> > struct iscsi_task *task = NULL;
> >
> > - sc->scsi_done = done;
> > sc->result = 0;
> > sc->SCp.ptr = NULL;
> >
> > host = sc->device->host;
> > ihost = shost_priv(host);
> > - spin_unlock(host->host_lock);
> >
> > cls_session = starget_to_session(scsi_target(sc->device));
> > session = cls_session->dd_data;
> > @@ -1706,7 +1704,6 @@ static int iscsi_queuecommand_lck(struct scsi_cmnd *sc, void (*done)(struct scsi
> >
> > session->queued_cmdsn++;
> > spin_unlock(&session->lock);
> > - spin_lock(host->host_lock);
> > return 0;
> >
> > prepd_reject:
> > @@ -1716,7 +1713,6 @@ reject:
> > spin_unlock(&session->lock);
> > ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
> > sc->cmnd[0], reason);
> > - spin_lock(host->host_lock);
> > return SCSI_MLQUEUE_TARGET_BUSY;
> >
> > prepd_fault:
> > @@ -1732,12 +1728,10 @@ fault:
> > scsi_out(sc)->resid = scsi_out(sc)->length;
> > scsi_in(sc)->resid = scsi_in(sc)->length;
> > }
> > - done(sc);
> > - spin_lock(host->host_lock);
> > + sc->scsi_done(sc);
> > return 0;
> > }
> >
> > -DEF_SCSI_QCMD(iscsi_queuecommand)
> > EXPORT_SYMBOL_GPL(iscsi_queuecommand);
> >
> > int iscsi_change_queue_depth(struct scsi_device *sdev, int depth, int reason)
> > --
> > 1.7.3.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Sun, 2010-12-19 at 17:15 -0800, Nicholas A. Bellinger wrote:
> On Sun, 2010-12-19 at 16:38 -0700, Matthew Wilcox wrote:
> > On Sun, Dec 19, 2010 at 01:21:56PM -0800, Nicholas A. Bellinger wrote:
> > > This patch changes iscsi_queuecommand_lck() to a host_lock less
> > > iscsi_queuecommand() that will internally disable interrupts using
> > > session->lock and drop the now legacy host_lock unlock.
> >
> > I think this patch is buggy. Before, iscsi_queuecommand_lck is called with
> > interrupts disabled. Now, it's called with interrupts disabled. Elsewhere,
> > session->lock is acquired with bh's disabled:
> >
> > void iscsi_put_task(struct iscsi_task *task)
> > {
> > struct iscsi_session *session = task->conn->session;
> >
> > spin_lock_bh(&session->lock);
> >
> > So I think you need to convert the
> > spin_lock(&session->lock);
> > in iscsi_queuecommand_lck to at least a spin_lock_bh, and possibly
> > a spin_lock_irq -- it's not clear to me whether it needs to exclude
> > against interrupt context, or only BH context. In some places, it's
> > taken with spin_lock_irqsave().
> >
> > I'd play it safe and use _irq, but someone more confident with this code
> > might choose to only use _bh.
> >
>
> Hmmm, indeed.. I must have dropped this recently as the last patch for
> libiscsi posted below still has been converted to spin_lock_irq() for
> session->lock here:
>
> http://marc.info/?l=linux-scsi&m=128952081412461&w=2
>
> Converting back to spin_lock_irq() usage for the moment.. Thanks alot
> of noticing this..
>
> MikeC and Hannes, do you think this is safe to use spin_lock_bh() as
> well..?
>
Actually sorry, Mike Christie did already make a clarification on this
subject here:
http://marc.info/?l=linux-scsi&m=129010439421506&w=2
I had originally thought the same that session->lock should be using
some flavour of spin_lock_irq*() as well, but apparently this is not the
case for libiscsi.
Best Regards,
--nab
On Sun, Dec 19, 2010 at 05:22:06PM -0800, Nicholas A. Bellinger wrote:
> Actually sorry, Mike Christie did already make a clarification on this
> subject here:
>
> http://marc.info/?l=linux-scsi&m=129010439421506&w=2
>
> I had originally thought the same that session->lock should be using
> some flavour of spin_lock_irq*() as well, but apparently this is not the
> case for libiscsi.
Right, so it seems. "the session lock is just locked in softirqs/timers"
means that it does need to be the _bh() version of spin_lock though.
I'm actually not sure ... is it safe to use the _bh versions in BH
context? I think it is because the preempt count is nested, unlike the
_irq variants of spinlocks.
--
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."
On 12/19/2010 11:21 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch converts the libsas queuecommand to run in host_lock less mode
> w/ the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling
> ->queuecommand() dispatch.
>
> Signed-off-by: Nicholas A. Bellinger <[email protected]>
> ---
> drivers/scsi/libsas/sas_scsi_host.c | 12 ++----------
> 1 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index 29251fa..011580f 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -185,24 +185,17 @@ int sas_queue_up(struct sas_task *task)
> /**
> * sas_queuecommand -- Enqueue a command for processing
> * @parameters: See SCSI Core documentation
> - *
> - * Note: XXX: Remove the host unlock/lock pair when SCSI Core can
> - * call us without holding an IRQ spinlock...
> */
> -static int sas_queuecommand_lck(struct scsi_cmnd *cmd,
> +static int sas_queuecommand_irq_disable(struct scsi_cmnd *cmd,
> void (*scsi_done)(struct scsi_cmnd *))
> - __releases(host->host_lock)
> __acquires(dev->sata_dev.ap->lock)
> __releases(dev->sata_dev.ap->lock)
> - __acquires(host->host_lock)
> {
> int res = 0;
> struct domain_device *dev = cmd_to_domain_dev(cmd);
> struct Scsi_Host *host = cmd->device->host;
> struct sas_internal *i = to_sas_internal(host->transportt);
>
> - spin_unlock_irq(host->host_lock);
> -
> {
> struct sas_ha_struct *sas_ha = dev->port->ha;
> struct sas_task *task;
> @@ -250,11 +243,10 @@ static int sas_queuecommand_lck(struct scsi_cmnd *cmd,
> }
> }
> out:
> - spin_lock_irq(host->host_lock);
> return res;
> }
>
> -DEF_SCSI_QCMD(sas_queuecommand)
> +IRQ_DISABLE_SCSI_QCMD(sas_queuecommand)
>
I hate this new macro. It is so simple by now. And anyway you are
doing them one by one and auditing the code. Please completely drop
this macro and open code it. There is no "safety" argument to this
ugliness, any more.
(Completely drop the [PATCH 02/12] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
patch and redo the users)
Thanks
Boaz
> static void sas_eh_finish_cmd(struct scsi_cmnd *cmd)
> {
On Sun, 2010-12-19 at 16:11 -0700, Matthew Wilcox wrote:
> On Sun, Dec 19, 2010 at 01:22:02PM -0800, Nicholas A. Bellinger wrote:
> > This patch converts qla2xxx to run in host_lock less mode with the new
> > IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
> > dispatch. It also drops the legacy host_lock unlock optimization.
>
> I'm not sure this is the right direction to go. Now that Jeff's done
> the pushdown and put in the compatibility macros, I don't think it makes
> sense to do another partial transition on each driver. Much better to
> take our time, analyse each driver thoroughly, and kill the DEF_SCSI_QCMD
> in each driver without introducing IRQ_DISABLE_SCSI_QCMD.
>
> In particular for this driver, it explicitly re-enables interrupts,
> so it's pretty easy to do a full conversion. Compile-tested only.
>
> Convert qla2xxx driver to run without the shost lock
>
> Signed-off-by: Matthew Wilcox <[email protected]>
Ok, this patch is functioning with LTP disktest I/O using .37-rc6
initiator side lock_less operation.
This has been added as an incremental patch for lock_less-LLDs-for-38-v3
here:
commit 355798dc3d97a0f07b14d1f3891ecca1802c9094
Author: Nicholas Bellinger <[email protected]>
Date: Mon Dec 20 01:24:12 2010 -0800
qla2xxx: Convert qla2xxx driver to run without the shost lock
This patch converts qla2xxx LLD code to run in fully lock-less mode
and removes IRQ_DISABLE_SCSI_QCMD usage. This also includes a handful
of cleanups for cmd->scsi_done assignment removal from qla2x00_get_new_sp()
and renames one goto of the exception patch with the '_lock' suffix in
qla2xxx_queuecommand().
So far this patch has been lightly tested with basic I/O functionality on
bare-metal x86_64 .37-rc6 using ISP-2532 8 Gb/sec HW.
Signed-off-by: Matthew Wilcox <[email protected]>
Signed-off-by: Nicholas A. Bellinger <[email protected]>
Thanks!
--nab
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 2c0876c..b44d986 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -513,7 +513,7 @@ qla24xx_fw_version_str(struct scsi_qla_host *vha, char *str)
>
> static inline srb_t *
> qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> - struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> + struct scsi_cmnd *cmd)
> {
> srb_t *sp;
> struct qla_hw_data *ha = vha->hw;
> @@ -527,16 +527,15 @@ qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> sp->cmd = cmd;
> sp->flags = 0;
> CMD_SP(cmd) = (void *)sp;
> - cmd->scsi_done = done;
> sp->ctx = NULL;
>
> return sp;
> }
>
> static int
> -qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> +qla2xxx_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> {
> - scsi_qla_host_t *vha = shost_priv(cmd->device->host);
> + scsi_qla_host_t *vha = shost_priv(shost);
> fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
> struct fc_rport *rport = starget_to_rport(scsi_target(cmd->device));
> struct qla_hw_data *ha = vha->hw;
> @@ -544,7 +543,6 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> srb_t *sp;
> int rval;
>
> - spin_unlock_irq(vha->host->host_lock);
> if (ha->flags.eeh_busy) {
> if (ha->flags.pci_channel_io_perm_failure)
> cmd->result = DID_NO_CONNECT << 16;
> @@ -577,39 +575,32 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> goto qc24_target_busy;
> }
>
> - sp = qla2x00_get_new_sp(base_vha, fcport, cmd, done);
> + sp = qla2x00_get_new_sp(base_vha, fcport, cmd);
> if (!sp)
> - goto qc24_host_busy_lock;
> + goto qc24_host_busy;
>
> rval = ha->isp_ops->start_scsi(sp);
> if (rval != QLA_SUCCESS)
> goto qc24_host_busy_free_sp;
>
> - spin_lock_irq(vha->host->host_lock);
> -
> return 0;
>
> qc24_host_busy_free_sp:
> qla2x00_sp_free_dma(sp);
> mempool_free(sp, ha->srb_mempool);
>
> -qc24_host_busy_lock:
> - spin_lock_irq(vha->host->host_lock);
> +qc24_host_busy:
> return SCSI_MLQUEUE_HOST_BUSY;
>
> qc24_target_busy:
> - spin_lock_irq(vha->host->host_lock);
> return SCSI_MLQUEUE_TARGET_BUSY;
>
> qc24_fail_command:
> - spin_lock_irq(vha->host->host_lock);
> - done(cmd);
> + cmd->scsi_done(cmd);
>
> return 0;
> }
>
> -static DEF_SCSI_QCMD(qla2xxx_queuecommand)
> -
>
> /*
> * qla2x00_eh_wait_on_command
>
On Sun, 2010-12-19 at 19:07 -0700, Matthew Wilcox wrote:
> On Sun, Dec 19, 2010 at 05:22:06PM -0800, Nicholas A. Bellinger wrote:
> > Actually sorry, Mike Christie did already make a clarification on this
> > subject here:
> >
> > http://marc.info/?l=linux-scsi&m=129010439421506&w=2
> >
> > I had originally thought the same that session->lock should be using
> > some flavour of spin_lock_irq*() as well, but apparently this is not the
> > case for libiscsi.
>
> Right, so it seems. "the session lock is just locked in softirqs/timers"
> means that it does need to be the _bh() version of spin_lock though.
>
> I'm actually not sure ... is it safe to use the _bh versions in BH
> context? I think it is because the preempt count is nested, unlike the
> _irq variants of spinlocks.
>
Hmmm, fair point. Merging the following incremental patch to convert
session->lock to use spin_lock_bh() in iscsi_queuecommand() into
lock_less-LLDs-for-38-v3:
commit 744f1c119b3fb0c1a6b3c67a7b490e234d1a7e75
Author: Nicholas Bellinger <[email protected]>
Date: Mon Dec 20 09:17:19 2010 +0000
libiscsi: Convert iscsi_queuecommand to use spin_lock_bh
This patch converts iscsi_queuecommand() code to obtain struct iscsi_session->lock
using spin_lock_bh() to properly handle bottom-half context operation.
Reported-by: Matthew Wilcox <[email protected]>
Signed-off-by: Nicholas A. Bellinger <[email protected]>
After a quick audit of iscsi_session->lock usage, and I see that
iscsi_complete_pdu(), iscsi_tmf_timedout(), iscsi_eh_cmd_timed_out(),
iscsi_check_transport_timeouts() are using spin_lock(), and
iscsi_session_failure() and iscsi_conn_failure() are using
spin_lock_irqsave().
Mike and Hannes, would you guys mind commenting on this..? From what I
can determine these should all be converted to use spin_lock_bh(),
yes..?
--nab
On Mon, 2010-12-20 at 10:58 +0200, Boaz Harrosh wrote:
> On 12/19/2010 11:21 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > This patch converts the libsas queuecommand to run in host_lock less mode
> > w/ the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling
> > ->queuecommand() dispatch.
> >
> > Signed-off-by: Nicholas A. Bellinger <[email protected]>
> > ---
> > drivers/scsi/libsas/sas_scsi_host.c | 12 ++----------
> > 1 files changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> > index 29251fa..011580f 100644
> > --- a/drivers/scsi/libsas/sas_scsi_host.c
> > +++ b/drivers/scsi/libsas/sas_scsi_host.c
> > @@ -185,24 +185,17 @@ int sas_queue_up(struct sas_task *task)
> > /**
> > * sas_queuecommand -- Enqueue a command for processing
> > * @parameters: See SCSI Core documentation
> > - *
> > - * Note: XXX: Remove the host unlock/lock pair when SCSI Core can
> > - * call us without holding an IRQ spinlock...
> > */
> > -static int sas_queuecommand_lck(struct scsi_cmnd *cmd,
> > +static int sas_queuecommand_irq_disable(struct scsi_cmnd *cmd,
> > void (*scsi_done)(struct scsi_cmnd *))
> > - __releases(host->host_lock)
> > __acquires(dev->sata_dev.ap->lock)
> > __releases(dev->sata_dev.ap->lock)
> > - __acquires(host->host_lock)
> > {
> > int res = 0;
> > struct domain_device *dev = cmd_to_domain_dev(cmd);
> > struct Scsi_Host *host = cmd->device->host;
> > struct sas_internal *i = to_sas_internal(host->transportt);
> >
> > - spin_unlock_irq(host->host_lock);
> > -
> > {
> > struct sas_ha_struct *sas_ha = dev->port->ha;
> > struct sas_task *task;
> > @@ -250,11 +243,10 @@ static int sas_queuecommand_lck(struct scsi_cmnd *cmd,
> > }
> > }
> > out:
> > - spin_lock_irq(host->host_lock);
> > return res;
> > }
> >
> > -DEF_SCSI_QCMD(sas_queuecommand)
> > +IRQ_DISABLE_SCSI_QCMD(sas_queuecommand)
> >
>
> I hate this new macro. It is so simple by now. And anyway you are
> doing them one by one and auditing the code. Please completely drop
> this macro and open code it. There is no "safety" argument to this
> ugliness, any more.
>
> (Completely drop the [PATCH 02/12] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
> patch and redo the users)
>
Hi Boaz,
I am happy to open code the handful of remaining LLDs that need proper
conversion for lock_less-LLDs-for-38-v4 and drop patch #2 now that we
have isolated the first round of host_lock-less LLD conversions.
Best Regards,
--nab
On Sun, Dec 19, 2010 at 01:21:57PM -0800, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch adds a IRQ_DISABLE_SCSI_QCMD() wrapper macro used by LLDs
> that can now run in host_lock less mode, but still need interrupts disabled
> using local_irq_save() before calling their lld_queuecommand() dispatcher.
>
> jgarzik says this method is in fact slower than doing a spin_lock_irqsave() on
> internal lib_lld_queuecommand() callers (as is done in libiscsi and libata)
> but is still needed by the majority of lock_less LLDs.
As mentioned before, please don't add more macro obsfucation - The
initial one Jeff added was ok for the quick transition and avoiding to
have two methods, but any additional one is not helpful.
In addition there's really no reason to every use this one. There is
not reason to disable local irqs in a driver without taking a spinlock.
> -----Original Message-----
> From: Nicholas A. Bellinger [mailto:[email protected]]
> Sent: Monday, December 20, 2010 2:52 AM
> To: linux-scsi; linux-kernel; James Bottomley; Jeff Garzik; Christoph
> Hellwig; FUJITA Tomonori; Hannes Reinecke; Mike Christie
> Cc: Mike Anderson; Tejun Heo; Vasu Dev; Tim Chen; Andi Kleen; Ravi
> Anand; Andrew Vasquez; Joe Eykholt; James Smart; Douglas Gilbert; adam
> radford; Desai, Kashyap; DL-MPT Fusion Linux; Nicholas Bellinger
> Subject: [PATCH 00/12] LLD host_lock-less conversion status for .38
>
> From: Nicholas Bellinger <[email protected]>
>
> Greetings James and Co,
>
> Attached are the current set of LLD host_lock-less conversions in the
> queue
> for .38, that have now been pushed into lio-core-2.6.git/lock_less-
> LLDs-for-38-v3
>
> New in this v3 series are the megaraid_sas LLD changes necessary to run
> in
> host_lock-less mode, which have been tested thus far in legacy x86
> interrupt
> pin mode using 8708EM2 QEMU HBA emulation into KVM guest using TCM_Loop
> virtual SCSI LUNs.
>
> This series also drops the drivers/scsi patches that have been resolved
> with
> the following (Thanks!)
>
> commit 459dbf72e4d2b4aa13620e6b70d54f098547bf13
> Author: James Bottomley <[email protected]>
> Date: Wed Nov 17 10:10:57 2010 -0600
>
> [SCSI] Eliminate error handler overload of the SCSI serial number
>
> At this point the remaining code TODO items for the LLD conversion
> patches
> include
>
> *) libiscsi: NULL sc->scsi_done() callback in exception path in
> iscsi_queuecommand().
> The last status on this from Mike Christie was:
>
> "This will NULL pointer. See a couple lines above where we NULL it.
> iscsi_free_task checks if the scsi_done pointer is set and if it is it
> will call scsi_done.
>
> It is a hack to prevent the normal completion path from calling
> scsi_done. For the case where we return SCSI_MLQUEUE_TARGET_BUSY (the
> prepd_reject case) we need something to prevent scsi_done from getting
> called.
>
> For the return 0/prepd_fault case we can just call sc->scsi_done, but
> we
> have to move some code around."
>
> mnc, have you been able to take another look at this..?
>
> *) mpt2sas: Locking considerations for firmware specific data
> structures
>
> A patch to mptsas was included originally to convert to host_lock less
> w/ interrupts
> disabled externally here:
>
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-
> 2.6.git;a=commitdiff;h=87ba9071b46f37f1adab1732d41b87bf195d191f
>
> but due to concerns from jgarzik this had been dropped from the
> lock_less-LLDs-for-38-v2
> during the last round. The feedback from Kashyap @ LSI is that mpt2sas
> technically should be
> able to run in host_lock-less, but there needs to be some explict
> clarification in the code
> for the conversion to happen. More details are here:
>
> http://marc.info/?l=linux-scsi&m=129022827729386&w=2
>
> Kashyap, can you take another look at this and make sure this is
> acceptable..?
Nicholas, I have replied my input on the same thread.
>
> *) megaraid_sas: Running in host_lock-less mode with MSI-X interrupts
>
> This has not been tested yet, but I would like to get some review from
> the LSI
> megaraid_sas folks now that their patches to enable MSI-X interrupts
> has been sent
> to linux-scsi.
>
> AdamR, any thoughts or comments to add here..?
>
> -----------------------------------------------------------------------
> -----------
>
> So at this point the next steps will be to shaking out breakage in
> corner cases for
> struct scsi_device hotplug removal, more testing+validation, and more
> LLD code review.
>
> For the CC'ed LLD maintainers, please have another look at the changes
> to enable
> host_lock-less operation within your code and send the necessary ACKs
> to linux-scsi
> at your earliest convience. Many thanks to everyone who has
> contributed!
>
> If anyone has any other patches, comments or concerns please let me
> know and they
> will be queued up for the next v4 for-38 round.
>
> Best Regards,
>
> Signed-off-by: Nicholas A. Bellinger <[email protected]>
>
> Nicholas Bellinger (12):
> libiscsi: Convert to host_lock less w/ interrupts disabled internally
> scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper
> libsas: Convert to host_lock less w/ interrupts disabled externally
> message: Convert to host_lock less w/ interrupts disabled externally
> fnic: Convert to host_lock less w/ interrupts disabled externally
> lpfc: Convert to host_lock less w/ interrupts disabled externally
> qla2xxx: Convert to host_lock less w/ interrupts disabled externally
> qla4xxx: Convert to host_lock less w/ interrupts disabled externally
> scsi_debug: Convert to host_lock less
> megaraid_sas: Add smp_mb__after_atomic_*() for
> instance->fw_outstanding
> megaraid_sas: Convert instance->issuepend_done to atomic_t
> megaraid_sas: Convert SHT->queuecommand() to run host_lock-less
>
> drivers/message/fusion/mptfc.c | 2 +-
> drivers/message/fusion/mptsas.c | 6 +-
> drivers/message/fusion/mptscsih.c | 8 ++-
> drivers/message/fusion/mptscsih.h | 2 +-
> drivers/message/fusion/mptspi.c | 6 +-
> drivers/scsi/fnic/fnic_scsi.c | 14 +----
> drivers/scsi/libiscsi.c | 10 +---
> drivers/scsi/libsas/sas_scsi_host.c | 12 +----
> drivers/scsi/lpfc/lpfc_scsi.c | 9 ++--
> drivers/scsi/megaraid/megaraid_sas.c | 85 ++++++++++++++++++++++----
> --------
> drivers/scsi/megaraid/megaraid_sas.h | 2 +-
> drivers/scsi/qla2xxx/qla_os.c | 11 +---
> drivers/scsi/qla4xxx/ql4_os.c | 10 +---
> drivers/scsi/scsi_debug.c | 16 +++----
> include/scsi/scsi_host.h | 14 ++++++
> 15 files changed, 107 insertions(+), 100 deletions(-)
>
> --
> 1.7.3.4
On Sun, Dec 19, 2010 at 1:21 PM, Nicholas A. Bellinger
<[email protected]> wrote:
> *) megaraid_sas: Running in host_lock-less mode with MSI-X interrupts
>
> This has not been tested yet, but I would like to get some review from the LSI
> megaraid_sas folks now that their patches to enable MSI-X interrupts has been sent
> to linux-scsi.
>
> AdamR, any thoughts or comments to add here..?
Nick,
Your patch will not work with the new MegaRAID adapter support I have
sent in recently. It requires the build IO path be synchronized with
map updates with the host_lock.
-Adam
All,
We had (internally @ QLogic) come up with a couple of patches (attached).
Patch 1: Similar to the patch from NAB and from Mathew Wilcox
Patch 2: A bit more changes with the host_lost within the qla2xxx driver.
The above 2 patches have gone through some testing as well, so far so good. Please take a look and let us know.
Cheers,
Madhu
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Nicholas A. Bellinger
Sent: Monday, December 20, 2010 1:24 AM
To: Matthew Wilcox
Cc: linux-scsi; linux-kernel; James Bottomley; Jeff Garzik; Christoph Hellwig; FUJITA Tomonori; Hannes Reinecke; Mike Christie; Mike Anderson; Tejun Heo; Vasu Dev; Tim Chen; Andi Kleen; Ravi Anand; Andrew Vasquez; Joe Eykholt; James Smart; Douglas Gilbert; adam radford; Kashyap Desai; MPTFusionLinux
Subject: Re: [PATCH 07/12] qla2xxx: Convert to host_lock less w/ interrupts disabled externally
On Sun, 2010-12-19 at 16:11 -0700, Matthew Wilcox wrote:
> On Sun, Dec 19, 2010 at 01:22:02PM -0800, Nicholas A. Bellinger wrote:
> > This patch converts qla2xxx to run in host_lock less mode with the new
> > IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
> > dispatch. It also drops the legacy host_lock unlock optimization.
>
> I'm not sure this is the right direction to go. Now that Jeff's done
> the pushdown and put in the compatibility macros, I don't think it makes
> sense to do another partial transition on each driver. Much better to
> take our time, analyse each driver thoroughly, and kill the DEF_SCSI_QCMD
> in each driver without introducing IRQ_DISABLE_SCSI_QCMD.
>
> In particular for this driver, it explicitly re-enables interrupts,
> so it's pretty easy to do a full conversion. Compile-tested only.
>
> Convert qla2xxx driver to run without the shost lock
>
> Signed-off-by: Matthew Wilcox <[email protected]>
Ok, this patch is functioning with LTP disktest I/O using .37-rc6
initiator side lock_less operation.
This has been added as an incremental patch for lock_less-LLDs-for-38-v3
here:
commit 355798dc3d97a0f07b14d1f3891ecca1802c9094
Author: Nicholas Bellinger <[email protected]>
Date: Mon Dec 20 01:24:12 2010 -0800
qla2xxx: Convert qla2xxx driver to run without the shost lock
This patch converts qla2xxx LLD code to run in fully lock-less mode
and removes IRQ_DISABLE_SCSI_QCMD usage. This also includes a handful
of cleanups for cmd->scsi_done assignment removal from qla2x00_get_new_sp()
and renames one goto of the exception patch with the '_lock' suffix in
qla2xxx_queuecommand().
So far this patch has been lightly tested with basic I/O functionality on
bare-metal x86_64 .37-rc6 using ISP-2532 8 Gb/sec HW.
Signed-off-by: Matthew Wilcox <[email protected]>
Signed-off-by: Nicholas A. Bellinger <[email protected]>
Thanks!
--nab
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 2c0876c..b44d986 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -513,7 +513,7 @@ qla24xx_fw_version_str(struct scsi_qla_host *vha, char *str)
>
> static inline srb_t *
> qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> - struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> + struct scsi_cmnd *cmd)
> {
> srb_t *sp;
> struct qla_hw_data *ha = vha->hw;
> @@ -527,16 +527,15 @@ qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> sp->cmd = cmd;
> sp->flags = 0;
> CMD_SP(cmd) = (void *)sp;
> - cmd->scsi_done = done;
> sp->ctx = NULL;
>
> return sp;
> }
>
> static int
> -qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> +qla2xxx_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> {
> - scsi_qla_host_t *vha = shost_priv(cmd->device->host);
> + scsi_qla_host_t *vha = shost_priv(shost);
> fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
> struct fc_rport *rport = starget_to_rport(scsi_target(cmd->device));
> struct qla_hw_data *ha = vha->hw;
> @@ -544,7 +543,6 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> srb_t *sp;
> int rval;
>
> - spin_unlock_irq(vha->host->host_lock);
> if (ha->flags.eeh_busy) {
> if (ha->flags.pci_channel_io_perm_failure)
> cmd->result = DID_NO_CONNECT << 16;
> @@ -577,39 +575,32 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> goto qc24_target_busy;
> }
>
> - sp = qla2x00_get_new_sp(base_vha, fcport, cmd, done);
> + sp = qla2x00_get_new_sp(base_vha, fcport, cmd);
> if (!sp)
> - goto qc24_host_busy_lock;
> + goto qc24_host_busy;
>
> rval = ha->isp_ops->start_scsi(sp);
> if (rval != QLA_SUCCESS)
> goto qc24_host_busy_free_sp;
>
> - spin_lock_irq(vha->host->host_lock);
> -
> return 0;
>
> qc24_host_busy_free_sp:
> qla2x00_sp_free_dma(sp);
> mempool_free(sp, ha->srb_mempool);
>
> -qc24_host_busy_lock:
> - spin_lock_irq(vha->host->host_lock);
> +qc24_host_busy:
> return SCSI_MLQUEUE_HOST_BUSY;
>
> qc24_target_busy:
> - spin_lock_irq(vha->host->host_lock);
> return SCSI_MLQUEUE_TARGET_BUSY;
>
> qc24_fail_command:
> - spin_lock_irq(vha->host->host_lock);
> - done(cmd);
> + cmd->scsi_done(cmd);
>
> return 0;
> }
>
> -static DEF_SCSI_QCMD(qla2xxx_queuecommand)
> -
>
> /*
> * qla2x00_eh_wait_on_command
>
On 12/20/2010 03:30 AM, Nicholas A. Bellinger wrote:
> After a quick audit of iscsi_session->lock usage, and I see that
> iscsi_complete_pdu(), iscsi_tmf_timedout(), iscsi_eh_cmd_timed_out(),
> iscsi_check_transport_timeouts() are using spin_lock(), and
> iscsi_session_failure() and iscsi_conn_failure() are using
> spin_lock_irqsave().
>
> Mike and Hannes, would you guys mind commenting on this..? From what I
> can determine these should all be converted to use spin_lock_bh(),
> yes..?
>
Yeah, they can use _bh locking. I was going to use them for qla4xxx eh,
which does iscsi processing in its irq, but we never did.
On 12/19/2010 03:21 PM, Nicholas A. Bellinger wrote:
> prepd_fault:
> @@ -1732,12 +1728,10 @@ fault:
> scsi_out(sc)->resid = scsi_out(sc)->length;
> scsi_in(sc)->resid = scsi_in(sc)->length;
> }
> - done(sc);
> - spin_lock(host->host_lock);
> + sc->scsi_done(sc);
> return 0;
Did you mean to send this patch or a different one? I think you sent a
patch with this before and I said it was wrong. Did you disagree with
that comment or did you change something else so it now works?
On 12/21/2010 02:42 AM, Mike Christie wrote:
> On 12/19/2010 03:21 PM, Nicholas A. Bellinger wrote:
>> prepd_fault:
>> @@ -1732,12 +1728,10 @@ fault:
>> scsi_out(sc)->resid = scsi_out(sc)->length;
>> scsi_in(sc)->resid = scsi_in(sc)->length;
>> }
>> - done(sc);
>> - spin_lock(host->host_lock);
>> + sc->scsi_done(sc);
>> return 0;
>
>
> Did you mean to send this patch or a different one? I think you sent a
> patch with this before and I said it was wrong. Did you disagree with
> that comment or did you change something else so it now works?
Mike hi
I think Nick is gone for the holidays. What I understood is that he's
waiting for you to fix it. Because he's not sure what the proper solution
is. Do you have time to look into it? (It will take you much faster then me)
On 12/19/2010 11:21 PM, Nicholas A. Bellinger wrote:
> *) libiscsi: NULL sc->scsi_done() callback in exception path in iscsi_queuecommand().
> The last status on this from Mike Christie was:
>
> "This will NULL pointer. See a couple lines above where we NULL it.
> iscsi_free_task checks if the scsi_done pointer is set and if it is it
> will call scsi_done.
>
> It is a hack to prevent the normal completion path from calling
> scsi_done. For the case where we return SCSI_MLQUEUE_TARGET_BUSY (the
> prepd_reject case) we need something to prevent scsi_done from getting
> called.
>
> For the return 0/prepd_fault case we can just call sc->scsi_done, but we
> have to move some code around."
>
> mnc, have you been able to take another look at this..?
>
Thanks
Boaz
On 12/21/2010 04:53 AM, Boaz Harrosh wrote:
> On 12/21/2010 02:42 AM, Mike Christie wrote:
>> On 12/19/2010 03:21 PM, Nicholas A. Bellinger wrote:
>>> prepd_fault:
>>> @@ -1732,12 +1728,10 @@ fault:
>>> scsi_out(sc)->resid = scsi_out(sc)->length;
>>> scsi_in(sc)->resid = scsi_in(sc)->length;
>>> }
>>> - done(sc);
>>> - spin_lock(host->host_lock);
>>> + sc->scsi_done(sc);
>>> return 0;
>>
>>
>> Did you mean to send this patch or a different one? I think you sent a
>> patch with this before and I said it was wrong. Did you disagree with
>> that comment or did you change something else so it now works?
>
> Mike hi
>
> I think Nick is gone for the holidays. What I understood is that he's
> waiting for you to fix it. Because he's not sure what the proper solution
> is. Do you have time to look into it? (It will take you much faster then me)
>
I am still working on it for the 2.6.38 feature window.
On Mon, 2010-12-20 at 11:33 -0800, adam radford wrote:
> On Sun, Dec 19, 2010 at 1:21 PM, Nicholas A. Bellinger
> <[email protected]> wrote:
> > *) megaraid_sas: Running in host_lock-less mode with MSI-X interrupts
> >
> > This has not been tested yet, but I would like to get some review from the LSI
> > megaraid_sas folks now that their patches to enable MSI-X interrupts has been sent
> > to linux-scsi.
> >
> > AdamR, any thoughts or comments to add here..?
>
> Nick,
>
> Your patch will not work with the new MegaRAID adapter support I have
> sent in recently. It requires the build IO path be synchronized with
> map updates with the host_lock.
>
Adam,
Thanks for the clarification wrt to your latest patches. Do you think
it would make sense to convert the synchronization between the build IO
path and map updates to use the LLD specific instance->hba_lock instead
of host_lock..? This is the method I have been employing thus far w/
with the host_lock-less conversion for megaraid_sas, and from a quick
grok it appears this might be the most sane approach for MSI-X support.
Any thoughts..?
Best Regards,
--nab
On Mon, 2010-12-20 at 18:36 -0600, Mike Christie wrote:
> On 12/20/2010 03:30 AM, Nicholas A. Bellinger wrote:
> > After a quick audit of iscsi_session->lock usage, and I see that
> > iscsi_complete_pdu(), iscsi_tmf_timedout(), iscsi_eh_cmd_timed_out(),
> > iscsi_check_transport_timeouts() are using spin_lock(), and
> > iscsi_session_failure() and iscsi_conn_failure() are using
> > spin_lock_irqsave().
> >
> > Mike and Hannes, would you guys mind commenting on this..? From what I
> > can determine these should all be converted to use spin_lock_bh(),
> > yes..?
> >
>
> Yeah, they can use _bh locking. I was going to use them for qla4xxx eh,
> which does iscsi processing in its irq, but we never did.
Thanks for the clarification. I would be happy to submit a patch next
week for this seperately from the host_lock-less conversion pieces, or
would you prefer handle this yourself..?
Best Regards,
--nab
On Tue, 2010-12-21 at 17:43 -0600, Mike Christie wrote:
> On 12/21/2010 04:53 AM, Boaz Harrosh wrote:
> > On 12/21/2010 02:42 AM, Mike Christie wrote:
> >> On 12/19/2010 03:21 PM, Nicholas A. Bellinger wrote:
> >>> prepd_fault:
> >>> @@ -1732,12 +1728,10 @@ fault:
> >>> scsi_out(sc)->resid = scsi_out(sc)->length;
> >>> scsi_in(sc)->resid = scsi_in(sc)->length;
> >>> }
> >>> - done(sc);
> >>> - spin_lock(host->host_lock);
> >>> + sc->scsi_done(sc);
> >>> return 0;
> >>
> >>
> >> Did you mean to send this patch or a different one? I think you sent a
> >> patch with this before and I said it was wrong. Did you disagree with
> >> that comment or did you change something else so it now works?
> >
> > Mike hi
> >
> > I think Nick is gone for the holidays. What I understood is that he's
> > waiting for you to fix it. Because he's not sure what the proper solution
> > is. Do you have time to look into it? (It will take you much faster then me)
> >
>
> I am still working on it for the 2.6.38 feature window.
I was still unsure on the exception path handling concerns you had
mentioned in iscsi_queuecommand() wrt to a NULL pointer deference with
sc->scsi_done() for SCSI_MLQUEUE_TARGET_BUSY. So at this point I will
again defer to your better judgement and await your patch.
Thanks Mike!
--nab
On Mon, 2010-12-20 at 16:37 -0800, Madhu Iyengar wrote:
> All,
>
> We had (internally @ QLogic) come up with a couple of patches (attached).
>
> Patch 1: Similar to the patch from NAB and from Mathew Wilcox
> Patch 2: A bit more changes with the host_lost within the qla2xxx driver.
>
> The above 2 patches have gone through some testing as well, so far so good. Please take a look and let us know.
>
Hi Madhu,
Thank you for your followup. I think both of these patches look OK, so
please feel free to add my:
Signed-off-by: Nicholas A. Bellinger <[email protected]>
Also let me know if you prefer these to be carried for the next series
in lio-core-2.6.git/lock_less-LLDs-for-38-v4, or if you would rather
these be picked up by James directly.
Best Regards,
--nab
> Cheers,
> Madhu
>
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Nicholas A. Bellinger
> Sent: Monday, December 20, 2010 1:24 AM
> To: Matthew Wilcox
> Cc: linux-scsi; linux-kernel; James Bottomley; Jeff Garzik; Christoph Hellwig; FUJITA Tomonori; Hannes Reinecke; Mike Christie; Mike Anderson; Tejun Heo; Vasu Dev; Tim Chen; Andi Kleen; Ravi Anand; Andrew Vasquez; Joe Eykholt; James Smart; Douglas Gilbert; adam radford; Kashyap Desai; MPTFusionLinux
> Subject: Re: [PATCH 07/12] qla2xxx: Convert to host_lock less w/ interrupts disabled externally
>
> On Sun, 2010-12-19 at 16:11 -0700, Matthew Wilcox wrote:
> > On Sun, Dec 19, 2010 at 01:22:02PM -0800, Nicholas A. Bellinger wrote:
> > > This patch converts qla2xxx to run in host_lock less mode with the new
> > > IRQ_DISABLE_SCSI_QCMD() that disables interrupts while calling ->queuecommand()
> > > dispatch. It also drops the legacy host_lock unlock optimization.
> >
> > I'm not sure this is the right direction to go. Now that Jeff's done
> > the pushdown and put in the compatibility macros, I don't think it makes
> > sense to do another partial transition on each driver. Much better to
> > take our time, analyse each driver thoroughly, and kill the DEF_SCSI_QCMD
> > in each driver without introducing IRQ_DISABLE_SCSI_QCMD.
> >
> > In particular for this driver, it explicitly re-enables interrupts,
> > so it's pretty easy to do a full conversion. Compile-tested only.
> >
> > Convert qla2xxx driver to run without the shost lock
> >
> > Signed-off-by: Matthew Wilcox <[email protected]>
>
> Ok, this patch is functioning with LTP disktest I/O using .37-rc6
> initiator side lock_less operation.
>
> This has been added as an incremental patch for lock_less-LLDs-for-38-v3
> here:
>
> commit 355798dc3d97a0f07b14d1f3891ecca1802c9094
> Author: Nicholas Bellinger <[email protected]>
> Date: Mon Dec 20 01:24:12 2010 -0800
>
> qla2xxx: Convert qla2xxx driver to run without the shost lock
>
> This patch converts qla2xxx LLD code to run in fully lock-less mode
> and removes IRQ_DISABLE_SCSI_QCMD usage. This also includes a handful
> of cleanups for cmd->scsi_done assignment removal from qla2x00_get_new_sp()
> and renames one goto of the exception patch with the '_lock' suffix in
> qla2xxx_queuecommand().
>
> So far this patch has been lightly tested with basic I/O functionality on
> bare-metal x86_64 .37-rc6 using ISP-2532 8 Gb/sec HW.
>
> Signed-off-by: Matthew Wilcox <[email protected]>
> Signed-off-by: Nicholas A. Bellinger <[email protected]>
>
> Thanks!
>
> --nab
>
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> > index 2c0876c..b44d986 100644
> > --- a/drivers/scsi/qla2xxx/qla_os.c
> > +++ b/drivers/scsi/qla2xxx/qla_os.c
> > @@ -513,7 +513,7 @@ qla24xx_fw_version_str(struct scsi_qla_host *vha, char *str)
> >
> > static inline srb_t *
> > qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> > - struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> > + struct scsi_cmnd *cmd)
> > {
> > srb_t *sp;
> > struct qla_hw_data *ha = vha->hw;
> > @@ -527,16 +527,15 @@ qla2x00_get_new_sp(scsi_qla_host_t *vha, fc_port_t *fcport,
> > sp->cmd = cmd;
> > sp->flags = 0;
> > CMD_SP(cmd) = (void *)sp;
> > - cmd->scsi_done = done;
> > sp->ctx = NULL;
> >
> > return sp;
> > }
> >
> > static int
> > -qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
> > +qla2xxx_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> > {
> > - scsi_qla_host_t *vha = shost_priv(cmd->device->host);
> > + scsi_qla_host_t *vha = shost_priv(shost);
> > fc_port_t *fcport = (struct fc_port *) cmd->device->hostdata;
> > struct fc_rport *rport = starget_to_rport(scsi_target(cmd->device));
> > struct qla_hw_data *ha = vha->hw;
> > @@ -544,7 +543,6 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> > srb_t *sp;
> > int rval;
> >
> > - spin_unlock_irq(vha->host->host_lock);
> > if (ha->flags.eeh_busy) {
> > if (ha->flags.pci_channel_io_perm_failure)
> > cmd->result = DID_NO_CONNECT << 16;
> > @@ -577,39 +575,32 @@ qla2xxx_queuecommand_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)
> > goto qc24_target_busy;
> > }
> >
> > - sp = qla2x00_get_new_sp(base_vha, fcport, cmd, done);
> > + sp = qla2x00_get_new_sp(base_vha, fcport, cmd);
> > if (!sp)
> > - goto qc24_host_busy_lock;
> > + goto qc24_host_busy;
> >
> > rval = ha->isp_ops->start_scsi(sp);
> > if (rval != QLA_SUCCESS)
> > goto qc24_host_busy_free_sp;
> >
> > - spin_lock_irq(vha->host->host_lock);
> > -
> > return 0;
> >
> > qc24_host_busy_free_sp:
> > qla2x00_sp_free_dma(sp);
> > mempool_free(sp, ha->srb_mempool);
> >
> > -qc24_host_busy_lock:
> > - spin_lock_irq(vha->host->host_lock);
> > +qc24_host_busy:
> > return SCSI_MLQUEUE_HOST_BUSY;
> >
> > qc24_target_busy:
> > - spin_lock_irq(vha->host->host_lock);
> > return SCSI_MLQUEUE_TARGET_BUSY;
> >
> > qc24_fail_command:
> > - spin_lock_irq(vha->host->host_lock);
> > - done(cmd);
> > + cmd->scsi_done(cmd);
> >
> > return 0;
> > }
> >
> > -static DEF_SCSI_QCMD(qla2xxx_queuecommand)
> > -
> >
> > /*
> > * qla2x00_eh_wait_on_command
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 12/23/2010 03:23 PM, Nicholas A. Bellinger wrote:
> On Mon, 2010-12-20 at 18:36 -0600, Mike Christie wrote:
>> On 12/20/2010 03:30 AM, Nicholas A. Bellinger wrote:
>>> After a quick audit of iscsi_session->lock usage, and I see that
>>> iscsi_complete_pdu(), iscsi_tmf_timedout(), iscsi_eh_cmd_timed_out(),
>>> iscsi_check_transport_timeouts() are using spin_lock(), and
>>> iscsi_session_failure() and iscsi_conn_failure() are using
>>> spin_lock_irqsave().
>>>
>>> Mike and Hannes, would you guys mind commenting on this..? From what I
>>> can determine these should all be converted to use spin_lock_bh(),
>>> yes..?
>>>
>>
>> Yeah, they can use _bh locking. I was going to use them for qla4xxx eh,
>> which does iscsi processing in its irq, but we never did.
>
> Thanks for the clarification. I would be happy to submit a patch next
> week for this seperately from the host_lock-less conversion pieces, or
> would you prefer handle this yourself..?
>
I am testing something now. Do not worry about it. Thanks for the work
on this.