Re: [PATCH] nvme-pci: add adaptive interrupt polling
From: changfengnan
Date: Wed Aug 19 2026 - 03:03:20 EST
> From: "Christoph Hellwig"<hch@xxxxxx>
> Date: Wed, Aug 19, 2026, 13:35
> Subject: Re: [PATCH] nvme-pci: add adaptive interrupt polling
> To: "Fengnan Chang"<changfengnan@xxxxxxxxxxxxx>
> Cc: "Keith Busch"<kbusch@xxxxxxxxxx>, "Jens Axboe"<axboe@xxxxxxxxx>, "Christoph Hellwig"<hch@xxxxxx>, "Sagi Grimberg"<sagi@xxxxxxxxxxx>, <linux-nvme@xxxxxxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, <jun1.zeng@xxxxxxxxx>, "Guzebing"<guzebing@xxxxxxxxxxxxx>
> 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.
Most of these are figures based on experience; I'll explain them in the next patch.
>
> > +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.
Sorry, these formatting issues will be fixed in the next version.
>
> > - /* 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?
Get.
>
> > +/* 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.
Get.
>
> > + 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;
Yes, it's better.
>
>
>
> > +}
> > +
> > +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?
Get.
>