Skip to contentSkip to conversation

SCSI: Make cmd_serial_number an atomic

PATCH In review Matthew Wilcox 5 participants 10 messages Apr 1, 2011 – Apr 3, 2011 SCSI

lore
Share & tools
lore

Matthew Wilcox

author
#0 lore raw

PATCH 1/3 SCSI: Make cmd_serial_number an atomic

In preparation for moving some drivers out from under the host_lock, make cmd_serial_number an atomic.

Signed-off-by Matthew Wilcox <[email protected]>
drivers/scsi/scsi.c +5 -4
index 2aeb2e9..13a3bf0 100644
@@ -631,14 +631,15 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
  * @host: the scsi host
  * @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.
+ * A serial number identifies a request for error recovery and debugging
+ * purposes.
  */
 void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
-	cmd->serial_number = host->cmd_serial_number++;
+ again:
+	cmd->serial_number = atomic_inc_return(&host->cmd_serial_number);
 	if (cmd->serial_number == 0) 
-		cmd->serial_number = host->cmd_serial_number++;
+		goto again;
 }
 EXPORT_SYMBOL(scsi_cmd_get_serial);
include/scsi/scsi_host.h +3 -5
index e7e3858..69dfabb 100644
@@ -621,11 +621,9 @@ struct Scsi_Host {
 	short unsigned int sg_prot_tablesize;
 	short unsigned int max_sectors;
 	unsigned long dma_boundary;
-	/* 
-	 * Used to assign serial numbers to the cmds.
-	 * Protected by the host lock.
-	 */
-	unsigned long cmd_serial_number;
+
+	/* Used to assign serial numbers to the cmds.  */
+	atomic_t cmd_serial_number;
 	
 	unsigned active_mode:2;
 	unsigned unchecked_isa_dma:1;
#1 lore raw
Quoted 3 lines · Matthew Wilcox

In preparation for moving some drivers out from under the host_lock, make cmd_serial_number an atomic.

The right fix is to stop abusing cmd_serial_nubmer in those few drivers, and use a driver-private and more scalable lookup data structure than a linear list search.

And yes, I promised to at least move cmd_serial_number into these drivers long time ago - time to get back to that.

#2 lore raw
Quoted 11 lines · Christoph Hellwig
Quoted 3 lines · Matthew Wilcox

In preparation for moving some drivers out from under the host_lock, make cmd_serial_number an atomic.

The right fix is to stop abusing cmd_serial_nubmer in those few drivers, and use a driver-private and more scalable lookup data structure than a linear list search.

And yes, I promised to at least move cmd_serial_number into these drivers long time ago - time to get back to that.

Gosh, I'm glad you said that ... I was afraid it would sound like I was a Nag.

James

#3 lore raw
In reply to Christoph Hellwig
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Matthew Wilcox

In preparation for moving some drivers out from under the host_lock, make cmd_serial_number an atomic.

The right fix is to stop abusing cmd_serial_nubmer in those few drivers, and use a driver-private and more scalable lookup data structure than a linear list search.

Agreed.

#4 lore raw
In reply to Christoph Hellwig
Quoted 11 lines · Christoph Hellwig
Quoted 3 lines · Matthew Wilcox

In preparation for moving some drivers out from under the host_lock, make cmd_serial_number an atomic.

The right fix is to stop abusing cmd_serial_nubmer in those few drivers, and use a driver-private and more scalable lookup data structure than a linear list search.

And yes, I promised to at least move cmd_serial_number into these drivers long time ago - time to get back to that.

Fair enough, but you're making the perfect the enemy of the good. How about putting this patch in for now (since it harms nothing), and then it'll all go away when you delete cmd_serial_number?

Signature · 4 lines
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#5 lore raw
Quoted 3 lines · Matthew Wilcox

Fair enough, but you're making the perfect the enemy of the good. How about putting this patch in for now (since it harms nothing), and then it'll all go away when you delete cmd_serial_number?

If you change it anyway for mpt2sas just keep the atomic counter in it's local structures. Or even better verify with LSI if the serial_number check can't simply be removed entirely, which I think it could.

#6 lore raw
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Matthew Wilcox

Fair enough, but you're making the perfect the enemy of the good. How about putting this patch in for now (since it harms nothing), and then it'll all go away when you delete cmd_serial_number?

If you change it anyway for mpt2sas just keep the atomic counter in it's local structures. Or even better verify with LSI if the serial_number check can't simply be removed entirely, which I think it could.

Also I'm not really sure atomic counter conversion is the correct thing to do. Someone from intel said it would perform worse than the current spinlock based serial number the last time we discussed the conversion (converting it to atomic was my first thought at the time).

James

#7 lore raw
In reply to Christoph Hellwig
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Matthew Wilcox

Fair enough, but you're making the perfect the enemy of the good. How about putting this patch in for now (since it harms nothing), and then it'll all go away when you delete cmd_serial_number?

If you change it anyway for mpt2sas just keep the atomic counter in it's local structures. Or even better verify with LSI if the serial_number check can't simply be removed entirely, which I think it could.

Hm, yeah, it looks like it's only used for checking whether the command we're aborting is the same command we're using to do the aborts. In which case, can't we simply do this? Eric?

drivers/scsi/mpt2sas/mpt2sas_scsih.c +1 -2
index 6ceb775..bce9043 100644
@@ -2133,8 +2133,7 @@ mpt2sas_scsih_issue_tm(struct MPT2SAS_ADAPTER *ioc, u16 handle, uint channel,
 	switch (type) {
 	case MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK:
 		scmd_lookup = _scsih_scsi_lookup_get(ioc, smid_task);
-		if (scmd_lookup && (scmd_lookup->serial_number ==
-		    scmd->serial_number))
+		if (scmd_lookup == scmd))
 			rc = FAILED;
 		else
 			rc = SUCCESS;
Signature · 4 lines
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#8 lore raw
Quoted 3 lines · Matthew Wilcox

Hm, yeah, it looks like it's only used for checking whether the command we're aborting is the same command we're using to do the aborts. In which case, can't we simply do this? Eric?

A similar question also applies for the old fusion driver. There it even compares the serial number with one taken from the same scsi_cmnd in the same function. If we could reuse a scsi_cmnd while in the ->eh_abort handler for fusion it would have to seriously mess up it's locking and lifetime rules.

#9 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 3 lines · Matthew Wilcox

Hm, yeah, it looks like it's only used for checking whether the command we're aborting is the same command we're using to do the aborts. In which case, can't we simply do this? Eric?

A similar question also applies for the old fusion driver. There it even compares the serial number with one taken from the same scsi_cmnd in the same function. If we could reuse a scsi_cmnd while in the ->eh_abort handler for fusion it would have to seriously mess up it's locking and lifetime rules.

Well, I think there's method in the madness for the old driver. What it seems to worry about is that the command gets reissued after the abort because the abort routine sleeps. If the reissue occurred, the same command could be on the active list with a different serial number.

The theory is currently bogus because we halt all activity on the host while error handler porcessing is active and the reissue won't occur until after the abort routine returns. Although if we did running error handling, problems like this would arise ...

The new code seems to be an incorrect extract from the old code, so both can currently go.

James