[PATCH] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[PATCH] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[Nicholas A. Bellinger]

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

This patch converts struct scsi_host->cmd_serial_number to a 'odd incremented by 2'
atomic_t counter in scsi_cmd_get_serial(), and moves the host_lock held call in
jgarzik's DEF_SCSI_QCMD() wrapper back into it's original location in scsi_dispatch_cmd().

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/hosts.c     |    1 +
 drivers/scsi/scsi.c      |   20 ++++++++++++++------
 include/scsi/scsi_host.h |    7 ++-----
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 4f7a582..317af8e 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -381,6 +381,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_tag = sht->ordered_tag;
+	atomic_set(&shost->cmd_serial_number, 1);
 
 	if (sht->supported_mode == MODE_UNKNOWN)
 		/* means we didn't set it ... default to INITIATOR */
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index e3d0629..bdc0837 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -632,15 +632,17 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
  * @cmd: command to assign serial number to
  *
  * Description: a serial number identifies a request for error recovery
- * and debugging purposes.  Protected by the Host_Lock of host.
+ * and debugging purposes.  Called directly by scsi_dispatch_cmd() for all LLDs
+ * after the great host_lock pushdown. 
  */
-void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
+static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
-	cmd->serial_number = host->cmd_serial_number++;
-	if (cmd->serial_number == 0) 
-		cmd->serial_number = host->cmd_serial_number++;
+	/*
+	 * 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);
 }
-EXPORT_SYMBOL(scsi_cmd_get_serial);
 
 /**
  * scsi_dispatch_command - Dispatch a command to the low-level driver.
@@ -736,6 +738,12 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 		scsi_done(cmd);
 		goto out;
 	}
+	/*
+	 * Assign a cmd->serial_number from host->cmd_serial_number that
+	 * is used by scsi_softirq_done() to signal scsi_try_to_abort_cmd()
+	 * that a outstanding cmd has been completed.
+	 */
+	scsi_cmd_get_serial(host, cmd);
 
 	if (unlikely(host->shost_state == SHOST_DEL)) {
 		cmd->result = (DID_NO_CONNECT << 16);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index cea74ba..6909038 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -518,7 +518,6 @@ struct scsi_host_template {
 		int rc;							\
 		struct Scsi_Host *shost = cmd->device->host;		\
 		spin_lock_irqsave(shost->host_lock, irq_flags);		\
-		scsi_cmd_get_serial(shost, cmd);			\
 		rc = func_name##_lck (cmd, done);			\
 		spin_unlock_irqrestore(shost->host_lock, irq_flags);	\
 		return rc;						\
@@ -625,10 +624,9 @@ struct Scsi_Host {
 	short unsigned int max_sectors;
 	unsigned long dma_boundary;
 	/* 
-	 * Used to assign serial numbers to the cmds.
-	 * Protected by the host lock.
+	 * Used to assign serial numbers to the cmds in scsi_cmd_get_serial()
 	 */
-	unsigned long cmd_serial_number;
+	atomic_t cmd_serial_number;
 	
 	unsigned active_mode:2;
 	unsigned unchecked_isa_dma:1;
@@ -773,7 +771,6 @@ extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
 extern void scsi_host_put(struct Scsi_Host *t);
 extern struct Scsi_Host *scsi_host_lookup(unsigned short);
 extern const char *scsi_host_state_name(enum scsi_host_state);
-extern void scsi_cmd_get_serial(struct Scsi_Host *, struct scsi_cmnd *);
 
 extern u64 scsi_calculate_bounce_limit(struct Scsi_Host *);
 
-- 
1.5.6.5

Re: [PATCH] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[James Bottomley]

On Thu, 2010-11-11 at 02:46 -0800, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch converts struct scsi_host->cmd_serial_number to a 'odd incremented by 2'
> atomic_t counter in scsi_cmd_get_serial(), and moves the host_lock held call in
> jgarzik's DEF_SCSI_QCMD() wrapper back into it's original location in scsi_dispatch_cmd().

Actually, no ... this isn't really a good idea; you're lengthening our
critical path here (an atomic costs a lot more than a simple add under
spinlock).

There are only a few drivers left that actually make use of a serial
number.  Of those, the only modern ones are qla4, lpfc, mpt2sas and
megaraid.

So the next logical step seems to be eliminate the overloading of the
serial number zero value, which removes the last mid-layer use (dpt_i2o
seems to abuse this unnecessarily as well), then the serial number code
can be pushed down into the queuecommand routines of only those drivers
that actually use it.  None of the modern ones seems to have a
legitimate use, so I think their uses can mostly be eliminated.  Thus,
we might be able to get away with a simple queuecommand push down and
never bother with atomics for this (since it's unlikely the legacy users
would convert away from a lock wrapping their queuecommand routines).

James

Re: [PATCH] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[Nicholas A. Bellinger]

On Thu, 2010-11-11 at 09:53 -0600, James Bottomley wrote:
> On Thu, 2010-11-11 at 02:46 -0800, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch converts struct scsi_host->cmd_serial_number to a 'odd incremented by 2'
> > atomic_t counter in scsi_cmd_get_serial(), and moves the host_lock held call in
> > jgarzik's DEF_SCSI_QCMD() wrapper back into it's original location in scsi_dispatch_cmd().
> 
> Actually, no ... this isn't really a good idea; you're lengthening our
> critical path here (an atomic costs a lot more than a simple add under
> spinlock).
> 

Fair enough, but this is only less expensive with a spinlock w/p
interrupts disabled, yes..?

> There are only a few drivers left that actually make use of a serial
> number.  Of those, the only modern ones are qla4, lpfc, mpt2sas and
> megaraid.
> 

I am not exactly sure that qla4 or lpfc is on the list of LLDs that use
and still need a cmd->serial_number for anything beyond simple printk()
purposes..  

> So the next logical step seems to be eliminate the overloading of the
> serial number zero value, which removes the last mid-layer use (dpt_i2o
> seems to abuse this unnecessarily as well), then the serial number code
> can be pushed down into the queuecommand routines of only those drivers
> that actually use it.

Correct, this is what we where originally doing in the
host_lock-less-for-37-v6 series here:

http://git.kernel.org/?p=linux/kernel/git/nab/lio-4.0.git;a=commitdiff;h=72a72033661506ead54e0f366218fd0aef2e5339

The LLDs that ended up requring the explict calls for:

mpt2sas: Add scsi_cmd_get_serial() call and set SHT->queuecommand_unlocked()
mpt/fusion: Add scsi_cmd_get_serial() call and set SHT->queuecommand_unlocked()
dpt_i2o: Add scsi_cmd_get_serial() call
eata: Add scsi_cmd_get_serial() call
u14-34f: Add scsi_cmd_get_serial() call

> None of the modern ones seems to have a
> legitimate use, so I think their uses can mostly be eliminated.

>From the above 5 LLDs, they all appear to be releated to using
cmd->serial_number in their internal error recovery handling.

> Thus,
> we might be able to get away with a simple queuecommand push down and
> never bother with atomics for this (since it's unlikely the legacy users
> would convert away from a lock wrapping their queuecommand routines).
> 

Sounds good to me, but you will recall the last attempt to make
scsi_cmd_get_serial() optional for the special case LLDs, that we
started running quickly in the legacy usage of cmd->serial_number in
scsi_softirq_done() and the side effects in scsi_try_to_abort_cmd(), who
use is complex enough that we have not found a proper resolution
sufficent to andmike discussed here:

http://marc.info/?l=linux-scsi&m=128535319915212&w=2
http://marc.info/?l=linux-scsi&m=128820726325009&w=2

Any takers..?  ;)

--nab

> James
> 
> 
> --
> 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] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[James Bottomley]

On Thu, 2010-11-11 at 13:37 -0800, Nicholas A. Bellinger wrote:
> On Thu, 2010-11-11 at 09:53 -0600, James Bottomley wrote:
> > On Thu, 2010-11-11 at 02:46 -0800, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > This patch converts struct scsi_host->cmd_serial_number to a 'odd incremented by 2'
> > > atomic_t counter in scsi_cmd_get_serial(), and moves the host_lock held call in
> > > jgarzik's DEF_SCSI_QCMD() wrapper back into it's original location in scsi_dispatch_cmd().
> > 
> > Actually, no ... this isn't really a good idea; you're lengthening our
> > critical path here (an atomic costs a lot more than a simple add under
> > spinlock).
> >
> 
> Fair enough, but this is only less expensive with a spinlock w/p
> interrupts disabled, yes..?

It's less expensive than the explicit lock around just the serial
number, yes ... but if all use cases use an explicit lock, we can just
use a simple inc in there without bothering with atomics.

> > There are only a few drivers left that actually make use of a serial
> > number.  Of those, the only modern ones are qla4, lpfc, mpt2sas and
> > megaraid.
> > 
> 
> I am not exactly sure that qla4 or lpfc is on the list of LLDs that use
> and still need a cmd->serial_number for anything beyond simple printk()
> purposes..  
> 
> > So the next logical step seems to be eliminate the overloading of the
> > serial number zero value, which removes the last mid-layer use (dpt_i2o
> > seems to abuse this unnecessarily as well), then the serial number code
> > can be pushed down into the queuecommand routines of only those drivers
> > that actually use it.
> 
> Correct, this is what we where originally doing in the
> host_lock-less-for-37-v6 series here:
> 
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-4.0.git;a=commitdiff;h=72a72033661506ead54e0f366218fd0aef2e5339
> 
> The LLDs that ended up requring the explict calls for:
> 
> mpt2sas: Add scsi_cmd_get_serial() call and set SHT->queuecommand_unlocked()
> mpt/fusion: Add scsi_cmd_get_serial() call and set SHT->queuecommand_unlocked()
> dpt_i2o: Add scsi_cmd_get_serial() call
> eata: Add scsi_cmd_get_serial() call
> u14-34f: Add scsi_cmd_get_serial() call
> 
> > None of the modern ones seems to have a
> > legitimate use, so I think their uses can mostly be eliminated.
> 
> >From the above 5 LLDs, they all appear to be releated to using
> cmd->serial_number in their internal error recovery handling.
> 
> > Thus,
> > we might be able to get away with a simple queuecommand push down and
> > never bother with atomics for this (since it's unlikely the legacy users
> > would convert away from a lock wrapping their queuecommand routines).
> > 
> 
> Sounds good to me, but you will recall the last attempt to make
> scsi_cmd_get_serial() optional for the special case LLDs, that we
> started running quickly in the legacy usage of cmd->serial_number in
> scsi_softirq_done() and the side effects in scsi_try_to_abort_cmd(), who
> use is complex enough that we have not found a proper resolution
> sufficent to andmike discussed here:

Yes, that's what I meant by "eliminate the overloading of the serial
number zero value" above.  This needs fixing before the serial number
can be dumped for fast hba drivers.

James


> http://marc.info/?l=linux-scsi&m=128535319915212&w=2
> http://marc.info/?l=linux-scsi&m=128820726325009&w=2
> 
> Any takers..?  ;)
> 
> --nab
> 
> > James
> > 
> > 
> > --
> > 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] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[Nicholas A. Bellinger]

On Thu, 2010-11-11 at 15:55 -0600, James Bottomley wrote:
> On Thu, 2010-11-11 at 13:37 -0800, Nicholas A. Bellinger wrote:
> > On Thu, 2010-11-11 at 09:53 -0600, James Bottomley wrote:
> > > On Thu, 2010-11-11 at 02:46 -0800, Nicholas A. Bellinger wrote:
> > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > 
> > > > This patch converts struct scsi_host->cmd_serial_number to a 'odd incremented by 2'
> > > > atomic_t counter in scsi_cmd_get_serial(), and moves the host_lock held call in
> > > > jgarzik's DEF_SCSI_QCMD() wrapper back into it's original location in scsi_dispatch_cmd().
> > > 
> > > Actually, no ... this isn't really a good idea; you're lengthening our
> > > critical path here (an atomic costs a lot more than a simple add under
> > > spinlock).
> > >
> > 
> > Fair enough, but this is only less expensive with a spinlock w/p
> > interrupts disabled, yes..?
> 
> It's less expensive than the explicit lock around just the serial
> number, yes ... but if all use cases use an explicit lock, we can just
> use a simple inc in there without bothering with atomics.
> 

Sure, that makes sense for the legacy drivers running in host_lock
mode..  But with the fully lock-less LLDs (like the freshly minted
tcm_loop patch to go fully lock-less), this is a easier transitional
step from:

full host_lock push down ->

	lock_less w/ atomic_t host serial_number counter ->

		lock_less w/o atomic cmd->serial_number counter + 
                scsi_error.c changes

So my tenative plan is to enable the host_lock less LLDs we know about
(include tcm_loop and core-iscsi) with the atomic_t serial number
counter, and continue getting polling feedback from various LLD
maintainers..  This part will be tagged as a for-38 branch.

> > > There are only a few drivers left that actually make use of a serial
> > > number.  Of those, the only modern ones are qla4, lpfc, mpt2sas and
> > > megaraid.
> > > 
> > 
> > I am not exactly sure that qla4 or lpfc is on the list of LLDs that use
> > and still need a cmd->serial_number for anything beyond simple printk()
> > purposes..  
> > 
> > > So the next logical step seems to be eliminate the overloading of the
> > > serial number zero value, which removes the last mid-layer use (dpt_i2o
> > > seems to abuse this unnecessarily as well), then the serial number code
> > > can be pushed down into the queuecommand routines of only those drivers
> > > that actually use it.
> > 
> > Correct, this is what we where originally doing in the
> > host_lock-less-for-37-v6 series here:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/nab/lio-4.0.git;a=commitdiff;h=72a72033661506ead54e0f366218fd0aef2e5339
> > 
> > The LLDs that ended up requring the explict calls for:
> > 
> > mpt2sas: Add scsi_cmd_get_serial() call and set SHT->queuecommand_unlocked()
> > mpt/fusion: Add scsi_cmd_get_serial() call and set SHT->queuecommand_unlocked()
> > dpt_i2o: Add scsi_cmd_get_serial() call
> > eata: Add scsi_cmd_get_serial() call
> > u14-34f: Add scsi_cmd_get_serial() call
> > 
> > > None of the modern ones seems to have a
> > > legitimate use, so I think their uses can mostly be eliminated.
> > 
> > >From the above 5 LLDs, they all appear to be releated to using
> > cmd->serial_number in their internal error recovery handling.
> > 
> > > Thus,
> > > we might be able to get away with a simple queuecommand push down and
> > > never bother with atomics for this (since it's unlikely the legacy users
> > > would convert away from a lock wrapping their queuecommand routines).
> > > 
> > 
> > Sounds good to me, but you will recall the last attempt to make
> > scsi_cmd_get_serial() optional for the special case LLDs, that we
> > started running quickly in the legacy usage of cmd->serial_number in
> > scsi_softirq_done() and the side effects in scsi_try_to_abort_cmd(), who
> > use is complex enough that we have not found a proper resolution
> > sufficent to andmike discussed here:
> 
> Yes, that's what I meant by "eliminate the overloading of the serial
> number zero value" above.  This needs fixing before the serial number
> can be dumped for fast hba drivers.
> 

Understood, so at least for the moment this is not an immediate
addressable item unless andmike, hch or someone else wants to take
another look at extracting the ghosts from this code.

Until then I will keep the atomic_t serial_number counter in
lio-core-2.6.git code, and plan to push the usage back down for the 5
legacy LLDs once scsi_error.c can be sorted out.

Best,

--nab

Re: [PATCH] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[Mike Anderson]

James Bottomley <James.Bottomley@suse.de> wrote:
> On Thu, 2010-11-11 at 13:37 -0800, Nicholas A. Bellinger wrote:
> >
> >  ..snip..
> >
> > Sounds good to me, but you will recall the last attempt to make
> > scsi_cmd_get_serial() optional for the special case LLDs, that we
> > started running quickly in the legacy usage of cmd->serial_number in
> > scsi_softirq_done() and the side effects in scsi_try_to_abort_cmd(), who
> > use is complex enough that we have not found a proper resolution
> > sufficent to andmike discussed here:
> 
> Yes, that's what I meant by "eliminate the overloading of the serial
> number zero value" above.  This needs fixing before the serial number
> can be dumped for fast hba drivers.
> 

In the last email referenced below I believed that since
scsi_softirq_done is calling scsi_eh_scmd_add without the
SCSI_EH_CANCEL_CMD flag set this will stop scsi_try_to_abort_cmd from
being called. Since scsi_softirq_done is the one setting serial_number
to 0 I do not believe we can hit the serial number == 0 check in
scsi_try_to_abort_cmd anymore.

> > http://marc.info/?l=linux-scsi&m=128820726325009&w=2

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

Re: [PATCH] scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

[James Bottomley]

On Thu, 2010-11-11 at 14:36 -0800, Mike Anderson wrote:
> James Bottomley <James.Bottomley@suse.de> wrote:
> > On Thu, 2010-11-11 at 13:37 -0800, Nicholas A. Bellinger wrote:
> > >
> > >  ..snip..
> > >
> > > Sounds good to me, but you will recall the last attempt to make
> > > scsi_cmd_get_serial() optional for the special case LLDs, that we
> > > started running quickly in the legacy usage of cmd->serial_number in
> > > scsi_softirq_done() and the side effects in scsi_try_to_abort_cmd(), who
> > > use is complex enough that we have not found a proper resolution
> > > sufficent to andmike discussed here:
> > 
> > Yes, that's what I meant by "eliminate the overloading of the serial
> > number zero value" above.  This needs fixing before the serial number
> > can be dumped for fast hba drivers.
> > 
> 
> In the last email referenced below I believed that since
> scsi_softirq_done is calling scsi_eh_scmd_add without the
> SCSI_EH_CANCEL_CMD flag set this will stop scsi_try_to_abort_cmd from
> being called. Since scsi_softirq_done is the one setting serial_number
> to 0 I do not believe we can hit the serial number == 0 check in
> scsi_try_to_abort_cmd anymore.
> 
> > > http://marc.info/?l=linux-scsi&m=128820726325009&w=2

I buy this.  The REQ_ATOM_COMPLETE flag in the block layer now mediates
timer vs completion atomically ... so either one or the other is allowed
to occur.

James