[Qemu-devel] [PATCH 0/2] COLO-compare: Optimize tcp compare performance and trace format.

[Qemu-devel] [PATCH 0/2] COLO-compare: Optimize tcp compare performance and trace format.

[Zhang Chen]

In the first patch, we add tcp options support to optimize compare performance.
and another patch simplified code and adjust trace print format.

Zhang Chen (2):
  COLO-compare: Optimize tcp compare for option field
  COLO-compare: Optimize tcp compare trace event

 net/colo-compare.c | 54 ++++++++++++++++++++++++++++++++++++++++++------------
 net/trace-events   |  3 +--
 2 files changed, 43 insertions(+), 14 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] COLO-compare: Optimize tcp compare for option field

[Zhang Chen]

In this patch we support packet that have tcp options field.
Add tcp options field check, If the packet have options
field we just skip it and compare tcp payload,
Avoid unnecessary checkpoint, optimize performance.

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
 net/colo-compare.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index aada04e..881d6b2 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -228,6 +228,7 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
 {
     struct tcphdr *ptcp, *stcp;
     int res;
+    int tcp_offset = 0;
 
     trace_colo_compare_main("compare tcp");
 
@@ -248,7 +249,31 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
         spkt->ip->ip_sum = ppkt->ip->ip_sum;
     }
 
-    if (ptcp->th_sum == stcp->th_sum) {
+    /*
+     * Check tcp header length for tcp option field.
+     * th_off > 5 means this tcp packet have options field.
+     * The tcp options maybe always different.
+     * for example:
+     * From RFC 7323.
+     * TCP Timestamps option (TSopt):
+     * Kind: 8
+     *
+     * Length: 10 bytes
+     *
+     *    +-------+-------+---------------------+---------------------+
+     *    |Kind=8 |  10   |   TS Value (TSval)  |TS Echo Reply (TSecr)|
+     *    +-------+-------+---------------------+---------------------+
+     *       1       1              4                     4
+     *
+     * In this case the primary guest's timestamp always different with
+     * the secondary guest's timestamp. COLO just focus on payload,
+     * so we just need skip this field.
+     */
+    if (ptcp->th_off > 5) {
+        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
+                     + (ptcp->th_off * 4);
+        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
+    } else if (ptcp->th_sum == stcp->th_sum) {
         res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
     } else {
         res = -1;
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] COLO-compare: Optimize tcp compare trace event

[Zhang Chen]

Optimize two trace events as one, adjust print format make
it easy to read. rename trace_colo_compare_pkt_info_src/dst
to trace_colo_compare_tcp_info.

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
---
 net/colo-compare.c | 29 +++++++++++++++++------------
 net/trace-events   |  3 +--
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 881d6b2..18cda58 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -279,18 +279,23 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
         res = -1;
     }
 
-    if (res != 0 && trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
-        trace_colo_compare_pkt_info_src(inet_ntoa(ppkt->ip->ip_src),
-                                        ntohl(stcp->th_seq),
-                                        ntohl(stcp->th_ack),
-                                        res, stcp->th_flags,
-                                        spkt->size);
-
-        trace_colo_compare_pkt_info_dst(inet_ntoa(ppkt->ip->ip_dst),
-                                        ntohl(ptcp->th_seq),
-                                        ntohl(ptcp->th_ack),
-                                        res, ptcp->th_flags,
-                                        ppkt->size);
+    if (res && trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
+        char ip_src[20], ip_dst[20];
+
+        strcpy(ip_src, inet_ntoa(ppkt->ip->ip_src));
+        strcpy(ip_dst, inet_ntoa(ppkt->ip->ip_dst));
+
+        trace_colo_compare_tcp_info(ip_src,
+                                    ip_dst,
+                                    ntohl(ptcp->th_seq),
+                                    ntohl(stcp->th_seq),
+                                    ntohl(ptcp->th_ack),
+                                    ntohl(stcp->th_ack),
+                                    res,
+                                    ptcp->th_flags,
+                                    stcp->th_flags,
+                                    ppkt->size,
+                                    spkt->size);
 
         qemu_hexdump((char *)ppkt->data, stderr,
                      "colo-compare ppkt", ppkt->size);
diff --git a/net/trace-events b/net/trace-events
index 35198bc..123cb28 100644
--- a/net/trace-events
+++ b/net/trace-events
@@ -13,8 +13,7 @@ colo_compare_icmp_miscompare(const char *sta, int size) ": %s = %d"
 colo_compare_ip_info(int psize, const char *sta, const char *stb, int ssize, const char *stc, const char *std) "ppkt size = %d, ip_src = %s, ip_dst = %s, spkt size = %d, ip_src = %s, ip_dst = %s"
 colo_old_packet_check_found(int64_t old_time) "%" PRId64
 colo_compare_miscompare(void) ""
-colo_compare_pkt_info_src(const char *src, uint32_t sseq, uint32_t sack, int res, uint32_t sflag, int ssize) "src/dst: %s s: seq/ack=%u/%u res=%d flags=%x spkt_size: %d\n"
-colo_compare_pkt_info_dst(const char *dst, uint32_t dseq, uint32_t dack, int res, uint32_t dflag, int dsize) "src/dst: %s d: seq/ack=%u/%u res=%d flags=%x dpkt_size: %d\n"
+colo_compare_tcp_info(const char *src, const char *dst, uint32_t pseq, uint32_t sseq, uint32_t pack, uint32_t sack, int res, uint32_t pflag, uint32_t sflag, int psize, int ssize) "src/dst: %s/%s pseq/sseq:%u/%u pack/sack:%u/%u res=%d pflags/sflag:%x/%x psize/ssize:%d/%d \n"
 
 # net/filter-rewriter.c
 colo_filter_rewriter_debug(void) ""
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] COLO-compare: Optimize tcp compare for option field

[Philippe Mathieu-Daudé]

Hi Zhang,

On 04/16/2017 06:24 AM, Zhang Chen wrote:
> In this patch we support packet that have tcp options field.
> Add tcp options field check, If the packet have options
> field we just skip it and compare tcp payload,
> Avoid unnecessary checkpoint, optimize performance.
>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> ---
>  net/colo-compare.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index aada04e..881d6b2 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -228,6 +228,7 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
>  {
>      struct tcphdr *ptcp, *stcp;
>      int res;
> +    int tcp_offset = 0;

No need to declare here, it can be restricted to the if {} body.

>
>      trace_colo_compare_main("compare tcp");
>
> @@ -248,7 +249,31 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
>          spkt->ip->ip_sum = ppkt->ip->ip_sum;
>      }
>
> -    if (ptcp->th_sum == stcp->th_sum) {
> +    /*
> +     * Check tcp header length for tcp option field.
> +     * th_off > 5 means this tcp packet have options field.
> +     * The tcp options maybe always different.
> +     * for example:
> +     * From RFC 7323.
> +     * TCP Timestamps option (TSopt):
> +     * Kind: 8
> +     *
> +     * Length: 10 bytes
> +     *
> +     *    +-------+-------+---------------------+---------------------+
> +     *    |Kind=8 |  10   |   TS Value (TSval)  |TS Echo Reply (TSecr)|
> +     *    +-------+-------+---------------------+---------------------+
> +     *       1       1              4                     4
> +     *
> +     * In this case the primary guest's timestamp always different with
> +     * the secondary guest's timestamp. COLO just focus on payload,
> +     * so we just need skip this field.
> +     */
> +    if (ptcp->th_off > 5) {

You can declare here.

I'd rather declare tcp_offset as a ptrdiff_t.

> +        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
> +                     + (ptcp->th_off * 4);
> +        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
> +    } else if (ptcp->th_sum == stcp->th_sum) {
>          res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
>      } else {
>          res = -1;
>

Re: [Qemu-devel] [PATCH 1/2] COLO-compare: Optimize tcp compare for option field

[Zhang Chen]

On 04/17/2017 09:43 PM, Philippe Mathieu-Daudé wrote:
> Hi Zhang,
>
> On 04/16/2017 06:24 AM, Zhang Chen wrote:
>> In this patch we support packet that have tcp options field.
>> Add tcp options field check, If the packet have options
>> field we just skip it and compare tcp payload,
>> Avoid unnecessary checkpoint, optimize performance.
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> ---
>>  net/colo-compare.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index aada04e..881d6b2 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -228,6 +228,7 @@ static int colo_packet_compare_tcp(Packet *spkt, 
>> Packet *ppkt)
>>  {
>>      struct tcphdr *ptcp, *stcp;
>>      int res;
>> +    int tcp_offset = 0;
>
> No need to declare here, it can be restricted to the if {} body.

OK~

>
>>
>>      trace_colo_compare_main("compare tcp");
>>
>> @@ -248,7 +249,31 @@ static int colo_packet_compare_tcp(Packet *spkt, 
>> Packet *ppkt)
>>          spkt->ip->ip_sum = ppkt->ip->ip_sum;
>>      }
>>
>> -    if (ptcp->th_sum == stcp->th_sum) {
>> +    /*
>> +     * Check tcp header length for tcp option field.
>> +     * th_off > 5 means this tcp packet have options field.
>> +     * The tcp options maybe always different.
>> +     * for example:
>> +     * From RFC 7323.
>> +     * TCP Timestamps option (TSopt):
>> +     * Kind: 8
>> +     *
>> +     * Length: 10 bytes
>> +     *
>> +     * +-------+-------+---------------------+---------------------+
>> +     *    |Kind=8 |  10   |   TS Value (TSval)  |TS Echo Reply (TSecr)|
>> +     * +-------+-------+---------------------+---------------------+
>> +     *       1       1              4                     4
>> +     *
>> +     * In this case the primary guest's timestamp always different with
>> +     * the secondary guest's timestamp. COLO just focus on payload,
>> +     * so we just need skip this field.
>> +     */
>> +    if (ptcp->th_off > 5) {
>
> You can declare here.
>
> I'd rather declare tcp_offset as a ptrdiff_t.

I got your point, will fix it in next version.

Thanks
Zhang Chen

>
>> +        tcp_offset = ppkt->transport_header - (uint8_t *)ppkt->data
>> +                     + (ptcp->th_off * 4);
>> +        res = colo_packet_compare_common(ppkt, spkt, tcp_offset);
>> +    } else if (ptcp->th_sum == stcp->th_sum) {
>>          res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
>>      } else {
>>          res = -1;
>>
>
>
> .
>

-- 
Thanks
Zhang Chen