[PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Nicholas A. Bellinger]

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

Greetings all,

This series contains the first round of a whole-sale conversion for droping
struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
that is protected by host_lock is scsi_cmd_get_serial)_.

The patches #2 -> #8 converts libiscsi, libsas, lpfc, qla4xxx, qla2xxx,
TCM_Loop, and libfc code to drop their own SHT->queuecommand() internal
host_lock unlock() + lock() optimization that certain high performance libs
and LLDs have adopted over the years.  The changes involved here are really
quite straight forward, but please note that none of this code has been tested
with actual hardware yet, and is intended for generating comments for the relivent
SCSI LLD driver maintainers and other interested folks.

There are probably still be more LLDs which use this optimization, so please
let me know if there is another piece of SHT->queuecommand() caller code that
does this legacy optimization and needs to be updated.  I will be sure to
CC the right driver maintainers and CC linux-scsi as I come across any more
conversions.

Many thanks to Vasu Dev and Tim Chen for their work in this area!!

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

Nicholas Bellinger (8):
  scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()
  libiscsi: Remove host_lock unlock() + lock() from
    iscsi_queuecommand()
  libsas: Remove host_lock unlock() + lock() from sas_queuecommand()
  lpfc: Remove host_lock unlock() + lock() from lpfc_queuecommand()
  qla4xxx: Remove host_lock unlock() + lock() from
    qla4xxx_queuecommand()
  qla2xxx: Remove host_lock unlock() + lock() from
    qla2xxx_queuecommand()
  tcm_loop: Remove host_lock unlock() + lock() from
    tcm_loop_queuecommand()
  libfc: Remove host_lock unlock() + lock() from fc_queuecommand()

 drivers/scsi/libfc/fc_fcp.c                    |    5 +----
 drivers/scsi/libiscsi.c                        |    4 ----
 drivers/scsi/libsas/sas_scsi_host.c            |    5 -----
 drivers/scsi/lpfc/lpfc_scsi.c                  |    2 --
 drivers/scsi/qla2xxx/qla_os.c                  |    7 ++-----
 drivers/scsi/qla4xxx/ql4_os.c                  |    8 +-------
 drivers/scsi/scsi.c                            |    3 ++-
 drivers/target/tcm_loop/tcm_loop_fabric_scsi.c |    7 -------
 8 files changed, 6 insertions(+), 35 deletions(-)

-- 
1.7.2.3

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Greetings all,
> 
> This series contains the first round of a whole-sale conversion for droping
> struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
> scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
> that is protected by host_lock is scsi_cmd_get_serial)_.
> 
> The patches #2 -> #8 converts libiscsi, libsas, lpfc, qla4xxx, qla2xxx,
> TCM_Loop, and libfc code to drop their own SHT->queuecommand() internal
> host_lock unlock() + lock() optimization that certain high performance libs
> and LLDs have adopted over the years.  The changes involved here are really
> quite straight forward, but please note that none of this code has been tested
> with actual hardware yet, and is intended for generating comments for the relivent
> SCSI LLD driver maintainers and other interested folks.
> 
> There are probably still be more LLDs which use this optimization, so please
> let me know if there is another piece of SHT->queuecommand() caller code that
> does this legacy optimization and needs to be updated.  I will be sure to
> CC the right driver maintainers and CC linux-scsi as I come across any more
> conversions.
> 
> Many thanks to Vasu Dev and Tim Chen for their work in this area!!

Also just FYI, I have pushed these changes into a branch here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=shortlog;h=refs/heads/drop-host_lock

Note that this tree is currently at .36-rc3, but will be jumping to -rc4
shortly.

Thanks!

--nab

> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> 
> Nicholas Bellinger (8):
>   scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand()
>   libiscsi: Remove host_lock unlock() + lock() from
>     iscsi_queuecommand()
>   libsas: Remove host_lock unlock() + lock() from sas_queuecommand()
>   lpfc: Remove host_lock unlock() + lock() from lpfc_queuecommand()
>   qla4xxx: Remove host_lock unlock() + lock() from
>     qla4xxx_queuecommand()
>   qla2xxx: Remove host_lock unlock() + lock() from
>     qla2xxx_queuecommand()
>   tcm_loop: Remove host_lock unlock() + lock() from
>     tcm_loop_queuecommand()
>   libfc: Remove host_lock unlock() + lock() from fc_queuecommand()
> 
>  drivers/scsi/libfc/fc_fcp.c                    |    5 +----
>  drivers/scsi/libiscsi.c                        |    4 ----
>  drivers/scsi/libsas/sas_scsi_host.c            |    5 -----
>  drivers/scsi/lpfc/lpfc_scsi.c                  |    2 --
>  drivers/scsi/qla2xxx/qla_os.c                  |    7 ++-----
>  drivers/scsi/qla4xxx/ql4_os.c                  |    8 +-------
>  drivers/scsi/scsi.c                            |    3 ++-
>  drivers/target/tcm_loop/tcm_loop_fabric_scsi.c |    7 -------
>  8 files changed, 6 insertions(+), 35 deletions(-)
> 

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Greetings all,
> 
> This series contains the first round of a whole-sale conversion for droping
> struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
> scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
> that is protected by host_lock is scsi_cmd_get_serial)_.
> 
> The patches #2 -> #8 converts libiscsi, libsas, lpfc, qla4xxx, qla2xxx,
> TCM_Loop, and libfc code to drop their own SHT->queuecommand() internal
> host_lock unlock() + lock() optimization that certain high performance libs
> and LLDs have adopted over the years.  The changes involved here are really
> quite straight forward, but please note that none of this code has been tested
> with actual hardware yet, and is intended for generating comments for the relivent
> SCSI LLD driver maintainers and other interested folks.
> 
> There are probably still be more LLDs which use this optimization, so please
> let me know if there is another piece of SHT->queuecommand() caller code that
> does this legacy optimization and needs to be updated.  I will be sure to
> CC the right driver maintainers and CC linux-scsi as I come across any more
> conversions.

Hi Guys,

drivers/scsi/fnic needs the conversion as well..  Here is that patch;

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=3c886b2ab6faccbe2803094deb7a9647199e5118

Thanks!

--nab

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 16:01 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Greetings all,
> > 
> > This series contains the first round of a whole-sale conversion for droping
> > struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
> > scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
> > that is protected by host_lock is scsi_cmd_get_serial)_.
> > 
> > The patches #2 -> #8 converts libiscsi, libsas, lpfc, qla4xxx, qla2xxx,
> > TCM_Loop, and libfc code to drop their own SHT->queuecommand() internal
> > host_lock unlock() + lock() optimization that certain high performance libs
> > and LLDs have adopted over the years.  The changes involved here are really
> > quite straight forward, but please note that none of this code has been tested
> > with actual hardware yet, and is intended for generating comments for the relivent
> > SCSI LLD driver maintainers and other interested folks.
> > 
> > There are probably still be more LLDs which use this optimization, so please
> > let me know if there is another piece of SHT->queuecommand() caller code that
> > does this legacy optimization and needs to be updated.  I will be sure to
> > CC the right driver maintainers and CC linux-scsi as I come across any more
> > conversions.
> 
> Hi Guys,
> 
> drivers/scsi/fnic needs the conversion as well..  Here is that patch;
> 
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=3c886b2ab6faccbe2803094deb7a9647199e5118
> 

Ok, after doing some more digging the BusLogic and the ever important
libata SCSI glue code use the legacy host_lock unlock() -> do_work ->
lock() optimization as well.  Here are those patches (jgarzik CC'ed):

buslogic: Remove host_lock unlock() + lock() from BusLogic_QueueCommand()
http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=b465dfeb8e6950faa9c074681c72ca0a6c21276c

libata: Remove host_lock unlock() + lock() from ata_scsi_queuecmd()
http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=29f2ded8876910d7eb84e4d270188de6f8a8c8ee

Thanks!

--nab

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Chetan Loke]

On Thu, Sep 16, 2010 at 6:35 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:

> and LLDs have adopted over the years.  The changes involved here are really
> quite straight forward, but please note that none of this code has been tested
> with actual hardware yet, and is intended for generating comments for the relivent
> SCSI LLD driver maintainers and other interested folks.
>

NO. Why not try testing it w/ an abort storm first else we will
seriously end up screwing the filesystems....
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 19:26 -0400, Chetan Loke wrote:
> On Thu, Sep 16, 2010 at 6:35 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> 
> > and LLDs have adopted over the years.  The changes involved here are really
> > quite straight forward, but please note that none of this code has been tested
> > with actual hardware yet, and is intended for generating comments for the relivent
> > SCSI LLD driver maintainers and other interested folks.
> >
> 
> NO. Why not try testing it w/ an abort storm first else we will
> seriously end up screwing the filesystems....

The only real issue here would be an immediate struct
Scsi_Host->host_lock dead lock for those LLDs that still use legacy
unlock() > do_work() -> lock() in their SHT->queuecommand() caller.
This is *not* going to happen under heavy load with a bunch of
outstanding WRITEs, but immediate the first couple of times that
scsi_dispatch_cmd() gets called, so there is really little to no fear of
filesystem corruption with these patches.

Anyways, please feel free to test them yourself with an TMR ABORT storm
if you so desire..

Best,

--nab

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Tim Chen]

On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Greetings all,
> 
> This series contains the first round of a whole-sale conversion for droping
> struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
> scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
> that is protected by host_lock is scsi_cmd_get_serial)_.
> 

Maybe we can change the host->cmd_serial_number to atomic and totally
avoid the need to take host_lock.

Tim

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Nicholas A. Bellinger]

On Thu, 2010-09-16 at 18:29 -0700, Tim Chen wrote:
> On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Greetings all,
> > 
> > This series contains the first round of a whole-sale conversion for droping
> > struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
> > scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
> > that is protected by host_lock is scsi_cmd_get_serial)_.
> > 
> 
> Maybe we can change the host->cmd_serial_number to atomic and totally
> avoid the need to take host_lock.

Hmmmm good point, then we would also need an atomic_t for signaling
(shost_state == SHOST_DEL) in scsi_dispatch_cmd() to be able to
completly drop host_lock usage within scsi_dispatch_cmd().

I can take a look at doing this specific part, if you would be willing
to have a look at the host->cmd_serial_number conversion piece.  ;)

Thanks Tim!

--nab

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Jeff Garzik]

On 09/16/2010 09:31 PM, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 18:29 -0700, Tim Chen wrote:
>> On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger<nab@linux-iscsi.org>
>>>
>>> Greetings all,
>>>
>>> This series contains the first round of a whole-sale conversion for droping
>>> struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
>>> scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
>>> that is protected by host_lock is scsi_cmd_get_serial)_.
>>>
>>
>> Maybe we can change the host->cmd_serial_number to atomic and totally
>> avoid the need to take host_lock.
>
> Hmmmm good point, then we would also need an atomic_t for signaling
> (shost_state == SHOST_DEL) in scsi_dispatch_cmd() to be able to
> completly drop host_lock usage within scsi_dispatch_cmd().
>
> I can take a look at doing this specific part, if you would be willing
> to have a look at the host->cmd_serial_number conversion piece.  ;)

But that raises the familiar tale of:  using multiple atomics (w/ their 
locked instructions) may cost more than a spinlock.

	Jeff

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Nicholas A. Bellinger]

On Fri, 2010-09-17 at 14:26 -0400, Jeff Garzik wrote:
> On 09/16/2010 09:31 PM, Nicholas A. Bellinger wrote:
> > On Thu, 2010-09-16 at 18:29 -0700, Tim Chen wrote:
> >> On Thu, 2010-09-16 at 15:35 -0700, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger<nab@linux-iscsi.org>
> >>>
> >>> Greetings all,
> >>>
> >>> This series contains the first round of a whole-sale conversion for droping
> >>> struct Scsi_Host->host_lock around struct Scsi_Host->queuecommand() within
> >>> scsi_dispatch_cmd().  So with this first patch the only part of scsi_dispatch_cmd()
> >>> that is protected by host_lock is scsi_cmd_get_serial)_.
> >>>
> >>
> >> Maybe we can change the host->cmd_serial_number to atomic and totally
> >> avoid the need to take host_lock.
> >
> > Hmmmm good point, then we would also need an atomic_t for signaling
> > (shost_state == SHOST_DEL) in scsi_dispatch_cmd() to be able to
> > completly drop host_lock usage within scsi_dispatch_cmd().
> >
> > I can take a look at doing this specific part, if you would be willing
> > to have a look at the host->cmd_serial_number conversion piece.  ;)
> 
> But that raises the familiar tale of:  using multiple atomics (w/ their 
> locked instructions) may cost more than a spinlock.

So patch #1 in the v2 series just posted uses the following code:

static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
{
        /*
         * The use of per struct scsi_cmnd->serial_number is disabled by default
         */
        if (!(host->use_serial_number))
                return;
        /*
         * Increment the host->cmd_serial_number by 2 so cmd->serial_number
         * is always odd and wraps to 1 instead of 0.
         */
        cmd->serial_number = atomic_add_return(2, &host->cmd_serial_number);
}


Note that host->use_serial_number is disabled by default, so for the typical case
no atomic_t reading (or writing) is required.

Best,

--nab

Re: [PATCH 0/8] Drop host_lock around LLD SHT->queuecommand() caller

[Tim Chen]

On Fri, 2010-09-17 at 14:26 -0400, Jeff Garzik wrote:

> 
> But that raises the familiar tale of:  using multiple atomics (w/ their 
> locked instructions) may cost more than a spinlock.
> 

As brought up by James earlier, the scsi_host lock was used in so many
places that getting rid of it here to reduce contention for it will be a
net win, even with the cost of the atomics.  And for most LLDs that
don't need the serial number, we can just initialize it to zero for
those and have no need to increment it (thus also avoiding the atomics'
cost).

Tim