| Message ID | 20211017020623.77815-7-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] block: add a struct io_comp_batch argument to fops->iopoll() | expand |
On Sat, Oct 16, 2021 at 08:06:23PM -0600, Jens Axboe wrote: > static inline int nvme_process_cq(struct nvme_queue *nvmeq) > { > - return nvme_poll_cq(nvmeq, NULL); > + DEFINE_IO_COMP_BATCH(iob); > + int found; > + > + found = nvme_poll_cq(nvmeq, &iob); > + if (iob.req_list) > + nvme_pci_complete_batch(&iob); > + return found; Ok, here the splitt makes sense. That being said I'd rather only add what is nvme_poll_cq as a separate function here, and I'd probably name it __nvme_process_cq as the poll name could create some confusion.
On 10/18/21 4:22 AM, Christoph Hellwig wrote: > On Sat, Oct 16, 2021 at 08:06:23PM -0600, Jens Axboe wrote: >> static inline int nvme_process_cq(struct nvme_queue *nvmeq) >> { >> - return nvme_poll_cq(nvmeq, NULL); >> + DEFINE_IO_COMP_BATCH(iob); >> + int found; >> + >> + found = nvme_poll_cq(nvmeq, &iob); >> + if (iob.req_list) >> + nvme_pci_complete_batch(&iob); >> + return found; > > Ok, here the splitt makes sense. That being said I'd rather only add > what is nvme_poll_cq as a separate function here, and I'd probably > name it __nvme_process_cq as the poll name could create some confusion. Sure, I can shuffle that around. Can I add your reviewed/acked by with that for those two, or do you want the series resent?
On Mon, Oct 18, 2021 at 07:41:30AM -0600, Jens Axboe wrote: > > Ok, here the splitt makes sense. That being said I'd rather only add > > what is nvme_poll_cq as a separate function here, and I'd probably > > name it __nvme_process_cq as the poll name could create some confusion. > > Sure, I can shuffle that around. Can I add your reviewed/acked by with > that for those two, or do you want the series resent? I'm fine with that change included: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 10/18/21 9:14 AM, Christoph Hellwig wrote: > On Mon, Oct 18, 2021 at 07:41:30AM -0600, Jens Axboe wrote: >>> Ok, here the splitt makes sense. That being said I'd rather only add >>> what is nvme_poll_cq as a separate function here, and I'd probably >>> name it __nvme_process_cq as the poll name could create some confusion. >> >> Sure, I can shuffle that around. Can I add your reviewed/acked by with >> that for those two, or do you want the series resent? > > I'm fine with that change included: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Great, thanks.
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index e916d5e167c1..fdb0716614c9 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1075,7 +1075,13 @@ static inline int nvme_poll_cq(struct nvme_queue *nvmeq, static inline int nvme_process_cq(struct nvme_queue *nvmeq) { - return nvme_poll_cq(nvmeq, NULL); + DEFINE_IO_COMP_BATCH(iob); + int found; + + found = nvme_poll_cq(nvmeq, &iob); + if (iob.req_list) + nvme_pci_complete_batch(&iob); + return found; } static irqreturn_t nvme_irq(int irq, void *data)
Trivial to do now, just need our own io_comp_batch on the stack and pass that in to the usual command completion handling. I pondered making this dependent on how many entries we had to process, but even for a single entry there's no discernable difference in performance or latency. Running a sync workload over io_uring: t/io_uring -b512 -d1 -s1 -c1 -p0 -F1 -B1 -n2 /dev/nvme1n1 /dev/nvme2n1 yields the below performance before the patch: IOPS=254820, BW=124MiB/s, IOS/call=1/1, inflight=(1 1) IOPS=251174, BW=122MiB/s, IOS/call=1/1, inflight=(1 1) IOPS=250806, BW=122MiB/s, IOS/call=1/1, inflight=(1 1) and the following after: IOPS=255972, BW=124MiB/s, IOS/call=1/1, inflight=(1 1) IOPS=251920, BW=123MiB/s, IOS/call=1/1, inflight=(1 1) IOPS=251794, BW=122MiB/s, IOS/call=1/1, inflight=(1 1) which definitely isn't slower, about the same if you factor in a bit of variance. For peak performance workloads, benchmarking shows a 2% improvement. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- drivers/nvme/host/pci.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)