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

> From: "Christoph Hellwig"<[email protected]>
> Date:  Wed, Aug 19, 2026, 13:35
> Subject:  Re: [PATCH] nvme-pci: add adaptive interrupt polling
> To: "Fengnan Chang"<[email protected]>
> Cc: "Keith Busch"<[email protected]>, "Jens Axboe"<[email protected]>, "Christoph Hellwig"<[email protected]>, "Sagi Grimberg"<[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, "Guzebing"<[email protected]>
> 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.

>