- Fix possible incorrect atomic context on exec stages

- Limit max tasklet commands via scst_max_tasklet_cmd global attribute to prevent possible starvation under heavy load and in some cases im
prove performance
 - Logging improvements
 - Docs update
 - Cleanups



git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@3214 d57e44dd-8a1f-0410-8b47-8ef2f437770f

scst/README

@@ -390,6 +390,14 @@ following entries:
 
  - handlers - this is a root subdirectory for all SCST dev handlers
 
+ - max_tasklet_cmd - specifies how many commands at max can be queued in
+   the SCST core simultaneously from all connected initiators to allow
+   processing commands in soft-IRQ context in tasklets. If the count of
+   the commands exceeds this value, then all of them will be processed
+   only in threads. This is to to prevent possible starvation under
+   heavy load and in some cases to improve performance by more evenly
+   spreading load over available CPUs.
+
  - sgv - this is a root subdirectory for all SCST SGV caches
 
  - targets - this is a root subdirectory for all SCST targets

scst/README_in-tree

@@ -258,6 +258,14 @@ following entries:
 
  - handlers - this is a root subdirectory for all SCST dev handlers
 
+ - max_tasklet_cmd - specifies how many commands at max can be queued in
+   the SCST core simultaneously from all connected initiators to allow
+   processing commands in soft-IRQ context in tasklets. If the count of
+   the commands exceeds this value, then all of them will be processed
+   only in threads. This is to to prevent possible starvation under
+   heavy load and in some cases to improve performance by more evenly
+   spreading load over available CPUs.
+
  - sgv - this is a root subdirectory for all SCST SGV caches
 
  - targets - this is a root subdirectory for all SCST targets

scst/src/scst_main.c

@@ -128,6 +128,8 @@ struct kmem_cache *scst_cmd_cachep;
 unsigned long scst_trace_flag;
 #endif
 
+int scst_max_tasklet_cmd = SCST_DEF_MAX_TASKLET_CMD;
+
 unsigned long scst_flags;
 atomic_t scst_cmd_count;
 

scst/src/scst_priv.h

@@ -155,6 +155,9 @@ extern struct scst_acg *scst_default_acg;
 extern unsigned int scst_setup_id;
 #endif
 
+#define SCST_DEF_MAX_TASKLET_CMD 20
+extern int scst_max_tasklet_cmd;
+
 extern spinlock_t scst_init_lock;
 extern struct list_head scst_init_cmd_list;
 extern wait_queue_head_t scst_init_cmd_list_waitQ;

scst/src/scst_sysfs.c

@@ -4877,6 +4877,49 @@ static struct kobj_attribute scst_setup_id_attr =
 	__ATTR(setup_id, S_IRUGO | S_IWUSR, scst_setup_id_show,
 	       scst_setup_id_store);
 
+static ssize_t scst_max_tasklet_cmd_show(struct kobject *kobj,
+				  struct kobj_attribute *attr, char *buf)
+{
+	int count;
+
+	TRACE_ENTRY();
+
+	count = sprintf(buf, "%d\n%s\n", scst_max_tasklet_cmd,
+		(scst_max_tasklet_cmd == SCST_DEF_MAX_TASKLET_CMD)
+			? "" : SCST_SYSFS_KEY_MARK);
+
+	TRACE_EXIT();
+	return count;
+}
+
+static ssize_t scst_max_tasklet_cmd_store(struct kobject *kobj,
+	struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	int res;
+	unsigned long val;
+
+	TRACE_ENTRY();
+
+	res = strict_strtoul(buf, 0, &val);
+	if (res != 0) {
+		PRINT_ERROR("strict_strtoul() for %s failed: %d ", buf, res);
+		goto out;
+	}
+
+	scst_max_tasklet_cmd = val;
+	PRINT_INFO("Changed scst_max_tasklet_cmd to %d", scst_max_tasklet_cmd);
+
+	res = count;
+
+out:
+	TRACE_EXIT_RES(res);
+	return res;
+}
+
+static struct kobj_attribute scst_max_tasklet_cmd_attr =
+	__ATTR(max_tasklet_cmd, S_IRUGO | S_IWUSR, scst_max_tasklet_cmd_show,
+	       scst_max_tasklet_cmd_store);
+
 #if defined(CONFIG_SCST_DEBUG) || defined(CONFIG_SCST_TRACING)
 
 static ssize_t scst_main_trace_level_show(struct kobject *kobj,
@@ -4998,6 +5041,7 @@ static struct kobj_attribute scst_last_sysfs_mgmt_res_attr =
 static struct attribute *scst_sysfs_root_default_attrs[] = {
 	&scst_threads_attr.attr,
 	&scst_setup_id_attr.attr,
+	&scst_max_tasklet_cmd_attr.attr,
 #if defined(CONFIG_SCST_DEBUG) || defined(CONFIG_SCST_TRACING)
 	&scst_main_trace_level_attr.attr,
 #endif

scst/src/scst_targ.c

@@ -82,13 +82,24 @@ static inline void scst_schedule_tasklet(struct scst_cmd *cmd)
 	struct scst_tasklet *t = &scst_tasklets[smp_processor_id()];
 	unsigned long flags;
 
-	spin_lock_irqsave(&t->tasklet_lock, flags);
-	TRACE_DBG("Adding cmd %p to tasklet %d cmd list", cmd,
-		smp_processor_id());
-	list_add_tail(&cmd->cmd_list_entry, &t->tasklet_cmd_list);
-	spin_unlock_irqrestore(&t->tasklet_lock, flags);
+	if (atomic_read(&scst_cmd_count) <= scst_max_tasklet_cmd) {
+		spin_lock_irqsave(&t->tasklet_lock, flags);
+		TRACE_DBG("Adding cmd %p to tasklet %d cmd list", cmd,
+			smp_processor_id());
+		list_add_tail(&cmd->cmd_list_entry, &t->tasklet_cmd_list);
+		spin_unlock_irqrestore(&t->tasklet_lock, flags);
 
-	tasklet_schedule(&t->tasklet);
+		tasklet_schedule(&t->tasklet);
+	} else {
+		spin_lock_irqsave(&cmd->cmd_threads->cmd_list_lock, flags);
+		TRACE_DBG("Too many tasklet commands (%d), adding cmd %p to "
+			"active cmd list", atomic_read(&scst_cmd_count), cmd);
+		list_add_tail(&cmd->cmd_list_entry,
+			&cmd->cmd_threads->active_cmd_list);
+		wake_up(&cmd->cmd_threads->cmd_list_waitQ);
+		spin_unlock_irqrestore(&cmd->cmd_threads->cmd_list_lock, flags);
+	}
+	return;
 }
 
 /**
@@ -184,10 +195,14 @@ static int scst_init_cmd(struct scst_cmd *cmd, enum scst_exec_context *context)
 #endif
 
 	/* Small context optimization */
-	if (((*context == SCST_CONTEXT_TASKLET) ||
-	     (*context == SCST_CONTEXT_DIRECT_ATOMIC)) &&
-	      scst_cmd_is_expected_set(cmd)) {
-		if (cmd->expected_data_direction & SCST_DATA_WRITE) {
+	if ((*context == SCST_CONTEXT_TASKLET) ||
+	    (*context == SCST_CONTEXT_DIRECT_ATOMIC)) {
+		/*
+		 * If any data_direction not set, it's SCST_DATA_UNKNOWN,
+		 * which is 0, so we can safely | them
+		 */
+		BUILD_BUG_ON(SCST_DATA_UNKNOWN != 0);
+		if ((cmd->data_direction | cmd->expected_data_direction) & SCST_DATA_WRITE) {
 			if (!test_bit(SCST_TGT_DEV_AFTER_INIT_WR_ATOMIC,
 					&cmd->tgt_dev->tgt_dev_flags))
 				*context = SCST_CONTEXT_THREAD;
@@ -277,6 +292,7 @@ void scst_cmd_init_done(struct scst_cmd *cmd,
 		PRINT_ERROR("Wrong context %d in IRQ from target %s, use "
 			"SCST_CONTEXT_THREAD instead", pref_context,
 			cmd->tgtt->name);
+		dump_stack();
 		pref_context = SCST_CONTEXT_THREAD;
 	}
 #endif
@@ -347,14 +363,6 @@ active:
 		scst_schedule_tasklet(cmd);
 		break;
 
-	case SCST_CONTEXT_DIRECT:
-		scst_process_active_cmd(cmd, false);
-		break;
-
-	case SCST_CONTEXT_DIRECT_ATOMIC:
-		scst_process_active_cmd(cmd, true);
-		break;
-
 	default:
 		PRINT_ERROR("Context %x is undefined, using the thread one",
 			pref_context);
@@ -371,6 +379,14 @@ active:
 		wake_up(&cmd->cmd_threads->cmd_list_waitQ);
 		spin_unlock_irqrestore(&cmd->cmd_threads->cmd_list_lock, flags);
 		break;
+
+	case SCST_CONTEXT_DIRECT:
+		scst_process_active_cmd(cmd, false);
+		break;
+
+	case SCST_CONTEXT_DIRECT_ATOMIC:
+		scst_process_active_cmd(cmd, true);
+		break;
 	}
 
 out:
@@ -484,7 +500,7 @@ static int scst_parse_cmd(struct scst_cmd *cmd)
 			 * It shouldn't be because of the SCST_TGT_DEV_AFTER_*
 			 * optimization.
 			 */
-			TRACE_DBG("Dev handler %s parse() needs thread "
+			TRACE_MGMT_DBG("Dev handler %s parse() needs thread "
 				"context, rescheduling", dev->handler->name);
 			res = SCST_CMD_STATE_RES_NEED_THREAD;
 			goto out;
@@ -782,6 +798,21 @@ set_res:
 	if (unlikely(cmd->completed))
 		goto out_done;
 
+#ifndef CONFIG_SCST_TEST_IO_IN_SIRQ
+	/*
+	 * We shouldn't allow atomic command to the exec stage. It shouldn't
+	 * be because of the SCST_TGT_DEV_AFTER_* optimization, but during
+	 * parsing data_direction can change, so we need to recheck.
+	 */
+	if (unlikely(scst_cmd_atomic(cmd) &&
+		     !(cmd->data_direction & SCST_DATA_WRITE))) {
+		TRACE_DBG_FLAG(TRACE_DEBUG|TRACE_MINOR, "Atomic context and "
+			"non-WRITE data direction, rescheduling (cmd %p)", cmd);
+		res = SCST_CMD_STATE_RES_NEED_THREAD;
+		goto out;
+	}
+#endif
+
 out:
 	TRACE_EXIT_HRES(res);
 	return res;
@@ -855,7 +886,7 @@ static int scst_prepare_space(struct scst_cmd *cmd)
 			 * It shouldn't be because of the SCST_TGT_DEV_AFTER_*
 			 * optimization.
 			 */
-			TRACE_DBG("Dev handler %s alloc_data_buf() needs "
+			TRACE_MGMT_DBG("Dev handler %s alloc_data_buf() needs "
 				"thread context, rescheduling",
 				dev->handler->name);
 			res = SCST_CMD_STATE_RES_NEED_THREAD;
@@ -1045,6 +1076,7 @@ void scst_restart_cmd(struct scst_cmd *cmd, int status,
 		PRINT_ERROR("Wrong context %d in IRQ from target %s, use "
 			"SCST_CONTEXT_THREAD instead", pref_context,
 			cmd->tgtt->name);
+		dump_stack();
 		pref_context = SCST_CONTEXT_THREAD;
 	}
 #endif
@@ -1071,6 +1103,7 @@ void scst_restart_cmd(struct scst_cmd *cmd, int status,
 
 	case SCST_PREPROCESS_STATUS_ERROR_SENSE_SET:
 		scst_set_cmd_abnormal_done_state(cmd);
+		pref_context = SCST_CONTEXT_THREAD;
 		break;
 
 	case SCST_PREPROCESS_STATUS_ERROR_FATAL:
@@ -1081,12 +1114,14 @@ void scst_restart_cmd(struct scst_cmd *cmd, int status,
 			scst_set_cmd_error(cmd,
 				SCST_LOAD_SENSE(scst_sense_hardw_error));
 		scst_set_cmd_abnormal_done_state(cmd);
+		pref_context = SCST_CONTEXT_THREAD;
 		break;
 
 	default:
 		PRINT_ERROR("%s() received unknown status %x", __func__,
 			status);
 		scst_set_cmd_abnormal_done_state(cmd);
+		pref_context = SCST_CONTEXT_THREAD;
 		break;
 	}
 
@@ -1111,7 +1146,15 @@ static int scst_rdy_to_xfer(struct scst_cmd *cmd)
 
 	if ((tgtt->rdy_to_xfer == NULL) || unlikely(cmd->internal)) {
 		cmd->state = SCST_CMD_STATE_TGT_PRE_EXEC;
-		res = SCST_CMD_STATE_RES_CONT_SAME;
+#ifndef CONFIG_SCST_TEST_IO_IN_SIRQ
+		/* We shouldn't allow atomic command to the exec stage */
+		if (scst_cmd_atomic(cmd)) {
+			TRACE_DBG("NULL rdy_to_xfer() and atomic context, "
+				" rescheduling (cmd %p)", cmd);
+			res = SCST_CMD_STATE_RES_NEED_THREAD;
+		} else
+#endif
+			res = SCST_CMD_STATE_RES_CONT_SAME;
 		goto out;
 	}
 
@@ -1120,8 +1163,8 @@ static int scst_rdy_to_xfer(struct scst_cmd *cmd)
 		 * It shouldn't be because of the SCST_TGT_DEV_AFTER_*
 		 * optimization.
 		 */
-		TRACE_DBG("Target driver %s rdy_to_xfer() needs thread "
-			      "context, rescheduling", tgtt->name);
+		TRACE_MGMT_DBG("Target driver %s rdy_to_xfer() needs thread "
+			"context, rescheduling", tgtt->name);
 		res = SCST_CMD_STATE_RES_NEED_THREAD;
 		goto out;
 	}
@@ -1219,6 +1262,9 @@ static void scst_process_redirect_cmd(struct scst_cmd *cmd,
 
 	TRACE_DBG("Context: %x", context);
 
+	if (check_retries)
+		scst_check_retries(tgt);
+
 	if (context == SCST_CONTEXT_SAME)
 		context = scst_cmd_atomic(cmd) ? SCST_CONTEXT_DIRECT_ATOMIC :
 						 SCST_CONTEXT_DIRECT;
@@ -1229,18 +1275,18 @@ static void scst_process_redirect_cmd(struct scst_cmd *cmd,
 		break;
 
 	case SCST_CONTEXT_DIRECT:
-		if (check_retries)
-			scst_check_retries(tgt);
 		scst_process_active_cmd(cmd, false);
 		break;
 
+	case SCST_CONTEXT_TASKLET:
+		scst_schedule_tasklet(cmd);
+		break;
+
 	default:
 		PRINT_ERROR("Context %x is unknown, using the thread one",
 			    context);
 		/* go through */
 	case SCST_CONTEXT_THREAD:
-		if (check_retries)
-			scst_check_retries(tgt);
 		spin_lock_irqsave(&cmd->cmd_threads->cmd_list_lock, flags);
 		TRACE_DBG("Adding cmd %p to active cmd list", cmd);
 		if (unlikely(cmd->queue_type == SCST_CMD_QUEUE_HEAD_OF_QUEUE))
@@ -1252,12 +1298,6 @@ static void scst_process_redirect_cmd(struct scst_cmd *cmd,
 		wake_up(&cmd->cmd_threads->cmd_list_waitQ);
 		spin_unlock_irqrestore(&cmd->cmd_threads->cmd_list_lock, flags);
 		break;
-
-	case SCST_CONTEXT_TASKLET:
-		if (check_retries)
-			scst_check_retries(tgt);
-		scst_schedule_tasklet(cmd);
-		break;
 	}
 
 	TRACE_EXIT();
@@ -1296,6 +1336,7 @@ void scst_rx_data(struct scst_cmd *cmd, int status,
 		PRINT_ERROR("Wrong context %d in IRQ from target %s, use "
 			"SCST_CONTEXT_THREAD instead", pref_context,
 			cmd->tgtt->name);
+		dump_stack();
 		pref_context = SCST_CONTEXT_THREAD;
 	}
 #endif
@@ -1347,6 +1388,7 @@ void scst_rx_data(struct scst_cmd *cmd, int status,
 
 	case SCST_RX_STATUS_ERROR_SENSE_SET:
 		scst_set_cmd_abnormal_done_state(cmd);
+		pref_context = SCST_CONTEXT_THREAD;
 		break;
 
 	case SCST_RX_STATUS_ERROR_FATAL:
@@ -1356,12 +1398,14 @@ void scst_rx_data(struct scst_cmd *cmd, int status,
 		scst_set_cmd_error(cmd,
 			   SCST_LOAD_SENSE(scst_sense_hardw_error));
 		scst_set_cmd_abnormal_done_state(cmd);
+		pref_context = SCST_CONTEXT_THREAD;
 		break;
 
 	default:
 		PRINT_ERROR("scst_rx_data() received unknown status %x",
 			status);
 		scst_set_cmd_abnormal_done_state(cmd);
+		pref_context = SCST_CONTEXT_THREAD;
 		break;
 	}
 
@@ -2514,7 +2558,7 @@ out_complete:
 	if (ctx_changed)
 		scst_reset_io_context(cmd->tgt_dev, old_ctx);
 
-	TRACE_EXIT();
+	TRACE_EXIT_RES(res);
 	return res;
 
 out_error:
@@ -2540,7 +2584,6 @@ static inline int scst_real_exec(struct scst_cmd *cmd)
 	__scst_cmd_get(cmd);
 
 	res = scst_do_real_exec(cmd);
-
 	if (likely(res == SCST_EXEC_COMPLETED)) {
 		scst_post_exec_sn(cmd, true);
 		if (cmd->dev->scsi_dev != NULL)
@@ -3182,8 +3225,8 @@ static int scst_dev_done(struct scst_cmd *cmd)
 			 * It shouldn't be because of the SCST_TGT_DEV_AFTER_*
 			 * optimization.
 			 */
-			TRACE_DBG("Dev handler %s dev_done() needs thread "
-			      "context, rescheduling", dev->handler->name);
+			TRACE_MGMT_DBG("Dev handler %s dev_done() needs thread "
+				"context, rescheduling", dev->handler->name);
 			res = SCST_CMD_STATE_RES_NEED_THREAD;
 			goto out;
 		}
@@ -3345,8 +3388,8 @@ static int scst_xmit_response(struct scst_cmd *cmd)
 		 * It shouldn't be because of the SCST_TGT_DEV_AFTER_*
 		 * optimization.
 		 */
-		TRACE_DBG("Target driver %s xmit_response() needs thread "
-			      "context, rescheduling", tgtt->name);
+		TRACE_MGMT_DBG("Target driver %s xmit_response() needs thread "
+			"context, rescheduling", tgtt->name);
 		res = SCST_CMD_STATE_RES_NEED_THREAD;
 		goto out;
 	}
@@ -3482,7 +3525,11 @@ void scst_tgt_cmd_done(struct scst_cmd *cmd,
 
 	cmd->cmd_hw_pending = 0;
 
+	if (unlikely(cmd->tgt_dev == NULL))
+		pref_context = SCST_CONTEXT_THREAD;
+
 	cmd->state = SCST_CMD_STATE_FINISHED;
+
 	scst_process_redirect_cmd(cmd, pref_context, 1);
 
 	TRACE_EXIT();