diff mbox series

[05/10] scsi/isci: Replace completion_tasklet with threaded irq

Message ID 20220530231512.9729-6-dave@stgolabs.net
State New
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 run in regular task
context.

Cc: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/scsi/isci/host.c | 12 ++++++------
 drivers/scsi/isci/host.h |  3 +--
 drivers/scsi/isci/init.c | 17 ++++++++++-------
 3 files changed, 17 insertions(+), 15 deletions(-)

Comments

Sebastian Andrzej Siewior June 2, 2022, 6:19 p.m. UTC | #1
On 2022-05-30 16:15:07 [-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 run in regular task
> context.

The convert looks okay. This driver even disables the interrupt source
before scheduling the tasklet :) However the whole routine
(sci_controller_completion_handler()) runs with disabled interrupt so it
makes no sense to use tasklets for completion or threaded interrupts…

> Cc: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Sebastian
Artur Paszkiewicz June 6, 2022, 10:24 a.m. UTC | #2
On 31.05.2022 01:15, 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 run in regular task
> context.
> 
> Cc: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Acked-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Sebastian Andrzej Siewior June 7, 2022, 9:13 a.m. UTC | #3
On 2022-06-06 12:24:22 [+0200], Artur Paszkiewicz wrote:
> Acked-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>

Did you see my reply?

Sebastian
diff mbox series

Patch

diff --git a/drivers/scsi/isci/host.c b/drivers/scsi/isci/host.c
index 35589b6af90d..b2445782979d 100644
--- a/drivers/scsi/isci/host.c
+++ b/drivers/scsi/isci/host.c
@@ -220,7 +220,7 @@  irqreturn_t isci_msix_isr(int vec, void *data)
 	struct isci_host *ihost = data;
 
 	if (sci_controller_isr(ihost))
-		tasklet_schedule(&ihost->completion_tasklet);
+		return IRQ_WAKE_THREAD;
 
 	return IRQ_HANDLED;
 }
@@ -610,8 +610,7 @@  irqreturn_t isci_intx_isr(int vec, void *data)
 
 	if (sci_controller_isr(ihost)) {
 		writel(SMU_ISR_COMPLETION, &ihost->smu_registers->interrupt_status);
-		tasklet_schedule(&ihost->completion_tasklet);
-		ret = IRQ_HANDLED;
+	        ret = IRQ_WAKE_THREAD;
 	} else if (sci_controller_error_isr(ihost)) {
 		spin_lock(&ihost->scic_lock);
 		sci_controller_error_handler(ihost);
@@ -1106,12 +1105,11 @@  void ireq_done(struct isci_host *ihost, struct isci_request *ireq, struct sas_ta
 /**
  * isci_host_completion_routine() - This function is the delayed service
  *    routine that calls the sci core library's completion handler. It's
- *    scheduled as a tasklet from the interrupt service routine when interrupts
+ *    scheduled as a task from the interrupt service routine when interrupts
  *    in use, or set as the timeout function in polled mode.
  * @data: This parameter specifies the ISCI host object
- *
  */
-void isci_host_completion_routine(unsigned long data)
+irqreturn_t isci_host_completion_routine(int vector, void *data)
 {
 	struct isci_host *ihost = (struct isci_host *)data;
 	u16 active;
@@ -1133,6 +1131,8 @@  void isci_host_completion_routine(unsigned long data)
 	writel(SMU_ICC_GEN_VAL(NUMBER, active) |
 	       SMU_ICC_GEN_VAL(TIMER, ISCI_COALESCE_BASE + ilog2(active)),
 	       &ihost->smu_registers->interrupt_coalesce_control);
+
+	return IRQ_HANDLED;
 }
 
 /**
diff --git a/drivers/scsi/isci/host.h b/drivers/scsi/isci/host.h
index 6bc3f022630a..f3c9ddc0ce5c 100644
--- a/drivers/scsi/isci/host.h
+++ b/drivers/scsi/isci/host.h
@@ -203,7 +203,6 @@  struct isci_host {
 	#define IHOST_IRQ_ENABLED 2
 	unsigned long flags;
 	wait_queue_head_t eventq;
-	struct tasklet_struct completion_tasklet;
 	spinlock_t scic_lock;
 	struct isci_request *reqs[SCI_MAX_IO_REQUESTS];
 	struct isci_remote_device devices[SCI_MAX_REMOTE_DEVICES];
@@ -478,7 +477,7 @@  void isci_tci_free(struct isci_host *ihost, u16 tci);
 void ireq_done(struct isci_host *ihost, struct isci_request *ireq, struct sas_task *task);
 
 int isci_host_init(struct isci_host *);
-void isci_host_completion_routine(unsigned long data);
+irqreturn_t isci_host_completion_routine(int vec, void *data);
 void isci_host_deinit(struct isci_host *);
 void sci_controller_disable_interrupts(struct isci_host *ihost);
 bool sci_controller_has_remote_devices_stopping(struct isci_host *ihost);
diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index e294d5d961eb..d3ec9423d2b1 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -358,8 +358,10 @@  static int isci_setup_interrupts(struct pci_dev *pdev)
 		else
 			isr = isci_msix_isr;
 
-		err = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, i),
-				isr, 0, DRV_NAME"-msix", ihost);
+		err = devm_request_threaded_irq(&pdev->dev,
+						pci_irq_vector(pdev, i), isr,
+						isci_host_completion_routine,
+						0, DRV_NAME"-msix", ihost);
 		if (!err)
 			continue;
 
@@ -377,9 +379,12 @@  static int isci_setup_interrupts(struct pci_dev *pdev)
 
  intx:
 	for_each_isci_host(i, ihost, pdev) {
-		err = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, 0),
-				isci_intx_isr, IRQF_SHARED, DRV_NAME"-intx",
-				ihost);
+		err = devm_request_threaded_irq(&pdev->dev,
+						pci_irq_vector(pdev, 0),
+						isci_intx_isr,
+						isci_host_completion_routine,
+						IRQF_SHARED, DRV_NAME"-intx",
+						ihost);
 		if (err)
 			break;
 	}
@@ -513,8 +518,6 @@  static struct isci_host *isci_host_alloc(struct pci_dev *pdev, int id)
 	init_waitqueue_head(&ihost->eventq);
 	ihost->sas_ha.dev = &ihost->pdev->dev;
 	ihost->sas_ha.lldd_ha = ihost;
-	tasklet_init(&ihost->completion_tasklet,
-		     isci_host_completion_routine, (unsigned long)ihost);
 
 	/* validate module parameters */
 	/* TODO: kill struct sci_user_parameters and reference directly */