Re: [Qemu-devel] [PATCH 08/15] virtio: decrease size of VirtQueueElement

[Ming Lei]

On Wed, Jul 30, 2014 at 10:40 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Wed, Jul 30, 2014 at 03:51:22PM +0200, Paolo Bonzini wrote:
>> Il 30/07/2014 13:39, Ming Lei ha scritto:
>> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> > index a60104c..943e72f 100644
>> > --- a/include/hw/virtio/virtio.h
>> > +++ b/include/hw/virtio/virtio.h
>> > @@ -84,12 +84,17 @@ typedef struct VirtQueue VirtQueue;
>> >  typedef struct VirtQueueElement
>> >  {
>> >      unsigned int index;
>> > +    unsigned int num;
>> >      unsigned int out_num;
>> >      unsigned int in_num;
>> > -    hwaddr in_addr[VIRTQUEUE_MAX_SIZE];
>> > -    hwaddr out_addr[VIRTQUEUE_MAX_SIZE];
>> > -    struct iovec in_sg[VIRTQUEUE_MAX_SIZE];
>> > -    struct iovec out_sg[VIRTQUEUE_MAX_SIZE];
>> > +
>> > +    hwaddr *in_addr;
>> > +    hwaddr *out_addr;
>> > +    struct iovec *in_sg;
>> > +    struct iovec *out_sg;
>> > +
>> > +    hwaddr addr[VIRTQUEUE_MAX_SIZE];
>> > +    struct iovec sg[VIRTQUEUE_MAX_SIZE];
>> >  } VirtQueueElement;
>> >
>> >  #define VIRTIO_PCI_QUEUE_MAX 64
>> > --
>>
>> since addr and sg aren't used directly, allocate them flexibly like
>>
>>     char *p;
>>     VirtQueueElement *elem;
>>     total_size = ROUND_UP(sizeof(struct VirtQueueElement),
>>                           __alignof__(elem->addr[0]);
>>     addr_offset = total_size;
>>     total_size = ROUND_UP(total_size + num * sizeof(elem->addr[0]),
>>                           __alignof__(elem->sg[0]));
>>     sg_offset = total_size;
>>     total_size += num * sizeof(elem->sg[0]);
>>
>>     elem = p = g_slice_alloc(total_size);
>>     elem->size = total_size;
>>     elem->in_addr = p + addr_offset;
>>     elem->out_addr = elem->in_addr + in_num;
>>     elem->in_sg = p + sg_offset;
>>     elem->out_sg = elem->in_sg + in_num;
>>
>> ...
>>
>>     g_slice_free1(elem->size, elem);
>>
>> The small size will help glib do slab-style allocation and should remove
>> the need for an object pool.
>>
>> Paolo
>
> As long as you relying on this, why not allocate in_sg/out_sg
> separately?

That won't avoid big allocation(includes the following writing) which
is always expensive.

Also using each array for in and out is totally wrong and inefficient.

> In any case, a bunch of code needs to be audited to make sure
> no one assumes we can always stick VIRTQUEUE_MAX_SIZE there.
>
>
> But what really worries me is this is an optimization patch
> without any numbers accompanying it.

One number is that almost half of sizeof(elem) is saved, it isn't
good enough, is it?

> I would say split this out from your virtio blk work,
> work on this separately.

This patch can ease elem preallocation for obj pool, that is related
with previous patches.

Thanks