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