On Wed, Jul 30, 2014 at 10:40 PM, Michael S. Tsirkin <m...@redhat.com> 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