| Message ID | 20200720113748.322965-5-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/nvme: dma handling and address mapping cleanup | expand |
Klaus, On 20-07-20 13:37:36, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Remove the has_sg member from NvmeRequest since it's redundant. > > Also, make sure the request iov is destroyed at completion time. > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > hw/block/nvme.c | 11 ++++++----- > hw/block/nvme.h | 1 - > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index cb236d1c8c46..6a1a1626b87b 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -548,16 +548,20 @@ static void nvme_rw_cb(void *opaque, int ret) > block_acct_failed(blk_get_stats(n->conf.blk), &req->acct); > req->status = NVME_INTERNAL_DEV_ERROR; > } > - if (req->has_sg) { > + > + if (req->qsg.nalloc) { Personally, I prefer has_xxx or is_xxx to check whether the request is based on sg or iov as an inline function, but 'nalloc' is also fine to figure out the meaning of purpose here. > qemu_sglist_destroy(&req->qsg); > } > + if (req->iov.nalloc) { > + qemu_iovec_destroy(&req->iov); > + } > + Maybe this can be in a separated commit? Otherwise, It looks good to me. Thanks,
On Jul 30 00:29, Minwoo Im wrote: > Klaus, > Hi Minwoo, Thanks for the reviews and welcome to the party! :) > On 20-07-20 13:37:36, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > Remove the has_sg member from NvmeRequest since it's redundant. > > > > Also, make sure the request iov is destroyed at completion time. > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > hw/block/nvme.c | 11 ++++++----- > > hw/block/nvme.h | 1 - > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index cb236d1c8c46..6a1a1626b87b 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -548,16 +548,20 @@ static void nvme_rw_cb(void *opaque, int ret) > > block_acct_failed(blk_get_stats(n->conf.blk), &req->acct); > > req->status = NVME_INTERNAL_DEV_ERROR; > > } > > - if (req->has_sg) { > > + > > + if (req->qsg.nalloc) { > > Personally, I prefer has_xxx or is_xxx to check whether the request is > based on sg or iov as an inline function, but 'nalloc' is also fine to > figure out the meaning of purpose here. > What I really want to do is get rid of this duality with qsg and iovs at some point. I kinda wanna get rid of the dma helpers and the qsg entirely and do the DMA handling directly. Maybe an `int flags` member in NvmeRequest would be better for this, such as NVME_REQ_DMA etc. > > qemu_sglist_destroy(&req->qsg); > > } > > + if (req->iov.nalloc) { > > + qemu_iovec_destroy(&req->iov); > > + } > > + > > Maybe this can be in a separated commit? > Yeah. I guess whenever a commit message includes "Also, ..." you really should factor the change out ;) I'll split it. > Otherwise, It looks good to me. > > Thanks, > Does that mean I can add your R-b? :)
> -----Original Message----- > From: Qemu-devel <qemu-devel-bounces+minwoo.im=samsung.com@nongnu.org> On > Behalf Of Klaus Jensen > Sent: Thursday, July 30, 2020 3:29 AM > To: Minwoo Im <minwoo.im.dev@gmail.com> > Cc: Kevin Wolf <kwolf@redhat.com>; qemu-block@nongnu.org; Klaus Jensen > <k.jensen@samsung.com>; qemu-devel@nongnu.org; Max Reitz <mreitz@redhat.com>; > Keith Busch <kbusch@kernel.org> > Subject: Re: [PATCH 04/16] hw/block/nvme: remove redundant has_sg member > > On Jul 30 00:29, Minwoo Im wrote: > > Klaus, > > > > Hi Minwoo, > > Thanks for the reviews and welcome to the party! :) > > > On 20-07-20 13:37:36, Klaus Jensen wrote: > > > From: Klaus Jensen <k.jensen@samsung.com> > > > > > > Remove the has_sg member from NvmeRequest since it's redundant. > > > > > > Also, make sure the request iov is destroyed at completion time. > > > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > > --- > > > hw/block/nvme.c | 11 ++++++----- > > > hw/block/nvme.h | 1 - > > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > > index cb236d1c8c46..6a1a1626b87b 100644 > > > --- a/hw/block/nvme.c > > > +++ b/hw/block/nvme.c > > > @@ -548,16 +548,20 @@ static void nvme_rw_cb(void *opaque, int ret) > > > block_acct_failed(blk_get_stats(n->conf.blk), &req->acct); > > > req->status = NVME_INTERNAL_DEV_ERROR; > > > } > > > - if (req->has_sg) { > > > + > > > + if (req->qsg.nalloc) { > > > > Personally, I prefer has_xxx or is_xxx to check whether the request is > > based on sg or iov as an inline function, but 'nalloc' is also fine to > > figure out the meaning of purpose here. > > > > What I really want to do is get rid of this duality with qsg and iovs at > some point. I kinda wanna get rid of the dma helpers and the qsg > entirely and do the DMA handling directly. > > Maybe an `int flags` member in NvmeRequest would be better for this, > such as NVME_REQ_DMA etc. That looks better, but it looks like this is out of scope of this series. Anyway, Please take my tag for this patch. Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com> > > > > qemu_sglist_destroy(&req->qsg); > > > } > > > + if (req->iov.nalloc) { > > > + qemu_iovec_destroy(&req->iov); > > > + } > > > + > > > > Maybe this can be in a separated commit? > > > > Yeah. I guess whenever a commit message includes "Also, ..." you really > should factor the change out ;) > > I'll split it. > > > Otherwise, It looks good to me. > > > > Thanks, > > > > Does that mean I can add your R-b? :)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index cb236d1c8c46..6a1a1626b87b 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -548,16 +548,20 @@ static void nvme_rw_cb(void *opaque, int ret) block_acct_failed(blk_get_stats(n->conf.blk), &req->acct); req->status = NVME_INTERNAL_DEV_ERROR; } - if (req->has_sg) { + + if (req->qsg.nalloc) { qemu_sglist_destroy(&req->qsg); } + if (req->iov.nalloc) { + qemu_iovec_destroy(&req->iov); + } + nvme_enqueue_req_completion(cq, req); } static uint16_t nvme_flush(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, NvmeRequest *req) { - req->has_sg = false; block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0, BLOCK_ACCT_FLUSH); req->aiocb = blk_aio_flush(n->conf.blk, nvme_rw_cb, req); @@ -583,7 +587,6 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, return NVME_LBA_RANGE | NVME_DNR; } - req->has_sg = false; block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0, BLOCK_ACCT_WRITE); req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, offset, count, @@ -621,7 +624,6 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, } if (req->qsg.nsg > 0) { - req->has_sg = true; block_acct_start(blk_get_stats(n->conf.blk), &req->acct, req->qsg.size, acct); req->aiocb = is_write ? @@ -630,7 +632,6 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd, dma_blk_read(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE, nvme_rw_cb, req); } else { - req->has_sg = false; block_acct_start(blk_get_stats(n->conf.blk), &req->acct, req->iov.size, acct); req->aiocb = is_write ? diff --git a/hw/block/nvme.h b/hw/block/nvme.h index 0b6a8ae66559..5519b5cc7686 100644 --- a/hw/block/nvme.h +++ b/hw/block/nvme.h @@ -22,7 +22,6 @@ typedef struct NvmeRequest { struct NvmeSQueue *sq; BlockAIOCB *aiocb; uint16_t status; - bool has_sg; NvmeCqe cqe; BlockAcctCookie acct; QEMUSGList qsg;