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

[Jason Wang <jasowang@redhat.com>]

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 <zhangchen.fnst@cn.fujitsu.com>
>>>>> ---
>>>>>   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;
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>>
>> .
>>
>