| Message ID | 1504505688-32455-2-git-send-email-zhangchen.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | Optimize COLO-compare performance | expand |
Hi Chen, At 09/04/2017 02:14 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 | 40 ++++++++++++++++++++++++++++------------ > 1 file changed, 28 insertions(+), 12 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index ca67c68..18a9ebf 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,14 @@ 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 || > + ppkt->size - poffset == spkt->size - soffset) { This logic has a problem: ppkt->data |-----------|---------rangeP---------| | \ | poffset ppkt->size \ equal range \ spkt->data \ \ |---------------|------rangeS--------| | | soffset spkt->size See the above picture, (ppkt->size == spkt->size) is true, if [soffset, spkt->size] == [poffset, poffset+(spkt->size- soffset) is also ture, the code will return 0, but actually, they are not equal. Please use following code instead, if (ppkt->size - poffset == spkt->size - soffset) I am a new boy in COLO, let's see what we actually want to compare, If I am wrong, please correct me. :-) ppkt->data |-----------|---------rangeP---------| | | poffset ppkt->size spkt->data |----|----------rangeS---------| | | soffset spkt->size The data in rangeP and rangeS is what we want to compare. So, we just need care about the rangeX's size and head pointer, not the whole size. Thanks, dou.
On 09/05/2017 10:11 AM, Dou Liyang wrote: > Hi Chen, > > At 09/04/2017 02:14 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 | 40 ++++++++++++++++++++++++++++------------ >> 1 file changed, 28 insertions(+), 12 deletions(-) >> >> diff --git a/net/colo-compare.c b/net/colo-compare.c >> index ca67c68..18a9ebf 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,14 @@ 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 || >> + ppkt->size - poffset == spkt->size - soffset) { > > This logic has a problem: > > ppkt->data > |-----------|---------rangeP---------| > | \ | > poffset ppkt->size > \ equal range \ > spkt->data \ \ > |---------------|------rangeS--------| > | | > soffset spkt->size > > See the above picture, (ppkt->size == spkt->size) is true, > if [soffset, spkt->size] == [poffset, poffset+(spkt->size- soffset) is > also ture, the code will return 0, but actually, they are not equal. > > Please use following code instead, > > if (ppkt->size - poffset == spkt->size - soffset) > > I am a new boy in COLO, let's see what we actually want to compare, > If I am wrong, please correct me. :-) > > ppkt->data > |-----------|---------rangeP---------| > | | > poffset ppkt->size > > spkt->data > |----|----------rangeS---------| > | | > soffset spkt->size > > The data in rangeP and rangeS is what we want to compare. > So, we just need care about the rangeX's size and head pointer, > not the whole size. Yes, I have already considered this rare situation, but for packet comparing efficiency I ignored it. As you know, every packet will be compared in COLO FT, most of packet have the same offset, in this time use pkt->size is ok. the other packet with different offset always have a different pkt->size. But we need cover all situation firstly, I will update the V6 later. Thanks Zhang Chen > > > Thanks, > dou. > . >
diff --git a/net/colo-compare.c b/net/colo-compare.c index ca67c68..18a9ebf 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,14 @@ 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 || + ppkt->size - poffset == spkt->size - 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 +268,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 +343,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 +381,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 +419,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 | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)