diff mbox series

[03/10] scsi/megaraid_sas: Replace instance->tasklet with threaded irq

Message ID 20220530231512.9729-4-dave@stgolabs.net (mailing list archive)
State Changes Requested
Headers show
Series scsi: Replace tasklets as BH | expand

Commit Message

Davidlohr Bueso May 30, 2022, 11:15 p.m. UTC 
Tasklets have long been deprecated as being too heavy on the system
by running in irq context - and this is not a performance critical
path. If a higher priority process wants to run, it must wait for
the tasklet to finish before doing so. A more suitable equivalent
is to converted to threaded irq instead and deal with the command
completion in task context.

While tasklets do not run concurrently amongst themselves, the
callback can compete vs megasas_wait_for_outstanding() so any races
with threads will also be present with the async thread completing
the command.

Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>
Cc: megaraidlinux.pdl@broadcom.com
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/scsi/megaraid/megaraid_sas.h        |  3 +-
 drivers/scsi/megaraid/megaraid_sas_base.c   | 51 ++++++++++-----------
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 18 +++++---
 3 files changed, 37 insertions(+), 35 deletions(-)

2.36.1

Comments

Sebastian Andrzej Siewior June 2, 2022, 10:11 a.m. UTC | #1 
On 2022-05-30 16:15:05 [-0700], Davidlohr Bueso wrote:
> Tasklets have long been deprecated as being too heavy on the system
> by running in irq context - and this is not a performance critical
> path. If a higher priority process wants to run, it must wait for
> the tasklet to finish before doing so. A more suitable equivalent
> is to converted to threaded irq instead and deal with the command
> completion in task context.
> 
> While tasklets do not run concurrently amongst themselves, the
> callback can compete vs megasas_wait_for_outstanding() so any races
> with threads will also be present with the async thread completing
> the command.

So that fusion part of the driver has a different tasklet/isr so we have
always the same callback except in the one case…
The s/tasklet/irqthread/ looks okay but that fusion part of the driver
which also does some eye brow raising:
The way irqpoll is scheduled does not look promising. We have:
|         if (!irq_context->irq_poll_scheduled) {
|                 irq_context->irq_poll_scheduled = true;
|                 irq_context->irq_line_enable = true;
|                 irq_poll_sched(&irq_context->irqpoll);
|         }

so we lack disabling the interrupt source. This means the interrupt will
continue trigger except on edge-trigger devices. Here however it might
also trigger if "another work item" is added. This might be reason why
|         if (irq_context->irq_poll_scheduled)
|                 return IRQ_HANDLED;

was added to megasas_isr_fusion() to skip that case. We also have this
piece:
|         if (irq_ctx->irq_line_enable) {
|                 disable_irq_nosync(irq_ctx->os_irq);
|                 irq_ctx->irq_line_enable = false;
|         }

in megasas_irqpoll(). It might have not been enough. But this a bold
move if this is not an MSI interrupt but an actual shared interrupt.
Without any proof I would say that disabling the interrupt source in HW
is cheaper than invoking disable_irq_nosync(). Also invoking
disable_irq_nosync() from the point where it should be disabled looks
late.

I would suggest to get rid of irqpoll, disable the interrupt source
before returning IRQ_WAKE_THREAD. I have currently no idea how to deal
with
|	if (threshold_reply_count >= instance->threshold_reply_count)

within the threaded-IRQ. It runs at SCHED_FIFO/50 so a cond_resched()
won't do a thing. It might not be a issue, it might…

So the while the s/tasklet/irqthread/ part look okay, the part where the
interrupt source seems not to be deactivated looks bad.

> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Sebastian
diff mbox series

Patch

diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
index 4919ea54b827..d119c02f0a9c 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -2387,7 +2387,6 @@  struct megasas_instance {
 	atomic64_t high_iops_outstanding;
 
 	struct megasas_instance_template *instancet;
-	struct tasklet_struct isr_tasklet;
 	struct work_struct work_init;
 	struct delayed_work fw_fault_work;
 	struct workqueue_struct *fw_fault_work_q;
@@ -2549,7 +2548,7 @@  struct megasas_instance_template {
 	int (*check_reset)(struct megasas_instance *, \
 		struct megasas_register_set __iomem *);
 	irqreturn_t (*service_isr)(int irq, void *devp);
-	void (*tasklet)(unsigned long);
+	irqreturn_t (*service_thr)(int irq, void *devp);
 	u32 (*init_adapter)(struct megasas_instance *);
 	u32 (*build_and_issue_cmd) (struct megasas_instance *,
 				    struct scsi_cmnd *);
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index c95360a3c186..24f31ebad877 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -229,12 +229,12 @@  static int
 megasas_adp_reset_gen2(struct megasas_instance *instance,
 		       struct megasas_register_set __iomem *reg_set);
 static irqreturn_t megasas_isr(int irq, void *devp);
+static irqreturn_t megasas_complete_cmd_dpc_irq(int irq, void *devp);
 static u32
 megasas_init_adapter_mfi(struct megasas_instance *instance);
 u32
 megasas_build_and_issue_cmd(struct megasas_instance *instance,
 			    struct scsi_cmnd *scmd);
-static void megasas_complete_cmd_dpc(unsigned long instance_addr);
 int
 wait_and_poll(struct megasas_instance *instance, struct megasas_cmd *cmd,
 	int seconds);
@@ -615,7 +615,7 @@  static struct megasas_instance_template megasas_instance_template_xscale = {
 	.adp_reset = megasas_adp_reset_xscale,
 	.check_reset = megasas_check_reset_xscale,
 	.service_isr = megasas_isr,
-	.tasklet = megasas_complete_cmd_dpc,
+	.service_thr = megasas_complete_cmd_dpc_irq,
 	.init_adapter = megasas_init_adapter_mfi,
 	.build_and_issue_cmd = megasas_build_and_issue_cmd,
 	.issue_dcmd = megasas_issue_dcmd,
@@ -754,7 +754,7 @@  static struct megasas_instance_template megasas_instance_template_ppc = {
 	.adp_reset = megasas_adp_reset_xscale,
 	.check_reset = megasas_check_reset_ppc,
 	.service_isr = megasas_isr,
-	.tasklet = megasas_complete_cmd_dpc,
+	.service_thr = megasas_complete_cmd_dpc_irq,
 	.init_adapter = megasas_init_adapter_mfi,
 	.build_and_issue_cmd = megasas_build_and_issue_cmd,
 	.issue_dcmd = megasas_issue_dcmd,
@@ -895,7 +895,7 @@  static struct megasas_instance_template megasas_instance_template_skinny = {
 	.adp_reset = megasas_adp_reset_gen2,
 	.check_reset = megasas_check_reset_skinny,
 	.service_isr = megasas_isr,
-	.tasklet = megasas_complete_cmd_dpc,
+	.service_thr = megasas_complete_cmd_dpc_irq,
 	.init_adapter = megasas_init_adapter_mfi,
 	.build_and_issue_cmd = megasas_build_and_issue_cmd,
 	.issue_dcmd = megasas_issue_dcmd,
@@ -1095,7 +1095,7 @@  static struct megasas_instance_template megasas_instance_template_gen2 = {
 	.adp_reset = megasas_adp_reset_gen2,
 	.check_reset = megasas_check_reset_gen2,
 	.service_isr = megasas_isr,
-	.tasklet = megasas_complete_cmd_dpc,
+	.service_thr = megasas_complete_cmd_dpc_irq,
 	.init_adapter = megasas_init_adapter_mfi,
 	.build_and_issue_cmd = megasas_build_and_issue_cmd,
 	.issue_dcmd = megasas_issue_dcmd,
@@ -2268,19 +2268,16 @@  megasas_check_and_restore_queue_depth(struct megasas_instance *instance)
 }
 
 /**
- * megasas_complete_cmd_dpc	 -	Returns FW's controller structure
- * @instance_addr:			Address of adapter soft state
+ * megasas_complete_cmd_dpc	 -	Completes command
+ * @instance:			        Adapter soft state instance
  *
- * Tasklet to complete cmds
  */
-static void megasas_complete_cmd_dpc(unsigned long instance_addr)
+static void megasas_complete_cmd_dpc(struct megasas_instance *instance)
 {
 	u32 producer;
 	u32 consumer;
 	u32 context;
 	struct megasas_cmd *cmd;
-	struct megasas_instance *instance =
-				(struct megasas_instance *)instance_addr;
 	unsigned long flags;
 
 	/* If we have already declared adapter dead, donot complete cmds */
@@ -2320,6 +2317,15 @@  static void megasas_complete_cmd_dpc(unsigned long instance_addr)
 	megasas_check_and_restore_queue_depth(instance);
 }
 
+/* Called from threaded IRQ, runs in task context. */
+static irqreturn_t megasas_complete_cmd_dpc_irq(int irq, void *devp)
+{
+	struct megasas_instance *instance = (struct megasas_instance *)devp;
+
+	megasas_complete_cmd_dpc(instance);
+	return IRQ_HANDLED;
+}
+
 static void megasas_sriov_heartbeat_handler(struct timer_list *t);
 
 /**
@@ -2825,7 +2831,7 @@  static int megasas_wait_for_outstanding(struct megasas_instance *instance)
 			 * Call cmd completion routine. Cmd to be
 			 * be completed directly without depending on isr.
 			 */
-			megasas_complete_cmd_dpc((unsigned long)instance);
+			megasas_complete_cmd_dpc(instance);
 		}
 
 		msleep(1000);
@@ -4078,8 +4084,7 @@  megasas_deplete_reply_queue(struct megasas_instance *instance,
 		}
 	}
 
-	tasklet_schedule(&instance->isr_tasklet);
-	return IRQ_HANDLED;
+	return IRQ_WAKE_THREAD;
 }
 
 /**
@@ -5682,9 +5687,11 @@  megasas_setup_irqs_ioapic(struct megasas_instance *instance)
 	instance->irq_context[0].MSIxIndex = 0;
 	snprintf(instance->irq_context->name, MEGASAS_MSIX_NAME_LEN, "%s%u",
 		"megasas", instance->host->host_no);
-	if (request_irq(pci_irq_vector(pdev, 0),
-			instance->instancet->service_isr, IRQF_SHARED,
-			instance->irq_context->name, &instance->irq_context[0])) {
+	if (request_threaded_irq(pci_irq_vector(pdev, 0),
+			instance->instancet->service_isr,
+			instance->instancet->service_thr, IRQF_SHARED,
+			instance->irq_context->name,
+			&instance->irq_context[0])) {
 		dev_err(&instance->pdev->dev,
 				"Failed to register IRQ from %s %d\n",
 				__func__, __LINE__);
@@ -6322,9 +6329,6 @@  static int megasas_init_fw(struct megasas_instance *instance)
 	dev_info(&instance->pdev->dev,
 		"RDPQ mode\t: (%s)\n", instance->is_rdpq ? "enabled" : "disabled");
 
-	tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet,
-		(unsigned long)instance);
-
 	/*
 	 * Below are default value for legacy Firmware.
 	 * non-fusion based controllers
@@ -7771,8 +7775,6 @@  megasas_suspend(struct device *dev)
 		instance->ev = NULL;
 	}
 
-	tasklet_kill(&instance->isr_tasklet);
-
 	pci_set_drvdata(instance->pdev, instance);
 	instance->instancet->disable_intr(instance);
 
@@ -7879,9 +7881,6 @@  megasas_resume(struct device *dev)
 	if (megasas_get_ctrl_info(instance) != DCMD_SUCCESS)
 		goto fail_init_mfi;
 
-	tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet,
-		     (unsigned long)instance);
-
 	if (instance->msix_vectors ?
 			megasas_setup_irqs_msix(instance, 0) :
 			megasas_setup_irqs_ioapic(instance))
@@ -8011,8 +8010,6 @@  static void megasas_detach_one(struct pci_dev *pdev)
 	/* cancel all wait events */
 	wake_up_all(&instance->int_cmd_wait_q);
 
-	tasklet_kill(&instance->isr_tasklet);
-
 	/*
 	 * Take the instance off the instance array. Note that we will not
 	 * decrement the max_index. We let this array be sparse array
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 5b5885d9732b..5887e3cb725e 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -3819,15 +3819,12 @@  int megasas_irqpoll(struct irq_poll *irqpoll, int budget)
 
 /**
  * megasas_complete_cmd_dpc_fusion -	Completes command
- * @instance_addr:			Adapter soft state address
+ * @instance:				Adapter soft state instance
  *
- * Tasklet to complete cmds
  */
 static void
-megasas_complete_cmd_dpc_fusion(unsigned long instance_addr)
+megasas_complete_cmd_dpc_fusion(struct megasas_instance *instance)
 {
-	struct megasas_instance *instance =
-		(struct megasas_instance *)instance_addr;
 	struct megasas_irq_context *irq_ctx = NULL;
 	u32 count, MSIxIndex;
 
@@ -3843,6 +3840,15 @@  megasas_complete_cmd_dpc_fusion(unsigned long instance_addr)
 	}
 }
 
+/* Called from threaded IRQ, runs in task context. */
+static irqreturn_t megasas_complete_cmd_dpc_fusion_irq(int irq, void *devp)
+{
+	struct megasas_instance *instance = (struct megasas_instance *)devp;
+
+	megasas_complete_cmd_dpc_fusion(instance);
+	return IRQ_HANDLED;
+}
+
 /**
  * megasas_isr_fusion - isr entry point
  * @irq:	IRQ number
@@ -5367,8 +5373,7 @@  struct megasas_instance_template megasas_instance_template_fusion = {
 	.adp_reset = megasas_adp_reset_fusion,
 	.check_reset = megasas_check_reset_fusion,
 	.service_isr = megasas_isr_fusion,
-	.tasklet = megasas_complete_cmd_dpc_fusion,
+	.service_thr = megasas_complete_cmd_dpc_fusion_irq,
 	.init_adapter = megasas_init_adapter_fusion,
 	.build_and_issue_cmd = megasas_build_and_issue_cmd_fusion,
 	.issue_dcmd = megasas_issue_dcmd_fusion,
--