Re: [PATCH] blk-merge: fix blk_recount_segments

Ming Lei
2014-09-03T06:59:06Z

On Wed, Sep 3, 2014 at 12:19 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Tue, 02 Sep 2014 10:24:24 -0600
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 09/02/2014 10:21 AM, Christoph Hellwig wrote:
>> > Btw, one thing we should reconsider is where we set
>> > QUEUE_FLAG_NO_SG_MERGE. At least for virtio-blk it seems to me that
>> > doing the S/G merge should be a lot cheaper than fanning out into the
>> > indirect descriptors.
>
> Indirect is always considered first no matter SG merge is off or on,
> at least from current virtio-blk implementation.
>
> But it is a good idea to try direct descriptor first, the below simple
> change can improve randread(libaio, O_DIRECT, multi-queue) 7% in my test,
> and 77% transfer starts to use direct descriptor, and almost all transfer
> uses indirect descriptor only in current upstream implementation.
>
> From 8803341503dedab282c15f2801fdb6d7420cea6f Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@canonical.com>
> Date: Wed, 3 Sep 2014 10:42:32 +0800
> Subject: [PATCH] virtio_ring: use direct descriptor as far as possible
>
> Direct descriptor can avoid one kmalloc() and should have
> better cache utilization, so try to use it if there are enough
> free direct descriptors.
>
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> drivers/virtio/virtio_ring.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 4d08f45a..c76b7a4 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -224,9 +224,13 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>
> total_sg = total_in + total_out;
>
> - /* If the host supports indirect descriptor tables, and we have multiple
> - * buffers, then go indirect. FIXME: tune this threshold */
> - if (vq->indirect && total_sg > 1 && vq->vq.num_free) {
> + /*
> + * If the host supports indirect descriptor tables, and we have
> + * multiple buffers, then go indirect only if free descriptors
> + * are less than 16.
> + */
> + if (vq->indirect && total_sg > 1 && vq->vq.num_free &&
> + vq->vq.num_free < 16) {

Oops, the above should have been below:

if (vq->indirect && total_sg > 1 && vq->vq.num_free &&
(vq->vq.num_free < 16 ||
(vq->vq.num_free < total_sg))) {

Thanks,