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

[Zhang Chen]

On 03/22/2017 11:13 AM, Jason Wang wrote:
> 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.

OK~ I got your point, will add related codes in next version.

Thanks
Zhang Chen

> Thanks
>
> > Thanks
> > Zhang Chen
>
>
>
>
> .

--
Thanks
Zhang Chen