Re: [PATCH 03/10] scsi/megaraid_sas: Replace instance->tasklet with threaded irq

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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