| Message ID | 1503305719-2512-2-git-send-email-zhangchen.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi chen, At 08/21/2017 04:55 PM, Zhang Chen wrote: > 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. > > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > --- > net/colo-compare.c | 39 +++++++++++++++++++++++++++------------ > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index ca67c68..f6bda41 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int mode) > * return: 0 means packet same > * > 0 || < 0 means packet different > */ > -static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset) > +static int colo_packet_compare_common(Packet *ppkt, > + Packet *spkt, > + int poffset, > + int soffset) > { > if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) { > char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20]; > @@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset) > sec_ip_src, sec_ip_dst); > } > > - offset = ppkt->vnet_hdr_len + offset; > + poffset = ppkt->vnet_hdr_len + poffset; > + soffset = ppkt->vnet_hdr_len + soffset; > > - if (ppkt->size == spkt->size) { > - return memcmp(ppkt->data + offset, > - spkt->data + offset, > - spkt->size - offset); > + if (ppkt->size == spkt->size || poffset != soffset) { > + return memcmp(ppkt->data + poffset, > + spkt->data + soffset, > + spkt->size - soffset); Here maybe a mistake, I guess you should make sure (ppkt->size - poffset) == (spkt->size - soffset) is true in case of (poffset != soffset) at the same time. Because, if sack make the header not equal, it is also possible that the data is not equal at the same time. > } else { > trace_colo_compare_main("Net packet size are not the same"); > return -1; > @@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt) > * so we just need skip this field. > */ > if (ptcp->th_off > 5) { > - ptrdiff_t tcp_offset; > + ptrdiff_t ptcp_offset, stcp_offset; > > - tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data > - + (ptcp->th_off * 4) - ppkt->vnet_hdr_len; > - res = colo_packet_compare_common(ppkt, spkt, tcp_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); we add a new parameter in xxx_common() just for xxx_tcp(). In my opinion, it seems not good. Can we split the tcp related code out from xxx_common, and make the xxx_common function just do like what its name says. > } else if (ptcp->th_sum == stcp->th_sum) { > - res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN); > + res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN, ETH_HLEN); > } else { > res = -1; > } > @@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt) > * the ip payload here. > */ > ret = colo_packet_compare_common(ppkt, spkt, > + network_header_length + ETH_HLEN, > network_header_length + ETH_HLEN); ditto > > if (ret) { > @@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt) > * the ip payload here. > */ > if (colo_packet_compare_common(ppkt, spkt, > + network_header_length + ETH_HLEN, > network_header_length + ETH_HLEN)) { ditto > trace_colo_compare_icmp_miscompare("primary pkt size", > ppkt->size); > @@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt) > sec_ip_src, sec_ip_dst); > } > > - return colo_packet_compare_common(ppkt, spkt, 0); > + return colo_packet_compare_common(ppkt, spkt, 0, 0); ditto Thanks, dou. > } > > static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time) >
On 09/01/2017 05:35 PM, Dou Liyang wrote: > Hi chen, > > At 08/21/2017 04:55 PM, Zhang Chen wrote: >> 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. >> >> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> --- >> net/colo-compare.c | 39 +++++++++++++++++++++++++++------------ >> 1 file changed, 27 insertions(+), 12 deletions(-) >> >> diff --git a/net/colo-compare.c b/net/colo-compare.c >> index ca67c68..f6bda41 100644 >> --- a/net/colo-compare.c >> +++ b/net/colo-compare.c >> @@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int >> mode) >> * return: 0 means packet same >> * > 0 || < 0 means packet different >> */ >> -static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, >> int offset) >> +static int colo_packet_compare_common(Packet *ppkt, >> + Packet *spkt, >> + int poffset, >> + int soffset) >> { >> if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) { >> char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], >> sec_ip_dst[20]; >> @@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet >> *ppkt, Packet *spkt, int offset) >> sec_ip_src, sec_ip_dst); >> } >> >> - offset = ppkt->vnet_hdr_len + offset; >> + poffset = ppkt->vnet_hdr_len + poffset; >> + soffset = ppkt->vnet_hdr_len + soffset; >> >> - if (ppkt->size == spkt->size) { >> - return memcmp(ppkt->data + offset, >> - spkt->data + offset, >> - spkt->size - offset); >> + if (ppkt->size == spkt->size || poffset != soffset) { >> + return memcmp(ppkt->data + poffset, >> + spkt->data + soffset, >> + spkt->size - soffset); > > Here maybe a mistake, > > I guess you should make sure > > (ppkt->size - poffset) == (spkt->size - soffset) is true in case of > (poffset != soffset) at the same time. > > Because, if sack make the header not equal, it is also possible that > the data is not equal at the same time. Good catch! In rare situation will miss a different packet, I will fix it in next version. > >> } else { >> trace_colo_compare_main("Net packet size are not the same"); >> return -1; >> @@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet >> *spkt, Packet *ppkt) >> * so we just need skip this field. >> */ >> if (ptcp->th_off > 5) { >> - ptrdiff_t tcp_offset; >> + ptrdiff_t ptcp_offset, stcp_offset; >> >> - tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data >> - + (ptcp->th_off * 4) - ppkt->vnet_hdr_len; >> - res = colo_packet_compare_common(ppkt, spkt, tcp_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); > > we add a new parameter in xxx_common() just for xxx_tcp(). > In my opinion, it seems not good. > > Can we split the tcp related code out from xxx_common, and make the > xxx_common function just do like what its name says. We introduce this parameter for all protocol, just tcp is the first user. We should have the ability of compare all kinds of packet. Maybe we must use different offset for other protocol except tcp in the future. Thanks Zhang Chen > > >> } else if (ptcp->th_sum == stcp->th_sum) { >> - res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN); >> + res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN, >> ETH_HLEN); >> } else { >> res = -1; >> } >> @@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt, >> Packet *ppkt) >> * the ip payload here. >> */ >> ret = colo_packet_compare_common(ppkt, spkt, >> + network_header_length + ETH_HLEN, >> network_header_length + ETH_HLEN); > > ditto > >> >> if (ret) { >> @@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt, >> Packet *ppkt) >> * the ip payload here. >> */ >> if (colo_packet_compare_common(ppkt, spkt, >> + network_header_length + ETH_HLEN, >> network_header_length + ETH_HLEN)) { > > ditto > >> trace_colo_compare_icmp_miscompare("primary pkt size", >> ppkt->size); >> @@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet >> *spkt, Packet *ppkt) >> sec_ip_src, sec_ip_dst); >> } >> >> - return colo_packet_compare_common(ppkt, spkt, 0); >> + return colo_packet_compare_common(ppkt, spkt, 0, 0); > > ditto > > Thanks, > dou. > >> } >> >> static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time) >> > > >
Hi Chen, At 09/02/2017 12:02 AM, Zhang Chen wrote: > > > On 09/01/2017 05:35 PM, Dou Liyang wrote: >> Hi chen, >> >> At 08/21/2017 04:55 PM, Zhang Chen wrote: >>> 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. >>> >>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >>> --- >>> net/colo-compare.c | 39 +++++++++++++++++++++++++++------------ >>> 1 file changed, 27 insertions(+), 12 deletions(-) >>> >>> diff --git a/net/colo-compare.c b/net/colo-compare.c >>> index ca67c68..f6bda41 100644 >>> --- a/net/colo-compare.c >>> +++ b/net/colo-compare.c >>> @@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int >>> mode) >>> * return: 0 means packet same >>> * > 0 || < 0 means packet different >>> */ >>> -static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, >>> int offset) >>> +static int colo_packet_compare_common(Packet *ppkt, >>> + Packet *spkt, >>> + int poffset, >>> + int soffset) >>> { >>> if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) { >>> char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], >>> sec_ip_dst[20]; >>> @@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet >>> *ppkt, Packet *spkt, int offset) >>> sec_ip_src, sec_ip_dst); >>> } >>> >>> - offset = ppkt->vnet_hdr_len + offset; >>> + poffset = ppkt->vnet_hdr_len + poffset; >>> + soffset = ppkt->vnet_hdr_len + soffset; >>> >>> - if (ppkt->size == spkt->size) { >>> - return memcmp(ppkt->data + offset, >>> - spkt->data + offset, >>> - spkt->size - offset); >>> + if (ppkt->size == spkt->size || poffset != soffset) { >>> + return memcmp(ppkt->data + poffset, >>> + spkt->data + soffset, >>> + spkt->size - soffset); >> >> Here maybe a mistake, >> >> I guess you should make sure >> >> (ppkt->size - poffset) == (spkt->size - soffset) is true in case of >> (poffset != soffset) at the same time. >> >> Because, if sack make the header not equal, it is also possible that >> the data is not equal at the same time. > > Good catch! In rare situation will miss a different packet, I will fix > it in next version. > Thank you very much! >> >>> } else { >>> trace_colo_compare_main("Net packet size are not the same"); >>> return -1; >>> @@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet >>> *spkt, Packet *ppkt) >>> * so we just need skip this field. >>> */ >>> if (ptcp->th_off > 5) { >>> - ptrdiff_t tcp_offset; >>> + ptrdiff_t ptcp_offset, stcp_offset; >>> >>> - tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data >>> - + (ptcp->th_off * 4) - ppkt->vnet_hdr_len; >>> - res = colo_packet_compare_common(ppkt, spkt, tcp_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); >> >> we add a new parameter in xxx_common() just for xxx_tcp(). >> In my opinion, it seems not good. >> >> Can we split the tcp related code out from xxx_common, and make the >> xxx_common function just do like what its name says. > > We introduce this parameter for all protocol, just tcp is the first user. > We should have the ability of compare all kinds of packet. > Maybe we must use different offset for other protocol except tcp in the > future. > Got it, Thanks, dou. > Thanks > Zhang Chen > >> >> >>> } else if (ptcp->th_sum == stcp->th_sum) { >>> - res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN); >>> + res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN, >>> ETH_HLEN); >>> } else { >>> res = -1; >>> } >>> @@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt, >>> Packet *ppkt) >>> * the ip payload here. >>> */ >>> ret = colo_packet_compare_common(ppkt, spkt, >>> + network_header_length + ETH_HLEN, >>> network_header_length + ETH_HLEN); >> >> ditto >> >>> >>> if (ret) { >>> @@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt, >>> Packet *ppkt) >>> * the ip payload here. >>> */ >>> if (colo_packet_compare_common(ppkt, spkt, >>> + network_header_length + ETH_HLEN, >>> network_header_length + ETH_HLEN)) { >> >> ditto >> >>> trace_colo_compare_icmp_miscompare("primary pkt size", >>> ppkt->size); >>> @@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet >>> *spkt, Packet *ppkt) >>> sec_ip_src, sec_ip_dst); >>> } >>> >>> - return colo_packet_compare_common(ppkt, spkt, 0); >>> + return colo_packet_compare_common(ppkt, spkt, 0, 0); >> >> ditto >> >> Thanks, >> dou. >> >>> } >>> >>> static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time) >>> >> >> >> >
diff --git a/net/colo-compare.c b/net/colo-compare.c index ca67c68..f6bda41 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -186,7 +186,10 @@ static int packet_enqueue(CompareState *s, int mode) * return: 0 means packet same * > 0 || < 0 means packet different */ -static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset) +static int colo_packet_compare_common(Packet *ppkt, + Packet *spkt, + int poffset, + int soffset) { if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) { char pri_ip_src[20], pri_ip_dst[20], sec_ip_src[20], sec_ip_dst[20]; @@ -201,12 +204,13 @@ static int colo_packet_compare_common(Packet *ppkt, Packet *spkt, int offset) sec_ip_src, sec_ip_dst); } - offset = ppkt->vnet_hdr_len + offset; + poffset = ppkt->vnet_hdr_len + poffset; + soffset = ppkt->vnet_hdr_len + soffset; - if (ppkt->size == spkt->size) { - return memcmp(ppkt->data + offset, - spkt->data + offset, - spkt->size - offset); + if (ppkt->size == spkt->size || poffset != soffset) { + return memcmp(ppkt->data + poffset, + spkt->data + soffset, + spkt->size - soffset); } else { trace_colo_compare_main("Net packet size are not the same"); return -1; @@ -263,13 +267,22 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt) * so we just need skip this field. */ if (ptcp->th_off > 5) { - ptrdiff_t tcp_offset; + ptrdiff_t ptcp_offset, stcp_offset; - tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data - + (ptcp->th_off * 4) - ppkt->vnet_hdr_len; - res = colo_packet_compare_common(ppkt, spkt, tcp_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); + res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN, ETH_HLEN); } else { res = -1; } @@ -329,6 +342,7 @@ static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt) * the ip payload here. */ ret = colo_packet_compare_common(ppkt, spkt, + network_header_length + ETH_HLEN, network_header_length + ETH_HLEN); if (ret) { @@ -366,6 +380,7 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt) * the ip payload here. */ if (colo_packet_compare_common(ppkt, spkt, + network_header_length + ETH_HLEN, network_header_length + ETH_HLEN)) { trace_colo_compare_icmp_miscompare("primary pkt size", ppkt->size); @@ -403,7 +418,7 @@ static int colo_packet_compare_other(Packet *spkt, Packet *ppkt) sec_ip_src, sec_ip_dst); } - return colo_packet_compare_common(ppkt, spkt, 0); + return colo_packet_compare_common(ppkt, spkt, 0, 0); } static int colo_old_packet_check_one(Packet *pkt, int64_t *check_time)
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. Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> --- net/colo-compare.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)