Re: [Qemu-devel] [PATCH v2 5/6] contrib: add libvhost-user

[Felipe Franciosi]

> On 27 Oct 2016, at 23:11, Michael S. Tsirkin <address@hidden> wrote:
> 
> On Tue, Oct 18, 2016 at 04:21:57PM +0000, Felipe Franciosi wrote:
>> 
>>> On 18 Oct 2016, at 16:25, Marc-André Lureau <address@hidden> wrote:
>>> 
>>> Hi Felipe
>>> 
>>> ----- Original Message -----
>>>> Hello,
>>>> 
>>>>> On 18 Oct 2016, at 10:24, Marc-André Lureau <address@hidden>
>>>>> wrote:
>>>>> 
>>>>> <...>
>>>>> 
>>>>> diff --git a/contrib/libvhost-user/libvhost-user.h
>>>>> b/contrib/libvhost-user/libvhost-user.h
>>>>> 
>>>>> <...>
>>>>> 
>>>>> +#define VHOST_MAX_NR_VIRTQUEUE 8
>>>>> +#define VIRTQUEUE_MAX_SIZE 1024
>>>> 
>>>> I think that the maximum number of VQs should be 1024 to match Qemu's.
>>>> 
>>>> http://git.qemu.org/?p=qemu.git;a=blob;f=include/hw/virtio/virtio.h;h=b913aac45589449bcc5d8161651332f4b0d69c7f;hb=HEAD#l55
>>> 
>>> That would make the VuDev structure quite big. We may want to set the nr of 
>>> max queues in vu_init() instead, and allocate it there. I think this is 
>>> rather a current limitation, but does not prevent iterating from there.
>> 
>> Well, it depends what we call "quite big". The only thing that depends on 
>> that is:
>> 
>>> struct VuDev {
>>> <...>
>>>    VuVirtq vq[VHOST_MAX_NR_VIRTQUEUE];
>>> <...>
>> 
>> And VuVirtq is 96 bytes in size (x86_64). So we're really talking about 768 
>> bytes vs 96 KiB.
>> 
>> Actually, you can bring it down to 88 bytes by...
>> 
>>> typedef struct VuRing {
>>>    unsigned int num;
>>>    struct vring_desc *desc;
>>>    struct vring_avail *avail;
>>>    struct vring_used *used;
>>>    uint64_t log_guest_addr;
>>>    uint32_t flags;
>> 
>> ... moving "flags" just below "num". Allows gcc to compact the struct better.
>> 
>>> } VuRing;
>> 
>> 
>> I'd rather fix this kind of thing sooner than later, but it's ultimately up 
>> to you.
>> 
>> Felipe
> 
> I suspect the right thing to do is allocating it dynamically,
> I'm fine merging as is for now though.

Awesome. I'm all in for getting this merged asap. It will make it easier for me 
(and others) to test the vhost-user-scsi patches. We can always submit other 
changes later on.

Felipe

> 
> -- 
> MST