Re: [Qemu-devel] [PATCH 2/3] COLO-compare: Optimize colo_packet_compare_common

[Zhang Chen]

On 02/27/2017 04:43 PM, Hailiang Zhang wrote:

On 2017/2/27 15:34, Zhang Chen wrote:

On 02/27/2017 03:28 PM, Hailiang Zhang wrote:

On 2017/2/27 15:03, Zhang Chen wrote:

On 02/25/2017 02:58 PM, Hailiang Zhang wrote:

On 2017/2/25 11:32, Zhang Chen wrote:

Add offset args for colo_packet_compare_common, optimize
colo_packet_compare_icmp() and colo_packet_compare_udp()
just compare the IP payload.

Signed-off-by: Zhang Chen <zhangchen.f...@cn.fujitsu.com>
---
    net/colo-compare.c | 28 +++++++++++++++++++++-------
    1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index e75f0ae..9853232 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -180,7 +180,7 @@ 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)
+static int colo_packet_compare_common(Packet *ppkt, Packet *spkt,
int offset)
    {
        trace_colo_compare_ip_info(ppkt->size,
inet_ntoa(ppkt->ip->ip_src),
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
@@ -188,7 +188,8 @@ static int colo_packet_compare_common(Packet
*ppkt, Packet *spkt)
inet_ntoa(spkt->ip->ip_dst));

        if (ppkt->size == spkt->size) {
-        return memcmp(ppkt->data, spkt->data, spkt->size);
+        return memcmp(ppkt->data + offset, spkt->data + offset,
+                      spkt->size - offset);
        } else {
            trace_colo_compare_main("Net packet size are not the
same");
            return -1;
@@ -237,8 +238,7 @@ static int colo_packet_compare_tcp(Packet *spkt,
Packet *ppkt)
            spkt->ip->ip_sum = ppkt->ip->ip_sum;
        }

-    res = memcmp(ppkt->data + ETH_HLEN, spkt->data + ETH_HLEN,
-                (spkt->size - ETH_HLEN));
+    res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);

For tcp packets check, why not ignore the ip headers, just like udp
packets check ?
Besides, here, can we compare the checksum stored in headers of tcp
and udp
before call colo_packet_compare_common(), which i think will improve
the comparing
performance.

That's another way to compare the packet suggested by Dr. David Alan
Gilbert,
It makes two packets IP header be same firstly, then compare all IP
packet.
This way can tell people why we ignore IP header explicitly in other
packets check like udp.

For performance, If we ignore the IP header that will reduce at least 20

byte not to be compared.
So, ignore ip header have better comparing performance.

OK, here, i think we can re-use the checksum value stored in headers
of tcp
or udp, comparing it first before comparing the complete payload of
tcp or udp
packets is another way to improve the performance, it is only 2 bytes.

No, The IP header's checksum are always different, Because the IP
header's ID field
are always different.

Not checksum in ip header， i mean checksum in tcp header or udp header.

TCP header can do it, but UDP header's checksum is optional.

Thanks
Zhang Chen

Thanks
Zhang Chen

Thanks.

Thanks
Zhang Chen

Thanks.
Hailiang

        if (res != 0 &&
trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
trace_colo_compare_pkt_info_src(inet_ntoa(ppkt->ip->ip_src),

@@ -277,7 +277,14 @@ static int colo_packet_compare_udp(Packet *spkt,

Packet *ppkt)
            return -1;
        }

-    ret = colo_packet_compare_common(ppkt, spkt);
+    /*
+     * Because of ppkt and spkt are both in the same connection,
+     * The ppkt's src ip, dst ip, src port, dst port, ip_proto all
are
+     * same with spkt. In addition, IP header's Identification is a
random
+     * field, we can handle it in IP fragmentation function later.
+     * So we just compare the ip payload here.
+     */
+    ret = colo_packet_compare_common(ppkt, spkt, network_length +
ETH_HLEN);

        if (ret) {
            trace_colo_compare_udp_miscompare("primary pkt size",
ppkt->size);
@@ -304,7 +311,14 @@ static int colo_packet_compare_icmp(Packet
*spkt, Packet *ppkt)
            return -1;
        }

-    if (colo_packet_compare_common(ppkt, spkt)) {
+    /*
+     * Because of ppkt and spkt are both in the same connection,
+     * The ppkt's src ip, dst ip, src port, dst port, ip_proto all
are
+     * same with spkt. In addition, IP header's Identification is a
random
+     * field, we can handle it in IP fragmentation function later.
+     * So we just compare the ip payload here.
+     */
+    if (colo_packet_compare_common(ppkt, spkt, network_length +
ETH_HLEN)) {
            trace_colo_compare_icmp_miscompare("primary pkt size",
ppkt->size);
            qemu_hexdump((char *)ppkt->data, stderr, "colo-compare",
@@ -330,7 +344,7 @@ static int colo_packet_compare_other(Packet
*spkt, Packet *ppkt)
inet_ntoa(ppkt->ip->ip_dst), spkt->size,
inet_ntoa(spkt->ip->ip_src),
inet_ntoa(spkt->ip->ip_dst));
-    return colo_packet_compare_common(ppkt, spkt);
+    return colo_packet_compare_common(ppkt, spkt, 0);
    }

    static int colo_old_packet_check_one(Packet *pkt, int64_t
*check_time)

.

.

.

--
Thanks
Zhang Chen