| Message ID | 20220420041053.7927-4-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-blk: small cleanup | expand |
On Tue, Apr 19, 2022 at 09:10:52PM -0700, Chaitanya Kulkarni wrote: > We can avoid a function call virtblk_map_data() in the fast path if > block layer request has no physical segments by moving the call > blk_rq_nr_phys_segments() from virtblk_map_data() to virtio_queue_rq(). > > Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/block/virtio_blk.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) virtblk_map_data() is a static function that is not called by anything else in virtio_blk.c. The disassembly on my x86_64 machine shows it has been inlined into virtio_queue_rq(). The compiler has already done what this patch is trying to do (and more). Can you share performance data or some more background on why this code change is necessary? > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index d038800474c2..74c3a48cd1e5 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -178,9 +178,6 @@ static int virtblk_map_data(struct blk_mq_hw_ctx *hctx, struct request *req, > { > int err; > > - if (!blk_rq_nr_phys_segments(req)) > - return 0; > - > vbr->sg_table.sgl = vbr->sg; > err = sg_alloc_table_chained(&vbr->sg_table, > blk_rq_nr_phys_segments(req), > @@ -303,7 +300,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx, > struct request *req = bd->rq; > struct virtblk_req *vbr = blk_mq_rq_to_pdu(req); > unsigned long flags; > - int num; > + int num = 0; > int qid = hctx->queue_num; > bool notify = false; > blk_status_t status; > @@ -315,10 +312,12 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx, > > blk_mq_start_request(req); > > - num = virtblk_map_data(hctx, req, vbr); > - if (unlikely(num < 0)) { > - virtblk_cleanup_cmd(req); > - return BLK_STS_RESOURCE; > + if (blk_rq_nr_phys_segments(req)) { > + num = virtblk_map_data(hctx, req, vbr); > + if (unlikely(num < 0)) { > + virtblk_cleanup_cmd(req); > + return BLK_STS_RESOURCE; > + } > } > > spin_lock_irqsave(&vblk->vqs[qid].lock, flags); > -- > 2.29.0 >
On 4/20/22 08:16, Stefan Hajnoczi wrote: > On Tue, Apr 19, 2022 at 09:10:52PM -0700, Chaitanya Kulkarni wrote: >> We can avoid a function call virtblk_map_data() in the fast path if >> block layer request has no physical segments by moving the call >> blk_rq_nr_phys_segments() from virtblk_map_data() to virtio_queue_rq(). >> >> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >> --- >> drivers/block/virtio_blk.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) > > virtblk_map_data() is a static function that is not called by anything > else in virtio_blk.c. The disassembly on my x86_64 machine shows it has > been inlined into virtio_queue_rq(). The compiler has already done what > this patch is trying to do (and more). > > Can you share performance data or some more background on why this > code change is necessary? > > I don't have performance numbers, but when reading the code it takes to a function call especially in the fast path where read/write requests has phys segments and those are probably most frequent requests... I'll drop it from V2, if I find performance numbers then I'll repost is with the quantitative data ... -ck
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index d038800474c2..74c3a48cd1e5 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -178,9 +178,6 @@ static int virtblk_map_data(struct blk_mq_hw_ctx *hctx, struct request *req, { int err; - if (!blk_rq_nr_phys_segments(req)) - return 0; - vbr->sg_table.sgl = vbr->sg; err = sg_alloc_table_chained(&vbr->sg_table, blk_rq_nr_phys_segments(req), @@ -303,7 +300,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req = bd->rq; struct virtblk_req *vbr = blk_mq_rq_to_pdu(req); unsigned long flags; - int num; + int num = 0; int qid = hctx->queue_num; bool notify = false; blk_status_t status; @@ -315,10 +312,12 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx, blk_mq_start_request(req); - num = virtblk_map_data(hctx, req, vbr); - if (unlikely(num < 0)) { - virtblk_cleanup_cmd(req); - return BLK_STS_RESOURCE; + if (blk_rq_nr_phys_segments(req)) { + num = virtblk_map_data(hctx, req, vbr); + if (unlikely(num < 0)) { + virtblk_cleanup_cmd(req); + return BLK_STS_RESOURCE; + } } spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
We can avoid a function call virtblk_map_data() in the fast path if block layer request has no physical segments by moving the call blk_rq_nr_phys_segments() from virtblk_map_data() to virtio_queue_rq(). Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> --- drivers/block/virtio_blk.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)