Re: [Qemu-devel] [PATCH V2 1/2] COLO-compare: Optimize tcp compare for option field

[Jason Wang]

On 2017年04月21日 16:22, Zhang Chen wrote:
> On 04/21/2017 12:10 PM, Jason Wang wrote:
> > On 2017年04月21日 11:48, Zhang Chen wrote:
> > > On 04/20/2017 02:43 PM, Jason Wang wrote:
> > > > On 2017年04月18日 10:20, Zhang Chen wrote:
> > > > > In this patch we support packet that have tcp options field.
> > > > > Add tcp options field check, If the packet have options
> > > > > field we just skip it and compare tcp payload,
> > > > > Avoid unnecessary checkpoint, optimize performance.
> > > > >
> > > > > Signed-off-by: Zhang Chen <address@hidden>
> > > > > ---
> > > > >   net/colo-compare.c | 27 ++++++++++++++++++++++++++-
> > > > >   1 file changed, 26 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c
> > > > > index aada04e..049f6f8 100644
> > > > > --- a/net/colo-compare.c
> > > > > +++ b/net/colo-compare.c
> > > > > @@ -248,7 +248,32 @@ static int colo_packet_compare_tcp(Packet 
> > > > > *spkt, Packet *ppkt)
> > > > >           spkt->ip->ip_sum = ppkt->ip->ip_sum;
> > > > >       }
> > > > >   -    if (ptcp->th_sum == stcp->th_sum) {
> > > > > +    /*
> > > > > +     * Check tcp header length for tcp option field.
> > > > > +     * th_off > 5 means this tcp packet have options field.
> > > > > +     * The tcp options maybe always different.
> > > > > +     * for example:
> > > > > +     * From RFC 7323.
> > > > > +     * TCP Timestamps option (TSopt):
> > > > > +     * Kind: 8
> > > > > +     *
> > > > > +     * Length: 10 bytes
> > > > > +     *
> > > > > +     * +-------+-------+---------------------+---------------------+
> > > > > +     *    |Kind=8 |  10   |   TS Value (TSval)  |TS Echo Reply 
> > > > > (TSecr)|
> > > > > +     * +-------+-------+---------------------+---------------------+
> > > > > +     *       1       1              4 4
> > > > > +     *
> > > > > +     * In this case the primary guest's timestamp always 
> > > > > different with
> > > > > +     * the secondary guest's timestamp. COLO just focus on payload,
> > > > > +     * so we just need skip this field.h
> > > >
> > > > Probably a good explanation why we can skip this kind of header. 
> > > > But it does not explain why we can skip all the rest?
> > >
> > > I found tcp options have many kind number to express different meaning,
> > > Here I just give an example for the different options situation,
> > > and this field not the COLO-proxy focus on, COLO just concern the 
> > > payload.
> > > Maybe we will optimize in the feature. Currently we want to make 
> > > COLO full-function
> > > running in qemu upstream.
> >
> > I see, but the questions are:
> >
> > - If we see packets with different options (except for the case you 
> > explain above), does it mean primary and secondary run out of sync?
> > - What will happen if we compare and ask for synchronization if we 
> > find options are not exactly the same?
>
>
> The tcp options begin from the first SYN packet, if client connect to 
> guest, the primary guest and secondary guest
> just give a responses to the client's options, so I think the 
> different options part mostly from guest's running
> statues(like the timestamp), and if kind number are not same, the 
> payload more likely not same too. finally, after
> next checkpoint, all things will be same. If guest connect to client, 
> primary guest send a packet and secondary not,
> It will trigger old packet scan to do checkpoint.
>
> Thanks
> Zhang Chen

Ok this sounds more or less a question of whether or not we want to be 
more relaxed.

Thanks

> > Thanks
> >
> > > Thanks
> > > Zhang Chen
> > >
> > > > Thanks
> > > >
> > > > > +     */
> > > > > +    if (ptcp->th_off > 5) {
> > > > > +        ptrdiff_t tcp_offset;
> > > > > +        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
> > > > > +                     + (ptcp->th_off * 4);
> > > > > +        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
> > > > > +    } else if (ptcp->th_sum == stcp->th_sum) {
> > > > >           res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
> > > > >       } else {
> > > > >           res = -1;
> > > >
> > > >
> > > >
> > > > .
> >
> >
> >
> > .