Re: hpsa: Convert SCSI LLD ->queuecommand() for host_lock less operation

[Jeff Garzik <jeff@garzik.org>]

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@redhat.com>

* 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