* [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-05-31 8:05 ` John Garry
2022-05-30 23:15 ` [PATCH 02/10] scsi/megaraid: Replace adapter->dpc_h tasklet with threaded irq Davidlohr Bueso
` (9 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi; +Cc: martin.petersen, ejb, bigeasy, tglx, dave
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.
As such remove CONFIG_SCSI_MVSAS_TASKLET (which is horrible to begin
with) and continue to do the async work, unconditionally now, just
in task context. Just as with the tasklet version, device interrupts
(MVS_IRQ_SAS_A/B) continues to be disabled from when the work was
putted until it is actually complete. Because there are no guarantees
vs ksoftirqd, if this is broken for threaded irqs, then they are
also broken for tasklets.
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
drivers/scsi/mvsas/Kconfig | 7 ------
drivers/scsi/mvsas/mv_init.c | 44 ++++++------------------------------
drivers/scsi/mvsas/mv_sas.h | 1 -
3 files changed, 7 insertions(+), 45 deletions(-)
diff --git a/drivers/scsi/mvsas/Kconfig b/drivers/scsi/mvsas/Kconfig
index 79812b80743b..e9dd8dc84b1c 100644
--- a/drivers/scsi/mvsas/Kconfig
+++ b/drivers/scsi/mvsas/Kconfig
@@ -23,10 +23,3 @@ config SCSI_MVSAS_DEBUG
help
Compiles the 88SE64XX/88SE94XX driver in debug mode. In debug mode,
the driver prints some messages to the console.
-config SCSI_MVSAS_TASKLET
- bool "Support for interrupt tasklet"
- default n
- depends on SCSI_MVSAS
- help
- Compiles the 88SE64xx/88SE94xx driver in interrupt tasklet mode.In this mode,
- the interrupt will schedule a tasklet.
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 2fde496fff5f..f36b270ba502 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -146,12 +146,10 @@ static void mvs_free(struct mvs_info *mvi)
kfree(mvi);
}
-#ifdef CONFIG_SCSI_MVSAS_TASKLET
-static void mvs_tasklet(unsigned long opaque)
+static irqreturn_t mvs_async(int irq, void *opaque)
{
u32 stat;
u16 core_nr, i = 0;
-
struct mvs_info *mvi;
struct sas_ha_struct *sha = (struct sas_ha_struct *)opaque;
@@ -172,46 +170,29 @@ static void mvs_tasklet(unsigned long opaque)
out:
MVS_CHIP_DISP->interrupt_enable(mvi);
+ return IRQ_HANDLED;
}
-#endif
static irqreturn_t mvs_interrupt(int irq, void *opaque)
{
u32 stat;
struct mvs_info *mvi;
struct sas_ha_struct *sha = opaque;
-#ifndef CONFIG_SCSI_MVSAS_TASKLET
- u32 i;
- u32 core_nr;
-
- core_nr = ((struct mvs_prv_info *)sha->lldd_ha)->n_host;
-#endif
mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[0];
if (unlikely(!mvi))
return IRQ_NONE;
-#ifdef CONFIG_SCSI_MVSAS_TASKLET
+
MVS_CHIP_DISP->interrupt_disable(mvi);
-#endif
stat = MVS_CHIP_DISP->isr_status(mvi, irq);
if (!stat) {
- #ifdef CONFIG_SCSI_MVSAS_TASKLET
MVS_CHIP_DISP->interrupt_enable(mvi);
- #endif
return IRQ_NONE;
}
-#ifdef CONFIG_SCSI_MVSAS_TASKLET
- tasklet_schedule(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
-#else
- for (i = 0; i < core_nr; i++) {
- mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[i];
- MVS_CHIP_DISP->isr(mvi, irq, stat);
- }
-#endif
- return IRQ_HANDLED;
+ return IRQ_WAKE_THREAD;
}
static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
@@ -557,14 +538,6 @@ static int mvs_pci_init(struct pci_dev *pdev, const struct pci_device_id *ent)
}
nhost++;
} while (nhost < chip->n_host);
-#ifdef CONFIG_SCSI_MVSAS_TASKLET
- {
- struct mvs_prv_info *mpi = SHOST_TO_SAS_HA(shost)->lldd_ha;
-
- tasklet_init(&(mpi->mv_tasklet), mvs_tasklet,
- (unsigned long)SHOST_TO_SAS_HA(shost));
- }
-#endif
mvs_post_sas_ha_init(shost, chip);
@@ -575,8 +548,9 @@ static int mvs_pci_init(struct pci_dev *pdev, const struct pci_device_id *ent)
rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
if (rc)
goto err_out_shost;
- rc = request_irq(pdev->irq, irq_handler, IRQF_SHARED,
- DRV_NAME, SHOST_TO_SAS_HA(shost));
+ rc = request_threaded_irq(pdev->irq, irq_handler, mvs_async,
+ IRQF_SHARED, DRV_NAME,
+ SHOST_TO_SAS_HA(shost));
if (rc)
goto err_not_sas;
@@ -607,10 +581,6 @@ static void mvs_pci_remove(struct pci_dev *pdev)
core_nr = ((struct mvs_prv_info *)sha->lldd_ha)->n_host;
mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[0];
-#ifdef CONFIG_SCSI_MVSAS_TASKLET
- tasklet_kill(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
-#endif
-
sas_unregister_ha(sha);
sas_remove_host(mvi->shost);
diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index 509d8f32a04f..a0e87fdab98a 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -403,7 +403,6 @@ struct mvs_prv_info{
u8 scan_finished;
u8 reserve;
struct mvs_info *mvi[2];
- struct tasklet_struct mv_tasklet;
};
struct mvs_wq {
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-30 23:15 ` [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET Davidlohr Bueso
@ 2022-05-31 8:05 ` John Garry
2022-05-31 14:52 ` Davidlohr Bueso
0 siblings, 1 reply; 37+ messages in thread
From: John Garry @ 2022-05-31 8:05 UTC (permalink / raw)
To: Davidlohr Bueso, linux-scsi; +Cc: martin.petersen, ejb, bigeasy, tglx
On 31/05/2022 00: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.
/s/converted/convert/
>
> As such remove CONFIG_SCSI_MVSAS_TASKLET (which is horrible to begin
> with) and continue to do the async work, unconditionally now, just
> in task context. Just as with the tasklet version, device interrupts
> (MVS_IRQ_SAS_A/B) continues to be disabled from when the work was
/s/continues/continue/
> putted until it is actually complete.
Question: Can there be any good reason to do this?
> Because there are no guarantees
> vs ksoftirqd, if this is broken for threaded irqs, then they are
> also broken for tasklets.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Apart some comments/nits:
Reviewed-by: John Garry <john.garry@huawei.com>
> ---
> drivers/scsi/mvsas/Kconfig | 7 ------
> drivers/scsi/mvsas/mv_init.c | 44 ++++++------------------------------
> drivers/scsi/mvsas/mv_sas.h | 1 -
> 3 files changed, 7 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/scsi/mvsas/Kconfig b/drivers/scsi/mvsas/Kconfig
> index 79812b80743b..e9dd8dc84b1c 100644
> --- a/drivers/scsi/mvsas/Kconfig
> +++ b/drivers/scsi/mvsas/Kconfig
> @@ -23,10 +23,3 @@ config SCSI_MVSAS_DEBUG
> help
> Compiles the 88SE64XX/88SE94XX driver in debug mode. In debug mode,
> the driver prints some messages to the console.
> -config SCSI_MVSAS_TASKLET
> - bool "Support for interrupt tasklet"
> - default n
> - depends on SCSI_MVSAS
> - help
> - Compiles the 88SE64xx/88SE94xx driver in interrupt tasklet mode.In this mode,
> - the interrupt will schedule a tasklet.
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index 2fde496fff5f..f36b270ba502 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -146,12 +146,10 @@ static void mvs_free(struct mvs_info *mvi)
> kfree(mvi);
> }
>
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> -static void mvs_tasklet(unsigned long opaque)
> +static irqreturn_t mvs_async(int irq, void *opaque)
> {
> u32 stat;
> u16 core_nr, i = 0;
> -
> struct mvs_info *mvi;
> struct sas_ha_struct *sha = (struct sas_ha_struct *)opaque;
nit: you could have dropped this cast
>
> @@ -172,46 +170,29 @@ static void mvs_tasklet(unsigned long opaque)
> out:
> MVS_CHIP_DISP->interrupt_enable(mvi);
>
> + return IRQ_HANDLED;
> }
> -#endif
>
> static irqreturn_t mvs_interrupt(int irq, void *opaque)
> {
> u32 stat;
> struct mvs_info *mvi;
> struct sas_ha_struct *sha = opaque;
> -#ifndef CONFIG_SCSI_MVSAS_TASKLET
> - u32 i;
> - u32 core_nr;
> -
> - core_nr = ((struct mvs_prv_info *)sha->lldd_ha)->n_host;
> -#endif
>
> mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[0];
>
> if (unlikely(!mvi))
> return IRQ_NONE;
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> +
> MVS_CHIP_DISP->interrupt_disable(mvi);
> -#endif
>
> stat = MVS_CHIP_DISP->isr_status(mvi, irq);
> if (!stat) {
> - #ifdef CONFIG_SCSI_MVSAS_TASKLET
> MVS_CHIP_DISP->interrupt_enable(mvi);
> - #endif
> return IRQ_NONE;
> }
>
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> - tasklet_schedule(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
> -#else
> - for (i = 0; i < core_nr; i++) {
> - mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[i];
> - MVS_CHIP_DISP->isr(mvi, irq, stat);
> - }
> -#endif
> - return IRQ_HANDLED;
> + return IRQ_WAKE_THREAD;
> }
>
> static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
> @@ -557,14 +538,6 @@ static int mvs_pci_init(struct pci_dev *pdev, const struct pci_device_id *ent)
> }
> nhost++;
> } while (nhost < chip->n_host);
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> - {
> - struct mvs_prv_info *mpi = SHOST_TO_SAS_HA(shost)->lldd_ha;
> -
> - tasklet_init(&(mpi->mv_tasklet), mvs_tasklet,
> - (unsigned long)SHOST_TO_SAS_HA(shost));
> - }
> -#endif
>
> mvs_post_sas_ha_init(shost, chip);
>
> @@ -575,8 +548,9 @@ static int mvs_pci_init(struct pci_dev *pdev, const struct pci_device_id *ent)
> rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
> if (rc)
> goto err_out_shost;
> - rc = request_irq(pdev->irq, irq_handler, IRQF_SHARED,
> - DRV_NAME, SHOST_TO_SAS_HA(shost));
> + rc = request_threaded_irq(pdev->irq, irq_handler, mvs_async,
any reason not to use devm version?
> + IRQF_SHARED, DRV_NAME,
> + SHOST_TO_SAS_HA(shost));
> if (rc)
> goto err_not_sas;
>
> @@ -607,10 +581,6 @@ static void mvs_pci_remove(struct pci_dev *pdev)
> core_nr = ((struct mvs_prv_info *)sha->lldd_ha)->n_host;
> mvi = ((struct mvs_prv_info *)sha->lldd_ha)->mvi[0];
>
> -#ifdef CONFIG_SCSI_MVSAS_TASKLET
> - tasklet_kill(&((struct mvs_prv_info *)sha->lldd_ha)->mv_tasklet);
> -#endif
> -
> sas_unregister_ha(sha);
> sas_remove_host(mvi->shost);
>
> diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
> index 509d8f32a04f..a0e87fdab98a 100644
> --- a/drivers/scsi/mvsas/mv_sas.h
> +++ b/drivers/scsi/mvsas/mv_sas.h
> @@ -403,7 +403,6 @@ struct mvs_prv_info{
> u8 scan_finished;
> u8 reserve;
> struct mvs_info *mvi[2];
> - struct tasklet_struct mv_tasklet;
> };
>
> struct mvs_wq {
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-31 8:05 ` John Garry
@ 2022-05-31 14:52 ` Davidlohr Bueso
2022-05-31 15:12 ` John Garry
0 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-31 14:52 UTC (permalink / raw)
To: John Garry; +Cc: linux-scsi, martin.petersen, ejb, bigeasy, tglx
On Tue, 31 May 2022, John Garry wrote:
>Question: Can there be any good reason to do this?
Removing tasklets altogether, albeit perhaps a pipe dream.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-31 14:52 ` Davidlohr Bueso
@ 2022-05-31 15:12 ` John Garry
2022-05-31 15:17 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 37+ messages in thread
From: John Garry @ 2022-05-31 15:12 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: linux-scsi, martin.petersen, ejb, bigeasy, tglx
On 31/05/2022 15:52, Davidlohr Bueso wrote:
> On Tue, 31 May 2022, John Garry wrote:
>> Question: Can there be any good reason to do this?
>
> Removing tasklets altogether, albeit perhaps a pipe dream.
Sorry, maybe I was not clear, but I was just asking if there was a good
reason to disable interrupts at source while handling the interrupt, and
not the change to stop using a tasklet.
Thanks,
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-31 15:12 ` John Garry
@ 2022-05-31 15:17 ` Sebastian Andrzej Siewior
2022-05-31 15:26 ` John Garry
2022-06-01 1:04 ` Davidlohr Bueso
0 siblings, 2 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-05-31 15:17 UTC (permalink / raw)
To: John Garry; +Cc: Davidlohr Bueso, linux-scsi, martin.petersen, ejb, tglx
On 2022-05-31 16:12:05 [+0100], John Garry wrote:
> On 31/05/2022 15:52, Davidlohr Bueso wrote:
> > On Tue, 31 May 2022, John Garry wrote:
> > > Question: Can there be any good reason to do this?
> >
> > Removing tasklets altogether, albeit perhaps a pipe dream.
>
> Sorry, maybe I was not clear, but I was just asking if there was a good
> reason to disable interrupts at source while handling the interrupt, and not
> the change to stop using a tasklet.
Without reading the patch first: You need to disable the interrupt
source while the tasklet/ threaded interrupt is handled. Otherwise the
interrupt will keep coming and the tasklet/ threaded interrupt will have
no chance to make progress. So the box will lock up. This is often
overseen on fast machines because the interrupt needs a few usecs to
trigger and so the CPU is able to make a little bit of progress between
each trigger.
> Thanks,
> John
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-31 15:17 ` Sebastian Andrzej Siewior
@ 2022-05-31 15:26 ` John Garry
2022-05-31 15:31 ` Sebastian Andrzej Siewior
2022-06-01 1:04 ` Davidlohr Bueso
1 sibling, 1 reply; 37+ messages in thread
From: John Garry @ 2022-05-31 15:26 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Davidlohr Bueso, linux-scsi, martin.petersen, ejb, tglx
On 31/05/2022 16:17, Sebastian Andrzej Siewior wrote:
>> Sorry, maybe I was not clear, but I was just asking if there was a good
>> reason to disable interrupts at source while handling the interrupt, and not
>> the change to stop using a tasklet.
> Without reading the patch first: You need to disable the interrupt
> source while the tasklet/ threaded interrupt is handled. Otherwise the
> interrupt will keep coming and the tasklet/ threaded interrupt will have
> no chance to make progress. So the box will lock up. This is often
> overseen on fast machines because the interrupt needs a few usecs to
> trigger and so the CPU is able to make a little bit of progress between
> each trigger.
>
ah, so we would need IRQF_ONESHOT flag set to keep the interrupt line
disabled until the threaded part completes, right?
Thanks,
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-31 15:26 ` John Garry
@ 2022-05-31 15:31 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-05-31 15:31 UTC (permalink / raw)
To: John Garry; +Cc: Davidlohr Bueso, linux-scsi, martin.petersen, ejb, tglx
On 2022-05-31 16:26:26 [+0100], John Garry wrote:
> On 31/05/2022 16:17, Sebastian Andrzej Siewior wrote:
> > > Sorry, maybe I was not clear, but I was just asking if there was a good
> > > reason to disable interrupts at source while handling the interrupt, and not
> > > the change to stop using a tasklet.
> > Without reading the patch first: You need to disable the interrupt
> > source while the tasklet/ threaded interrupt is handled. Otherwise the
> > interrupt will keep coming and the tasklet/ threaded interrupt will have
> > no chance to make progress. So the box will lock up. This is often
> > overseen on fast machines because the interrupt needs a few usecs to
> > trigger and so the CPU is able to make a little bit of progress between
> > each trigger.
> >
>
> ah, so we would need IRQF_ONESHOT flag set to keep the interrupt line
> disabled until the threaded part completes, right?
This is one way of doing it and this is what threaded irqs do. However
this requires all handler to specify that option which is not very
compatible with shared handler.
> Thanks,
> John
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-05-31 15:17 ` Sebastian Andrzej Siewior
2022-05-31 15:26 ` John Garry
@ 2022-06-01 1:04 ` Davidlohr Bueso
2022-06-01 8:12 ` John Garry
1 sibling, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-06-01 1:04 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: John Garry, linux-scsi, martin.petersen, ejb, tglx
On Tue, 31 May 2022, Sebastian Andrzej Siewior wrote:
>On 2022-05-31 16:12:05 [+0100], John Garry wrote:
>> Sorry, maybe I was not clear, but I was just asking if there was a good
>> reason to disable interrupts at source while handling the interrupt, and not
>> the change to stop using a tasklet.
>
>Without reading the patch first: You need to disable the interrupt
>source while the tasklet/ threaded interrupt is handled. Otherwise the
>interrupt will keep coming and the tasklet/ threaded interrupt will have
>no chance to make progress. So the box will lock up. This is often
>overseen on fast machines because the interrupt needs a few usecs to
>trigger and so the CPU is able to make a little bit of progress between
>each trigger.
In addition it keeps current semantics wrt ksoftirqd, so no guarantees
this runs in irq context in the first place.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET
2022-06-01 1:04 ` Davidlohr Bueso
@ 2022-06-01 8:12 ` John Garry
0 siblings, 0 replies; 37+ messages in thread
From: John Garry @ 2022-06-01 8:12 UTC (permalink / raw)
To: Davidlohr Bueso, Sebastian Andrzej Siewior
Cc: linux-scsi, martin.petersen, ejb, tglx
On 01/06/2022 02:04, Davidlohr Bueso wrote:
> On Tue, 31 May 2022, Sebastian Andrzej Siewior wrote:
>
>> On 2022-05-31 16:12:05 [+0100], John Garry wrote:
>>> Sorry, maybe I was not clear, but I was just asking if there was a good
>>> reason to disable interrupts at source while handling the interrupt,
>>> and not
>>> the change to stop using a tasklet.
>>
>> Without reading the patch first: You need to disable the interrupt
>> source while the tasklet/ threaded interrupt is handled. Otherwise the
>> interrupt will keep coming and the tasklet/ threaded interrupt will have
>> no chance to make progress. So the box will lock up. This is often
>> overseen on fast machines because the interrupt needs a few usecs to
>> trigger and so the CPU is able to make a little bit of progress between
>> each trigger.
>
> In addition it keeps current semantics wrt ksoftirqd, so no guarantees
> this runs in irq context in the first place.
ok, Fine.
So is it actually documented anywhere what any low-level driver should
do in terms of masking interrupts at source for interrupt handling, i.e.
when and where we should ever do this? I see pci.rst does mention how we
may need this at driver removal time in "Stop IRQs on the device"
section, but not totally related.
Thanks,
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 02/10] scsi/megaraid: Replace adapter->dpc_h tasklet with threaded irq
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
2022-05-30 23:15 ` [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-02 8:36 ` Sebastian Andrzej Siewior
2022-05-30 23:15 ` [PATCH 03/10] scsi/megaraid_sas: Replace instance->tasklet " Davidlohr Bueso
` (8 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi
Cc: martin.petersen, ejb, bigeasy, tglx, dave, Kashyap Desai,
Sumit Saxena, Shivasharan S, megaraidlinux.pdl
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 do the ack sequence
in task context.
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/mega_common.h | 2 --
drivers/scsi/megaraid/megaraid_mbox.c | 52 ++++++++++-----------------
2 files changed, 19 insertions(+), 35 deletions(-)
diff --git a/drivers/scsi/megaraid/mega_common.h b/drivers/scsi/megaraid/mega_common.h
index 2ad0aa2f837d..3e56f74061b4 100644
--- a/drivers/scsi/megaraid/mega_common.h
+++ b/drivers/scsi/megaraid/mega_common.h
@@ -95,7 +95,6 @@ typedef struct {
/**
* struct adapter_t - driver's initialization structure
- * @aram dpc_h : tasklet handle
* @pdev : pci configuration pointer for kernel
* @host : pointer to host structure of mid-layer
* @lock : synchronization lock for mid-layer and driver
@@ -149,7 +148,6 @@ typedef struct {
#define VERSION_SIZE 16
typedef struct {
- struct tasklet_struct dpc_h;
struct pci_dev *pdev;
struct Scsi_Host *host;
spinlock_t lock;
diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
index 2a339d4a7e9d..b76f67887592 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -118,8 +118,7 @@ static void megaraid_mbox_prepare_epthru(adapter_t *, scb_t *,
struct scsi_cmnd *);
static irqreturn_t megaraid_isr(int, void *);
-
-static void megaraid_mbox_dpc(unsigned long);
+static irqreturn_t megaraid_mbox_dpc(int, void *);
static ssize_t megaraid_mbox_app_hndl_show(struct device *, struct device_attribute *attr, char *);
static ssize_t megaraid_mbox_ld_show(struct device *, struct device_attribute *attr, char *);
@@ -764,9 +763,8 @@ megaraid_init_mbox(adapter_t *adapter)
*/
/* request IRQ and register the interrupt service routine */
- if (request_irq(adapter->irq, megaraid_isr, IRQF_SHARED, "megaraid",
- adapter)) {
-
+ if (request_threaded_irq(adapter->irq, megaraid_isr, megaraid_mbox_dpc,
+ IRQF_SHARED, "megaraid", adapter)) {
con_log(CL_ANN, (KERN_WARNING
"megaraid: Couldn't register IRQ %d!\n", adapter->irq));
goto out_alloc_cmds;
@@ -879,10 +877,6 @@ megaraid_init_mbox(adapter_t *adapter)
}
}
- // setup tasklet for DPC
- tasklet_init(&adapter->dpc_h, megaraid_mbox_dpc,
- (unsigned long)adapter);
-
con_log(CL_DLEVEL1, (KERN_INFO
"megaraid mbox hba successfully initialized\n"));
@@ -917,8 +911,6 @@ megaraid_fini_mbox(adapter_t *adapter)
// flush all caches
megaraid_mbox_flush_cache(adapter);
- tasklet_kill(&adapter->dpc_h);
-
megaraid_sysfs_free_resources(adapter);
megaraid_free_cmd_packets(adapter);
@@ -2027,7 +2019,7 @@ megaraid_mbox_prepare_epthru(adapter_t *adapter, scb_t *scb,
*
* Returns: 1 if the interrupt is valid, 0 otherwise
*/
-static int
+static irqreturn_t
megaraid_ack_sequence(adapter_t *adapter)
{
mraid_device_t *raid_dev = ADAP2RAIDDEV(adapter);
@@ -2036,7 +2028,7 @@ megaraid_ack_sequence(adapter_t *adapter)
uint8_t nstatus;
uint8_t completed[MBOX_MAX_FIRMWARE_STATUS];
struct list_head clist;
- int handled;
+ int ret = IRQ_NONE;
uint32_t dword;
unsigned long flags;
int i, j;
@@ -2048,7 +2040,6 @@ megaraid_ack_sequence(adapter_t *adapter)
INIT_LIST_HEAD(&clist);
// loop till F/W has more commands for us to complete
- handled = 0;
spin_lock_irqsave(MAILBOX_LOCK(raid_dev), flags);
do {
/*
@@ -2056,9 +2047,10 @@ megaraid_ack_sequence(adapter_t *adapter)
* interrupt line low.
*/
dword = RDOUTDOOR(raid_dev);
- if (dword != 0x10001234) break;
+ if (dword != 0x10001234)
+ break;
- handled = 1;
+ ret = IRQ_WAKE_THREAD;
WROUTDOOR(raid_dev, 0x10001234);
@@ -2124,12 +2116,7 @@ megaraid_ack_sequence(adapter_t *adapter)
spin_unlock_irqrestore(COMPLETED_LIST_LOCK(adapter), flags);
-
- // schedule the DPC if there is some work for it
- if (handled)
- tasklet_schedule(&adapter->dpc_h);
-
- return handled;
+ return ret;
}
@@ -2144,29 +2131,27 @@ static irqreturn_t
megaraid_isr(int irq, void *devp)
{
adapter_t *adapter = devp;
- int handled;
+ int ret;
- handled = megaraid_ack_sequence(adapter);
+ ret = megaraid_ack_sequence(adapter);
/* Loop through any pending requests */
if (!adapter->quiescent) {
megaraid_mbox_runpendq(adapter, NULL);
}
- return IRQ_RETVAL(handled);
+ return ret;
}
/**
- * megaraid_mbox_dpc - the tasklet to complete the commands from completed list
- * @devp : pointer to HBA soft state
+ * megaraid_mbox_dpc - complete the commands from completed list
*
* Pick up the commands from the completed list and send back to the owners.
* This is a reentrant function and does not assume any locks are held while
- * it is being called.
+ * it is being called. Runs in process context.
*/
-static void
-megaraid_mbox_dpc(unsigned long devp)
+static irqreturn_t megaraid_mbox_dpc(int irq, void *devp)
{
adapter_t *adapter = (adapter_t *)devp;
mraid_device_t *raid_dev;
@@ -2188,7 +2173,8 @@ megaraid_mbox_dpc(unsigned long devp)
uioc_t *kioc;
- if (!adapter) return;
+ if (!adapter)
+ goto done;
raid_dev = ADAP2RAIDDEV(adapter);
@@ -2361,8 +2347,8 @@ megaraid_mbox_dpc(unsigned long devp)
// send the scsi packet back to kernel
scsi_done(scp);
}
-
- return;
+done:
+ return IRQ_HANDLED;
}
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 02/10] scsi/megaraid: Replace adapter->dpc_h tasklet with threaded irq
2022-05-30 23:15 ` [PATCH 02/10] scsi/megaraid: Replace adapter->dpc_h tasklet with threaded irq Davidlohr Bueso
@ 2022-06-02 8:36 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-02 8:36 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-scsi, martin.petersen, ejb, tglx, Kashyap Desai,
Sumit Saxena, Shivasharan S, megaraidlinux.pdl
On 2022-05-30 16:15:04 [-0700], Davidlohr Bueso wrote:
> @@ -2036,7 +2028,7 @@ megaraid_ack_sequence(adapter_t *adapter)
> uint8_t nstatus;
> uint8_t completed[MBOX_MAX_FIRMWARE_STATUS];
> struct list_head clist;
> - int handled;
> + int ret = IRQ_NONE;
irqreturn_t ret = IRQ_NONE;
> uint32_t dword;
> unsigned long flags;
> int i, j;
> @@ -2124,12 +2116,7 @@ megaraid_ack_sequence(adapter_t *adapter)
>
> spin_unlock_irqrestore(COMPLETED_LIST_LOCK(adapter), flags);
>
> -
> - // schedule the DPC if there is some work for it
> - if (handled)
> - tasklet_schedule(&adapter->dpc_h);
> -
> - return handled;
> + return ret;
> }
megaraid_ack_sequence() has another caller, the
scsi_host_template::eh_host_reset_handler callback
(megaraid_reset_handler). With that change, that threaded handler will
not be invoked in the reset case.
> @@ -2144,29 +2131,27 @@ static irqreturn_t
> megaraid_isr(int irq, void *devp)
> {
> adapter_t *adapter = devp;
> - int handled;
> + int ret;
>
> - handled = megaraid_ack_sequence(adapter);
> + ret = megaraid_ack_sequence(adapter);
>
> /* Loop through any pending requests */
> if (!adapter->quiescent) {
> megaraid_mbox_runpendq(adapter, NULL);
> }
>
> - return IRQ_RETVAL(handled);
> + return ret;
> }
>
>
> /**
> - * megaraid_mbox_dpc - the tasklet to complete the commands from completed list
> - * @devp : pointer to HBA soft state
> + * megaraid_mbox_dpc - complete the commands from completed list
> *
> * Pick up the commands from the completed list and send back to the owners.
> * This is a reentrant function and does not assume any locks are held while
> - * it is being called.
> + * it is being called. Runs in process context.
> */
> -static void
> -megaraid_mbox_dpc(unsigned long devp)
> +static irqreturn_t megaraid_mbox_dpc(int irq, void *devp)
> {
> adapter_t *adapter = (adapter_t *)devp;
that cast could vanish.
> mraid_device_t *raid_dev;
> @@ -2188,7 +2173,8 @@ megaraid_mbox_dpc(unsigned long devp)
> uioc_t *kioc;
>
>
> - if (!adapter) return;
> + if (!adapter)
> + goto done;
return IRQ_NONE;
> raid_dev = ADAP2RAIDDEV(adapter);
>
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 03/10] scsi/megaraid_sas: Replace instance->tasklet with threaded irq
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
2022-05-30 23:15 ` [PATCH 01/10] scsi/mvsas: Kill CONFIG_SCSI_MVSAS_TASKLET Davidlohr Bueso
2022-05-30 23:15 ` [PATCH 02/10] scsi/megaraid: Replace adapter->dpc_h tasklet with threaded irq Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-02 10:11 ` Sebastian Andrzej Siewior
2022-05-30 23:15 ` [PATCH 04/10] scsi/aic94xx: Replace the donelist tasklet " Davidlohr Bueso
` (7 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi
Cc: martin.petersen, ejb, bigeasy, tglx, dave, Kashyap Desai,
Sumit Saxena, Shivasharan S, megaraidlinux.pdl
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(-)
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,
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 03/10] scsi/megaraid_sas: Replace instance->tasklet with threaded irq
2022-05-30 23:15 ` [PATCH 03/10] scsi/megaraid_sas: Replace instance->tasklet " Davidlohr Bueso
@ 2022-06-02 10:11 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-02 10:11 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-scsi, martin.petersen, ejb, tglx, Kashyap Desai,
Sumit Saxena, Shivasharan S, megaraidlinux.pdl
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 04/10] scsi/aic94xx: Replace the donelist tasklet with threaded irq
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (2 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 03/10] scsi/megaraid_sas: Replace instance->tasklet " Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-02 10:31 ` Sebastian Andrzej Siewior
2022-05-30 23:15 ` [PATCH 05/10] scsi/isci: Replace completion_tasklet " Davidlohr Bueso
` (6 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi; +Cc: martin.petersen, ejb, bigeasy, tglx, dave
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 processing
of donelist entries task context.
Also rename a lot of calls removing the 'tasklet' part, which
of course no longer applies.
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
drivers/scsi/aic94xx/aic94xx_hwi.c | 23 ++------
drivers/scsi/aic94xx/aic94xx_hwi.h | 5 +-
drivers/scsi/aic94xx/aic94xx_init.c | 5 +-
drivers/scsi/aic94xx/aic94xx_scb.c | 88 ++++++++---------------------
drivers/scsi/aic94xx/aic94xx_task.c | 16 +++---
drivers/scsi/aic94xx/aic94xx_tmf.c | 40 ++++++-------
6 files changed, 63 insertions(+), 114 deletions(-)
diff --git a/drivers/scsi/aic94xx/aic94xx_hwi.c b/drivers/scsi/aic94xx/aic94xx_hwi.c
index 3dd110143471..562a59b03aa1 100644
--- a/drivers/scsi/aic94xx/aic94xx_hwi.c
+++ b/drivers/scsi/aic94xx/aic94xx_hwi.c
@@ -248,8 +248,6 @@ static void asd_get_max_scb_ddb(struct asd_ha_struct *asd_ha)
/* ---------- Done List initialization ---------- */
-static void asd_dl_tasklet_handler(unsigned long);
-
static int asd_init_dl(struct asd_ha_struct *asd_ha)
{
asd_ha->seq.actual_dl
@@ -261,8 +259,6 @@ static int asd_init_dl(struct asd_ha_struct *asd_ha)
asd_ha->seq.dl = asd_ha->seq.actual_dl->vaddr;
asd_ha->seq.dl_toggle = ASD_DEF_DL_TOGGLE;
asd_ha->seq.dl_next = 0;
- tasklet_init(&asd_ha->seq.dl_tasklet, asd_dl_tasklet_handler,
- (unsigned long) asd_ha);
return 0;
}
@@ -711,7 +707,7 @@ static void asd_chip_reset(struct asd_ha_struct *asd_ha)
/* ---------- Done List Routines ---------- */
-static void asd_dl_tasklet_handler(unsigned long data)
+irqreturn_t asd_dl_handler(int irq, void *data)
{
struct asd_ha_struct *asd_ha = (struct asd_ha_struct *) data;
struct asd_seq_data *seq = &asd_ha->seq;
@@ -741,26 +737,19 @@ static void asd_dl_tasklet_handler(unsigned long data)
seq->pending--;
spin_unlock_irqrestore(&seq->pend_q_lock, flags);
out:
- ascb->tasklet_complete(ascb, dl);
+ ascb->complete(ascb, dl);
next_1:
seq->dl_next = (seq->dl_next + 1) & (ASD_DL_SIZE-1);
if (!seq->dl_next)
seq->dl_toggle ^= DL_TOGGLE_MASK;
}
+
+ return IRQ_HANDLED;
}
/* ---------- Interrupt Service Routines ---------- */
-/**
- * asd_process_donelist_isr -- schedule processing of done list entries
- * @asd_ha: pointer to host adapter structure
- */
-static void asd_process_donelist_isr(struct asd_ha_struct *asd_ha)
-{
- tasklet_schedule(&asd_ha->seq.dl_tasklet);
-}
-
/**
* asd_com_sas_isr -- process device communication interrupt (COMINT)
* @asd_ha: pointer to host adapter structure
@@ -1011,8 +1000,6 @@ irqreturn_t asd_hw_isr(int irq, void *dev_id)
asd_write_reg_dword(asd_ha, CHIMINT, chimint);
(void) asd_read_reg_dword(asd_ha, CHIMINT);
- if (chimint & DLAVAIL)
- asd_process_donelist_isr(asd_ha);
if (chimint & COMINT)
asd_com_sas_isr(asd_ha);
if (chimint & DEVINT)
@@ -1021,6 +1008,8 @@ irqreturn_t asd_hw_isr(int irq, void *dev_id)
asd_rbi_exsi_isr(asd_ha);
if (chimint & HOSTERR)
asd_hst_pcix_isr(asd_ha);
+ if (chimint & DLAVAIL)
+ return IRQ_WAKE_THREAD;
return IRQ_HANDLED;
}
diff --git a/drivers/scsi/aic94xx/aic94xx_hwi.h b/drivers/scsi/aic94xx/aic94xx_hwi.h
index 930e192b1cd4..e3a97b5cfab5 100644
--- a/drivers/scsi/aic94xx/aic94xx_hwi.h
+++ b/drivers/scsi/aic94xx/aic94xx_hwi.h
@@ -117,7 +117,7 @@ struct asd_ascb {
struct asd_dma_tok dma_scb;
struct asd_dma_tok *sg_arr;
- void (*tasklet_complete)(struct asd_ascb *, struct done_list_struct *);
+ void (*complete)(struct asd_ascb *, struct done_list_struct *);
u8 uldd_timer:1;
/* internally generated command */
@@ -152,7 +152,6 @@ struct asd_seq_data {
void *tc_index_bitmap;
int tc_index_bitmap_bits;
- struct tasklet_struct dl_tasklet;
struct done_list_struct *dl; /* array of done list entries, equals */
struct asd_dma_tok *actual_dl; /* actual_dl->vaddr */
int dl_toggle;
@@ -356,7 +355,7 @@ static inline void asd_ascb_free_list(struct asd_ascb *ascb_list)
int asd_init_hw(struct asd_ha_struct *asd_ha);
irqreturn_t asd_hw_isr(int irq, void *dev_id);
-
+irqreturn_t asd_dl_handler(int irq, void *data);
struct asd_ascb *asd_ascb_alloc_list(struct asd_ha_struct
*asd_ha, int *num,
diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 954d0c5ae2e2..b0c4af0e6479 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -790,8 +790,9 @@ static int asd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
if (use_msi)
pci_enable_msi(asd_ha->pcidev);
- err = request_irq(asd_ha->pcidev->irq, asd_hw_isr, IRQF_SHARED,
- ASD_DRIVER_NAME, asd_ha);
+ err = request_threaded_irq(asd_ha->pcidev->irq, asd_hw_isr,
+ asd_dl_handler, IRQF_SHARED,
+ ASD_DRIVER_NAME, asd_ha);
if (err) {
asd_printk("couldn't get irq %d for %s\n",
asd_ha->pcidev->irq, pci_name(asd_ha->pcidev));
diff --git a/drivers/scsi/aic94xx/aic94xx_scb.c b/drivers/scsi/aic94xx/aic94xx_scb.c
index 68214a58b160..abe28d80892b 100644
--- a/drivers/scsi/aic94xx/aic94xx_scb.c
+++ b/drivers/scsi/aic94xx/aic94xx_scb.c
@@ -64,8 +64,8 @@ static void get_lrate_mode(struct asd_phy *phy, u8 oob_mode)
phy->sas_phy.oob_mode = SATA_OOB_MODE;
}
-static void asd_phy_event_tasklet(struct asd_ascb *ascb,
- struct done_list_struct *dl)
+static void asd_phy_event(struct asd_ascb *ascb,
+ struct done_list_struct *dl)
{
struct asd_ha_struct *asd_ha = ascb->ha;
int phy_id = dl->status_block[0] & DL_PHY_MASK;
@@ -215,9 +215,9 @@ static void asd_deform_port(struct asd_ha_struct *asd_ha, struct asd_phy *phy)
spin_unlock_irqrestore(&asd_ha->asd_ports_lock, flags);
}
-static void asd_bytes_dmaed_tasklet(struct asd_ascb *ascb,
- struct done_list_struct *dl,
- int edb_id, int phy_id)
+static void asd_bytes_dmaed(struct asd_ascb *ascb,
+ struct done_list_struct *dl,
+ int edb_id, int phy_id)
{
unsigned long flags;
int edb_el = edb_id + ascb->edb_index;
@@ -237,9 +237,9 @@ static void asd_bytes_dmaed_tasklet(struct asd_ascb *ascb,
sas_notify_port_event(&phy->sas_phy, PORTE_BYTES_DMAED, GFP_ATOMIC);
}
-static void asd_link_reset_err_tasklet(struct asd_ascb *ascb,
- struct done_list_struct *dl,
- int phy_id)
+static void asd_link_reset_err(struct asd_ascb *ascb,
+ struct done_list_struct *dl,
+ int phy_id)
{
struct asd_ha_struct *asd_ha = ascb->ha;
struct sas_ha_struct *sas_ha = &asd_ha->sas_ha;
@@ -290,9 +290,9 @@ static void asd_link_reset_err_tasklet(struct asd_ascb *ascb,
;
}
-static void asd_primitive_rcvd_tasklet(struct asd_ascb *ascb,
- struct done_list_struct *dl,
- int phy_id)
+static void asd_primitive_rcvd(struct asd_ascb *ascb,
+ struct done_list_struct *dl,
+ int phy_id)
{
unsigned long flags;
struct sas_ha_struct *sas_ha = &ascb->ha->sas_ha;
@@ -361,7 +361,6 @@ static void asd_primitive_rcvd_tasklet(struct asd_ascb *ascb,
*
* After an EDB has been invalidated, if all EDBs in this ESCB have been
* invalidated, the ESCB is posted back to the sequencer.
- * Context is tasklet/IRQ.
*/
void asd_invalidate_edb(struct asd_ascb *ascb, int edb_id)
{
@@ -396,8 +395,8 @@ void asd_invalidate_edb(struct asd_ascb *ascb, int edb_id)
}
}
-static void escb_tasklet_complete(struct asd_ascb *ascb,
- struct done_list_struct *dl)
+static void escb_complete(struct asd_ascb *ascb,
+ struct done_list_struct *dl)
{
struct asd_ha_struct *asd_ha = ascb->ha;
struct sas_ha_struct *sas_ha = &asd_ha->sas_ha;
@@ -546,21 +545,21 @@ static void escb_tasklet_complete(struct asd_ascb *ascb,
switch (sb_opcode) {
case BYTES_DMAED:
ASD_DPRINTK("%s: phy%d: BYTES_DMAED\n", __func__, phy_id);
- asd_bytes_dmaed_tasklet(ascb, dl, edb, phy_id);
+ asd_bytes_dmaed(ascb, dl, edb, phy_id);
break;
case PRIMITIVE_RECVD:
ASD_DPRINTK("%s: phy%d: PRIMITIVE_RECVD\n", __func__,
phy_id);
- asd_primitive_rcvd_tasklet(ascb, dl, phy_id);
+ asd_primitive_rcvd(ascb, dl, phy_id);
break;
case PHY_EVENT:
ASD_DPRINTK("%s: phy%d: PHY_EVENT\n", __func__, phy_id);
- asd_phy_event_tasklet(ascb, dl);
+ asd_phy_event(ascb, dl);
break;
case LINK_RESET_ERROR:
ASD_DPRINTK("%s: phy%d: LINK_RESET_ERROR\n", __func__,
phy_id);
- asd_link_reset_err_tasklet(ascb, dl, phy_id);
+ asd_link_reset_err(ascb, dl, phy_id);
break;
case TIMER_EVENT:
ASD_DPRINTK("%s: phy%d: TIMER_EVENT, lost dw sync\n",
@@ -600,7 +599,7 @@ int asd_init_post_escbs(struct asd_ha_struct *asd_ha)
int i;
for (i = 0; i < seq->num_escbs; i++)
- seq->escb_arr[i]->tasklet_complete = escb_tasklet_complete;
+ seq->escb_arr[i]->complete = escb_complete;
ASD_DPRINTK("posting %d escbs\n", i);
return asd_post_escb_list(asd_ha, seq->escb_arr[0], seq->num_escbs);
@@ -613,7 +612,7 @@ int asd_init_post_escbs(struct asd_ha_struct *asd_ha)
| CURRENT_OOB_ERROR)
/**
- * control_phy_tasklet_complete -- tasklet complete for CONTROL PHY ascb
+ * control_phy_complete -- complete for CONTROL PHY ascb
* @ascb: pointer to an ascb
* @dl: pointer to the done list entry
*
@@ -623,8 +622,8 @@ int asd_init_post_escbs(struct asd_ha_struct *asd_ha)
* - if a device is connected to the LED, it is lit,
* - if no device is connected to the LED, is is dimmed (off).
*/
-static void control_phy_tasklet_complete(struct asd_ascb *ascb,
- struct done_list_struct *dl)
+static void control_phy_complete(struct asd_ascb *ascb,
+ struct done_list_struct *dl)
{
struct asd_ha_struct *asd_ha = ascb->ha;
struct scb *scb = ascb->scb;
@@ -758,10 +757,9 @@ static void set_speed_mask(u8 *speed_mask, struct asd_phy_desc *pd)
*
* This function builds a CONTROL PHY scb. No allocation of any kind
* is performed. @ascb is allocated with the list function.
- * The caller can override the ascb->tasklet_complete to point
+ * The caller can override the ascb->complete to point
* to its own callback function. It must call asd_ascb_free()
- * at its tasklet complete function.
- * See the default implementation.
+ * at its complete function. See the default implementation.
*/
void asd_build_control_phy(struct asd_ascb *ascb, int phy_id, u8 subfunc)
{
@@ -806,47 +804,9 @@ void asd_build_control_phy(struct asd_ascb *ascb, int phy_id, u8 subfunc)
control_phy->conn_handle = cpu_to_le16(0xFFFF);
- ascb->tasklet_complete = control_phy_tasklet_complete;
+ ascb->complete = control_phy_complete;
}
-/* ---------- INITIATE LINK ADM TASK ---------- */
-
-#if 0
-
-static void link_adm_tasklet_complete(struct asd_ascb *ascb,
- struct done_list_struct *dl)
-{
- u8 opcode = dl->opcode;
- struct initiate_link_adm *link_adm = &ascb->scb->link_adm;
- u8 phy_id = link_adm->phy_id;
-
- if (opcode != TC_NO_ERROR) {
- asd_printk("phy%d: link adm task 0x%x completed with error "
- "0x%x\n", phy_id, link_adm->sub_func, opcode);
- }
- ASD_DPRINTK("phy%d: link adm task 0x%x: 0x%x\n",
- phy_id, link_adm->sub_func, opcode);
-
- asd_ascb_free(ascb);
-}
-
-void asd_build_initiate_link_adm_task(struct asd_ascb *ascb, int phy_id,
- u8 subfunc)
-{
- struct scb *scb = ascb->scb;
- struct initiate_link_adm *link_adm = &scb->link_adm;
-
- scb->header.opcode = INITIATE_LINK_ADM_TASK;
-
- link_adm->phy_id = phy_id;
- link_adm->sub_func = subfunc;
- link_adm->conn_handle = cpu_to_le16(0xFFFF);
-
- ascb->tasklet_complete = link_adm_tasklet_complete;
-}
-
-#endif /* 0 */
-
/* ---------- SCB timer ---------- */
/**
diff --git a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c
index ed119a3f6f2e..63e0a6db6683 100644
--- a/drivers/scsi/aic94xx/aic94xx_task.c
+++ b/drivers/scsi/aic94xx/aic94xx_task.c
@@ -135,10 +135,10 @@ static void asd_unmap_scatterlist(struct asd_ascb *ascb)
task->num_scatter, task->data_dir);
}
-/* ---------- Task complete tasklet ---------- */
+/* ---------- Task complete ---------- */
-static void asd_get_response_tasklet(struct asd_ascb *ascb,
- struct done_list_struct *dl)
+static void asd_get_response(struct asd_ascb *ascb,
+ struct done_list_struct *dl)
{
struct asd_ha_struct *asd_ha = ascb->ha;
struct sas_task *task = ascb->uldd_task;
@@ -191,7 +191,7 @@ static void asd_get_response_tasklet(struct asd_ascb *ascb,
asd_invalidate_edb(escb, edb_id);
}
-static void asd_task_tasklet_complete(struct asd_ascb *ascb,
+static void asd_task_complete(struct asd_ascb *ascb,
struct done_list_struct *dl)
{
struct sas_task *task = ascb->uldd_task;
@@ -221,7 +221,7 @@ static void asd_task_tasklet_complete(struct asd_ascb *ascb,
case TC_ATA_RESP:
ts->resp = SAS_TASK_COMPLETE;
ts->stat = SAS_PROTO_RESPONSE;
- asd_get_response_tasklet(ascb, dl);
+ asd_get_response(ascb, dl);
break;
case TF_OPEN_REJECT:
ts->resp = SAS_TASK_UNDELIVERED;
@@ -394,7 +394,7 @@ static int asd_build_ata_ascb(struct asd_ascb *ascb, struct sas_task *task,
flags |= STP_AFFIL_POLICY;
scb->ata_task.flags = flags;
}
- ascb->tasklet_complete = asd_task_tasklet_complete;
+ ascb->complete = asd_task_complete;
if (likely(!task->ata_task.device_control_reg_update))
res = asd_map_scatterlist(task, scb->ata_task.sg_element,
@@ -442,7 +442,7 @@ static int asd_build_smp_ascb(struct asd_ascb *ascb, struct sas_task *task,
scb->smp_task.conn_handle = cpu_to_le16((u16)
(unsigned long)dev->lldd_dev);
- ascb->tasklet_complete = asd_task_tasklet_complete;
+ ascb->complete = asd_task_complete;
return 0;
}
@@ -495,7 +495,7 @@ static int asd_build_ssp_ascb(struct asd_ascb *ascb, struct sas_task *task,
scb->ssp_task.data_dir = data_dir_flags[task->data_dir];
scb->ssp_task.retry_count = scb->ssp_task.retry_count;
- ascb->tasklet_complete = asd_task_tasklet_complete;
+ ascb->complete = asd_task_complete;
res = asd_map_scatterlist(task, scb->ssp_task.sg_element, gfp_flags);
diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c
index 27d32b8c2987..00a148f73250 100644
--- a/drivers/scsi/aic94xx/aic94xx_tmf.c
+++ b/drivers/scsi/aic94xx/aic94xx_tmf.c
@@ -15,13 +15,13 @@
/* ---------- Internal enqueue ---------- */
static int asd_enqueue_internal(struct asd_ascb *ascb,
- void (*tasklet_complete)(struct asd_ascb *,
- struct done_list_struct *),
+ void (*complete)(struct asd_ascb *,
+ struct done_list_struct *),
void (*timed_out)(struct timer_list *t))
{
int res;
- ascb->tasklet_complete = tasklet_complete;
+ ascb->complete = complete;
ascb->uldd_timer = 1;
ascb->timer.function = timed_out;
@@ -37,7 +37,7 @@ static int asd_enqueue_internal(struct asd_ascb *ascb,
/* ---------- CLEAR NEXUS ---------- */
-struct tasklet_completion_status {
+struct completion_status {
int dl_opcode;
int tmf_state;
u8 tag_valid:1;
@@ -45,7 +45,7 @@ struct tasklet_completion_status {
};
#define DECLARE_TCS(tcs) \
- struct tasklet_completion_status tcs = { \
+ struct completion_status tcs = { \
.dl_opcode = 0, \
.tmf_state = 0, \
.tag_valid = 0, \
@@ -53,10 +53,10 @@ struct tasklet_completion_status {
}
-static void asd_clear_nexus_tasklet_complete(struct asd_ascb *ascb,
- struct done_list_struct *dl)
+static void asd_clear_nexus_complete(struct asd_ascb *ascb,
+ struct done_list_struct *dl)
{
- struct tasklet_completion_status *tcs = ascb->uldd_task;
+ struct completion_status *tcs = ascb->uldd_task;
ASD_DPRINTK("%s: here\n", __func__);
if (!del_timer(&ascb->timer)) {
ASD_DPRINTK("%s: couldn't delete timer\n", __func__);
@@ -71,7 +71,7 @@ static void asd_clear_nexus_tasklet_complete(struct asd_ascb *ascb,
static void asd_clear_nexus_timedout(struct timer_list *t)
{
struct asd_ascb *ascb = from_timer(ascb, t, timer);
- struct tasklet_completion_status *tcs = ascb->uldd_task;
+ struct completion_status *tcs = ascb->uldd_task;
ASD_DPRINTK("%s: here\n", __func__);
tcs->dl_opcode = TMF_RESP_FUNC_FAILED;
@@ -98,7 +98,7 @@ static void asd_clear_nexus_timedout(struct timer_list *t)
#define CLEAR_NEXUS_POST \
ASD_DPRINTK("%s: POST\n", __func__); \
- res = asd_enqueue_internal(ascb, asd_clear_nexus_tasklet_complete, \
+ res = asd_enqueue_internal(ascb, asd_clear_nexus_complete, \
asd_clear_nexus_timedout); \
if (res) \
goto out_err; \
@@ -245,14 +245,14 @@ static int asd_clear_nexus_index(struct sas_task *task)
static void asd_tmf_timedout(struct timer_list *t)
{
struct asd_ascb *ascb = from_timer(ascb, t, timer);
- struct tasklet_completion_status *tcs = ascb->uldd_task;
+ struct completion_status *tcs = ascb->uldd_task;
ASD_DPRINTK("tmf timed out\n");
tcs->tmf_state = TMF_RESP_FUNC_FAILED;
complete(ascb->completion);
}
-static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb,
+static int asd_get_tmf_resp(struct asd_ascb *ascb,
struct done_list_struct *dl)
{
struct asd_ha_struct *asd_ha = ascb->ha;
@@ -270,7 +270,7 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb,
struct ssp_response_iu *ru;
int res = TMF_RESP_FUNC_FAILED;
- ASD_DPRINTK("tmf resp tasklet\n");
+ ASD_DPRINTK("tmf resp\n");
spin_lock_irqsave(&asd_ha->seq.tc_index_lock, flags);
escb = asd_tc_index_find(&asd_ha->seq,
@@ -298,21 +298,21 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb,
return res;
}
-static void asd_tmf_tasklet_complete(struct asd_ascb *ascb,
- struct done_list_struct *dl)
+static void asd_tmf_complete(struct asd_ascb *ascb,
+ struct done_list_struct *dl)
{
- struct tasklet_completion_status *tcs;
+ struct completion_status *tcs;
if (!del_timer(&ascb->timer))
return;
tcs = ascb->uldd_task;
- ASD_DPRINTK("tmf tasklet complete\n");
+ ASD_DPRINTK("tmf complete\n");
tcs->dl_opcode = dl->opcode;
if (dl->opcode == TC_SSP_RESP) {
- tcs->tmf_state = asd_get_tmf_resp_tasklet(ascb, dl);
+ tcs->tmf_state = asd_get_tmf_resp(ascb, dl);
tcs->tag_valid = ascb->tag_valid;
tcs->tag = ascb->tag;
}
@@ -452,7 +452,7 @@ int asd_abort_task(struct sas_task *task)
scb->abort_task.index = cpu_to_le16((u16)tascb->tc_index);
scb->abort_task.itnl_to = cpu_to_le16(ITNL_TIMEOUT_CONST);
- res = asd_enqueue_internal(ascb, asd_tmf_tasklet_complete,
+ res = asd_enqueue_internal(ascb, asd_tmf_complete,
asd_tmf_timedout);
if (res)
goto out_free;
@@ -600,7 +600,7 @@ static int asd_initiate_ssp_tmf(struct domain_device *dev, u8 *lun,
if (tmf == TMF_QUERY_TASK)
scb->ssp_tmf.index = cpu_to_le16(index);
- res = asd_enqueue_internal(ascb, asd_tmf_tasklet_complete,
+ res = asd_enqueue_internal(ascb, asd_tmf_complete,
asd_tmf_timedout);
if (res)
goto out_err;
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 04/10] scsi/aic94xx: Replace the donelist tasklet with threaded irq
2022-05-30 23:15 ` [PATCH 04/10] scsi/aic94xx: Replace the donelist tasklet " Davidlohr Bueso
@ 2022-06-02 10:31 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-02 10:31 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: linux-scsi, martin.petersen, ejb, tglx
On 2022-05-30 16:15:06 [-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 processing
> of donelist entries task context.
>
> Also rename a lot of calls removing the 'tasklet' part, which
> of course no longer applies.
The change looks okay except that I don't see where the interrupt source
is disabled. It looks like asd_disable_ints() plus the last part of
asd_enable_ints() is needed.
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 05/10] scsi/isci: Replace completion_tasklet with threaded irq
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (3 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 04/10] scsi/aic94xx: Replace the donelist tasklet " Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-02 18:19 ` Sebastian Andrzej Siewior
2022-06-06 10:24 ` Artur Paszkiewicz
2022-05-30 23:15 ` [PATCH 06/10] scsi/ibmvscsi_tgt: Replace work tasklet " Davidlohr Bueso
` (5 subsequent siblings)
10 siblings, 2 replies; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi; +Cc: martin.petersen, ejb, bigeasy, tglx, dave, Artur Paszkiewicz
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(-)
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 */
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 05/10] scsi/isci: Replace completion_tasklet with threaded irq
2022-05-30 23:15 ` [PATCH 05/10] scsi/isci: Replace completion_tasklet " Davidlohr Bueso
@ 2022-06-02 18:19 ` Sebastian Andrzej Siewior
2022-06-06 10:24 ` Artur Paszkiewicz
1 sibling, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-02 18:19 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: linux-scsi, martin.petersen, ejb, tglx, Artur Paszkiewicz
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 05/10] scsi/isci: Replace completion_tasklet with threaded irq
2022-05-30 23:15 ` [PATCH 05/10] scsi/isci: Replace completion_tasklet " Davidlohr Bueso
2022-06-02 18:19 ` Sebastian Andrzej Siewior
@ 2022-06-06 10:24 ` Artur Paszkiewicz
2022-06-07 9:13 ` Sebastian Andrzej Siewior
1 sibling, 1 reply; 37+ messages in thread
From: Artur Paszkiewicz @ 2022-06-06 10:24 UTC (permalink / raw)
To: Davidlohr Bueso, linux-scsi; +Cc: martin.petersen, ejb, bigeasy, tglx
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>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 06/10] scsi/ibmvscsi_tgt: Replace work tasklet with threaded irq
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (4 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 05/10] scsi/isci: Replace completion_tasklet " Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-03 11:05 ` Sebastian Andrzej Siewior
2022-05-30 23:15 ` [PATCH 07/10] scsi/esas2r: Replace tasklet with workqueue Davidlohr Bueso
` (4 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi
Cc: martin.petersen, ejb, bigeasy, tglx, dave, Michael Cyr, linuxppc-dev
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(-)
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;
};
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 06/10] scsi/ibmvscsi_tgt: Replace work tasklet with threaded irq
2022-05-30 23:15 ` [PATCH 06/10] scsi/ibmvscsi_tgt: Replace work tasklet " Davidlohr Bueso
@ 2022-06-03 11:05 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-03 11:05 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-scsi, martin.petersen, ejb, tglx, Michael Cyr, linuxppc-dev
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 07/10] scsi/esas2r: Replace tasklet with workqueue
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (5 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 06/10] scsi/ibmvscsi_tgt: Replace work tasklet " Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-09 12:14 ` Sebastian Andrzej Siewior
2022-05-30 23:15 ` [PATCH 08/10] scsi/ibmvfc: Replace tasklet with work Davidlohr Bueso
` (3 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi; +Cc: martin.petersen, ejb, bigeasy, tglx, dave, Bradley Grove
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 an dedicated (single threaded) high-priority workqueue instead
such that async work can be done in task context instead. The
AF_WORK_SCHEDULED semantics remain the same for the tasklet scope.
Cc: Bradley Grove <linuxdrivers@attotech.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
drivers/scsi/esas2r/esas2r.h | 19 +++++++++--------
drivers/scsi/esas2r/esas2r_init.c | 20 ++++++++----------
drivers/scsi/esas2r/esas2r_int.c | 20 +++++++++---------
drivers/scsi/esas2r/esas2r_io.c | 2 +-
drivers/scsi/esas2r/esas2r_main.c | 34 ++++++++++++++++++++-----------
5 files changed, 52 insertions(+), 43 deletions(-)
diff --git a/drivers/scsi/esas2r/esas2r.h b/drivers/scsi/esas2r/esas2r.h
index ed63f7a9ea54..732309425956 100644
--- a/drivers/scsi/esas2r/esas2r.h
+++ b/drivers/scsi/esas2r/esas2r.h
@@ -64,6 +64,7 @@
#define ESAS2R_H
/* Global Variables */
+extern struct workqueue_struct *esas2r_wq;
extern struct esas2r_adapter *esas2r_adapters[];
extern u8 *esas2r_buffered_ioctl;
extern dma_addr_t esas2r_buffered_ioctl_addr;
@@ -815,7 +816,7 @@ struct esas2r_adapter {
#define AF_NVR_VALID 12
#define AF_DEGRADED_MODE 13
#define AF_DISC_PENDING 14
- #define AF_TASKLET_SCHEDULED 15
+ #define AF_WORK_SCHEDULED 15
#define AF_HEARTBEAT 16
#define AF_HEARTBEAT_ENB 17
#define AF_NOT_PRESENT 18
@@ -900,7 +901,7 @@ struct esas2r_adapter {
struct esas2r_flash_context flash_context;
u32 num_targets_backend;
u32 ioctl_tunnel;
- struct tasklet_struct tasklet;
+ struct work_struct work;
struct pci_dev *pcid;
struct Scsi_Host *host;
unsigned int index;
@@ -992,7 +993,7 @@ int esas2r_write_vda(struct esas2r_adapter *a, const char *buf, long off,
int esas2r_read_fs(struct esas2r_adapter *a, char *buf, long off, int count);
int esas2r_write_fs(struct esas2r_adapter *a, const char *buf, long off,
int count);
-void esas2r_adapter_tasklet(unsigned long context);
+void esas2r_adapter_work(struct work_struct *work);
irqreturn_t esas2r_interrupt(int irq, void *dev_id);
irqreturn_t esas2r_msi_interrupt(int irq, void *dev_id);
void esas2r_kickoff_timer(struct esas2r_adapter *a);
@@ -1022,7 +1023,7 @@ bool esas2r_init_adapter_hw(struct esas2r_adapter *a, bool init_poll);
void esas2r_start_request(struct esas2r_adapter *a, struct esas2r_request *rq);
bool esas2r_send_task_mgmt(struct esas2r_adapter *a,
struct esas2r_request *rqaux, u8 task_mgt_func);
-void esas2r_do_tasklet_tasks(struct esas2r_adapter *a);
+void esas2r_do_work_tasks(struct esas2r_adapter *a);
void esas2r_adapter_interrupt(struct esas2r_adapter *a);
void esas2r_do_deferred_processes(struct esas2r_adapter *a);
void esas2r_reset_bus(struct esas2r_adapter *a);
@@ -1283,7 +1284,7 @@ static inline void esas2r_rq_destroy_request(struct esas2r_request *rq,
rq->data_buf = NULL;
}
-static inline bool esas2r_is_tasklet_pending(struct esas2r_adapter *a)
+static inline bool esas2r_is_work_pending(struct esas2r_adapter *a)
{
return test_bit(AF_BUSRST_NEEDED, &a->flags) ||
@@ -1324,14 +1325,14 @@ static inline void esas2r_enable_chip_interrupts(struct esas2r_adapter *a)
ESAS2R_INT_ENB_MASK);
}
-/* Schedule a TASKLET to perform non-interrupt tasks that may require delays
+/* Schedule work to perform non-interrupt tasks that may require delays
* or long completion times.
*/
-static inline void esas2r_schedule_tasklet(struct esas2r_adapter *a)
+static inline void esas2r_schedule_work(struct esas2r_adapter *a)
{
/* make sure we don't schedule twice */
- if (!test_and_set_bit(AF_TASKLET_SCHEDULED, &a->flags))
- tasklet_hi_schedule(&a->tasklet);
+ if (!test_and_set_bit(AF_WORK_SCHEDULED, &a->flags))
+ queue_work(esas2r_wq, &a->work);
}
static inline void esas2r_enable_heartbeat(struct esas2r_adapter *a)
diff --git a/drivers/scsi/esas2r/esas2r_init.c b/drivers/scsi/esas2r/esas2r_init.c
index c1a5ab662dc8..c7ca9435d395 100644
--- a/drivers/scsi/esas2r/esas2r_init.c
+++ b/drivers/scsi/esas2r/esas2r_init.c
@@ -401,9 +401,7 @@ int esas2r_init_adapter(struct Scsi_Host *host, struct pci_dev *pcid,
return 0;
}
- tasklet_init(&a->tasklet,
- esas2r_adapter_tasklet,
- (unsigned long)a);
+ INIT_WORK(&a->work, esas2r_adapter_work);
/*
* Disable chip interrupts to prevent spurious interrupts
@@ -441,7 +439,7 @@ static void esas2r_adapter_power_down(struct esas2r_adapter *a,
&& (!test_bit(AF_DEGRADED_MODE, &a->flags))) {
if (!power_management) {
del_timer_sync(&a->timer);
- tasklet_kill(&a->tasklet);
+ cancel_work_sync(&a->work);
}
esas2r_power_down(a);
@@ -1338,7 +1336,7 @@ bool esas2r_init_adapter_hw(struct esas2r_adapter *a, bool init_poll)
* usually requested during initial driver load and possibly when
* resuming from a low power state. deferred device waiting will use
* interrupts. chip reset recovery always defers device waiting to
- * avoid being in a TASKLET too long.
+ * avoid being in a work too long.
*/
if (init_poll) {
u32 currtime = a->disc_start_time;
@@ -1346,10 +1344,10 @@ bool esas2r_init_adapter_hw(struct esas2r_adapter *a, bool init_poll)
u32 deltatime;
/*
- * Block Tasklets from getting scheduled and indicate this is
+ * Block async work from getting scheduled and indicate this is
* polled discovery.
*/
- set_bit(AF_TASKLET_SCHEDULED, &a->flags);
+ set_bit(AF_WORK_SCHEDULED, &a->flags);
set_bit(AF_DISC_POLLED, &a->flags);
/*
@@ -1394,8 +1392,8 @@ bool esas2r_init_adapter_hw(struct esas2r_adapter *a, bool init_poll)
nexttick -= deltatime;
/* Do any deferred processing */
- if (esas2r_is_tasklet_pending(a))
- esas2r_do_tasklet_tasks(a);
+ if (esas2r_is_work_pending(a))
+ esas2r_do_work_tasks(a);
}
@@ -1403,7 +1401,7 @@ bool esas2r_init_adapter_hw(struct esas2r_adapter *a, bool init_poll)
atomic_inc(&a->disable_cnt);
clear_bit(AF_DISC_POLLED, &a->flags);
- clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
+ clear_bit(AF_WORK_SCHEDULED, &a->flags);
}
@@ -1463,7 +1461,7 @@ void esas2r_reset_adapter(struct esas2r_adapter *a)
{
set_bit(AF_OS_RESET, &a->flags);
esas2r_local_reset_adapter(a);
- esas2r_schedule_tasklet(a);
+ esas2r_schedule_work(a);
}
void esas2r_reset_chip(struct esas2r_adapter *a)
diff --git a/drivers/scsi/esas2r/esas2r_int.c b/drivers/scsi/esas2r/esas2r_int.c
index 5281d9356327..1b1b8b65539d 100644
--- a/drivers/scsi/esas2r/esas2r_int.c
+++ b/drivers/scsi/esas2r/esas2r_int.c
@@ -86,7 +86,7 @@ void esas2r_polled_interrupt(struct esas2r_adapter *a)
/*
* Legacy and MSI interrupt handlers. Note that the legacy interrupt handler
- * schedules a TASKLET to process events, whereas the MSI handler just
+ * schedules work to process events, whereas the MSI handler just
* processes interrupt events directly.
*/
irqreturn_t esas2r_interrupt(int irq, void *dev_id)
@@ -97,7 +97,7 @@ irqreturn_t esas2r_interrupt(int irq, void *dev_id)
return IRQ_NONE;
set_bit(AF2_INT_PENDING, &a->flags2);
- esas2r_schedule_tasklet(a);
+ esas2r_schedule_work(a);
return IRQ_HANDLED;
}
@@ -162,7 +162,7 @@ irqreturn_t esas2r_msi_interrupt(int irq, void *dev_id)
if (likely(atomic_read(&a->disable_cnt) == 0))
esas2r_do_deferred_processes(a);
- esas2r_do_tasklet_tasks(a);
+ esas2r_do_work_tasks(a);
return 1;
}
@@ -327,8 +327,8 @@ void esas2r_do_deferred_processes(struct esas2r_adapter *a)
/* Clear off the completed list to be processed later. */
- if (esas2r_is_tasklet_pending(a)) {
- esas2r_schedule_tasklet(a);
+ if (esas2r_is_work_pending(a)) {
+ esas2r_schedule_work(a);
startreqs = 0;
}
@@ -476,7 +476,7 @@ static void esas2r_process_bus_reset(struct esas2r_adapter *a)
esas2r_trace_exit();
}
-static void esas2r_chip_rst_needed_during_tasklet(struct esas2r_adapter *a)
+static void esas2r_chip_rst_needed_during_work(struct esas2r_adapter *a)
{
clear_bit(AF_CHPRST_NEEDED, &a->flags);
@@ -558,7 +558,7 @@ static void esas2r_chip_rst_needed_during_tasklet(struct esas2r_adapter *a)
}
}
-static void esas2r_handle_chip_rst_during_tasklet(struct esas2r_adapter *a)
+static void esas2r_handle_chip_rst_during_work(struct esas2r_adapter *a)
{
while (test_bit(AF_CHPRST_DETECTED, &a->flags)) {
/*
@@ -614,15 +614,15 @@ static void esas2r_handle_chip_rst_during_tasklet(struct esas2r_adapter *a)
/* Perform deferred tasks when chip interrupts are disabled */
-void esas2r_do_tasklet_tasks(struct esas2r_adapter *a)
+void esas2r_do_work_tasks(struct esas2r_adapter *a)
{
if (test_bit(AF_CHPRST_NEEDED, &a->flags) ||
test_bit(AF_CHPRST_DETECTED, &a->flags)) {
if (test_bit(AF_CHPRST_NEEDED, &a->flags))
- esas2r_chip_rst_needed_during_tasklet(a);
+ esas2r_chip_rst_needed_during_work(a);
- esas2r_handle_chip_rst_during_tasklet(a);
+ esas2r_handle_chip_rst_during_work(a);
}
if (test_bit(AF_BUSRST_NEEDED, &a->flags)) {
diff --git a/drivers/scsi/esas2r/esas2r_io.c b/drivers/scsi/esas2r/esas2r_io.c
index a8df916cd57a..d45e6e16a858 100644
--- a/drivers/scsi/esas2r/esas2r_io.c
+++ b/drivers/scsi/esas2r/esas2r_io.c
@@ -851,7 +851,7 @@ void esas2r_reset_bus(struct esas2r_adapter *a)
set_bit(AF_BUSRST_PENDING, &a->flags);
set_bit(AF_OS_RESET, &a->flags);
- esas2r_schedule_tasklet(a);
+ esas2r_schedule_work(a);
}
}
diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
index 7a4eadad23d7..abe45a934cce 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -530,7 +530,7 @@ static void esas2r_remove(struct pci_dev *pdev)
static int __init esas2r_init(void)
{
- int i;
+ int i, ret;
esas2r_log(ESAS2R_LOG_INFO, "%s called", __func__);
@@ -606,7 +606,15 @@ static int __init esas2r_init(void)
for (i = 0; i < MAX_ADAPTERS; i++)
esas2r_adapters[i] = NULL;
- return pci_register_driver(&esas2r_pci_driver);
+ esas2r_wq = alloc_ordered_workqueue("esas2r_wq", WQ_HIGHPRI);
+ if (!esas2r_wq)
+ return -ENOMEM;
+
+ ret = pci_register_driver(&esas2r_pci_driver);
+ if (ret)
+ destroy_workqueue(esas2r_wq);
+
+ return ret;
}
/* Handle ioctl calls to "/proc/scsi/esas2r/ATTOnode" */
@@ -649,6 +657,8 @@ static void __exit esas2r_exit(void)
esas2r_log(ESAS2R_LOG_INFO, "pci_unregister_driver() called");
pci_unregister_driver(&esas2r_pci_driver);
+
+ destroy_workqueue(esas2r_wq);
}
int esas2r_show_info(struct seq_file *m, struct Scsi_Host *sh)
@@ -1540,10 +1550,10 @@ void esas2r_complete_request_cb(struct esas2r_adapter *a,
esas2r_free_request(a, rq);
}
-/* Run tasklet to handle stuff outside of interrupt context. */
-void esas2r_adapter_tasklet(unsigned long context)
+/* Handle stuff outside of interrupt context. */
+void esas2r_adapter_work(struct work_struct *work)
{
- struct esas2r_adapter *a = (struct esas2r_adapter *)context;
+ struct esas2r_adapter *a = (struct esas2r_adapter *)work;
if (unlikely(test_bit(AF2_TIMER_TICK, &a->flags2))) {
clear_bit(AF2_TIMER_TICK, &a->flags2);
@@ -1555,16 +1565,16 @@ void esas2r_adapter_tasklet(unsigned long context)
esas2r_adapter_interrupt(a);
}
- if (esas2r_is_tasklet_pending(a))
- esas2r_do_tasklet_tasks(a);
+ if (esas2r_is_work_pending(a))
+ esas2r_do_work_tasks(a);
- if (esas2r_is_tasklet_pending(a)
+ if (esas2r_is_work_pending(a)
|| (test_bit(AF2_INT_PENDING, &a->flags2))
|| (test_bit(AF2_TIMER_TICK, &a->flags2))) {
- clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
- esas2r_schedule_tasklet(a);
+ clear_bit(AF_WORK_SCHEDULED, &a->flags);
+ esas2r_schedule_work(a);
} else {
- clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
+ clear_bit(AF_WORK_SCHEDULED, &a->flags);
}
}
@@ -1586,7 +1596,7 @@ static void esas2r_timer_callback(struct timer_list *t)
set_bit(AF2_TIMER_TICK, &a->flags2);
- esas2r_schedule_tasklet(a);
+ esas2r_schedule_work(a);
esas2r_kickoff_timer(a);
}
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 07/10] scsi/esas2r: Replace tasklet with workqueue
2022-05-30 23:15 ` [PATCH 07/10] scsi/esas2r: Replace tasklet with workqueue Davidlohr Bueso
@ 2022-06-09 12:14 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-09 12:14 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: linux-scsi, martin.petersen, ejb, tglx, Bradley Grove
On 2022-05-30 16:15:09 [-0700], Davidlohr Bueso wrote:
> diff --git a/drivers/scsi/esas2r/esas2r_int.c b/drivers/scsi/esas2r/esas2r_int.c
> index 5281d9356327..1b1b8b65539d 100644
> --- a/drivers/scsi/esas2r/esas2r_int.c
> +++ b/drivers/scsi/esas2r/esas2r_int.c
> @@ -86,7 +86,7 @@ void esas2r_polled_interrupt(struct esas2r_adapter *a)
>
> /*
> * Legacy and MSI interrupt handlers. Note that the legacy interrupt handler
> - * schedules a TASKLET to process events, whereas the MSI handler just
> + * schedules work to process events, whereas the MSI handler just
> * processes interrupt events directly.
Yeah, and why? What is special about the legacy vs MSI that different
behaviour is needed. Why not do the tasklet/workqueue thingy for MSI,
too?
> */
> irqreturn_t esas2r_interrupt(int irq, void *dev_id)
> diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
> index 7a4eadad23d7..abe45a934cce 100644
> --- a/drivers/scsi/esas2r/esas2r_main.c
> +++ b/drivers/scsi/esas2r/esas2r_main.c
> @@ -1540,10 +1550,10 @@ void esas2r_complete_request_cb(struct esas2r_adapter *a,
> esas2r_free_request(a, rq);
> }
>
> -/* Run tasklet to handle stuff outside of interrupt context. */
> -void esas2r_adapter_tasklet(unsigned long context)
> +/* Handle stuff outside of interrupt context. */
> +void esas2r_adapter_work(struct work_struct *work)
> {
> - struct esas2r_adapter *a = (struct esas2r_adapter *)context;
> + struct esas2r_adapter *a = (struct esas2r_adapter *)work;
container_of()
>
> if (unlikely(test_bit(AF2_TIMER_TICK, &a->flags2))) {
> clear_bit(AF2_TIMER_TICK, &a->flags2);
> @@ -1555,16 +1565,16 @@ void esas2r_adapter_tasklet(unsigned long context)
> esas2r_adapter_interrupt(a);
> }
>
> - if (esas2r_is_tasklet_pending(a))
> - esas2r_do_tasklet_tasks(a);
> + if (esas2r_is_work_pending(a))
> + esas2r_do_work_tasks(a);
>
> - if (esas2r_is_tasklet_pending(a)
> + if (esas2r_is_work_pending(a)
> || (test_bit(AF2_INT_PENDING, &a->flags2))
> || (test_bit(AF2_TIMER_TICK, &a->flags2))) {
> - clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
> - esas2r_schedule_tasklet(a);
> + clear_bit(AF_WORK_SCHEDULED, &a->flags);
> + esas2r_schedule_work(a);
This AF_TASKLET_SCHEDULED bit is odd and shouldn't be needed. What you
usually do is set_bit() + schedule_tasklet() and clear_bit() within the
tasklet. If the tasklet is already running then it will be scheduled
again which is intended. If it is not yet running then it will run once.
> } else {
> - clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
> + clear_bit(AF_WORK_SCHEDULED, &a->flags);
> }
> }
>
> @@ -1586,7 +1596,7 @@ static void esas2r_timer_callback(struct timer_list *t)
>
> set_bit(AF2_TIMER_TICK, &a->flags2);
>
> - esas2r_schedule_tasklet(a);
> + esas2r_schedule_work(a);
>
> esas2r_kickoff_timer(a);
It appears that the timer is always scheduled (except in degraded
mode) and the timer re-arms itself and schedules the tasklet. The timer
and the tasklet are synchronized and the tasklet will always after the
timer and one can not interrupt the other. But with the workqueue you
can get the following depending on how much the workqueue was delayed:
worker timer
esas2r_adapter_work();
-> test_bit(AF2_TIMER_TICK, &a->flags2) -> false
esas2r_timer_callback
set_bit(AF2_TIMER_TICK, &a->flags2);
esas2r_schedule_tasklet()
-> if (!test_and_set_bit(AF_TASKLET_SCHEDULED, &a->flags)) (bit set)
no schedule since bit was set
esas2r_kickoff_timer();
clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
No more tasklets. So another reason to get rid of this
AF_TASKLET_SCHEDULED bit ;)
> }
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 08/10] scsi/ibmvfc: Replace tasklet with work
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (6 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 07/10] scsi/esas2r: Replace tasklet with workqueue Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-09 12:30 ` Sebastian Andrzej Siewior
2022-05-30 23:15 ` [PATCH 09/10] scsi/ibmvscsi: Replace srp " Davidlohr Bueso
` (2 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi
Cc: martin.petersen, ejb, bigeasy, tglx, dave, Tyrel Datwyler, linuxppc-dev
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 workqueue instead and
run in task context - albeit the increased concurrency (tasklets
safe against themselves), but the handler is done under both the
vhost's host_lock + crq.q_lock so should be safe.
Cc: Tyrel Datwyler <tyreld@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 21 ++++++++++++---------
drivers/scsi/ibmvscsi/ibmvfc.h | 3 ++-
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index d0eab5700dc5..31b1900489e7 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -891,7 +891,7 @@ static void ibmvfc_release_crq_queue(struct ibmvfc_host *vhost)
ibmvfc_dbg(vhost, "Releasing CRQ\n");
free_irq(vdev->irq, vhost);
- tasklet_kill(&vhost->tasklet);
+ cancel_work_sync(&vhost->work);
do {
if (rc)
msleep(100);
@@ -3689,22 +3689,22 @@ static irqreturn_t ibmvfc_interrupt(int irq, void *dev_instance)
spin_lock_irqsave(vhost->host->host_lock, flags);
vio_disable_interrupts(to_vio_dev(vhost->dev));
- tasklet_schedule(&vhost->tasklet);
+ schedule_work(&vhost->work);
spin_unlock_irqrestore(vhost->host->host_lock, flags);
return IRQ_HANDLED;
}
/**
- * ibmvfc_tasklet - Interrupt handler tasklet
+ * ibmvfc_work - work handler
* @data: ibmvfc host struct
*
* Returns:
* Nothing
**/
-static void ibmvfc_tasklet(void *data)
+static void ibmvfc_workfn(struct work_struct *work)
{
- struct ibmvfc_host *vhost = data;
- struct vio_dev *vdev = to_vio_dev(vhost->dev);
+ struct ibmvfc_host *vhost;
+ struct vio_dev *vdev;
struct ibmvfc_crq *crq;
struct ibmvfc_async_crq *async;
struct ibmvfc_event *evt, *temp;
@@ -3712,6 +3712,9 @@ static void ibmvfc_tasklet(void *data)
int done = 0;
LIST_HEAD(evt_doneq);
+ vhost = container_of(work, struct ibmvfc_host, work);
+ vdev = to_vio_dev(vhost->dev);
+
spin_lock_irqsave(vhost->host->host_lock, flags);
spin_lock(vhost->crq.q_lock);
while (!done) {
@@ -5722,7 +5725,7 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost)
retrc = 0;
- tasklet_init(&vhost->tasklet, (void *)ibmvfc_tasklet, (unsigned long)vhost);
+ INIT_WORK(&vhost->work, ibmvfc_workfn);
if ((rc = request_irq(vdev->irq, ibmvfc_interrupt, 0, IBMVFC_NAME, vhost))) {
dev_err(dev, "Couldn't register irq 0x%x. rc=%d\n", vdev->irq, rc);
@@ -5738,7 +5741,7 @@ static int ibmvfc_init_crq(struct ibmvfc_host *vhost)
return retrc;
req_irq_failed:
- tasklet_kill(&vhost->tasklet);
+ cancel_work_sync(&vhost->work);
do {
rc = plpar_hcall_norets(H_FREE_CRQ, vdev->unit_address);
} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));
@@ -6213,7 +6216,7 @@ static int ibmvfc_resume(struct device *dev)
spin_lock_irqsave(vhost->host->host_lock, flags);
vio_disable_interrupts(vdev);
- tasklet_schedule(&vhost->tasklet);
+ schedule_work(&vhost->work);
spin_unlock_irqrestore(vhost->host->host_lock, flags);
return 0;
}
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index 3718406e0988..7eca3622a2fa 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -12,6 +12,7 @@
#include <linux/list.h>
#include <linux/types.h>
+#include <linux/workqueue.h>
#include <scsi/viosrp.h>
#define IBMVFC_NAME "ibmvfc"
@@ -892,7 +893,7 @@ struct ibmvfc_host {
char partition_name[97];
void (*job_step) (struct ibmvfc_host *);
struct task_struct *work_thread;
- struct tasklet_struct tasklet;
+ struct work_struct work;
struct work_struct rport_add_work_q;
wait_queue_head_t init_wait_q;
wait_queue_head_t work_wait_q;
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 08/10] scsi/ibmvfc: Replace tasklet with work
2022-05-30 23:15 ` [PATCH 08/10] scsi/ibmvfc: Replace tasklet with work Davidlohr Bueso
@ 2022-06-09 12:30 ` Sebastian Andrzej Siewior
2022-06-28 15:18 ` Davidlohr Bueso
0 siblings, 1 reply; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-09 12:30 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-scsi, martin.petersen, ejb, tglx, Tyrel Datwyler, linuxppc-dev
On 2022-05-30 16:15:10 [-0700], Davidlohr Bueso wrote:
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index d0eab5700dc5..31b1900489e7 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -891,7 +891,7 @@ static void ibmvfc_release_crq_queue(struct ibmvfc_host *vhost)
>
> ibmvfc_dbg(vhost, "Releasing CRQ\n");
> free_irq(vdev->irq, vhost);
> - tasklet_kill(&vhost->tasklet);
> + cancel_work_sync(&vhost->work);
s/ {8}/\t/
is there a reason not to use threaded interrupts? The workqueue _might_
migrate to another CPU. The locking ensures that nothing bad happens but
ibmvfc_tasklet() has this piece:
| spin_lock_irqsave(vhost->host->host_lock, flags);
…
| while ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
| ibmvfc_handle_async(async, vhost);
| async->valid = 0;
| wmb();
| }
…
| vio_enable_interrupts(vdev);
potentially enables interrupts which fires right away.
| if ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
| vio_disable_interrupts(vdev);
disables it again.
| }
|
| spin_unlock(vhost->crq.q_lock);
| spin_unlock_irqrestore(vhost->host->host_lock, flags);
If the worker runs on another CPU then the CPU servicing the interrupt
will be blocked on the lock which is not nice.
My guess is that you could enable interrupts right before unlocking but
is a different story.
> do {
> if (rc)
> msleep(100);
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 08/10] scsi/ibmvfc: Replace tasklet with work
2022-06-09 12:30 ` Sebastian Andrzej Siewior
@ 2022-06-28 15:18 ` Davidlohr Bueso
0 siblings, 0 replies; 37+ messages in thread
From: Davidlohr Bueso @ 2022-06-28 15:18 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-scsi, martin.petersen, ejb, tglx, Tyrel Datwyler, linuxppc-dev
On Thu, 09 Jun 2022, Sebastian Andrzej Siewior wrote:
>On 2022-05-30 16:15:10 [-0700], Davidlohr Bueso wrote:
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index d0eab5700dc5..31b1900489e7 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -891,7 +891,7 @@ static void ibmvfc_release_crq_queue(struct ibmvfc_host *vhost)
>>
>> ibmvfc_dbg(vhost, "Releasing CRQ\n");
>> free_irq(vdev->irq, vhost);
>> - tasklet_kill(&vhost->tasklet);
>> + cancel_work_sync(&vhost->work);
>s/ {8}/\t/
>
>is there a reason not to use threaded interrupts?
I went with a workqueue here because the resume from suspend also schedules
async processing, so threaded irqs didn't seem like a good fit. This is also
similar to patch 2 (but in that case I overlooked the resume caller which you
pointed out).
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 09/10] scsi/ibmvscsi: Replace srp tasklet with work
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (7 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 08/10] scsi/ibmvfc: Replace tasklet with work Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-09 15:02 ` Sebastian Andrzej Siewior
2022-05-30 23:15 ` [PATCH 10/10] scsi/lpfc: Remove bogus references to discovery tasklet Davidlohr Bueso
2022-06-02 7:57 ` [PATCH 00/10] scsi: Replace tasklets as BH Sebastian Andrzej Siewior
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi
Cc: martin.petersen, ejb, bigeasy, tglx, dave, Tyrel Datwyler, linuxppc-dev
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.
Process srps asynchronously in process context in a dedicated
single threaded workqueue.
Cc: Tyrel Datwyler <tyreld@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 38 ++++++++++++++++++++++----------
drivers/scsi/ibmvscsi/ibmvscsi.h | 3 ++-
2 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 63f32f843e75..37cbea8bb0af 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -86,6 +86,8 @@ static DEFINE_SPINLOCK(ibmvscsi_driver_lock);
static struct scsi_transport_template *ibmvscsi_transport_template;
+static struct workqueue_struct *ibmvscsi_wq;
+
#define IBMVSCSI_VERSION "1.5.9"
MODULE_DESCRIPTION("IBM Virtual SCSI");
@@ -117,7 +119,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
* @irq: number of irq to handle, not used
* @dev_instance: ibmvscsi_host_data of host that received interrupt
*
- * Disables interrupts and schedules srp_task
+ * Disables interrupts and schedules srp_work
* Always returns IRQ_HANDLED
*/
static irqreturn_t ibmvscsi_handle_event(int irq, void *dev_instance)
@@ -125,7 +127,7 @@ static irqreturn_t ibmvscsi_handle_event(int irq, void *dev_instance)
struct ibmvscsi_host_data *hostdata =
(struct ibmvscsi_host_data *)dev_instance;
vio_disable_interrupts(to_vio_dev(hostdata->dev));
- tasklet_schedule(&hostdata->srp_task);
+ queue_work(ibmvscsi_wq, &hostdata->srp_work);
return IRQ_HANDLED;
}
@@ -145,7 +147,7 @@ static void ibmvscsi_release_crq_queue(struct crq_queue *queue,
long rc = 0;
struct vio_dev *vdev = to_vio_dev(hostdata->dev);
free_irq(vdev->irq, (void *)hostdata);
- tasklet_kill(&hostdata->srp_task);
+ cancel_work_sync(&hostdata->srp_work);
do {
if (rc)
msleep(100);
@@ -206,16 +208,19 @@ static int ibmvscsi_send_crq(struct ibmvscsi_host_data *hostdata,
}
/**
- * ibmvscsi_task: - Process srps asynchronously
+ * ibmvscsi_workfn: - Process srps asynchronously
* @data: ibmvscsi_host_data of host
*/
-static void ibmvscsi_task(void *data)
+static void ibmvscsi_workfn(struct work_struct *work)
{
- struct ibmvscsi_host_data *hostdata = (struct ibmvscsi_host_data *)data;
- struct vio_dev *vdev = to_vio_dev(hostdata->dev);
+ struct ibmvscsi_host_data *hostdata;
+ struct vio_dev *vdev;
struct viosrp_crq *crq;
int done = 0;
+ hostdata = container_of(work, struct ibmvscsi_host_data, srp_work);
+ vdev = to_vio_dev(hostdata->dev);
+
while (!done) {
/* Pull all the valid messages off the CRQ */
while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) {
@@ -367,8 +372,7 @@ static int ibmvscsi_init_crq_queue(struct crq_queue *queue,
queue->cur = 0;
spin_lock_init(&queue->lock);
- tasklet_init(&hostdata->srp_task, (void *)ibmvscsi_task,
- (unsigned long)hostdata);
+ INIT_WORK(&hostdata->srp_work, ibmvscsi_workfn);
if (request_irq(vdev->irq,
ibmvscsi_handle_event,
@@ -387,7 +391,7 @@ static int ibmvscsi_init_crq_queue(struct crq_queue *queue,
return retrc;
req_irq_failed:
- tasklet_kill(&hostdata->srp_task);
+ cancel_work_sync(&hostdata->srp_work);
rc = 0;
do {
if (rc)
@@ -2371,7 +2375,7 @@ static int ibmvscsi_resume(struct device *dev)
{
struct ibmvscsi_host_data *hostdata = dev_get_drvdata(dev);
vio_disable_interrupts(to_vio_dev(hostdata->dev));
- tasklet_schedule(&hostdata->srp_task);
+ queue_work(ibmvscsi_wq, &hostdata->srp_work);
return 0;
}
@@ -2418,15 +2422,25 @@ static int __init ibmvscsi_module_init(void)
if (!ibmvscsi_transport_template)
return -ENOMEM;
+ ibmvscsi_wq = alloc_ordered_workqueue("ibmvscsi_wq", 0);
+ if (!ibmvscsi_wq) {
+ srp_release_transport(ibmvscsi_transport_template);
+ return -ENOMEM;
+ }
+
ret = vio_register_driver(&ibmvscsi_driver);
- if (ret)
+ if (ret) {
+ destroy_workqueue(ibmvscsi_wq);
srp_release_transport(ibmvscsi_transport_template);
+ }
+
return ret;
}
static void __exit ibmvscsi_module_exit(void)
{
vio_unregister_driver(&ibmvscsi_driver);
+ destroy_workqueue(ibmvscsi_wq);
srp_release_transport(ibmvscsi_transport_template);
}
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index e60916ef7a49..f7c52744a206 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -18,6 +18,7 @@
#include <linux/types.h>
#include <linux/list.h>
#include <linux/completion.h>
+#include <linux/workqueue.h>
#include <linux/interrupt.h>
#include <scsi/viosrp.h>
@@ -90,7 +91,7 @@ struct ibmvscsi_host_data {
struct device *dev;
struct event_pool pool;
struct crq_queue queue;
- struct tasklet_struct srp_task;
+ struct work_struct srp_work;
struct list_head sent;
struct Scsi_Host *host;
struct task_struct *work_thread;
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 09/10] scsi/ibmvscsi: Replace srp tasklet with work
2022-05-30 23:15 ` [PATCH 09/10] scsi/ibmvscsi: Replace srp " Davidlohr Bueso
@ 2022-06-09 15:02 ` Sebastian Andrzej Siewior
2022-06-09 15:46 ` David Laight
0 siblings, 1 reply; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-09 15:02 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-scsi, martin.petersen, ejb, tglx, Tyrel Datwyler, linuxppc-dev
On 2022-05-30 16:15:11 [-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.
>
> Process srps asynchronously in process context in a dedicated
> single threaded workqueue.
I would suggest threaded interrupts instead. The pattern here is the
same as in the previous driver except here is less locking.
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH 09/10] scsi/ibmvscsi: Replace srp tasklet with work
2022-06-09 15:02 ` Sebastian Andrzej Siewior
@ 2022-06-09 15:46 ` David Laight
2022-06-14 13:25 ` 'Sebastian Andrzej Siewior'
0 siblings, 1 reply; 37+ messages in thread
From: David Laight @ 2022-06-09 15:46 UTC (permalink / raw)
To: 'Sebastian Andrzej Siewior', Davidlohr Bueso
Cc: Tyrel Datwyler, ejb, linux-scsi, martin.petersen, tglx, linuxppc-dev
From: Sebastian Andrzej Siewior
> Sent: 09 June 2022 16:03
>
> On 2022-05-30 16:15:11 [-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.
> >
> > Process srps asynchronously in process context in a dedicated
> > single threaded workqueue.
>
> I would suggest threaded interrupts instead. The pattern here is the
> same as in the previous driver except here is less locking.
How long do these actions runs for, and what is waiting for
them to finish?
These changes seem to drop the priority from above that of the
highest priority RT process down to that of a default priority
user process.
There is no real guarantee that the latter will run 'any time soon'.
Consider some workloads I'm setting up where most of the cpu are
likely to spend 90%+ of the time running processes under the RT
scheduler that are processing audio.
It is quite likely that a non-RT thread (especially one bound
to a specific cpu) won't run for several milliseconds.
(We have to go through 'hoops' to avoid dropping ethernet frames.)
I'd have thought that some of these kernel threads really
need to run at a 'middling' RT priority.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 09/10] scsi/ibmvscsi: Replace srp tasklet with work
2022-06-09 15:46 ` David Laight
@ 2022-06-14 13:25 ` 'Sebastian Andrzej Siewior'
2022-06-14 13:34 ` David Laight
0 siblings, 1 reply; 37+ messages in thread
From: 'Sebastian Andrzej Siewior' @ 2022-06-14 13:25 UTC (permalink / raw)
To: David Laight
Cc: Davidlohr Bueso, Tyrel Datwyler, ejb, linux-scsi,
martin.petersen, tglx, linuxppc-dev
On 2022-06-09 15:46:04 [+0000], David Laight wrote:
> From: Sebastian Andrzej Siewior
> > Sent: 09 June 2022 16:03
> >
> > On 2022-05-30 16:15:11 [-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.
> > >
> > > Process srps asynchronously in process context in a dedicated
> > > single threaded workqueue.
> >
> > I would suggest threaded interrupts instead. The pattern here is the
> > same as in the previous driver except here is less locking.
>
> How long do these actions runs for, and what is waiting for
> them to finish?
That is something that one with hardware and workload can answer.
> These changes seem to drop the priority from above that of the
> highest priority RT process down to that of a default priority
> user process.
> There is no real guarantee that the latter will run 'any time soon'.
Not sure I can follow. Using threaded interrupts will run at FIFO-50 by
default. Workqueue however is SCHED_OTHER. But then it is not bound to
any CPU so it will run on an available CPU.
> Consider some workloads I'm setting up where most of the cpu are
> likely to spend 90%+ of the time running processes under the RT
> scheduler that are processing audio.
>
> It is quite likely that a non-RT thread (especially one bound
> to a specific cpu) won't run for several milliseconds.
> (We have to go through 'hoops' to avoid dropping ethernet frames.)
>
> I'd have thought that some of these kernel threads really
> need to run at a 'middling' RT priority.
The threaded interrupts do this by default. If you run your own RT
threads you need to decide if they are more or less important than the
interrupts.
> David
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH 09/10] scsi/ibmvscsi: Replace srp tasklet with work
2022-06-14 13:25 ` 'Sebastian Andrzej Siewior'
@ 2022-06-14 13:34 ` David Laight
0 siblings, 0 replies; 37+ messages in thread
From: David Laight @ 2022-06-14 13:34 UTC (permalink / raw)
To: 'Sebastian Andrzej Siewior'
Cc: Davidlohr Bueso, Tyrel Datwyler, ejb, linux-scsi,
martin.petersen, tglx, linuxppc-dev
From: Sebastian Andrzej Siewior
> Sent: 14 June 2022 14:26
...
> > These changes seem to drop the priority from above that of the
> > highest priority RT process down to that of a default priority
> > user process.
> > There is no real guarantee that the latter will run 'any time soon'.
>
> Not sure I can follow. Using threaded interrupts will run at FIFO-50 by
> default. Workqueue however is SCHED_OTHER. But then it is not bound to
> any CPU so it will run on an available CPU.
Ok, I'd only looked at normal workqueues, softints and napi.
They are all SCHED_OTHER.
Unbound FIFO is moderately ok - they are sticky but can move.
The only problem is that they won't move if a process is
spinning in kernel on the cpu they last run on.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 10/10] scsi/lpfc: Remove bogus references to discovery tasklet
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (8 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 09/10] scsi/ibmvscsi: Replace srp " Davidlohr Bueso
@ 2022-05-30 23:15 ` Davidlohr Bueso
2022-06-09 15:21 ` Sebastian Andrzej Siewior
2022-06-02 7:57 ` [PATCH 00/10] scsi: Replace tasklets as BH Sebastian Andrzej Siewior
10 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-05-30 23:15 UTC (permalink / raw)
To: linux-scsi
Cc: martin.petersen, ejb, bigeasy, tglx, dave, James Smart, Dick Kennedy
This is done as thread. Also remove an unused member of the
lpfc_vport structure.
Cc: James Smart <james.smart@broadcom.com>
Cc: Dick Kennedy <dick.kennedy@broadcom.com>
Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
---
drivers/scsi/lpfc/lpfc.h | 2 --
drivers/scsi/lpfc/lpfc_disc.h | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc.h b/drivers/scsi/lpfc/lpfc.h
index da9070cdad91..05da8ccb0933 100644
--- a/drivers/scsi/lpfc/lpfc.h
+++ b/drivers/scsi/lpfc/lpfc.h
@@ -645,8 +645,6 @@ struct lpfc_vport {
struct lpfc_name fc_nodename; /* fc nodename */
struct lpfc_name fc_portname; /* fc portname */
- struct lpfc_work_evt disc_timeout_evt;
-
struct timer_list fc_disctmo; /* Discovery rescue timer */
uint8_t fc_ns_retry; /* retries for fabric nameserver */
uint32_t fc_prli_sent; /* cntr for outstanding PRLIs */
diff --git a/drivers/scsi/lpfc/lpfc_disc.h b/drivers/scsi/lpfc/lpfc_disc.h
index 37a4b79010bf..40f458ee1aec 100644
--- a/drivers/scsi/lpfc/lpfc_disc.h
+++ b/drivers/scsi/lpfc/lpfc_disc.h
@@ -44,7 +44,7 @@ enum lpfc_work_type {
LPFC_EVT_RECOVER_PORT
};
-/* structure used to queue event to the discovery tasklet */
+/* structure used to queue event to the discovery thread */
struct lpfc_work_evt {
struct list_head evt_listp;
void *evt_arg1;
--
2.36.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 00/10] scsi: Replace tasklets as BH
2022-05-30 23:15 [PATCH 00/10] scsi: Replace tasklets as BH Davidlohr Bueso
` (9 preceding siblings ...)
2022-05-30 23:15 ` [PATCH 10/10] scsi/lpfc: Remove bogus references to discovery tasklet Davidlohr Bueso
@ 2022-06-02 7:57 ` Sebastian Andrzej Siewior
2022-06-07 15:59 ` Davidlohr Bueso
10 siblings, 1 reply; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-02 7:57 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: linux-scsi, martin.petersen, ejb, tglx
On 2022-05-30 16:15:02 [-0700], Davidlohr Bueso wrote:
> and thus are more important than all tasks on the system. Furthermore
> there are no guarantees it will run in irq context at all if ksoftirq
> kicks in.
They never run in IRQ context, they always run in softirq context. Even
if ksoftirqd kicks in, the callbacks (tasklets) are invoked in softirq
context.
That is sort of preemptible but in tasklet's case it will run/ complete
all callbacks before any kind of preemption can kick in.
So there is the lack of preemption for many/ long running callbacks and
the callbacks have no context/ owner which means there is no way of
preferring one over the other.
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 00/10] scsi: Replace tasklets as BH
2022-06-02 7:57 ` [PATCH 00/10] scsi: Replace tasklets as BH Sebastian Andrzej Siewior
@ 2022-06-07 15:59 ` Davidlohr Bueso
2022-06-07 16:20 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 37+ messages in thread
From: Davidlohr Bueso @ 2022-06-07 15:59 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: linux-scsi, martin.petersen, ejb, tglx
Thanks for the careful reviewing and feedback. I'll address
them in a v2 as soon as I get a chance.
Thanks,
Davidlohr
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 00/10] scsi: Replace tasklets as BH
2022-06-07 15:59 ` Davidlohr Bueso
@ 2022-06-07 16:20 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 37+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-06-07 16:20 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: linux-scsi, martin.petersen, ejb, tglx
On 2022-06-07 08:59:18 [-0700], Davidlohr Bueso wrote:
>
> Thanks for the careful reviewing and feedback. I'll address
> them in a v2 as soon as I get a chance.
I didn't get through the whole series as of now but I guess you can
apply what I said so far also to the remaining part ;)
The series actually reminding me that I have a be2iscsi patch hiding
somewhere asking to get reposted…
> Thanks,
> Davidlohr
Sebastian
^ permalink raw reply [flat|nested] 37+ messages in thread