From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44258) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ay9aW-0006XB-Rh for qemu-devel@nongnu.org; Wed, 04 May 2016 23:04:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ay9aK-0001bQ-8Y for qemu-devel@nongnu.org; Wed, 04 May 2016 23:04:23 -0400 Received: from [59.151.112.132] (port=36576 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ay9aI-0001Uc-Vq for qemu-devel@nongnu.org; Wed, 04 May 2016 23:04:16 -0400 References: <1460977906-25218-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <1460977906-25218-5-git-send-email-zhangchen.fnst@cn.fujitsu.com> <20160428194456.GD25006@work-vm> From: Zhang Chen Message-ID: <572AB813.2090801@cn.fujitsu.com> Date: Thu, 5 May 2016 11:03:47 +0800 MIME-Version: 1.0 In-Reply-To: <20160428194456.GD25006@work-vm> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: qemu devel , Jason Wang , Li Zhijian , Gui jianfeng , Wen Congyang , zhanghailiang , Yang Hongyang , "eddie.dong" On 04/29/2016 03:44 AM, Dr. David Alan Gilbert wrote: > * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote: >> Signed-off-by: Zhang Chen >> Signed-off-by: Li Zhijian >> Signed-off-by: Wen Congyang >> --- >> net/colo-compare.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 154 insertions(+), 4 deletions(-) >> >> diff --git a/net/colo-compare.c b/net/colo-compare.c >> index 4b5a2d4..3dad461 100644 >> --- a/net/colo-compare.c >> +++ b/net/colo-compare.c >> @@ -385,9 +385,148 @@ static int colo_packet_compare(Packet *ppkt, Packet *spkt) >> } >> } >> >> -static int colo_packet_compare_all(Packet *spkt, Packet *ppkt) >> +/* >> + * called from the compare thread on the primary >> + * for compare tcp packet >> + * compare_tcp copied from Dr. David Alan Gilbert's branch >> + */ >> +static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt) >> +{ >> + struct tcphdr *ptcp, *stcp; >> + int res; >> + char *sdebug, *ddebug; >> + ptrdiff_t offset; >> + >> + trace_colo_compare_main("compare tcp"); >> + ptcp = (struct tcphdr *)ppkt->transport_layer; >> + stcp = (struct tcphdr *)spkt->transport_layer; >> + >> + /* Initial is compare the whole packet */ >> + offset = 12; /* Hack! Skip virtio header */ > So, when I post a set of patches and mark it saying that I know they've > got a lot of hacks in them, it's good for those reusing those patches > to check they need the hacks! > > In my world I found I needed to skip over that header and I didn't understand > why; but hadn't figured out the details yet, and I'd added the 12 everywhere - > I think this is the only place you've got it, so it's almost certainly wrong. I test in my world it hadn't that header,so if I remove the 12 offset,then the function is almost OK? > >> + if (ptcp->th_flags == stcp->th_flags && >> + ((ptcp->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) { >> + /* This is the syn/ack response from the guest to an incoming >> + * connection; the secondary won't have matched the sequence number >> + * Note: We should probably compare the IP level? >> + * Note hack: This already has the virtio offset >> + */ >> + offset = sizeof(ptcp->th_ack) + (void *)&ptcp->th_ack - ppkt->data; >> + } >> + /* Note - we want to compare everything as long as it's not the syn/ack? */ >> + assert(offset > 0); >> + assert(spkt->size > offset); >> + >> + /* The 'identification' field in the IP header is *very* random >> + * it almost never matches. Fudge this by ignoring differences in >> + * unfragmented packets; they'll normally sort themselves out if different >> + * anyway, and it should recover at the TCP level. >> + * An alternative would be to get both the primary and secondary to rewrite >> + * somehow; but that would need some sync traffic to sync the state >> + */ >> + if (ntohs(ppkt->ip->ip_off) & IP_DF) { >> + spkt->ip->ip_id = ppkt->ip->ip_id; >> + /* and the sum will be different if the IDs were different */ >> + spkt->ip->ip_sum = ppkt->ip->ip_sum; >> + } >> + >> + res = memcmp(ppkt->data + offset, spkt->data + offset, >> + (spkt->size - offset)); >> + >> + if (res && DEBUG_TCP_COMPARE) { >> + sdebug = strdup(inet_ntoa(ppkt->ip->ip_src)); >> + ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst)); >> + fprintf(stderr, "%s: src/dst: %s/%s offset=%zd p: seq/ack=%u/%u" >> + " s: seq/ack=%u/%u res=%d flags=%x/%x\n", __func__, >> + sdebug, ddebug, offset, >> + ntohl(ptcp->th_seq), ntohl(ptcp->th_ack), >> + ntohl(stcp->th_seq), ntohl(stcp->th_ack), >> + res, ptcp->th_flags, stcp->th_flags); >> + if (res && (ptcp->th_seq == stcp->th_seq)) { >> + trace_colo_compare_with_int("Primary len", ppkt->size); >> + colo_dump_packet(ppkt); >> + trace_colo_compare_with_int("Secondary len", spkt->size); >> + colo_dump_packet(spkt); >> + } > Try and use meaningful traceing for this - don't use a 'compare_with_int' > trace; but use a name that says what you're doing - for example > trace_colo_tcp_miscompare ; that way if you're running COLO and just > want to see why you're getting so many miscompares, you can look > at this without turning on all the rest of the debug. OK,I will fix in next version. > > Also, in my version instead of using a DEBUG_TCP macro, I again used > the trace system, so, my code here was: > > if (trace_event_get_state(TRACE_COLO_PROXY_MISCOMPARE) && res) { > > that means you can switch it on and off at runtime using the > trace system. Then just as it's running I can get to the (qemu) prompt > and do: > trace-event colo_proxy_miscompare on > > and see what's happening without recompiling. OK,I will fix. > >> + g_free(sdebug); >> + g_free(ddebug); >> + } >> + >> + return res; >> +} >> + >> +/* >> + * called from the compare thread on the primary >> + * for compare udp packet >> + */ >> +static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt) >> +{ >> + int ret = 1; >> + >> + trace_colo_compare_main("compare udp"); >> + ret = colo_packet_compare(ppkt, spkt); >> + >> + if (ret) { >> + trace_colo_compare_main("primary udp"); >> + colo_dump_packet(ppkt); >> + trace_colo_compare_main("secondary udp"); >> + colo_dump_packet(spkt); >> + } >> + >> + return ret; >> +} >> + >> +/* >> + * called from the compare thread on the primary >> + * for compare icmp packet >> + */ >> +static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt) >> +{ >> + int network_length; >> + struct icmp *icmp_ppkt, *icmp_spkt; >> + >> + trace_colo_compare_main("compare icmp"); >> + network_length = (ppkt->ip->ip_hl & 0x0F) * 4; > Do you need that & 0x0f - the definition in ip.h is ip_hl:4 ? I will fix it in next version. > >> + icmp_ppkt = (struct icmp *)(ppkt->data + network_length + ETH_HLEN); >> + icmp_spkt = (struct icmp *)(spkt->data + network_length + ETH_HLEN); >> + >> + if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) && >> + (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) { >> + if (icmp_ppkt->icmp_type == ICMP_REDIRECT) { > Do you need to check the lengths again before reading any of these fields? OK, I will check it. Thanks Zhang Chen > >> + if (icmp_ppkt->icmp_gwaddr.s_addr != >> + icmp_spkt->icmp_gwaddr.s_addr) { >> + trace_colo_compare_main("icmp_gwaddr.s_addr not same"); >> + trace_colo_compare_with_char("ppkt s_addr", >> + inet_ntoa(icmp_ppkt->icmp_gwaddr)); >> + trace_colo_compare_with_char("spkt s_addr", >> + inet_ntoa(icmp_spkt->icmp_gwaddr)); >> + return -1; >> + } >> + } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) && >> + (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) { >> + if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) { >> + trace_colo_compare_main("icmp_nextmtu not same"); >> + trace_colo_compare_with_int("ppkt s_addr", >> + icmp_ppkt->icmp_nextmtu); >> + trace_colo_compare_with_int("spkt s_addr", >> + icmp_spkt->icmp_nextmtu); >> + return -1; >> + } >> + } >> + } else { >> + return -1; >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * called from the compare thread on the primary >> + * for compare other packet >> + */ >> +static int colo_packet_compare_other(Packet *spkt, Packet *ppkt) >> { >> - trace_colo_compare_main("compare all"); >> + trace_colo_compare_main("compare other"); > Try and make the traces give you all the information you're likely to need - for > example, adding ip_proto here would be really useful for when you find you've > suddenly got a lot of miscompare compare others and want to figure out why. > >> return colo_packet_compare(ppkt, spkt); >> } >> >> @@ -406,8 +545,19 @@ static void colo_compare_connection(void *opaque, void *user_data) >> while (!g_queue_is_empty(&conn->primary_list) && >> !g_queue_is_empty(&conn->secondary_list)) { >> pkt = g_queue_pop_head(&conn->primary_list); >> - result = g_queue_find_custom(&conn->secondary_list, >> - pkt, (GCompareFunc)colo_packet_compare_all); >> + if (conn->ip_proto == IPPROTO_TCP) { >> + result = g_queue_find_custom(&conn->secondary_list, >> + pkt, (GCompareFunc)colo_packet_compare_tcp); >> + } else if (conn->ip_proto == IPPROTO_UDP) { >> + result = g_queue_find_custom(&conn->secondary_list, >> + pkt, (GCompareFunc)colo_packet_compare_udp); >> + } else if (conn->ip_proto == IPPROTO_ICMP) { >> + result = g_queue_find_custom(&conn->secondary_list, >> + pkt, (GCompareFunc)colo_packet_compare_icmp); >> + } else { >> + result = g_queue_find_custom(&conn->secondary_list, >> + pkt, (GCompareFunc)colo_packet_compare_other); >> + } >> >> if (result) { >> ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size); >> -- >> 1.9.1 > Dave > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > . > -- Thanks zhangchen