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

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

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.

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