diff mbox series

[v2,14/16] hw/block/nvme: consolidate qsg/iov clearing

Message ID 20200729220638.344477-15-its@irrelevant.dk (mailing list archive)
State New, archived
Headers show
Series hw/block/nvme: dma handling and address mapping cleanup | expand

Commit Message

Klaus Jensen July 29, 2020, 10:06 p.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

Always destroy the request qsg/iov at the end of request use.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 52 ++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

Comments

Minwoo Im July 30, 2020, 10:31 a.m. UTC | #1 
On 20-07-30 00:06:36, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Always destroy the request qsg/iov at the end of request use.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 52 ++++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 3d7275eae369..045dd55376a5 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -217,6 +217,17 @@ static void nvme_req_clear(NvmeRequest *req)
>      memset(&req->cqe, 0x0, sizeof(req->cqe));
>  }
>  
> +static void nvme_req_exit(NvmeRequest *req)
> +{
> +    if (req->qsg.sg) {
> +        qemu_sglist_destroy(&req->qsg);
> +    }
> +
> +    if (req->iov.iov) {
> +        qemu_iovec_destroy(&req->iov);
> +    }
> +}
> +

Klaus,

What is differences between 'clear' and 'exit' from the request
perspective?

Thanks,
Maxim Levitsky July 30, 2020, 11:02 a.m. UTC | #2 
On Thu, 2020-07-30 at 19:31 +0900, Minwoo Im wrote:
> On 20-07-30 00:06:36, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Always destroy the request qsg/iov at the end of request use.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 52 ++++++++++++++++++++-----------------------------
> >  1 file changed, 21 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 3d7275eae369..045dd55376a5 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -217,6 +217,17 @@ static void nvme_req_clear(NvmeRequest *req)
> >      memset(&req->cqe, 0x0, sizeof(req->cqe));
> >  }
> >  
> > +static void nvme_req_exit(NvmeRequest *req)
> > +{
> > +    if (req->qsg.sg) {
> > +        qemu_sglist_destroy(&req->qsg);
> > +    }
> > +
> > +    if (req->iov.iov) {
> > +        qemu_iovec_destroy(&req->iov);
> > +    }
> > +}
> > +
> 
> Klaus,
> 
> What is differences between 'clear' and 'exit' from the request
> perspective?
> 
> Thanks,
> 
In my personal opinion, I don't think the name matters that much here.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky
Klaus Jensen July 30, 2020, 11:09 a.m. UTC | #3 
On Jul 30 19:31, Minwoo Im wrote:
> On 20-07-30 00:06:36, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Always destroy the request qsg/iov at the end of request use.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 52 ++++++++++++++++++++-----------------------------
> >  1 file changed, 21 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 3d7275eae369..045dd55376a5 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -217,6 +217,17 @@ static void nvme_req_clear(NvmeRequest *req)
> >      memset(&req->cqe, 0x0, sizeof(req->cqe));
> >  }
> >  
> > +static void nvme_req_exit(NvmeRequest *req)
> > +{
> > +    if (req->qsg.sg) {
> > +        qemu_sglist_destroy(&req->qsg);
> > +    }
> > +
> > +    if (req->iov.iov) {
> > +        qemu_iovec_destroy(&req->iov);
> > +    }
> > +}
> > +
> 
> Klaus,
> 
> What is differences between 'clear' and 'exit' from the request
> perspective?
> 

Hi Minwoo,

The is that on 'exit' we release request resources (like the qsg and
iov). On 'clear' we initialize the request by clearing the struct. I
guess I could call it nvme_req_init instead maybe, but really - it is
clearing it.
Minwoo Im July 30, 2020, 12:26 p.m. UTC | #4 
> Hi Minwoo,
>
> The is that on 'exit' we release request resources (like the qsg and
> iov). On 'clear' we initialize the request by clearing the struct. I
> guess I could call it nvme_req_init instead maybe, but really - it is
> clearing it.

Yeah, I just wanted to make it clear to understand myself here.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 3d7275eae369..045dd55376a5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -217,6 +217,17 @@  static void nvme_req_clear(NvmeRequest *req)
     memset(&req->cqe, 0x0, sizeof(req->cqe));
 }
 
+static void nvme_req_exit(NvmeRequest *req)
+{
+    if (req->qsg.sg) {
+        qemu_sglist_destroy(&req->qsg);
+    }
+
+    if (req->iov.iov) {
+        qemu_iovec_destroy(&req->iov);
+    }
+}
+
 static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr addr,
                                   size_t len)
 {
@@ -297,15 +308,14 @@  static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
 
     status = nvme_map_addr(n, qsg, iov, prp1, trans_len);
     if (status) {
-        goto unmap;
+        return status;
     }
 
     len -= trans_len;
     if (len) {
         if (unlikely(!prp2)) {
             trace_pci_nvme_err_invalid_prp2_missing();
-            status = NVME_INVALID_FIELD | NVME_DNR;
-            goto unmap;
+            return NVME_INVALID_FIELD | NVME_DNR;
         }
 
         if (len > n->page_size) {
@@ -326,13 +336,11 @@  static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
                 if (i == n->max_prp_ents - 1 && len > n->page_size) {
                     if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
                         trace_pci_nvme_err_invalid_prplist_ent(prp_ent);
-                        status = NVME_INVALID_FIELD | NVME_DNR;
-                        goto unmap;
+                        return NVME_INVALID_FIELD | NVME_DNR;
                     }
 
                     if (prp_list_in_cmb != nvme_addr_is_cmb(n, prp_ent)) {
-                        status = NVME_INVALID_USE_OF_CMB | NVME_DNR;
-                        goto unmap;
+                        return NVME_INVALID_USE_OF_CMB | NVME_DNR;
                     }
 
                     i = 0;
@@ -345,14 +353,13 @@  static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
 
                 if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
                     trace_pci_nvme_err_invalid_prplist_ent(prp_ent);
-                    status = NVME_INVALID_FIELD | NVME_DNR;
-                    goto unmap;
+                    return NVME_INVALID_FIELD | NVME_DNR;
                 }
 
                 trans_len = MIN(len, n->page_size);
                 status = nvme_map_addr(n, qsg, iov, prp_ent, trans_len);
                 if (status) {
-                    goto unmap;
+                    return status;
                 }
 
                 len -= trans_len;
@@ -361,27 +368,16 @@  static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
         } else {
             if (unlikely(prp2 & (n->page_size - 1))) {
                 trace_pci_nvme_err_invalid_prp2_align(prp2);
-                status = NVME_INVALID_FIELD | NVME_DNR;
-                goto unmap;
+                return NVME_INVALID_FIELD | NVME_DNR;
             }
             status = nvme_map_addr(n, qsg, iov, prp2, len);
             if (status) {
-                goto unmap;
+                return status;
             }
         }
     }
+
     return NVME_SUCCESS;
-
-unmap:
-    if (iov && iov->iov) {
-        qemu_iovec_destroy(iov);
-    }
-
-    if (qsg && qsg->sg) {
-        qemu_sglist_destroy(qsg);
-    }
-
-    return status;
 }
 
 static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
@@ -466,6 +462,7 @@  static void nvme_post_cqes(void *opaque)
         nvme_inc_cq_tail(cq);
         pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
             sizeof(req->cqe));
+        nvme_req_exit(req);
         QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
     }
     if (cq->tail != cq->head) {
@@ -602,13 +599,6 @@  static void nvme_rw_cb(void *opaque, int ret)
         req->status = NVME_INTERNAL_DEV_ERROR;
     }
 
-    if (req->qsg.nalloc) {
-        qemu_sglist_destroy(&req->qsg);
-    }
-    if (req->iov.nalloc) {
-        qemu_iovec_destroy(&req->iov);
-    }
-
     nvme_enqueue_req_completion(cq, req);
 }