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

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

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.fnst@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.


> 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)
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>>
>> .
>>
>