diff mbox series

[06/10] scsi/ibmvscsi_tgt: Replace work tasklet with threaded irq

Message ID 20220530231512.9729-7-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 deal with the async
work in task context.

Cc: Michael Cyr <mikecyr@linux.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 17 +++++++----------
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h |  1 -
 2 files changed, 7 insertions(+), 11 deletions(-)

Comments

Sebastian Andrzej Siewior June 3, 2022, 11:05 a.m. UTC | #1 
On 2022-05-30 16:15:08 [-0700], Davidlohr Bueso wrote:
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index eee1a24f7e15..fafadb7158a3 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -2948,9 +2948,8 @@ static irqreturn_t ibmvscsis_interrupt(int dummy, void *data)
>  	struct scsi_info *vscsi = data;
>  
>  	vio_disable_interrupts(vscsi->dma_dev);
> -	tasklet_schedule(&vscsi->work_task);
looks good.

> -	return IRQ_HANDLED;
> +	return IRQ_WAKE_THREAD;
>  }
>  
>  /**
> @@ -3340,7 +3339,7 @@ static void ibmvscsis_handle_crq(unsigned long data)
>  		dev_dbg(&vscsi->dev, "handle_crq, don't process: flags 0x%x, state 0x%hx\n",
>  			vscsi->flags, vscsi->state);
>  		spin_unlock_bh(&vscsi->intr_lock);

So if you move it away from from tasklet you can replace the spin_lock_bh()
with spin_lock() since BH does not matter anymore. Except if there is a
timer because it matters for a timer_list timer which is invoked in
softirq context. This isn't the case except that there is a hrtimer
invoking ibmvscsis_service_wait_q(). This is bad because a hrtimer is
which is invoked in hard-irq context so a BH lock must not be acquired.
lockdep would scream here. You could use HRTIMER_MODE_REL_SOFT which
would make it a BH timer. Then you could keep the BH locking but
actually you want to get rid of it ;)

> -		return;
> +	        goto done;
>  	}
>  
>  	rc = vscsi->flags & SCHEDULE_DISCONNECT;

Sebastian
diff mbox series

Patch

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index eee1a24f7e15..fafadb7158a3 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2948,9 +2948,8 @@  static irqreturn_t ibmvscsis_interrupt(int dummy, void *data)
 	struct scsi_info *vscsi = data;
 
 	vio_disable_interrupts(vscsi->dma_dev);
-	tasklet_schedule(&vscsi->work_task);
 
-	return IRQ_HANDLED;
+	return IRQ_WAKE_THREAD;
 }
 
 /**
@@ -3317,7 +3316,7 @@  static int ibmvscsis_rdma(struct ibmvscsis_cmd *cmd, struct scatterlist *sg,
  *
  * Note: this is an edge triggered interrupt. It can not be shared.
  */
-static void ibmvscsis_handle_crq(unsigned long data)
+static irqreturn_t ibmvscsis_handle_crq(int irq, void *data)
 {
 	struct scsi_info *vscsi = (struct scsi_info *)data;
 	struct viosrp_crq *crq;
@@ -3340,7 +3339,7 @@  static void ibmvscsis_handle_crq(unsigned long data)
 		dev_dbg(&vscsi->dev, "handle_crq, don't process: flags 0x%x, state 0x%hx\n",
 			vscsi->flags, vscsi->state);
 		spin_unlock_bh(&vscsi->intr_lock);
-		return;
+	        goto done;
 	}
 
 	rc = vscsi->flags & SCHEDULE_DISCONNECT;
@@ -3417,6 +3416,8 @@  static void ibmvscsis_handle_crq(unsigned long data)
 		vscsi->state);
 
 	spin_unlock_bh(&vscsi->intr_lock);
+done:
+	return IRQ_HANDLED;
 }
 
 static int ibmvscsis_probe(struct vio_dev *vdev,
@@ -3530,9 +3531,6 @@  static int ibmvscsis_probe(struct vio_dev *vdev,
 	dev_dbg(&vscsi->dev, "probe hrc %ld, client partition num %d\n",
 		hrc, vscsi->client_data.partition_number);
 
-	tasklet_init(&vscsi->work_task, ibmvscsis_handle_crq,
-		     (unsigned long)vscsi);
-
 	init_completion(&vscsi->wait_idle);
 	init_completion(&vscsi->unconfig);
 
@@ -3544,7 +3542,8 @@  static int ibmvscsis_probe(struct vio_dev *vdev,
 		goto unmap_buf;
 	}
 
-	rc = request_irq(vdev->irq, ibmvscsis_interrupt, 0, "ibmvscsis", vscsi);
+	rc = request_threaded_irq(vdev->irq, ibmvscsis_interrupt,
+				  ibmvscsis_handle_crq, 0, "ibmvscsis", vscsi);
 	if (rc) {
 		rc = -EPERM;
 		dev_err(&vscsi->dev, "probe: request_irq failed, rc %d\n", rc);
@@ -3565,7 +3564,6 @@  static int ibmvscsis_probe(struct vio_dev *vdev,
 free_buf:
 	kfree(vscsi->map_buf);
 destroy_queue:
-	tasklet_kill(&vscsi->work_task);
 	ibmvscsis_unregister_command_q(vscsi);
 	ibmvscsis_destroy_command_q(vscsi);
 free_timer:
@@ -3602,7 +3600,6 @@  static void ibmvscsis_remove(struct vio_dev *vdev)
 	dma_unmap_single(&vdev->dev, vscsi->map_ioba, PAGE_SIZE,
 			 DMA_BIDIRECTIONAL);
 	kfree(vscsi->map_buf);
-	tasklet_kill(&vscsi->work_task);
 	ibmvscsis_destroy_command_q(vscsi);
 	ibmvscsis_freetimer(vscsi);
 	ibmvscsis_free_cmds(vscsi);
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
index 7ae074e5d7a1..b66c982b8b00 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
@@ -295,7 +295,6 @@  struct scsi_info {
 	struct vio_dev *dma_dev;
 	struct srp_target target;
 	struct ibmvscsis_tport tport;
-	struct tasklet_struct work_task;
 	struct work_struct proc_work;
 };