[v2,1/4] colo: remove the incorrect if conditions in colo_compare_packet_tcp()

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 <zhangckid@gmail.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
Cc: Jason Wang <jasowang@redhat.com>

Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 net/colo-compare.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

This patch should be moved behind the patch with payload comparition.
People needs look your changes to understand why we need this patch.

Thanks
Zhang Chen


On Wed, Dec 6, 2017 at 9:57 AM, Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
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 <zhangckid@gmail.com>
> Cc: Li Zhijian <lizhijian@cn.fujitsu.com>
> Cc: Jason Wang <jasowang@redhat.com>
>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  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
>
>
>
>

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.

> 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.

Thanks,
Mao

>
> Thanks
> Zhang Chen
>
>
> On Wed, Dec 6, 2017 at 9:57 AM, Mao Zhongyi <maozy.fnst@cn.fujitsu.com <mailto:maozy.fnst@cn.fujitsu.com>> 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 <zhangckid@gmail.com <mailto:zhangckid@gmail.com>>
>     Cc: Li Zhijian <lizhijian@cn.fujitsu.com <mailto:lizhijian@cn.fujitsu.com>>
>     Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
>
>     Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com <mailto:maozy.fnst@cn.fujitsu.com>>
>     Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com <mailto:lizhijian@cn.fujitsu.com>>
>     ---
>      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
>
>
>
>

On Wed, Dec 13, 2017 at 5:32 AM, Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
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 <maozy.fnst@cn.fujitsu.com
>> <mailto:maozy.fnst@cn.fujitsu.com>> 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 <zhangckid@gmail.com <mailto:zhangckid@gmail.com>>
>>     Cc: Li Zhijian <lizhijian@cn.fujitsu.com <mailto:
>> lizhijian@cn.fujitsu.com>>
>>     Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
>>
>>     Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com <mailto:
>> maozy.fnst@cn.fujitsu.com>>
>>     Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com <mailto:
>> lizhijian@cn.fujitsu.com>>
>>
>>     ---
>>      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
>>
>>
>>
>>
>>
>
>