On 01/29/2011 05:38 PM, Nicholas A. Bellinger wrote:
Hi Stephen and Co,
Attached is a quick patch to enable modern SCSI host_lock' less
operation for the HPSA SCSI LLD driver on>= .37 kernels. After an
initial LLD conversion and review, I would like to verify with you that
the following calls are currently protected by HPDA LLD internal locks
that disable interrupts from the newly renamed hpsa_scsi_queue_command()
dispatcher below..
*) For cmd_alloc(), struct ctlr_info *h->lock + spin_lock_irqsave() is
obtained and released.
*) For enqueue_cmd_and_start_io(), struct ctlr_info *h->lock +
spin_lock_irqsave() is obtained for addQ() + start_io() and released.
So the one new piece wrt to host_lock less mode that is *not* protected
in hpsa_scsi_queue_command() is the call to hpsa_scatter_gather(). This
should be OK to get called without h->lock held w/ spin_lock_irqsave(),
yes..?
So far this patch has been compile tested only. Please have a look and
comment.
Thanks!
--nab
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 12deffc..e205f33 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -1906,9 +1906,11 @@ sglist_finished:
return 0;
}
-
-static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
- void (*done)(struct scsi_cmnd *))
+/*
+ * Running in struct Scsi_Host->host_lock less mode using LLD internal
+ * struct ctlr_info *h->lock w/ spin_lock_irqsave() protection.
+ */
The way I read this comment, it initially misled me into thinking that
this queuecommand was somehow wrapped entirely within this protection
you described.
Only after an in-depth review did I figure out that cmd_alloc() and
enqueue_cmd_and_start_io() did all the locking necessary.
Therefore, here are some observations and recommendations:
* the code changes are correct. Reviewed-by: Jeff Garzik
<jgarzik@xxxxxxxxxx>
* delete the comment. lack of "_lck" alone tells you its lockless.
* I question whether the following hpsa.c logic
lock_irqsave
cmd_alloc()
unlock_irqrestore
init cmd
lock_irqsave
enqueue
unlock_irqrestore
isn't more expensive than simply
lock_irqsave
cmd_alloc()
init cmd
enqueue
unlock_irqrestore
It seems like the common case would cycle the spinlock and interrupts
twice for an uncontended lock, which the initialization portion of the
cmd, which may indeed occur in parallel, is so cheap you'll spend more
time on the double-lock than anywhere else.
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
