[PATCH] softirq queue is now list_head, eliminate bh_next

[PATCH] softirq queue is now list_head, eliminate bh_next

[Luben Tuikov]

The following patch gets rid of softscsi_data struct and
array for the more manageable
static struct list_head done_q[NR_CPUS] __cacheline_aligned;

Thus, scsi_cmnd::bh_next is eliminated, since it was used only
in the scsi softirq processing code.

The comments are updated.

80 chars per line for the affected functions: scsi_done()
and scsi_softirq().

Eliminated is the double loop in scsi_softirq() -- this is
better handled in do_softirq() and gives the system a ``breather''.
(There are pros and cons for either side and if you guys
think that it was better with the double loop, I'll change it and
resubmit the patch.)

-- 
Luben

diff -Naur -X dontdiff linux-2.5.64/drivers/scsi.orig/scsi.c linux-2.5.64/drivers/scsi/scsi.c
--- linux-2.5.64/drivers/scsi.orig/scsi.c	2003-03-04 22:29:19.000000000 -0500
+++ linux-2.5.64/drivers/scsi/scsi.c	2003-03-07 14:28:43.000000000 -0500
@@ -96,12 +96,7 @@
  Scsi_Cmnd *last_cmnd;
  static unsigned long serial_number;

-struct softscsi_data {
-	Scsi_Cmnd *head;
-	Scsi_Cmnd *tail;
-};
-
-static struct softscsi_data softscsi_data[NR_CPUS] __cacheline_aligned;
+static struct list_head done_q[NR_CPUS] __cacheline_aligned;

  /*
   * List of all highlevel drivers.
@@ -637,79 +632,60 @@
  }

  /**
- * scsi_done - Mark this command as done
- * @SCpnt: The SCSI Command which we think we've completed.
- *
- * This function is the mid-level interrupt routine, which decides how
- * to handle error conditions.  Each invocation of this function must
- * do one and *only* one of the following:
+ * scsi_done - Enqueue the finished SCSI command into the done queue.
+ * @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
+ * ownership back to SCSI Core -- i.e. the LLDD has finished with it.
   *
- *      1) Insert command in BH queue.
- *      2) Activate error handler for host.
+ * This function is the mid-level's (SCSI Core) interrupt routine, which
+ * regains ownership of the SCSI command (de facto) from a LLDD, and enqueues
+ * the command to the done queue for further processing.
   *
- * There is no longer a problem with stack overflow.  Interrupts queue
- * Scsi_Cmnd on a per-CPU queue and the softirq handler removes them
- * from the queue one at a time.
+ * This is the producer of the done queue who enqueues at the tail.
   *
- * This function is sometimes called from interrupt context, but sometimes
- * from task context.
+ * This function is interrupt context safe.
   */
-void scsi_done(Scsi_Cmnd * SCpnt)
+void scsi_done(struct scsi_cmnd *cmd)
  {
+	int cpu;
  	unsigned long flags;
-	int cpu, tstatus;
-	struct softscsi_data *queue;
+	struct list_head *pdone_q;

  	/*
  	 * We don't have to worry about this one timing out any more.
-	 */
-	tstatus = scsi_delete_timer(SCpnt);
-
-	/*
-	 * If we are unable to remove the timer, it means that the command
-	 * has already timed out.  In this case, we have no choice but to
+	 * If we are unable to remove the timer, then the command
+	 * has already timed out.  In which case, we have no choice but to
  	 * let the timeout function run, as we have no idea where in fact
  	 * that function could really be.  It might be on another processor,
  	 * etc, etc.
  	 */
-	if (!tstatus) {
+	if (!scsi_delete_timer(cmd))
  		return;
-	}

  	/* Set the serial numbers back to zero */
-	SCpnt->serial_number = 0;
-	SCpnt->serial_number_at_timeout = 0;
-	SCpnt->state = SCSI_STATE_BHQUEUE;
-	SCpnt->owner = SCSI_OWNER_BH_HANDLER;
-	SCpnt->bh_next = NULL;
+	cmd->serial_number = 0;
+	cmd->serial_number_at_timeout = 0;
+	cmd->state = SCSI_STATE_BHQUEUE;
+	cmd->owner = SCSI_OWNER_BH_HANDLER;

  	/*
-	 * Next, put this command in the softirq queue.
-	 *
-	 * This is a per-CPU queue, so we just disable local interrupts
+	 * Next, enqueue the command into the done queue.
+	 * It is a per-CPU queue, so we just disable local interrupts
  	 * and need no spinlock.
  	 */
-
  	local_irq_save(flags);

  	cpu = smp_processor_id();
-	queue = &softscsi_data[cpu];
-
-	if (!queue->head) {
-		queue->head = SCpnt;
-		queue->tail = SCpnt;
-	} else {
-		queue->tail->bh_next = SCpnt;
-		queue->tail = SCpnt;
-	}
-
+	pdone_q = &done_q[cpu];
+	list_add_tail(&cmd->eh_entry, pdone_q);
  	cpu_raise_softirq(cpu, SCSI_SOFTIRQ);

  	local_irq_restore(flags);
-}
+} /* end scsi_done() */

  /**
- * scsi_softirq - Perform post-interrupt handling for completed commands
+ * scsi_softirq - Perform post-interrupt processing of finished SCSI commands.
+ *
+ * This is the consumer of the done queue.
   *
   * This is called with all interrupts enabled.  This should reduce
   * interrupt latency, stack depth, and reentrancy of the low-level
@@ -717,89 +693,93 @@
   */
  static void scsi_softirq(struct softirq_action *h)
  {
-	int cpu = smp_processor_id();
-	struct softscsi_data *queue = &softscsi_data[cpu];
+	LIST_HEAD(local_q);

-	while (queue->head) {
-		Scsi_Cmnd *SCpnt, *SCnext;
+	local_irq_disable();
+	list_splice_init(&done_q[smp_processor_id()], &local_q);
+	local_irq_enable();
+
+	while (!list_empty(&local_q)) {
+		struct scsi_cmnd *cmd = list_entry(local_q.next,
+						   struct scsi_cmnd, eh_entry);
+		list_del_init(&cmd->eh_entry);

-		local_irq_disable();
-		SCpnt = queue->head;
-		queue->head = NULL;
-		local_irq_enable();
+		switch (scsi_decide_disposition(cmd)) {
+		case SUCCESS:
+			/*
+			 * Add to BH queue.
+			 */
+			SCSI_LOG_MLCOMPLETE(3,
+					    printk("Command finished %d %d "
+						   "0x%x\n",
+					   cmd->device->host->host_busy,
+					   cmd->device->host->host_failed,
+						   cmd->result));

-		for (; SCpnt; SCpnt = SCnext) {
-			SCnext = SCpnt->bh_next;
+			scsi_finish_command(cmd);
+			break;
+		case NEEDS_RETRY:
+			/*
+			 * We only come in here if we want to retry a
+			 * command.  The test to see whether the
+			 * command should be retried should be keeping
+			 * track of the number of tries, so we don't
+			 * end up looping, of course.
+			 */
+			SCSI_LOG_MLCOMPLETE(3, printk("Command needs retry "
+						      "%d %d 0x%x\n",
+					      cmd->device->host->host_busy,
+					      cmd->device->host->host_failed,
+						      cmd->result));

-			switch (scsi_decide_disposition(SCpnt)) {
-			case SUCCESS:
-				/*
-				 * Add to BH queue.
-				 */
-				SCSI_LOG_MLCOMPLETE(3, printk("Command finished %d %d 0x%x\n", SCpnt->device->host->host_busy,
-						SCpnt->device->host->host_failed,
-							 SCpnt->result));
-
-				scsi_finish_command(SCpnt);
-				break;
-			case NEEDS_RETRY:
-				/*
-				 * We only come in here if we want to retry a
-				 * command.  The test to see whether the
-				 * command should be retried should be keeping
-				 * track of the number of tries, so we don't
-				 * end up looping, of course.
-				 */
-				SCSI_LOG_MLCOMPLETE(3, printk("Command needs retry %d %d 0x%x\n", SCpnt->device->host->host_busy,
-				SCpnt->device->host->host_failed, SCpnt->result));
+			scsi_retry_command(cmd);
+			break;
+		case ADD_TO_MLQUEUE:
+			/*
+			 * This typically happens for a QUEUE_FULL
+			 * message - typically only when the queue
+			 * depth is only approximate for a given
+			 * device.  Adding a command to the queue for
+			 * the device will prevent further commands
+			 * from being sent to the device, so we
+			 * shouldn't end up with tons of things being
+			 * sent down that shouldn't be.
+			 */
+			SCSI_LOG_MLCOMPLETE(3, printk("Command rejected as "
+						      "device queue full, "
+						      "put on ml queue %p\n",
+						      cmd));
+			scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
+			break;
+		default:
+			/*
+			 * Here we have a fatal error of some sort.
+			 * Turn it over to the error handler.
+			 */
+			SCSI_LOG_MLCOMPLETE(3,
+					    printk("Command failed %p %x "
+						   "busy=%d failed=%d\n",
+						   cmd, cmd->result,
+					   cmd->device->host->host_busy,
+					   cmd->device->host->host_failed));

-				scsi_retry_command(SCpnt);
-				break;
-			case ADD_TO_MLQUEUE:
-				/*
-				 * This typically happens for a QUEUE_FULL
-				 * message - typically only when the queue
-				 * depth is only approximate for a given
-				 * device.  Adding a command to the queue for
-				 * the device will prevent further commands
-				 * from being sent to the device, so we
-				 * shouldn't end up with tons of things being
-				 * sent down that shouldn't be.
-				 */
-				SCSI_LOG_MLCOMPLETE(3, printk("Command rejected as device queue full, put on ml queue %p\n",
-                                                              SCpnt));
-				scsi_queue_insert(SCpnt, SCSI_MLQUEUE_DEVICE_BUSY);
-				break;
-			default:
-				/*
-				 * Here we have a fatal error of some sort.
-				 * Turn it over to the error handler.
-				 */
-				SCSI_LOG_MLCOMPLETE(3,
-					printk("Command failed %p %x busy=%d failed=%d\n",
-						SCpnt, SCpnt->result,
-						SCpnt->device->host->host_busy,
-						SCpnt->device->host->host_failed));
+			/*
+			 * Dump the sense information too.
+			 */
+			if ((status_byte(cmd->result)&CHECK_CONDITION) != 0) {
+				SCSI_LOG_MLCOMPLETE(3, print_sense("bh", cmd));
+			}

+			if (!scsi_eh_scmd_add(cmd, 0)) {
  				/*
-				 * Dump the sense information too.
+				 * We only get here if the error
+				 * recovery thread has died.
  				 */
-				if ((status_byte(SCpnt->result) & CHECK_CONDITION) != 0) {
-					SCSI_LOG_MLCOMPLETE(3, print_sense("bh", SCpnt));
-				}
-
-				if (!scsi_eh_scmd_add(SCpnt, 0))
-				{
-					/*
-					 * We only get here if the error
-					 * recovery thread has died.
-					 */
-					scsi_finish_command(SCpnt);
-				}
-			}	/* switch */
-		}		/* for(; SCpnt...) */
-	}			/* while(queue->head) */
-}
+				scsi_finish_command(cmd);
+			}
+		} /* switch (command disposition) */
+	} /* while (local queue is not emtpy) */
+} /* end scsi_softirq() */

  /*
   * Function:    scsi_retry_command
@@ -1481,7 +1461,7 @@

  static int __init init_scsi(void)
  {
-	int error;
+	int error, i;

  	error = scsi_init_queue();
  	if (error)
@@ -1496,6 +1476,9 @@
  	if (error)
  		goto cleanup_devlist;

+	for (i = 0; i < NR_CPUS; i++)
+		INIT_LIST_HEAD(&done_q[i]);
+
  	scsi_host_init();
  	devfs_mk_dir(NULL, "scsi", NULL);
  	open_softirq(SCSI_SOFTIRQ, scsi_softirq, NULL);
diff -Naur -X dontdiff linux-2.5.64/drivers/scsi.orig/scsi.h linux-2.5.64/drivers/scsi/scsi.h
--- linux-2.5.64/drivers/scsi.orig/scsi.h	2003-03-04 22:29:03.000000000 -0500
+++ linux-2.5.64/drivers/scsi/scsi.h	2003-03-07 14:19:50.000000000 -0500
@@ -760,8 +760,7 @@
  	 * abort, etc are in process.
  	 */
  	unsigned volatile char internal_timeout;
-	struct scsi_cmnd *bh_next;	/* To enumerate the commands waiting
-					   to be processed. */
+
  	unsigned char cmd_len;
  	unsigned char old_cmd_len;
  	unsigned char sc_data_direction;
diff -Naur -X dontdiff linux-2.5.64/drivers/scsi.orig/scsi_lib.c linux-2.5.64/drivers/scsi/scsi_lib.c
--- linux-2.5.64/drivers/scsi.orig/scsi_lib.c	2003-03-04 22:29:19.000000000 -0500
+++ linux-2.5.64/drivers/scsi/scsi_lib.c	2003-03-07 15:02:53.000000000 -0500
@@ -125,7 +125,6 @@
  	 */
  	cmd->state = SCSI_STATE_MLQUEUE;
  	cmd->owner = SCSI_OWNER_MIDLEVEL;
-	cmd->bh_next = NULL;

  	/*
  	 * Decrement the counters, since these commands are no longer

Re: [PATCH] softirq queue is now list_head, eliminate bh_next

[Luben Tuikov]

Sorry for the line-wrap. (My line wrap is at 999 chars, but
mozilla has a mind of it's own -- is anyone using imaps with Emacs?)

The patch is here:

http://www.splentec.com/~luben/done_q.html

-- 
Luben

Re: [PATCH] softirq queue is now list_head, eliminate bh_next

[Patrick Mansfield]

Hi Luben -

On Fri, Mar 07, 2003 at 06:55:42PM -0500, Luben Tuikov wrote:
> Eliminated is the double loop in scsi_softirq() -- this is
> better handled in do_softirq() and gives the system a ``breather''.
> (There are pros and cons for either side and if you guys
> think that it was better with the double loop, I'll change it and
> resubmit the patch.)

I think it is better to have one loop (per your patch) - in breather cases
when we get multiple interrupts before we can service all of them the
do_softirq() can wakeup ksoftirqd for us to run on.

>   static void scsi_softirq(struct softirq_action *h)
>   {
> -	int cpu = smp_processor_id();
> -	struct softscsi_data *queue = &softscsi_data[cpu];
> +	LIST_HEAD(local_q);
> 
> -	while (queue->head) {
> -		Scsi_Cmnd *SCpnt, *SCnext;
> +	local_irq_disable();
> +	list_splice_init(&done_q[smp_processor_id()], &local_q);
> +	local_irq_enable();
> +
> +	while (!list_empty(&local_q)) {

Why not list_for_each_safe rather than a while?

> +		} /* switch (command disposition) */
> +	} /* while (local queue is not emtpy) */
> +} /* end scsi_softirq() */
> 

You should get rid of the comments after the }'s.

-- Patrick Mansfield