| Message ID | 20220530231512.9729-2-dave@stgolabs.net |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: Replace tasklets as BH | expand |
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 {
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
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
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
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
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
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
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
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 {
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(-)