Re: [Qemu-devel] [virtio-dev] Re: [PATCH] virtio-net: keep the packet layout intact

[Wei Wang <wei.w.wang@intel.com>]

On 05/16/2017 12:27 PM, Wei Wang wrote:
> On 05/15/2017 10:46 PM, Michael S. Tsirkin wrote:
>> On Mon, May 15, 2017 at 05:29:15PM +0800, Wei Wang wrote:
>>> Ping for comments, thanks.
>>>
>>> On 05/11/2017 12:57 PM, Wei Wang wrote:
>>>> The current implementation may change the packet layout when
>>>> vnet_hdr needs an endianness swap. The layout change causes
>>>> one more iov to be added to the iov[] passed from the guest, which
>>>> is a barrier to making the TX queue size 1024 due to the possible
>>>> off-by-one issue.
>> It blocks making it 512 but I don't think we can make it 1024
>> as entries might cross page boundaries and get split.
>>
>
> I agree with the performance lose issue you mentioned
> below, thanks. To understand more here, could you please
> shed some light on "entries can't cross page boundaries"?
>
> Seems the virtio spec doesn't mention that vring_desc entries
> shouldn't be in two physically continuous pages. Also didn't
> find an issue from the implementation.
> On the device side, the writev manual does 
typo - "does" -> "doesn't"


> require the iov[]
> array to be in one page only, and the limit to iovcnt is 1024.
>
>
>>>> This patch changes the implementation to remain the packet layout
>>>> intact. In this case, the number of iov[] passed to writev will be
>>>> equal to the number obtained from the guest.
>>>>
>>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> As this is at the cost of a full data copy, I don't think
>> this makes sense. We could limit this when sg list does not fit
>> in 1024.
>
> Yes, this would cause a performance loss with the layout that data
> is adjacent with vnet_hdr. Since you prefer another solution below,
> I will skip and not discuss my ideas to avoid that copy.
>
>> But I really think we should just add a max s/g field to virtio
>> and then we'll be free to increase the ring size.
>
> Yes, that's also a good way to solve it. So, add a new device
> property, "max_chain_size" and a feature bit to detect it?
>
>

Best,
Wei