Re: [Qemu-devel] [PATCH 0/3] Add COLO-proxy virtio-net support

[Jason Wang <jasowang@redhat.com>]

On 2017年03月22日 09:50, Zhang Chen wrote:
>
>
> On 03/21/2017 05:15 PM, Jason Wang wrote:
>>
>>
>> On 2017年03月21日 15:08, Zhang Chen wrote:
>>>
>>>
>>> On 03/21/2017 02:30 PM, Jason Wang wrote:
>>>>
>>>>
>>>> On 2017年03月21日 14:16, Zhang Chen wrote:
>>>>>
>>>>>
>>>>> On 03/21/2017 02:10 PM, Jason Wang wrote:
>>>>>>
>>>>>>
>>>>>> On 2017年03月21日 13:47, Zhang Chen wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 03/21/2017 11:39 AM, Jason Wang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2017年03月16日 17:52, Zhang Chen wrote:
>>>>>>>>> If user use -device virtio-net-pci, virtio-net driver will add 
>>>>>>>>> a header
>>>>>>>>> to raw net packet that colo-proxy can't handle it. COLO-proxy 
>>>>>>>>> just
>>>>>>>>> focus on the packet payload, so we skip the virtio-net header 
>>>>>>>>> to compare
>>>>>>>>> the sent packet that primary guest's to secondary guest's.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Zhang Chen (3):
>>>>>>>>>    COLO-proxy: Add virtio-net packet parse function
>>>>>>>>>    COLO-proxy: Add a tag to mark virtio-net packet
>>>>>>>>>    COLO-compare: Add virtio-net packet compare support
>>>>>>>>>
>>>>>>>>>   net/colo-compare.c    | 42 
>>>>>>>>> +++++++++++++++++++++++++++++++++---------
>>>>>>>>>   net/colo.c            | 14 ++++++++++----
>>>>>>>>>   net/colo.h            |  7 ++++++-
>>>>>>>>>   net/filter-rewriter.c | 15 ++++++++++-----
>>>>>>>>>   4 files changed, 59 insertions(+), 19 deletions(-)
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi:
>>>>>>>>
>>>>>>>> Git grep told us virtio-net is not the only user for vnet 
>>>>>>>> header. E1000e and vmxnet3 uses it too.
>>>>>>>>
>>>>>>>> So we need solve them all instead of being virtio-net specific.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, In this series I just try to parse vnet header, if failed I 
>>>>>>> will try to parse normal net packet.
>>>>>>> So, I just focus on vnet header rather than virtio-net driver.
>>>>>>> But the patch comments really make people confused , Should I 
>>>>>>> fix all the comments send the V2 ?
>>>>>>
>>>>>> Yes, please. Beside this, instead of using fixed vnet header len 
>>>>>> macro, you should query the backend for the length of vnet header.
>>>>>
>>>>> I want query the backend too, but colo-compare is not a netfilter 
>>>>> means it no need attach any netdev.
>>>>> How can we query the backend for the length of vnet header?
>>>>
>>>> Filters can know this, then how about let filters add the vnet 
>>>> header length before the real packet and send it to colo-compare?
>>>>
>>>
>>> I don't know whether I understand your comments...
>>> Do you means filter send a vnet header length firstly(independent 
>>> length packet), then send the real packet?
>>
>> This requires too much changes and may break the compatibility of 
>> socket net backend.
>>
>>> If yes, I think it so expensive, in colo we use filter mirror or 
>>> redirect packet between primary and secondary
>>> that no need to know the vnet header length.
>>> Or do you means add a fake vnet header to real packet in same one 
>>> packet?
>>
>> Kind of, see below.
>>
>>> If yes, filter-mirror and filter-redirector must touch the packet do 
>>> some job like colo-compare.
>>> We initial design split colo-proxy to 4 modules have make 
>>> filter-mirror and filter-redirector not to touch
>>> the packet content, colo-compare and filter-rewriter do the packet 
>>> data related job.
>>> Or you have other way to tell colo-compare the length?
>>>
>>> Thanks
>>> Zhang Chen
>>
>> I mean e.g: We send a packet like struct {int size; const uint8_t 
>> buf[];} currently. We can try to add vnet header information like 
>> {int size; int vnet_hdr_len; const uint8_t buf[];}. Or even just pack 
>> the vnet_hdr_len information to the higher bits of size. Then we just 
>> need some encoding and decoding and the changes were limited.
>>
>> Does this make sense?
>
> Yes, encoding is OK in filter, but decoding we must touch the 
> "net_fill_rstate()", net/socket.c also use this
> function to receive packet, other codes try to send packet like struct 
> {int size; const uint8_t buf[];} maybe conflict with it.
> For the vnet header length do this job is acceptable?

Yes, I think so.

> and in what situation the vnet header length will be change?

If guest support mrg_rxbuf or virtio 1.0, there will be 2 more bytes 
(__virtio16 num_buffers). And we plan to add more information in the 
header in the future.

Thanks

>
> Thanks
> Zhang Chen