| Message ID | aaf5c35f-7e81-fa44-d467-53b574debdd4@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> I think you missed: >>> http://git.infradead.org/nvme.git/commit/49c91e3e09dc3c9dd1718df85112a8cce3ab7007 >> >> I indeed did, thanks. >> > But it doesn't help. > > We're still having to wait for the first interrupt, and if we're really > fast that's the only completion we have to process. > > Try this: > > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index b4b32e6..e2dd9e2 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -623,6 +623,8 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, > } > __nvme_submit_cmd(nvmeq, &cmnd); > spin_unlock(&nvmeq->sq_lock); > + disable_irq_nosync(nvmeq_irq(irq)); > + irq_poll_sched(&nvmeq->iop); a. This would trigger a condition that we disable irq twice which is wrong at least because it will generate a warning. b. This would cause a way-too-much triggers of ksoftirqd. In order for it to be effective we need to to run only when it should and optimally when the completion queue has a batch of completions waiting. After a deeper analysis, I agree with Bart that interrupt coalescing is needed for it to work. The problem with nvme coalescing as Jens said, is a death penalty of 100us granularity. Hannes, Johannes, how does it look like with the devices you are testing with? Also, I think that adaptive moderation is needed in order for it to work well. I know that some networking drivers implemented adaptive moderation in SW before having HW support for it. It can be done by maintaining stats and having a periodic work that looks at it and changes the moderation parameters. Does anyone think that this is something we should consider? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph suggest to me once that we can take a hybrid approach where we consume a small amount of completions (say 4) right away from the interrupt handler and if we have more we schedule irq-poll to reap the rest. But back then it didn't work better which is not aligned with my observations that we consume only 1 completion per interrupt... I can give it another go... What do people think about it? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 19, 2017 at 10:12:17AM +0200, Sagi Grimberg wrote: > > >>>I think you missed: > >>>http://git.infradead.org/nvme.git/commit/49c91e3e09dc3c9dd1718df85112a8cce3ab7007 > >> > >>I indeed did, thanks. > >> > >But it doesn't help. > > > >We're still having to wait for the first interrupt, and if we're really > >fast that's the only completion we have to process. > > > >Try this: > > > > > >diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > >index b4b32e6..e2dd9e2 100644 > >--- a/drivers/nvme/host/pci.c > >+++ b/drivers/nvme/host/pci.c > >@@ -623,6 +623,8 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, > > } > > __nvme_submit_cmd(nvmeq, &cmnd); > > spin_unlock(&nvmeq->sq_lock); > >+ disable_irq_nosync(nvmeq_irq(irq)); > >+ irq_poll_sched(&nvmeq->iop); > > a. This would trigger a condition that we disable irq twice which > is wrong at least because it will generate a warning. > > b. This would cause a way-too-much triggers of ksoftirqd. In order for > it to be effective we need to to run only when it should and optimally > when the completion queue has a batch of completions waiting. > > After a deeper analysis, I agree with Bart that interrupt coalescing is > needed for it to work. The problem with nvme coalescing as Jens said, is > a death penalty of 100us granularity. Hannes, Johannes, how does it look > like with the devices you are testing with? I haven't had a look at AHCI's Command Completion Coalescing yet but hopefully I find the time today (+SSD testing!!!). Don't know if Hannes did (but I _think_ no). The problem is we've already maxed out our test HW w/o irq_poll and so the only changes we're seeing currently is an increase of wasted CPU cycles. Not what we wanted to have. > > Also, I think that adaptive moderation is needed in order for it to > work well. I know that some networking drivers implemented adaptive > moderation in SW before having HW support for it. It can be done by > maintaining stats and having a periodic work that looks at it and > changes the moderation parameters. > > Does anyone think that this is something we should consider? Yes we've been discussing this internally as well and it sounds good but thats still all pure theory and nothing actually implemented and tested. Byte, Johannes
On Thu, Jan 19, 2017 at 10:23:28AM +0200, Sagi Grimberg wrote: > Christoph suggest to me once that we can take a hybrid > approach where we consume a small amount of completions (say 4) > right away from the interrupt handler and if we have more > we schedule irq-poll to reap the rest. But back then it > didn't work better which is not aligned with my observations > that we consume only 1 completion per interrupt... > > I can give it another go... What do people think about it? This could be good. What's also possible (see answer to my previous mail) is measuring the time it takes for a completion to arrive and if the average time is lower than the context switch time just busy loop insted of waiting for the IRQ to arrive. If it is higher we can always schedule a timer to hit _before_ the IRQ will likely arrive and start polling. Is this something that sounds reasonable to you guys as well? Johannes
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index b4b32e6..e2dd9e2 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -623,6 +623,8 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, } __nvme_submit_cmd(nvmeq, &cmnd); spin_unlock(&nvmeq->sq_lock); + disable_irq_nosync(nvmeq_irq(irq)); + irq_poll_sched(&nvmeq->iop); return BLK_MQ_RQ_QUEUE_OK; out_cleanup_iod: nvme_free_iod(dev, req);