Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing
> 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
Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing
On Jul 30 19:31, Minwoo Im wrote:
> On 20-07-30 00:06:36, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Always destroy the request qsg/iov at the end of request use.
> >
> > Signed-off-by: Klaus Jensen
> > ---
> > 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(>cqe, 0x0, sizeof(req->cqe));
> > }
> >
> > +static void nvme_req_exit(NvmeRequest *req)
> > +{
> > +if (req->qsg.sg) {
> > +qemu_sglist_destroy(>qsg);
> > +}
> > +
> > +if (req->iov.iov) {
> > +qemu_iovec_destroy(>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.
Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing
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
> >
> > Always destroy the request qsg/iov at the end of request use.
> >
> > Signed-off-by: Klaus Jensen
> > ---
> > 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(>cqe, 0x0, sizeof(req->cqe));
> > }
> >
> > +static void nvme_req_exit(NvmeRequest *req)
> > +{
> > +if (req->qsg.sg) {
> > +qemu_sglist_destroy(>qsg);
> > +}
> > +
> > +if (req->iov.iov) {
> > +qemu_iovec_destroy(>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
Best regards,
Maxim Levitsky
Re: [PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing
On 20-07-30 00:06:36, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Always destroy the request qsg/iov at the end of request use.
>
> Signed-off-by: Klaus Jensen
> ---
> 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(>cqe, 0x0, sizeof(req->cqe));
> }
>
> +static void nvme_req_exit(NvmeRequest *req)
> +{
> +if (req->qsg.sg) {
> +qemu_sglist_destroy(>qsg);
> +}
> +
> +if (req->iov.iov) {
> +qemu_iovec_destroy(>iov);
> +}
> +}
> +
Klaus,
What is differences between 'clear' and 'exit' from the request
perspective?
Thanks,
[PATCH v2 14/16] hw/block/nvme: consolidate qsg/iov clearing
From: Klaus Jensen
Always destroy the request qsg/iov at the end of request use.
Signed-off-by: Klaus Jensen
---
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(>cqe, 0x0, sizeof(req->cqe));
}
+static void nvme_req_exit(NvmeRequest *req)
+{
+if (req->qsg.sg) {
+qemu_sglist_destroy(>qsg);
+}
+
+if (req->iov.iov) {
+qemu_iovec_destroy(>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(>parent_obj, addr, (void *)>cqe,
sizeof(req->cqe));
+nvme_req_exit(req);
QTAILQ_INSERT_TAIL(>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(>qsg);
-}
-if (req->iov.nalloc) {
-qemu_iovec_destroy(>iov);
-}
-
nvme_enqueue_req_completion(cq, req);
}
--
2.27.0
