Re: [PATCH] nvme-pci: add adaptive interrupt polling

[Christoph Hellwig]

On Tue, Aug 18, 2026 at 11:38:46AM +0800, Fengnan Chang wrote:
> +#define NVME_ADAPTIVE_POLL_PERIOD_NS	(10U * NSEC_PER_USEC)
> +#define NVME_ADAPTIVE_EPISODE_CQES	8192U
> +#define NVME_ADAPTIVE_REEVAL_CQES	(64U * NVME_ADAPTIVE_EPISODE_CQES)
> +#define NVME_ADAPTIVE_POLL_RETRIES	2U

It would be good to describe these paramters and how we picked the
constants here.

> +struct nvme_adaptive_poll {
> +	struct hrtimer timer;		/* fires the next poll drain */
> +	struct irq_poll iopoll;		/* softirq context for the drain */
> +	struct nvme_queue *nvmeq;
> +	u64 start_ns;			/* when the current sample/episode started */
> +	u32 retry_completions;		/* completions until retry or IRQ rebaseline */
> +	u32 interval_ns;		/* sampled average gap between completions */
> +	u32 completions;		/* completions seen so far this sample/episode */
> +	int irq;
> +	u8 poll_failures;		/* consecutive rejected polling trials */

Lots of overly long lines.  Just move the comments above the fields.

> -	 /* only used for poll queues: */
> +	struct nvme_adaptive_poll *adaptive;
> +	/* Used for both poll queues and adaptive interrupt polling. */

s/both //

> +static inline unsigned int nvme_poll_cq(struct nvme_queue *nvmeq,
> +					struct io_comp_batch *iob)

Two-tab indents please.  Also for various other spots later on.

>  {
> -	bool found = false;
> +	unsigned int found = 0;
>  
>  	while (nvme_cqe_pending(nvmeq)) {
> -		found = true;
>  		/*
>  		 * load-load control dependency between phase and the rest of
>  		 * the cqe requires a full read memory barrier
> @@ -1620,6 +1649,7 @@ static inline bool nvme_poll_cq(struct nvme_queue *nvmeq,
>  		dma_rmb();
>  		nvme_handle_cqe(nvmeq, iob, nvmeq->cq_head);
>  		nvme_update_cq_head(nvmeq);
> +		found++;
>  	}
>  
>  	if (found)
> @@ -1627,17 +1657,22 @@ static inline bool nvme_poll_cq(struct nvme_queue *nvmeq,
>  	return found;
>  }

And maybe split this into a prep patch?

> +/* Keep the normal completion loop branch-free. */

That is a rather terse comment.  I also don't really see what is
branch free here.

> +static enum hrtimer_restart nvme_adaptive_poll_timer(struct hrtimer *timer)
> +{
> +	struct nvme_adaptive_poll *adaptive = container_of(timer,
> +					struct nvme_adaptive_poll, timer);

container_of statements tend to read a lot nicer like:
(same for the next one)

	struct nvme_adaptive_poll *adaptive =
		container_of(timer, struct nvme_adaptive_poll, timer);


> +
> +static irqreturn_t nvme_irq(int irq, void *data);

Please add the new interrupt handlers below nvme_irq to remove the need
for this forward declaration.

> +	unsigned int completions;
> +	unsigned long flags;
> +	DEFINE_IO_COMP_BATCH(iob);
> +
> +	spin_lock_irqsave(&nvmeq->cq_poll_lock, flags);
> +	if (unlikely(test_bit(NVMEQ_ADAPTIVE_POLLING, &nvmeq->flags))) {
> +		spin_unlock_irqrestore(&nvmeq->cq_poll_lock, flags);
> +		return IRQ_HANDLED;
> +	}
> +	completions = nvme_poll_cq(nvmeq, &iob);
> +	if (completions)
> +		nvme_adaptive_sample(nvmeq, completions);
> +	spin_unlock_irqrestore(&nvmeq->cq_poll_lock, flags);
> +	if (!completions)
> +		return test_and_clear_bit(NVMEQ_ADAPTIVE_STALE_IRQ,
> +					  &nvmeq->flags) ? IRQ_HANDLED : IRQ_NONE;
> +	if (!rq_list_empty(&iob.req_list))
> +		nvme_pci_complete_batch(&iob);
> +	return IRQ_HANDLED;

This reads a bit weird, I'd reflow the end to:

	if (completions) {
		if (!rq_list_empty(&iob.req_list))
			nvme_pci_complete_batch(&iob);
	} else {
		if (!test_and_clear_bit(NVMEQ_ADAPTIVE_STALE_IRQ,
				&nvmeq->flags))
			return IRQ_NONE;
	}
	return IRQ_HANDLED;



> +}
> +
> +static irqreturn_t nvme_irq_adaptive(int irq, void *data)
> +{
> +	struct nvme_queue *nvmeq = data;
> +	irqreturn_t ret;
> +
> +	if (!test_bit(NVMEQ_ADAPTIVE_ENABLED, &nvmeq->flags)) {
> +		ret = nvme_irq(irq, data);
> +		if (ret == IRQ_NONE &&
> +		    test_and_clear_bit(NVMEQ_ADAPTIVE_STALE_IRQ, &nvmeq->flags))
> +			return IRQ_HANDLED;
> +		return ret;
> +	}
> +	return nvme_irq_adaptive_enabled(irq, data);
> +}
> +
> +static irqreturn_t nvme_irq(int irq, void *data)
> +{
> +	struct nvme_queue *nvmeq = data;
> +	DEFINE_IO_COMP_BATCH(iob);
> +
> +	if (nvme_poll_cq(nvmeq, &iob)) {
> +		if (!rq_list_empty(&iob.req_list))
> +			nvme_pci_complete_batch(&iob);
> +		return IRQ_HANDLED;
> +	}
> +	return IRQ_NONE;

A lot of this irq handler code is repetitive.  Could there be a way
to share the code to remove the duplication?