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

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

[Nicholas A. Bellinger]

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.
+ */
+static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
 {
        struct ctlr_info *h;
        struct hpsa_scsi_dev_t *dev;
@@ -1921,7 +1923,7 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
        dev = cmd->device->hostdata;
        if (!dev) {
                cmd->result = DID_NO_CONNECT << 16;
-               done(cmd);
+               cmd->scsi_done(cmd);
                return 0;
        }
        memcpy(scsi3addr, dev->scsi3addr, sizeof(scsi3addr));
@@ -1936,9 +1938,6 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
        }
 
        /* Fill in the command list header */
-
-       cmd->scsi_done = done;    /* save this for use by completion code */
-
        /* save c in case we have to abort it  */
        cmd->host_scribble = (unsigned char *) c;
 
@@ -2001,8 +2000,6 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
        return 0;
 }
 
-static DEF_SCSI_QCMD(hpsa_scsi_queue_command)
-
 static void hpsa_scan_start(struct Scsi_Host *sh)
 {
        struct ctlr_info *h = shost_to_hba(sh);

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

[Jeff Garzik]

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

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

[Nicholas A. Bellinger]

On Sat, 2011-01-29 at 22:26 -0500, Jeff Garzik wrote:
> 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.
> 

Hi Jeff,

I was wondering about the overhead of double h->lock cycle for parallel
struct scsi_cmnd -> struct CommandList initialization vs. single h->
lock cycle..  Thanks for your input here!

Attached is a v2 to address your comments.

--nab

------------------------------------------------------------------------

[PATCH] hpsa: Convert SCSI LLD ->queuecommand() for host_lock less operation

This patch first converts HPSA to run in struct Scsi_Host->host_lock'
less operation by removing DEF_SCSI_QCMD() and '_lck' suffix from the
hpsa_scsi_queue_command() I/O dispatcher.

Secondly in hpsa_scsi_queue_command() the struct ctlr_info *h->lock is
now held a single lock obtain/release cycle while struct CommandList
initialization is performed, and enqueued into HW.  This enqueuing
is done using a new h->lock unlocked __enqueue_cmd_and_start_io(),
wrapper and conversion of the the original enqueue_cmd_and_start_io()
to use this new code.

Reviewed-by: Jeff Garzik <jgarzik@redhat.com>
Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/hpsa.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 12deffc..fff7cd4 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -329,16 +329,25 @@ static void set_performant_mode(struct ctlr_info *h, struct CommandList *c)
                c->busaddr |= 1 | (h->blockFetchTable[c->Header.SGList] << 1);
 }

-static void enqueue_cmd_and_start_io(struct ctlr_info *h,
+/*
+ * Must be called with struct ctlr_info *h->lock held w/ interrupts disabled
+ */
+static void __enqueue_cmd_and_start_io(struct ctlr_info *h,
        struct CommandList *c)
 {
-       unsigned long flags;
-
        set_performant_mode(h, c);
-       spin_lock_irqsave(&h->lock, flags);
        addQ(&h->reqQ, c);
        h->Qdepth++;
        start_io(h);
+}
+
+static void enqueue_cmd_and_start_io(struct ctlr_info *h,
+       struct CommandList *c)
+{
+       unsigned long flags;
+
+       spin_lock_irqsave(&h->lock, flags);
+       __enqueue_cmd_and_start_io(h, c);
        spin_unlock_irqrestore(&h->lock, flags);
 }

@@ -1906,9 +1915,7 @@ sglist_finished:
        return 0;
 }

-
-static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
-       void (*done)(struct scsi_cmnd *))
+static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
 {
        struct ctlr_info *h;
        struct hpsa_scsi_dev_t *dev;
@@ -1921,7 +1928,7 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
        dev = cmd->device->hostdata;
        if (!dev) {
                cmd->result = DID_NO_CONNECT << 16;
-               done(cmd);
+               cmd->scsi_done(cmd);
                return 0;
        }
        memcpy(scsi3addr, dev->scsi3addr, sizeof(scsi3addr));
@@ -1929,16 +1936,13 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
        /* Need a lock as this is being allocated from the pool */
        spin_lock_irqsave(&h->lock, flags);
        c = cmd_alloc(h);
-       spin_unlock_irqrestore(&h->lock, flags);
        if (c == NULL) {                        /* trouble... */
                dev_err(&h->pdev->dev, "cmd_alloc returned NULL!\n");
+               spin_unlock_irqrestore(&h->lock, flags);
                return SCSI_MLQUEUE_HOST_BUSY;
        }

        /* Fill in the command list header */
-
-       cmd->scsi_done = done;    /* save this for use by completion code */
-
        /* save c in case we have to abort it  */
        cmd->host_scribble = (unsigned char *) c;

@@ -1994,15 +1998,15 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,

        if (hpsa_scatter_gather(h, c, cmd) < 0) { /* Fill SG list */
                cmd_free(h, c);
+               spin_unlock_irqrestore(&h->lock, flags);
                return SCSI_MLQUEUE_HOST_BUSY;
        }
-       enqueue_cmd_and_start_io(h, c);
+       __enqueue_cmd_and_start_io(h, c);
+       spin_unlock_irqrestore(&h->lock, flags);
        /* the cmd'll come back via intr handler in complete_scsi_command()  */
        return 0;
 }

-static DEF_SCSI_QCMD(hpsa_scsi_queue_command)
-
 static void hpsa_scan_start(struct Scsi_Host *sh)
 {
        struct ctlr_info *h = shost_to_hba(sh);
-- 
1.7.3.5

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

[scameron]

On Sat, Jan 29, 2011 at 02:38:22PM -0800, 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.

Yes (but not inside cmd_alloc()), the lock is taken to protect
the bitmap that's used to track allocated commands, h->cmd_pool_bits.

> 
> *) For enqueue_cmd_and_start_io(), struct ctlr_info *h->lock +
> spin_lock_irqsave() is obtained for addQ() + start_io() and released.

Yes.

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

Yes, hpsa_scatter_gather() doesn't require the lock to be held.

> 
> So far this patch has been compile tested only.  Please have a look and
> comment.

Nothing obviously wrong that I see.

-- steve

> 
> 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.
> + */
> +static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
>  {
>         struct ctlr_info *h;
>         struct hpsa_scsi_dev_t *dev;
> @@ -1921,7 +1923,7 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
>         dev = cmd->device->hostdata;
>         if (!dev) {
>                 cmd->result = DID_NO_CONNECT << 16;
> -               done(cmd);
> +               cmd->scsi_done(cmd);
>                 return 0;
>         }
>         memcpy(scsi3addr, dev->scsi3addr, sizeof(scsi3addr));
> @@ -1936,9 +1938,6 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
>         }
>  
>         /* Fill in the command list header */
> -
> -       cmd->scsi_done = done;    /* save this for use by completion code */
> -
>         /* save c in case we have to abort it  */
>         cmd->host_scribble = (unsigned char *) c;
>  
> @@ -2001,8 +2000,6 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
>         return 0;
>  }
>  
> -static DEF_SCSI_QCMD(hpsa_scsi_queue_command)
> -
>  static void hpsa_scan_start(struct Scsi_Host *sh)
>  {
>         struct ctlr_info *h = shost_to_hba(sh);
> 

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

[scameron]

On Sun, Jan 30, 2011 at 01:18:19PM -0800, Nicholas A. Bellinger wrote:
> On Sat, 2011-01-29 at 22:26 -0500, Jeff Garzik wrote:
> > 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.
> > 
> 
> Hi Jeff,
> 
> I was wondering about the overhead of double h->lock cycle for parallel
> struct scsi_cmnd -> struct CommandList initialization vs. single h->
> lock cycle..  Thanks for your input here!
> 

I'll defer to Jeff's expertise on this, though some measurements 
might be nice.  Might be hard to measure though, as I suspect if
you were able to make this part of the code infinitely fast, you'd
still have a hard time telling the difference, as commands spend 
such a small proportion of their lifetimes in there... though if
it eliminates or mitigates some sort of bad, weird interlocking
behavior between this code and the interrupt handler or something
like that, (not that I'm aware of any such bad behavior) I suppose
it might make a noticeable difference.
 
> Attached is a v2 to address your comments.
> 
> --nab
> 
> ------------------------------------------------------------------------
> 
> [PATCH] hpsa: Convert SCSI LLD ->queuecommand() for host_lock less operation
> 
> This patch first converts HPSA to run in struct Scsi_Host->host_lock'
> less operation by removing DEF_SCSI_QCMD() and '_lck' suffix from the
> hpsa_scsi_queue_command() I/O dispatcher.
> 
> Secondly in hpsa_scsi_queue_command() the struct ctlr_info *h->lock is
> now held a single lock obtain/release cycle while struct CommandList
> initialization is performed, and enqueued into HW.  This enqueuing
> is done using a new h->lock unlocked __enqueue_cmd_and_start_io(),
> wrapper and conversion of the the original enqueue_cmd_and_start_io()
> to use this new code.
> 
> Reviewed-by: Jeff Garzik <jgarzik@redhat.com>
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/scsi/hpsa.c |   34 +++++++++++++++++++---------------
>  1 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 12deffc..fff7cd4 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -329,16 +329,25 @@ static void set_performant_mode(struct ctlr_info *h, struct CommandList *c)
>                 c->busaddr |= 1 | (h->blockFetchTable[c->Header.SGList] << 1);
>  }
> 
> -static void enqueue_cmd_and_start_io(struct ctlr_info *h,
> +/*
> + * Must be called with struct ctlr_info *h->lock held w/ interrupts disabled
> + */
> +static void __enqueue_cmd_and_start_io(struct ctlr_info *h,
>         struct CommandList *c)
>  {
> -       unsigned long flags;
> -
>         set_performant_mode(h, c);
> -       spin_lock_irqsave(&h->lock, flags);
>         addQ(&h->reqQ, c);
>         h->Qdepth++;
>         start_io(h);
> +}
> +
> +static void enqueue_cmd_and_start_io(struct ctlr_info *h,
> +       struct CommandList *c)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&h->lock, flags);
> +       __enqueue_cmd_and_start_io(h, c);
>         spin_unlock_irqrestore(&h->lock, flags);
>  }

Should that be "static inline"?  Or maybe the compiler's smart enough
to decide whether to inline that on its own these days?

> 
> @@ -1906,9 +1915,7 @@ sglist_finished:
>         return 0;
>  }
> 
> -
> -static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
> -       void (*done)(struct scsi_cmnd *))
> +static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
>  {
>         struct ctlr_info *h;
>         struct hpsa_scsi_dev_t *dev;
> @@ -1921,7 +1928,7 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
>         dev = cmd->device->hostdata;
>         if (!dev) {
>                 cmd->result = DID_NO_CONNECT << 16;
> -               done(cmd);
> +               cmd->scsi_done(cmd);
>                 return 0;
>         }
>         memcpy(scsi3addr, dev->scsi3addr, sizeof(scsi3addr));
> @@ -1929,16 +1936,13 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
>         /* Need a lock as this is being allocated from the pool */
>         spin_lock_irqsave(&h->lock, flags);
>         c = cmd_alloc(h);
> -       spin_unlock_irqrestore(&h->lock, flags);
>         if (c == NULL) {                        /* trouble... */
>                 dev_err(&h->pdev->dev, "cmd_alloc returned NULL!\n");
> +               spin_unlock_irqrestore(&h->lock, flags);
>                 return SCSI_MLQUEUE_HOST_BUSY;
>         }
> 
>         /* Fill in the command list header */
> -
> -       cmd->scsi_done = done;    /* save this for use by completion code */
> -
>         /* save c in case we have to abort it  */
>         cmd->host_scribble = (unsigned char *) c;
> 
> @@ -1994,15 +1998,15 @@ static int hpsa_scsi_queue_command_lck(struct scsi_cmnd *cmd,
> 
>         if (hpsa_scatter_gather(h, c, cmd) < 0) { /* Fill SG list */
>                 cmd_free(h, c);
> +               spin_unlock_irqrestore(&h->lock, flags);
>                 return SCSI_MLQUEUE_HOST_BUSY;
>         }
> -       enqueue_cmd_and_start_io(h, c);
> +       __enqueue_cmd_and_start_io(h, c);
> +       spin_unlock_irqrestore(&h->lock, flags);
>         /* the cmd'll come back via intr handler in complete_scsi_command()  */
>         return 0;
>  }
> 
> -static DEF_SCSI_QCMD(hpsa_scsi_queue_command)
> -
>  static void hpsa_scan_start(struct Scsi_Host *sh)
>  {
>         struct ctlr_info *h = shost_to_hba(sh);
> -- 
> 1.7.3.5
> 
> 
> 
> 

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

[Nicholas A. Bellinger]

On Mon, 2011-01-31 at 08:42 -0600, scameron@beardog.cce.hp.com wrote:
> On Sun, Jan 30, 2011 at 01:18:19PM -0800, Nicholas A. Bellinger wrote:
> > On Sat, 2011-01-29 at 22:26 -0500, Jeff Garzik wrote:
> > > On 01/29/2011 05:38 PM, Nicholas A. Bellinger wrote:

<SNIP>

> > > > 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.
> > > 
> > 
> > Hi Jeff,
> > 
> > I was wondering about the overhead of double h->lock cycle for parallel
> > struct scsi_cmnd -> struct CommandList initialization vs. single h->
> > lock cycle..  Thanks for your input here!
> > 
> 
> I'll defer to Jeff's expertise on this, though some measurements 
> might be nice.  Might be hard to measure though, as I suspect if
> you were able to make this part of the code infinitely fast, you'd
> still have a hard time telling the difference, as commands spend 
> such a small proportion of their lifetimes in there...

Unfortuately I do not have HPSA hardware to test this one in-house, but
this patch is actually intended for John + kernel.org mirrors, whom have
been moving from CCISS raw struct block_device -> HPSA LLD struct
scsi_device recently.

He has been observing some unusually high spin_lock contention on one
particular machine using HPSA LUNs w/ 38-rc2 code, and while it's pretty
certain that the HPSA LLD is not the root culprit, I figured that
getting this code properly converted to run w/o host_lock could not hurt
at this point.

> though if
> it eliminates or mitigates some sort of bad, weird interlocking
> behavior between this code and the interrupt handler or something
> like that, (not that I'm aware of any such bad behavior) I suppose
> it might make a noticeable difference.
>  

Understood, and thanks for the clarification here.  Running with
DEF_SCSI_QCMD() host_lock'ed code w/ the original double h->lock cycle
would have likely prevented any bad behaviour here, so holding h->lock
from before cmd_alloc() until after __enque_cmd_and_start_io() for a
single lock cycle per struct scsi_cmnd would seem to be provide similar
protection.


> > Attached is a v2 to address your comments.
> > 
> > --nab
> > 
> > ------------------------------------------------------------------------
> > 
> > [PATCH] hpsa: Convert SCSI LLD ->queuecommand() for host_lock less operation
> > 
> > This patch first converts HPSA to run in struct Scsi_Host->host_lock'
> > less operation by removing DEF_SCSI_QCMD() and '_lck' suffix from the
> > hpsa_scsi_queue_command() I/O dispatcher.
> > 
> > Secondly in hpsa_scsi_queue_command() the struct ctlr_info *h->lock is
> > now held a single lock obtain/release cycle while struct CommandList
> > initialization is performed, and enqueued into HW.  This enqueuing
> > is done using a new h->lock unlocked __enqueue_cmd_and_start_io(),
> > wrapper and conversion of the the original enqueue_cmd_and_start_io()
> > to use this new code.
> > 
> > Reviewed-by: Jeff Garzik <jgarzik@redhat.com>
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/scsi/hpsa.c |   34 +++++++++++++++++++---------------
> >  1 files changed, 19 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 12deffc..fff7cd4 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -329,16 +329,25 @@ static void set_performant_mode(struct ctlr_info *h, struct CommandList *c)
> >                 c->busaddr |= 1 | (h->blockFetchTable[c->Header.SGList] << 1);
> >  }
> > 
> > -static void enqueue_cmd_and_start_io(struct ctlr_info *h,
> > +/*
> > + * Must be called with struct ctlr_info *h->lock held w/ interrupts disabled
> > + */
> > +static void __enqueue_cmd_and_start_io(struct ctlr_info *h,
> >         struct CommandList *c)
> >  {
> > -       unsigned long flags;
> > -
> >         set_performant_mode(h, c);
> > -       spin_lock_irqsave(&h->lock, flags);
> >         addQ(&h->reqQ, c);
> >         h->Qdepth++;
> >         start_io(h);
> > +}
> > +
> > +static void enqueue_cmd_and_start_io(struct ctlr_info *h,
> > +       struct CommandList *c)
> > +{
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&h->lock, flags);
> > +       __enqueue_cmd_and_start_io(h, c);
> >         spin_unlock_irqrestore(&h->lock, flags);
> >  }
> 
> Should that be "static inline"?  Or maybe the compiler's smart enough
> to decide whether to inline that on its own these days?
> 

Inlining both of these makes sense to me.  I will add this change and
send out an updated [PATCH] with your Reviewed-by shortly.

Thanks for your comments!

--nab

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

[Boaz Harrosh]

On 01/31/2011 10:26 PM, Nicholas A. Bellinger wrote:
>>> +static void enqueue_cmd_and_start_io(struct ctlr_info *h,
>>> +       struct CommandList *c)
>>> +{
>>> +       unsigned long flags;
>>> +
>>> +       spin_lock_irqsave(&h->lock, flags);
>>> +       __enqueue_cmd_and_start_io(h, c);
>>>         spin_unlock_irqrestore(&h->lock, flags);
>>>  }
>>
>> Should that be "static inline"?  Or maybe the compiler's smart enough
>> to decide whether to inline that on its own these days?
>>
> 
> Inlining both of these makes sense to me.  I will add this change and
> send out an updated [PATCH] with your Reviewed-by shortly.
> 

Don't do that. Yes in an header but in a .c file never do.

If there is only a single user the compiler will inline it.
if there are lots of users then the compiler will choose by
the compile-for-size or not and the function size. In any way
the compiler will make a better choice then you regarding size
and speed gained.

I never put inline in .c files. Just make sure that functions
are not forward declared. (that will kill the inline) and
the gcc will always makes the better choice.

Thanks
Boaz

> Thanks for your comments!
> 
> --nab