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@xxxxxxxxxxxx>
Sebastian
