Re: [PATCH RFC v2 2/2] ufs: core: delegate the interrupt service routine to a threaded irq handler

From: Neil Armstrong
Date: Thu Mar 27 2025 - 08:49:49 EST


Hi,

On 27/03/2025 12:56, Bart Van Assche wrote:
On 3/26/25 4:36 AM, Neil Armstrong wrote:
> When MCQ & Interrupt Aggregation are supported, the interrupt
> are directly handled in the "hard" interrupt routine to
> keep IOPs high since queues handling is done in separate
> per-queue interrupt routines.

The above explanation suggests that I/O completions are handled by the
modified interrupt handler. This is not necessarily the case. With MCQ,
I/O completions are either handled by dedicated interrupts or by the
legacy interrupt handler.

Will update the sentence with that


Reported bandwidth is not affected on various tests.

This kind of patch can only affect command completion latency but not
the bandwidth, isn't it?

Yes, but on a fully loaded system, it will enhance bandwidth
but with a greater latency, but without eating irq handling time
for other routines.


+/**
+ * ufshcd_intr - Main interrupt service routine
+ * @irq: irq number
+ * @__hba: pointer to adapter instance
+ *
+ * Return:
+ *  IRQ_HANDLED     - If interrupt is valid
+ *  IRQ_WAKE_THREAD - If handling is moved to threaded handled
+ *  IRQ_NONE        - If invalid interrupt
+ */
+static irqreturn_t ufshcd_intr(int irq, void *__hba)
+{
+    struct ufs_hba *hba = __hba;
+
+    /*
+     * Move interrupt handling to thread when MCQ is not supported
+     * or when Interrupt Aggregation is not supported, leading to
+     * potentially longer interrupt handling.
+     */
+    if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba))
+        return IRQ_WAKE_THREAD;
+
+    /* Directly handle interrupts since MCQ handlers does the hard job */
+    return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) &
+                   ufshcd_readl(hba, REG_INTERRUPT_ENABLE));
+}

Where has ufshcd_is_intr_aggr_allowed() been defined? I can't find this
function.

It's in include/ufs/ufshcd.h


For the MCQ case, this patch removes the loop from around
ufshcd_sl_intr() without explaining in the patch description why this change has been made. Please explain all changes in the patch
description.

Ack will update explaining this change.

Thanks,
Neil


Thanks,

Bart.