Re: [PATCH v1 05/10] mpi3mr: Fix performance regression caused by extended IRQ poll sleep

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing dma_rmb() when reading DMA-mapped reply descriptors, risking stale data processing on weakly ordered architectures.
--

commit f91311d5d4eeee7718829b492ee38d52ed25a32a
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

    mpi3mr: Fix performance regression caused by extended IRQ poll sleep

This commit restores the threaded IRQ poll sleep range from 20-21 us back to
2-20 us. The previously extended interval caused an approximate 7% throughput
regression on NVMe direct-attached RAID10 configurations. This reverts the
sleep behavior to recover the lost throughput.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr.h b/drivers/scsi/mpi3mr/mpi3mr.h
> index 1f2f0951b560e..1d11d7c69536b 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr.h
> +++ b/drivers/scsi/mpi3mr/mpi3mr.h
> @@ -178,7 +178,7 @@ extern atomic64_t event_counter;
>  #define MPI3MR_DEFAULT_SDEV_QD	32
>  
>  /* Definitions for Threaded IRQ poll*/
> -#define MPI3MR_IRQ_POLL_SLEEP			20
> +#define MPI3MR_IRQ_POLL_SLEEP			2
>  #define MPI3MR_IRQ_POLL_TRIGGER_IOCOUNT		8
>  
>  /* Definitions for the controller security status*/
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 496d7ca3ab372..32aeae20481ec 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -744,7 +744,7 @@ static irqreturn_t mpi3mr_isr_poll(int irq, void *privdata)
>  			    mpi3mr_process_op_reply_q(mrioc,
>  				intr_info->op_reply_q);

[Severity: High]
This isn't a bug introduced by this patch, but is a dma_rmb() missing when
reading the DMA-mapped reply descriptors?

Looking at mpi3mr_process_op_reply_q() which is called here, the CPU checks
the phase bit of the DMA-mapped descriptor, and then immediately reads the
payload:

mpi3mr_process_op_reply_q() {
...
	if ((le16_to_cpu(reply_desc->reply_flags) &
	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
		atomic_dec(&op_reply_q->in_use);
		return 0;
	}

	do {
		if (mrioc->unrecoverable || mrioc->io_admin_reset_sync)
			break;

		req_q_idx = le16_to_cpu(reply_desc->request_queue_id) - 1;
...
}

And similarly at the end of the loop:

		if ((le16_to_cpu(reply_desc->reply_flags) &
		    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase)
			break;
...

Without an intervening dma_rmb(), could weakly ordered architectures (like
ARM64) fetch the descriptor payload via out-of-order execution or speculation
before the phase bit check is complete?

If the hardware is still updating it, could this result in processing stale
data such as an incorrect request_queue_id?

It looks like mpi3mr_process_admin_reply_q() might have the same pattern
where it reads request_queue_ci right after checking the phase bit without
a memory barrier.

> -		usleep_range(MPI3MR_IRQ_POLL_SLEEP, MPI3MR_IRQ_POLL_SLEEP + 1);
> +		usleep_range(MPI3MR_IRQ_POLL_SLEEP, 10 * MPI3MR_IRQ_POLL_SLEEP);
>  
>  	} while (atomic_read(&intr_info->op_reply_q->pend_ios) &&
>  	    (num_op_reply < mrioc->max_host_ios));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=5