[Qemu-devel] [PATCH] nvme: Only generate interupt if warranted

Guenter Roeck posted 1 patch 5 years, 4 months ago
Test asan passed
Test checkpatch passed
Test docker-quick@centos7 passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1543187035-32757-1-git-send-email-linux@roeck-us.net
hw/block/nvme.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH] nvme: Only generate interupt if warranted
Posted by Guenter Roeck 5 years, 4 months ago
So far the code generates interrupts even if it doesn't pass any new
information to the user. This can result in spurious interrupts as
sometimes observed with Linux.

Only generate interrupts if warranted, ie if anything new is passed
to the emulated code.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/block/nvme.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9fbe567..c543c01 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
     NvmeCQueue *cq = opaque;
     NvmeCtrl *n = cq->ctrl;
     NvmeRequest *req, *next;
+    bool assert_irq = false;
 
     QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
         NvmeSQueue *sq;
@@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
         pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
             sizeof(req->cqe));
         QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
+        assert_irq = true;
+    }
+    if (assert_irq) {
+        nvme_irq_assert(n, cq);
     }
-    nvme_irq_assert(n, cq);
 }
 
 static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
-- 
2.7.4


Re: [Qemu-devel] [PATCH] nvme: Only generate interupt if warranted
Posted by Keith Busch 5 years, 4 months ago
On Sun, Nov 25, 2018 at 03:03:55PM -0800, Guenter Roeck wrote:
> So far the code generates interrupts even if it doesn't pass any new
> information to the user. This can result in spurious interrupts as
> sometimes observed with Linux.
> 
> Only generate interrupts if warranted, ie if anything new is passed
> to the emulated code.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/block/nvme.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9fbe567..c543c01 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
>      NvmeCQueue *cq = opaque;
>      NvmeCtrl *n = cq->ctrl;
>      NvmeRequest *req, *next;
> +    bool assert_irq = false;
>  
>      QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
>          NvmeSQueue *sq;
> @@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
>          pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
>              sizeof(req->cqe));
>          QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
> +        assert_irq = true;
> +    }
> +    if (assert_irq) {
> +        nvme_irq_assert(n, cq);
>      }
> -    nvme_irq_assert(n, cq);
>  }
>  
>  static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> -- 

There is a corner case this may break. For example, a controller posts
2 completions. The host happens to only see one of those completions due
to the inherent race when reading the phase bit. After the host updates
the CQ DB, the controller ought to send another interrupt even if it
didn't post any new CQEs to prevent command completion timeouts. This
might get the right behavior:

---
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb816..486db6e561 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
             sizeof(req->cqe));
         QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
     }
-    nvme_irq_assert(n, cq);
+    if (cq->tail != cq->head) {
+        nvme_irq_assert(n, cq);
+    }
 }

 static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
--

Re: [Qemu-devel] [PATCH] nvme: Only generate interupt if warranted
Posted by Guenter Roeck 5 years, 4 months ago
Hi Keith,

On Mon, Nov 26, 2018 at 09:17:17AM -0700, Keith Busch wrote:
> On Sun, Nov 25, 2018 at 03:03:55PM -0800, Guenter Roeck wrote:
> > So far the code generates interrupts even if it doesn't pass any new
> > information to the user. This can result in spurious interrupts as
> > sometimes observed with Linux.
> > 
> > Only generate interrupts if warranted, ie if anything new is passed
> > to the emulated code.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/block/nvme.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 9fbe567..c543c01 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
> >      NvmeCQueue *cq = opaque;
> >      NvmeCtrl *n = cq->ctrl;
> >      NvmeRequest *req, *next;
> > +    bool assert_irq = false;
> >  
> >      QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
> >          NvmeSQueue *sq;
> > @@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
> >          pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
> >              sizeof(req->cqe));
> >          QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
> > +        assert_irq = true;
> > +    }
> > +    if (assert_irq) {
> > +        nvme_irq_assert(n, cq);
> >      }
> > -    nvme_irq_assert(n, cq);
> >  }
> >  
> >  static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> > -- 
> 
> There is a corner case this may break. For example, a controller posts
> 2 completions. The host happens to only see one of those completions due
> to the inherent race when reading the phase bit. After the host updates
> the CQ DB, the controller ought to send another interrupt even if it
> didn't post any new CQEs to prevent command completion timeouts. This
> might get the right behavior:
> 
> ---
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..486db6e561 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
>              sizeof(req->cqe));
>          QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
>      }
> -    nvme_irq_assert(n, cq);
> +    if (cq->tail != cq->head) {
> +        nvme_irq_assert(n, cq);
> +    }
>  }
> 
That works for me as well, and looks much cleaner. Should I resubmit,
or do you want to take it from here ?

Thanks,
Guenter