Re: [Qemu-devel] [PATCH v2 1/4] colo: remove the incorrect if conditions in colo_compare_packet_tcp()

[Zhang Chen]

On Wed, Dec 13, 2017 at 5:32 AM, Mao Zhongyi <address@hidden>
wrote:

> Hi, Chen
>
> On 12/12/2017 11:19 PM, Zhang Chen wrote:
>
>> This patch should be moved behind the patch with payload comparition.
>>
>
> Well, it is not impossible, but I think it is better to be here. Because
> it is
> the correct logic to firstly ensure that there are no missing cases by
> fixing
> the small nits in the original tcp comparison then consider whether or not
> to
> rebuild it. It's two things. The payload comparison patch belongs to the
> latter,
> So this patch should be placed in front of it.


Rethink about this patch, combine this to the next patch is better in logic.



>
>
> People needs look your changes to understand why we need this patch.
>>
>
> Actually, in colo_packet_compare_tcp() the if condition (ptcp->th_off > 5)
> is
> not reasonable, it make the packets with ‘ptcp->th_off == 5’ lose the
> chance of
> comparison even if the same payload.
>
> Since colo is focus on the payload we only need to get the length of
> packet header
> rather than care about whether the packet carries option fields. So this
> is the
> meaning of this patch.
>

No, this is reasonable.
  if  (ptcp->th_off > 5) means primary side have more TCP option in this
moment,
At this time secondary side have a big probability without the
option(ptcp->th_off != stcp->th_off ) like SACK option.
So, the old way in here can handle some different pkt size situation about
TCP options.
I remember in my test, here can optimize the performance in some case very
huge.


Thanks
Zhang Chen



>
> Thanks,
> Mao
>
>
>> Thanks
>> Zhang Chen
>>
>>
>> On Wed, Dec 6, 2017 at 9:57 AM, Mao Zhongyi <address@hidden
>> <mailto:address@hidden>> wrote:
>>
>>     The th_off field specifies the size of the TCP header, if th_off > 5,
>>     it only means this tcp packet have options field. Regardless of
>> whether
>>     the packet carries option fields, it doesn't effect us to obtain the
>>     length of the header use a general method. So there is no need to add
>>     the limiting conditions(if (th_off > 5)). In addtion, here we just
>> focus
>>     on the payload, don't care about the option field. So removed the
>>     'if (th_off > 5)' and 'if (ptcp->th_sum == stcp->th_sum)' conditions
>>     together.
>>
>>     Cc: Zhang Chen <address@hidden <mailto:address@hidden>>
>>     Cc: Li Zhijian <address@hidden <mailto:
>> address@hidden>>
>>     Cc: Jason Wang <address@hidden <mailto:address@hidden>>
>>
>>     Signed-off-by: Mao Zhongyi <address@hidden <mailto:
>> address@hidden>>
>>     Signed-off-by: Li Zhijian <address@hidden <mailto:
>> address@hidden>>
>>
>>     ---
>>      net/colo-compare.c | 31 ++++++++++++-------------------
>>      1 file changed, 12 insertions(+), 19 deletions(-)
>>
>>     diff --git a/net/colo-compare.c b/net/colo-compare.c
>>     index 1ce195f..0afb5f0 100644
>>     --- a/net/colo-compare.c
>>     +++ b/net/colo-compare.c
>>     @@ -271,26 +271,19 @@ static int colo_packet_compare_tcp(Packet
>> *spkt, Packet *ppkt)
>>           * the secondary guest's timestamp. COLO just focus on payload,
>>           * so we just need skip this field.
>>           */
>>     -    if (ptcp->th_off > 5) {
>>     -        ptrdiff_t ptcp_offset, stcp_offset;
>>     +    ptrdiff_t ptcp_offset, stcp_offset;
>>
>>     -        ptcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
>>     -                      + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
>>     -        stcp_offset = spkt->transport_header - (uint8_t *)spkt->data
>>     -                      + (stcp->th_off * 4) - spkt->vnet_hdr_len;
>>     -
>>     -        /*
>>     -         * When network is busy, some tcp options(like sack) will
>> unpredictable
>>     -         * occur in primary side or secondary side. it will make
>> packet size
>>     -         * not same, but the two packet's payload is identical. colo
>> just
>>     -         * care about packet payload, so we skip the option field.
>>     -         */
>>     -        res = colo_packet_compare_common(ppkt, spkt, ptcp_offset,
>> stcp_offset);
>>     -    } else if (ptcp->th_sum == stcp->th_sum) {
>>     -        res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN,
>> ETH_HLEN);
>>     -    } else {
>>     -        res = -1;
>>     -    }
>>     +    ptcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
>>     +                  + (ptcp->th_off * 4) - ppkt->vnet_hdr_len;
>>     +    stcp_offset = spkt->transport_header - (uint8_t *)spkt->data
>>     +                  + (stcp->th_off * 4) - spkt->vnet_hdr_len;
>>     +    /*
>>     +     * When network is busy, some tcp options(like sack) will
>> unpredictable
>>     +     * occur in primary side or secondary side. it will make packet
>> size
>>     +     * not same, but the two packet's payload is identical. colo just
>>     +     * care about packet payload, so we skip the option field.
>>     +     */
>>     +    res = colo_packet_compare_common(ppkt, spkt, ptcp_offset,
>> stcp_offset);
>>
>>          if (res != 0 &&
>>              trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE))
>> {
>>     --
>>     2.9.4
>>
>>
>>
>>
>>
>
>