From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from verein.lst.de (verein.lst.de [213.95.11.211]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 467903CB2E9 for ; Wed, 19 Aug 2026 05:35:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.95.11.211 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787117748; cv=none; b=luEOZZxrL4ztfWeBKm6iaBvlb/0J1+ZiYo8ZbN9UI0xsjoEkk5UO1U7ks62bvEnSf+ipcff+gTh6vBI29jIbXlC9ccf5EmFWJiMUKpAuXqzjJxlxZIqiGIBHPm2iMZdC6FI8J5OMljuMyWp7GAKCYHH5CcwgMTyHiDLdUmzKcQY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787117748; c=relaxed/simple; bh=gdbl1oYveWV9tjyrqYaKSlZOwt6fDvjObVxNKiLkws0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ii/OKlPUj9NNLpa0gTujAMJp3B8p0Rz29ufKBRUAhLDcAqQa9swQaH5eawCETZJVgJlwmEpNCzts8oaI4vCqiJQ/TP7/pag/JKu31wLbrTGlVWJ3qj2lyPeWgY4eSQSYwpe/+KShBhw6oUqfWD1Ch1G4Ih84O7kwN+FlCZ65lD4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lst.de; spf=pass smtp.mailfrom=lst.de; arc=none smtp.client-ip=213.95.11.211 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lst.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lst.de Received: by verein.lst.de (Postfix, from userid 2407) id 5052968C7B; Wed, 19 Aug 2026 07:35:41 +0200 (CEST) Date: Wed, 19 Aug 2026 07:35:41 +0200 From: Christoph Hellwig To: Fengnan Chang Cc: Keith Busch , Jens Axboe , Christoph Hellwig , Sagi Grimberg , linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org, jun1.zeng@intel.com, Guzebing Subject: Re: [PATCH] nvme-pci: add adaptive interrupt polling Message-ID: <20260819053541.GC32364@lst.de> References: <20260818033846.53790-1-changfengnan@bytedance.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260818033846.53790-1-changfengnan@bytedance.com> User-Agent: Mutt/1.5.17 (2007-11-01) 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?