Performance enhancements
Subsystems (3)
cover Performance enhancements
A few IO micro-optimizations for IO polling and NVMe. I'm really working to close the performance gap with userspace drivers, and this gets me halfway there on latency. The fastest hardware I could get measured roundtrip read latency at 5usec with this series that was previously measuring 5.7usec.
Note with NVMe, you really need to crank up the interrupt coalescing to see the completion polling benefit.
Test pre-setup:
echo performance | tee /sys/devices/system/cpu/cpufreq/policy*/scaling_governor echo 0 > /sys/block/nvme0n1/queue/iostats
echo -1 > /sys/block/nvme0n1/queue/io_poll_delay
nvme set-feature /dev/nvme0 -f 8 -v 0x4ff
fio profile:
[global]
ioengine=pvsync2
rw=randread
norandommap
direct=1
bs=4k
hipri
[hi-pri]
filename=/dev/nvme0n1
cpus_allowed=2
Keith Busch (3):
nvme/pci: Start request after doorbell ring nvme/pci: Remove cq_vector check in IO path
block: Polling completion performance optimization
drivers/nvme/host/pci.c | 14 +++----------- fs/block_dev.c | 5 ++++- 2 files changed, 7 insertions(+), 12 deletions(-)
Signature · 1 line
2.13.6
PATCH 1/3 nvme/pci: Start request after doorbell ring
This is a performance optimization that allows the hardware to work on a command in parallel with the kernel's stats and timeout tracking.
--- drivers/nvme/host/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/nvme/host/pci.c +1 -2
@@ -886,8 +886,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, goto out_cleanup_iod; } -blk_mq_start_request (req); -spin_lock_irq (&nvmeq->q_lock);if (unlikely (nvmeq->cq_vector <0 )) { ret =BLK_STS_IOERR ;
@@ -895,6 +893,7 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, goto out_cleanup_iod; }__nvme_submit_cmd (nvmeq, &cmnd); +blk_mq_start_request (req);nvme_process_cq (nvmeq);spin_unlock_irq (&nvmeq->q_lock);return BLK_STS_OK ;
Signature · 1 line
2.13.6
Jens Axboe
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 22 lines · Keith Busch
This is a performance optimization that allows the hardware to work on a command in parallel with the kernel's stats and timeout tracking.
Signed-off-by Keith Busch <[email protected]>--- drivers/nvme/host/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/nvme/host/pci.c +0
-2@@ -886,8 +886,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, goto out_cleanup_iod; } -blk_mq_start_request (req); -spin_lock_irq (&nvmeq->q_lock);if (unlikely (nvmeq->cq_vector <0 )) { ret =BLK_STS_IOERR ;
drivers/nvme/host/pci.c static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 899
Quoted 7 lines
drivers/nvme/host/pci.c +1
-0@@ -895,6 +893,7 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, goto out_cleanup_iod; }__nvme_submit_cmd (nvmeq, &cmnd); +blk_mq_start_request (req);nvme_process_cq (nvmeq);spin_unlock_irq (&nvmeq->q_lock);return BLK_STS_OK ;
I guess this will work since we hold the q_lock, but you should probably include a comment to that effect since it's important that we can't find the completion before we've called started.
Actually, you'd need to reorder this after #2 (I'm assuming, it hasn't shown up yet, but I'm guessing it's kill the cq check after submission) since if we have multiple CPUs on the same hardware queue, one of the other CPUs could find a completion event between your __nvme_submit_cmd() and blk_mq_start_request() call. Very short window, but it does exist.
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 35 lines · Jens Axboe
Quoted 22 lines · Keith Busch
This is a performance optimization that allows the hardware to work on a command in parallel with the kernel's stats and timeout tracking.
Signed-off-by Keith Busch <[email protected]>--- drivers/nvme/host/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/nvme/host/pci.c +0
-2@@ -886,8 +886,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, goto out_cleanup_iod; } -blk_mq_start_request (req); -spin_lock_irq (&nvmeq->q_lock);if (unlikely (nvmeq->cq_vector <0 )) { ret =BLK_STS_IOERR ;drivers/nvme/host/pci.c static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 899
Quoted 7 lines
drivers/nvme/host/pci.c +1
-0@@ -895,6 +893,7 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, goto out_cleanup_iod; }__nvme_submit_cmd (nvmeq, &cmnd); +blk_mq_start_request (req);nvme_process_cq (nvmeq);spin_unlock_irq (&nvmeq->q_lock);return BLK_STS_OK ;I guess this will work since we hold the
q_lock, but you should probably include a comment to that effect since it's important that we can't find the completion before we've called started.
Quoted 6 lines
Actually, you'd need to reorder this after #2 (I'm assuming, it hasn't shown up yet, but I'm guessing it's kill the cq check after submission) since if we have multiple CPUs on the same hardware queue, one of the other CPUs could find a completion event between your
__nvme_submit_cmd()andblk_mq_start_request()call. Very short window, but it does exist.
Turns out that wasn't what patch 2 was. And the code is right there above as well, and under the q_lock, so I guess that race doesn't exist.
But that does bring up the fact if we should always be doing the nvme_process_cq(nvmeq) after IO submission. For direct/hipri IO, maybe it's better to make the submission path faster and skip it?
Signature · 1 line
Jens Axboe
Re: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 7 lines · Jens Axboe
Turns out that wasn't what patch 2 was. And the code is right there above as well, and under the
q_lock, so I guess that race doesn't exist.But that does bring up the fact if we should always be doing the
nvme_process_cq(nvmeq)after IO submission. For direct/hipri IO, maybe it's better to make the submission path faster and skip it?
Yes, I am okay to remove the opprotunistic nvme_process_cq in the submission path. Even under deeply queued IO, I've not seen this provide any measurable benefit.
Jens Axboe
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 5 lines · Keith Busch
Quoted 4 lines · Jens Axboe
Turns out that wasn't what patch 2 was. And the code is right there above as well, and under the
q_lock, so I guess that race doesn't exist.
Quoted 7 lines
Quoted 3 lines
But that does bring up the fact if we should always be doing the
nvme_process_cq(nvmeq)after IO submission. For direct/hipri IO, maybe it's better to make the submission path faster and skip it?Yes, I am okay to remove the opprotunistic
nvme_process_cqin the submission path. Even under deeply queued IO, I've not seen this provide any measurable benefit.
I haven't either, but curious if others had. It's mostly just extra overhead, I haven't seen it provide a latency reduction of any kind.
Signature · 1 line
Jens Axboe
Re: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 2 lines · Jens Axboe
I haven't either, but curious if others had. It's mostly just extra overhead, I haven't seen it provide a latency reduction of any kind.
I've removed the submission side poll in a local build, and amazingly I am observing a not insignificant increase in latency without it on some workloads with certain hardware. I will have to delay recommending/posting removal of this pending further investigation.
Re: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 5 lines · Keith Busch
I've removed the submission side poll in a local build, and amazingly I am observing a not insignificant increase in latency without it on some workloads with certain hardware. I will have to delay recommending/posting removal of this pending further investigation.
This took longer to narrow than I hoped. I had been getting very wild results, and there were two things to blame: a kernel irq issue[*], and a platform bios causing a lot of CPU thermal frequency throttling.
Now that's sorted out, I am back to tuning this and the opprotunistic nvme submission side polling once again does not produce a measurable difference. My world is sane again. Will try to send an update for consideration this week after some more testing.
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=a0c9259dc4
Sagi Grimberg
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
In reply to Keith BuschQuoted 8 lines
Quoted 4 lines
But that does bring up the fact if we should always be doing the
nvme_process_cq(nvmeq)after IO submission. For direct/hipri IO, maybe it's better to make the submission path faster and skip it?Yes, I am okay to remove the opprotunistic
nvme_process_cqin the submission path. Even under deeply queued IO, I've not seen this provide any measurable benefit.
+100 for removing it. Never really understood why it gets us anything...
Christoph Hellwig
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 10 lines · Sagi Grimberg
Quoted 8 lines
Quoted 4 lines
But that does bring up the fact if we should always be doing the
nvme_process_cq(nvmeq)after IO submission. For direct/hipri IO, maybe it's better to make the submission path faster and skip it?Yes, I am okay to remove the opprotunistic
nvme_process_cqin the submission path. Even under deeply queued IO, I've not seen this provide any measurable benefit.+100 for removing it. Never really understood why it gets us anything...
Yes, we should get rid of it.
Sagi Grimberg
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
In reply to Keith BuschQuoted 3 lines
This is a performance optimization that allows the hardware to work on a command in parallel with the kernel's stats and timeout tracking.
Curious to know what does this buy us?
Re: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 4 lines · Sagi Grimberg
Quoted 2 lines
This is a performance optimization that allows the hardware to work on a command in parallel with the kernel's stats and timeout tracking.
Curious to know what does this buy us?
blk_mq_start_request doesn't do anything to make the command ready to submit to hardware, so this is pure software overhead, somewhere between 200-300 nanoseconds as far as I can measure (YMMV). We can post the command to hardware before taking on this software overhead so the hardware gets to fetch the command that much sooner.
One thing to remember is we need to ensure the driver can't complete the request before starting it. The driver's exisitng locking does ensure that with this patch, so it's just something to keep in mind if anything should ever change in the future.
Sagi Grimberg
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
Quoted 9 lines
Quoted 2 lines
Curious to know what does this buy us?
blk_mq_start_requestdoesn't do anything to make the command ready to submit to hardware, so this is pure software overhead, somewhere between 200-300 nanoseconds as far as I can measure (YMMV). We can post the command to hardware before taking on this software overhead so the hardware gets to fetch the command that much sooner.
Quoted 4 lines
One thing to remember is we need to ensure the driver can't complete the request before starting it. The driver's exisitng locking does ensure that with this patch, so it's just something to keep in mind if anything should ever change in the future.
Needs to be well documented.
This mean we won't be able to split the submission and completion locks now... So it does present a tradeoff.
Christoph Hellwig
maintainerRe: [PATCH 1/3] nvme/pci: Start request after doorbell ring
In reply to Keith BuschLooks fine, although I agree a comment on the q_lock exclusion would be useful.
PATCH 2/3 nvme/pci: Remove cq_vector check in IO path
In reply to Keith BuschThis is a micro-optimization removing unnecessary check for a disabled queue. We no longer need this check because blk-mq provides the ability to quiesce queues that nvme uses, and the doorbell registers are never unmapped as long as requests are active.
--- drivers/nvme/host/pci.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/nvme/host/pci.c +2 -9
@@ -887,11 +887,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, }spin_lock_irq (&nvmeq->q_lock); -if (unlikely (nvmeq->cq_vector <0 )) { - ret =BLK_STS_IOERR ; -spin_unlock_irq (&nvmeq->q_lock); - goto out_cleanup_iod; - }__nvme_submit_cmd (nvmeq, &cmnd);blk_mq_start_request (req);nvme_process_cq (nvmeq);
@@ -923,11 +918,9 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq) {u16 head = nvmeq->cq_head; -if (likely (nvmeq->cq_vector >=0 )) { -if (nvme_dbbuf_update_and_check_event (head, nvmeq->dbbuf_cq_db, +if (nvme_dbbuf_update_and_check_event (head, nvmeq->dbbuf_cq_db, nvmeq->dbbuf_cq_ei)) -writel (head, nvmeq->q_db + nvmeq->dev->db_stride); - } +writel (head, nvmeq->q_db + nvmeq->dev->db_stride); } static inline void nvme_handle_cqe(struct nvme_queue *nvmeq,
Signature · 1 line
2.13.6
Re: [PATCH 2/3] nvme/pci: Remove cq_vector check in IO path
Quoted 4 lines · Keith Busch
This is a micro-optimization removing unnecessary check for a disabled queue. We no longer need this check because blk-mq provides the ability to quiesce queues that nvme uses, and the doorbell registers are never unmapped as long as requests are active.
Signature · 1 line
Jens Axboe
Re: [PATCH 2/3] nvme/pci: Remove cq_vector check in IO path
In reply to Keith BuschAwesome!
Sagi Grimberg
maintainerRe: [PATCH 2/3] nvme/pci: Remove cq_vector check in IO path
Quoted 4 lines
Awesome!
Reviewed-by Sagi Grimberg <[email protected]>
Wait, aren't we unquiesce queues also in nvme_dev_disable?
Doesn't that rely that the queues are suspended and queue_rq will fail there?
Christoph Hellwig
maintainerRe: [PATCH 2/3] nvme/pci: Remove cq_vector check in IO path
Quoted 9 lines · Sagi Grimberg
Quoted 4 lines
Awesome!
Reviewed-by Sagi Grimberg <sagi at grimberg.me>Wait, aren't we unquiesce queues also in
nvme_dev_disable?Doesn't that rely that the queues are suspended and
queue_rqwill fail there?
We don't seem to have any other check as far as I can tell.
Re: [PATCH 2/3] nvme/pci: Remove cq_vector check in IO path
Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Sagi Grimberg
Quoted 3 lines
Awesome!
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Reviewed-by Sagi Grimberg <[email protected]>Wait, aren't we unquiesce queues also in
nvme_dev_disable?Doesn't that rely that the queues are suspended and
queue_rqwill fail there?We don't seem to have any other check as far as I can tell.
Ok, we currently do need this check in the submission side to flush entered requests on invalidated hw contexts. We didn't have a way to back out entered requests before, so that's why this request killing check still exists. We can steal bio's to back out requests now, so I think we should do that instead of failing them.
I'll do rework the series a bit to make that possible, plus add the removal of the submission side nvme_process_cq, and resend.
Sagi Grimberg
maintainerRe: [PATCH 2/3] nvme/pci: Remove cq_vector check in IO path
Quoted 12 lines
Quoted 11 lines
Quoted 9 lines
Quoted 4 lines
Awesome!
Reviewed-by Sagi Grimberg <[email protected]>Wait, aren't we unquiesce queues also in
nvme_dev_disable?Doesn't that rely that the queues are suspended and
queue_rqwill fail there?We don't seem to have any other check as far as I can tell.
Quoted 8 lines
Ok, we currently do need this check in the submission side to flush entered requests on invalidated hw contexts. We didn't have a way to back out entered requests before, so that's why this request killing check still exists. We can steal bio's to back out requests now, so I think we should do that instead of failing them.
I'll do rework the series a bit to make that possible, plus add the removal of the submission side
nvme_process_cq, and resend.
Not sure if stealing bios from requests is a better design. Note that we do exactly this in other transport (nvme_[rdma|loop|fc]_is_ready).
I think it would be better to stick to a coherent behavior across the nvme subsystem. If this condition statement is really something that is buying us measurable performance gain, I think we should apply it for other transports as well (although in fabrics we're a bit different because we have a dedicated connect that enters .queue_rq)
Re: [PATCH 2/3] nvme/pci: Remove cq_vector check in IO path
Quoted 2 lines · Sagi Grimberg
Not sure if stealing bios from requests is a better design. Note that we do exactly this in other transport (nvme_[rdma|loop|fc]_is_ready).
Well, we're currently failing requests that may succeed if we could back them out for re-entry. While such scenarios are uncommon, I think we can handle it better than ending them in failure.
Quoted 5 lines
I think it would be better to stick to a coherent behavior across the nvme subsystem. If this condition statement is really something that is buying us measurable performance gain, I think we should apply it for other transports as well (although in fabrics we're a bit different because we have a dedicated connect that enters .queue_rq)
We should be able to remove all the state checks in the IO path because the handlers putting the controller in an IO-incapable state should be able to quiece the queues before transitioning to that state.
This is not a significant gain compared to the other two patches in this series, but the sum of all the little things become meaningful.
We'd need to remove this check anyway if we're considering exclusively polled queues since they wouldn't have a CQ vector.
PATCH 3/3 block: Polling completion performance optimization
In reply to Keith BuschWhen a request completion is polled, the completion task wakes itself up. This is unnecessary, as the task can just set itself back to running.
--- fs/block_dev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
fs/block_dev.c +4 -1
@@ -181,7 +181,10 @@ static void blkdev_bio_end_io_simple(struct bio *bio)struct task_struct *waiter = bio->bi_private;WRITE_ONCE (bio->bi_private,NULL ); -wake_up_process (waiter); +if (current != waiter) +wake_up_process (waiter); +else +__set_current_state (TASK_RUNNING ); } static ssize_t
Signature · 1 line
2.13.6
Re: [PATCH 3/3] block: Polling completion performance optimization
Quoted 14 lines · Keith Busch
When a request completion is polled, the completion task wakes itself up. This is unnecessary, as the task can just set itself back to running.
Signed-off-by Keith Busch <[email protected]>--- fs/block_dev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)fs/block_dev.c +0
-0
fs/block_dev.c static void blkdev_bio_end_io_simple(struct bio *bio) · line 187
Quoted 8 lines
fs/block_dev.c +4
-1@@ -181,7 +181,10 @@ static void blkdev_bio_end_io_simple(struct bio *bio)struct task_struct *waiter = bio->bi_private;WRITE_ONCE (bio->bi_private,NULL ); -wake_up_process (waiter); +if (current != waiter) +wake_up_process (waiter); +else +__set_current_state (TASK_RUNNING );
Do we actually need to set this to TASK_RUNNING? If we get here we're already running, right?
Everywhere I see uses of __set_current_state(TASK_RUNNING) it's after we've done a set_current_state(TASK_INTERRUPTIBLE).
Jens Axboe
maintainerRe: [PATCH 3/3] block: Polling completion performance optimization
Quoted 28 lines · Scott Bauer
Quoted 14 lines · Keith Busch
When a request completion is polled, the completion task wakes itself up. This is unnecessary, as the task can just set itself back to running.
Signed-off-by Keith Busch <[email protected]>--- fs/block_dev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)fs/block_dev.c +0
-0fs/block_dev.c static void blkdev_bio_end_io_simple(struct bio *bio) · line 187
Quoted 8 lines
fs/block_dev.c +4
-1@@ -181,7 +181,10 @@ static void blkdev_bio_end_io_simple(struct bio *bio)struct task_struct *waiter = bio->bi_private;WRITE_ONCE (bio->bi_private,NULL ); -wake_up_process (waiter); +if (current != waiter) +wake_up_process (waiter); +else +__set_current_state (TASK_RUNNING );Do we actually need to set this to
TASK_RUNNING? If we get here we're already running, right?
Quoted 1 line
Everywhere I see uses of
__set_current_state(TASK_RUNNING)it's after we've done aset_current_state(TASK_INTERRUPTIBLE).
We'd only be TASK_RUNNING if the IRQ got to it first. And that's something that should be removed as well - I suspect that'd be a much bigger win, getting rid of the IRQ trigger for polled IO, than most of the micro optimizations. For Keith's testing, looks like he reduced the cost by turning on coalescing, but it'd be cheaper (and better) to not have to rely on that.
In any case, the __set_current_state() is very cheap. Given that and the above, I'd rather keep that.
Signature · 1 line
Jens Axboe
Re: [PATCH 3/3] block: Polling completion performance optimization
Quoted 14 lines · Jens Axboe
Quoted 13 lines · Scott Bauer
fs/block_dev.c static void blkdev_bio_end_io_simple(struct bio *bio) · line 187
Quoted 9 lines · Keith Busch
fs/block_dev.c +4
-1@@ -181,7 +181,10 @@ static void blkdev_bio_end_io_simple(struct bio *bio)struct task_struct *waiter = bio->bi_private;WRITE_ONCE (bio->bi_private,NULL ); -wake_up_process (waiter); +if (current != waiter) +wake_up_process (waiter); +else +__set_current_state (TASK_RUNNING );Do we actually need to set this to
TASK_RUNNING? If we get here we're already running, right?
Quoted 7 lines
Quoted 1 line
Everywhere I see uses of
__set_current_state(TASK_RUNNING)it's after we've done aset_current_state(TASK_INTERRUPTIBLE).We'd only be
TASK_RUNNINGif the IRQ got to it first. And that's something that should be removed as well - I suspect that'd be a much bigger win, getting rid of the IRQ trigger for polled IO, than most of the micro optimizations. For Keith's testing, looks like he reduced the cost by turning on coalescing, but it'd be cheaper (and better) to not have to rely on that.
It would be nice, but the driver doesn't know a request's completion is going to be a polled. Even if it did, we don't have a spec defined way to tell the controller not to send an interrupt with this command's compeletion, which would be negated anyway if any interrupt driven IO is mixed in the same queue. We could possibly create a special queue with interrupts disabled for this purpose if we can pass the HIPRI hint through the request.
Jens Axboe
maintainerRe: [PATCH 3/3] block: Polling completion performance optimization
Quoted 17 lines · Keith Busch
Quoted 16 lines · Jens Axboe
Quoted 14 lines · Scott Bauer
fs/block_dev.c static void blkdev_bio_end_io_simple(struct bio *bio) · line 187
Quoted 9 lines · Keith Busch
fs/block_dev.c +4
-1@@ -181,7 +181,10 @@ static void blkdev_bio_end_io_simple(struct bio *bio)struct task_struct *waiter = bio->bi_private;WRITE_ONCE (bio->bi_private,NULL ); -wake_up_process (waiter); +if (current != waiter) +wake_up_process (waiter); +else +__set_current_state (TASK_RUNNING );Do we actually need to set this to
TASK_RUNNING? If we get here we're already running, right?Everywhere I see uses of
__set_current_state(TASK_RUNNING)it's after we've done aset_current_state(TASK_INTERRUPTIBLE).
Quoted 8 lines
Quoted 5 lines
We'd only be
TASK_RUNNINGif the IRQ got to it first. And that's something that should be removed as well - I suspect that'd be a much bigger win, getting rid of the IRQ trigger for polled IO, than most of the micro optimizations. For Keith's testing, looks like he reduced the cost by turning on coalescing, but it'd be cheaper (and better) to not have to rely on that.It would be nice, but the driver doesn't know a request's completion is going to be a polled.
That's trivially solvable though, since the information is available at submission time.
Quoted 6 lines
Even if it did, we don't have a spec defined
way to tell the controller not to send an interrupt with this command's compeletion, which would be negated anyway if any interrupt driven IO is mixed in the same queue. We could possibly create a special queue with interrupts disabled for this purpose if we can pass the HIPRI hint through the request.
There's on way to do it per IO, right. But you can create a sq/cq pair without interrupts enabled. This would also allow you to scale better with multiple users of polling, a case where we currently don't perform as well spdk, for instance.
Signature · 1 line
Jens Axboe
Re: [PATCH 3/3] block: Polling completion performance optimization
Quoted 14 lines · Jens Axboe
Quoted 2 lines · Keith Busch
It would be nice, but the driver doesn't know a request's completion is going to be a polled.
That's trivially solvable though, since the information is available at submission time.
Quoted 6 lines
Even if it did, we don't have a spec defined
way to tell the controller not to send an interrupt with this command's compeletion, which would be negated anyway if any interrupt driven IO is mixed in the same queue. We could possibly create a special queue with interrupts disabled for this purpose if we can pass the HIPRI hint through the request.
Quoted 4 lines
There's on way to do it per IO, right. But you can create a sq/cq pair without interrupts enabled. This would also allow you to scale better with multiple users of polling, a case where we currently don't perform as well spdk, for instance.
Would you be open to have blk-mq provide special hi-pri hardware contexts for all these requests to come through? Maybe one per NUMA node? If not, I don't think have enough unused bits in the NVMe command id to stash the hctx id to extract the original request.
Jens Axboe
maintainerRe: [PATCH 3/3] block: Polling completion performance optimization
Quoted 20 lines · Keith Busch
Quoted 14 lines · Jens Axboe
Quoted 2 lines · Keith Busch
It would be nice, but the driver doesn't know a request's completion is going to be a polled.
That's trivially solvable though, since the information is available at submission time.
Quoted 6 lines
Even if it did, we don't have a spec defined
way to tell the controller not to send an interrupt with this command's compeletion, which would be negated anyway if any interrupt driven IO is mixed in the same queue. We could possibly create a special queue with interrupts disabled for this purpose if we can pass the HIPRI hint through the request.Quoted 4 lines
There's on way to do it per IO, right. But you can create a sq/cq pair without interrupts enabled. This would also allow you to scale better with multiple users of polling, a case where we currently don't perform as well spdk, for instance.
Quoted 4 lines
Would you be open to have blk-mq provide special hi-pri hardware contexts for all these requests to come through? Maybe one per NUMA node? If not, I don't think have enough unused bits in the NVMe command id to stash the hctx id to extract the original request.
Yeah, in fact I think we HAVE to do it this way. I've been thinking about this for a while, and ideally I'd really like blk-mq to support multiple queue "pools". It's basically just a mapping thing. Right now you hand blk-mq all your queues, and the mappings are defined for one set of queues. It'd be nifty to support multiple sets, so we could do things like "reserve X for polling", for example, and just have the mappings magically work. blk_mq_map_queue() then just needs to take the bio or request (or just cmd flags) to be able to decide what set the request belongs to, making the mapping a function of {cpu,type}.
I originally played with this in the context of isolating writes on a single queue, to reduce the amount of resources they can grab. And it'd work nicely on this as well.
Completions could be configurable to where the submitter would do it (like now, best for sync single thread), or to where you have a/more kernel threads doing it (spdk'ish, best for high qd / thread count).
Signature · 1 line
Jens Axboe
Christoph Hellwig
maintainerRe: [PATCH 3/3] block: Polling completion performance optimization
In reply to Keith BuschQuoted 2 lines · Keith Busch
It would be nice, but the driver doesn't know a request's completion is going to be a polled.
We can trivially set a REQ_POLL bit. In fact my initial patch kit had those on the insitence of Jens, but then I removed it because we had no users for it.
Quoted 4 lines
Even if it did, we don't have a spec defined
way to tell the controller not to send an interrupt with this command's compeletion, which would be negated anyway if any interrupt driven IO is mixed in the same queue.
Time to add such a flag to the spec then..
Quoted 3 lines
We could possibly create a special queue
with interrupts disabled for this purpose if we can pass the HIPRI hint through the request.
Eww. That means we double our resource usage both in the host and the device, which isn't going to be pretty.
Re: [PATCH 3/3] block: Polling completion performance optimization
Quoted 6 lines · Christoph Hellwig
Quoted 3 lines
We could possibly create a special queue
with interrupts disabled for this purpose if we can pass the HIPRI hint through the request.Eww. That means we double our resource usage both in the host and the device, which isn't going to be pretty.
Yes, this would create some trouble for our simple policy for divvying queue resources. We wouldn't be able to use the PCI IRQ affinity for these queues CPU map, for example.
Maybe if we can figure out a good policy for WRR queues, that might present a similar infrastructure for supporting polled queues as well.
Sagi Grimberg
maintainerRe: [PATCH 3/3] block: Polling completion performance optimization
In reply to Christoph HellwigQuoted 8 lines · Christoph Hellwig
Quoted 2 lines · Keith Busch
It would be nice, but the driver doesn't know a request's completion is going to be a polled.
We can trivially set a
REQ_POLLbit. In fact my initial patch kit had those on the insitence of Jens, but then I removed it because we had no users for it.
Quoted 6 lines
Quoted 4 lines
Even if it did, we don't have a spec defined
way to tell the controller not to send an interrupt with this command's compeletion, which would be negated anyway if any interrupt driven IO is mixed in the same queue.Time to add such a flag to the spec then..
That would be very useful, ideally we can also hook it into libaio to submit without triggering an interrupt and have io_getevents to poll the underlying bdev (blk_poll) similar to how the net stack implements low latency sockets [1].
Having the ability to suppress interrupts and poll for completions would be very useful for file servers or targets living in userspace.
Jens Axboe
maintainerRe: [PATCH 3/3] block: Polling completion performance optimization
In reply to Keith BuschQuoted 3 lines · Keith Busch
When a request completion is polled, the completion task wakes itself up. This is unnecessary, as the task can just set itself back to running.
Looks good to me, I can take it for 4.16 in the block tree.
Signature · 1 line
Jens Axboe
Christoph Hellwig
maintainerRe: [PATCH 3/3] block: Polling completion performance optimization
In reply to Keith BuschThis looks fine, but we also need the same in blkdev_bio_end_io and iomap_dio_bio_end_io. Probably best to add a little helper.