[PATCH] mailbox: bcm: Replace tasklet with threaded irq

[PATCH] mailbox: bcm: Replace tasklet with threaded irq

[Davidlohr Bueso]

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.

Use a more suitable alternative such as threaded irqs and do the
async work in process context.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 drivers/mailbox/bcm-pdc-mailbox.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/mailbox/bcm-pdc-mailbox.c b/drivers/mailbox/bcm-pdc-mailbox.c
index 5b375985f7b8..4718722c3c67 100644
--- a/drivers/mailbox/bcm-pdc-mailbox.c
+++ b/drivers/mailbox/bcm-pdc-mailbox.c
@@ -294,9 +294,6 @@ struct pdc_state {
 
 	unsigned int pdc_irq;
 
-	/* tasklet for deferred processing after DMA rx interrupt */
-	struct tasklet_struct rx_tasklet;
-
 	/* Number of bytes of receive status prior to each rx frame */
 	u32 rx_status_len;
 	/* Whether a BCM header is prepended to each frame */
@@ -953,23 +950,23 @@ static irqreturn_t pdc_irq_handler(int irq, void *data)
 	iowrite32(intstatus, pdcs->pdc_reg_vbase + PDC_INTSTATUS_OFFSET);
 
 	/* Wakeup IRQ thread */
-	tasklet_schedule(&pdcs->rx_tasklet);
-	return IRQ_HANDLED;
+	return IRQ_WAKE_THREAD;
 }
 
 /**
- * pdc_tasklet_cb() - Tasklet callback that runs the deferred processing after
+ * pdc_task_cb() - Threaded IRQ that runs the deferred processing after
  * a DMA receive interrupt. Reenables the receive interrupt.
  * @data: PDC state structure
  */
-static void pdc_tasklet_cb(struct tasklet_struct *t)
+static irqreturn_t pdc_task_cb(int irq, void *data)
 {
-	struct pdc_state *pdcs = from_tasklet(pdcs, t, rx_tasklet);
+	struct pdc_state *pdcs = data;
 
 	pdc_receive(pdcs);
 
 	/* reenable interrupts */
 	iowrite32(PDC_INTMASK, pdcs->pdc_reg_vbase + PDC_INTMASK_OFFSET);
+	return IRQ_HANDLED;
 }
 
 /**
@@ -1405,8 +1402,8 @@ static int pdc_interrupts_init(struct pdc_state *pdcs)
 	dev_dbg(dev, "pdc device %s irq %u for pdcs %p",
 		dev_name(dev), pdcs->pdc_irq, pdcs);
 
-	err = devm_request_irq(dev, pdcs->pdc_irq, pdc_irq_handler, 0,
-			       dev_name(dev), dev);
+	err = devm_request_threaded_irq(dev, pdcs->pdc_irq, pdc_irq_handler,
+					pdc_task_cb, 0, dev_name(dev), dev);
 	if (err) {
 		dev_err(dev, "IRQ %u request failed with err %d\n",
 			pdcs->pdc_irq, err);
@@ -1588,9 +1585,6 @@ static int pdc_probe(struct platform_device *pdev)
 
 	pdc_hw_init(pdcs);
 
-	/* Init tasklet for deferred DMA rx processing */
-	tasklet_setup(&pdcs->rx_tasklet, pdc_tasklet_cb);
-
 	err = pdc_interrupts_init(pdcs);
 	if (err)
 		goto cleanup_buf_pool;
@@ -1606,7 +1600,6 @@ static int pdc_probe(struct platform_device *pdev)
 	return PDC_SUCCESS;
 
 cleanup_buf_pool:
-	tasklet_kill(&pdcs->rx_tasklet);
 	dma_pool_destroy(pdcs->rx_buf_pool);
 
 cleanup_ring_pool:
@@ -1622,8 +1615,6 @@ static int pdc_remove(struct platform_device *pdev)
 
 	pdc_free_debugfs();
 
-	tasklet_kill(&pdcs->rx_tasklet);
-
 	pdc_hw_disable(pdcs);
 
 	dma_pool_destroy(pdcs->rx_buf_pool);
-- 
2.26.2

Re: [PATCH] mailbox: bcm: Replace tasklet with threaded irq

[Jassi Brar]

On Thu, Jan 14, 2021 at 6:21 PM Davidlohr Bueso <dave@stgolabs.net> 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.
>
> Use a more suitable alternative such as threaded irqs and do the
> async work in process context.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
Please cc the author and other contributors to this file, esp when
this is vendor specific code.

thanks.

Re: [PATCH] mailbox: bcm: Replace tasklet with threaded irq

[Davidlohr Bueso]

On Thu, 14 Jan 2021, Jassi Brar wrote:

>On Thu, Jan 14, 2021 at 6:21 PM Davidlohr Bueso <dave@stgolabs.net> 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.
>>
>> Use a more suitable alternative such as threaded irqs and do the
>> async work in process context.
>>
>> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
>> ---
>Please cc the author and other contributors to this file, esp when
>this is vendor specific code.

So looking at who to Cc I noticed:

commit 8aef00f090bcbe5237c5a6628e7c000890267efe
Author: Rob Rice <rob.rice@broadcom.com>
Date:   Mon Nov 14 13:26:01 2016 -0500

     mailbox: bcm-pdc: Convert from threaded IRQ to tasklet

     Previously used threaded IRQs in the PDC driver to defer
     processing the rx DMA ring after getting an rx done interrupt.
     Instead, use a tasklet at normal priority for deferred processing.

... which is exactly the opposite of what modern Linux should be
doing. Rob, could this not be done in process context?

Thanks,
Davidlohr

Re: [PATCH] mailbox: bcm: Replace tasklet with threaded irq

[Rob Rice]

[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]

Davidlohr,

I haven’t worked on this driver for 4 years. So my memory is a little vague. But I have notes that we found using the tasklet was 40% faster than threaded IRQ. We were trying to get performance up. So that seemed important at the time. I’m not prepared to defend that now. :-)

Rob

> On Jan 14, 2021, at 10:53 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> On Thu, 14 Jan 2021, Jassi Brar wrote:
> 
>> On Thu, Jan 14, 2021 at 6:21 PM Davidlohr Bueso <dave@stgolabs.net> 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.
>>> 
>>> Use a more suitable alternative such as threaded irqs and do the
>>> async work in process context.
>>> 
>>> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
>>> ---
>> Please cc the author and other contributors to this file, esp when
>> this is vendor specific code.
> 
> So looking at who to Cc I noticed:
> 
> commit 8aef00f090bcbe5237c5a6628e7c000890267efe
> Author: Rob Rice <rob.rice@broadcom.com>
> Date:   Mon Nov 14 13:26:01 2016 -0500
> 
>    mailbox: bcm-pdc: Convert from threaded IRQ to tasklet
> 
>    Previously used threaded IRQs in the PDC driver to defer
>    processing the rx DMA ring after getting an rx done interrupt.
>    Instead, use a tasklet at normal priority for deferred processing.
> 
> ... which is exactly the opposite of what modern Linux should be
> doing. Rob, could this not be done in process context?
> 
> Thanks,
> Davidlohr


-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4154 bytes --]