Re: [Qemu-devel] [PATCH V2 2/3] COLO-compare: Optimize compare_common and compare_tcp

[Jason Wang]

On 2017年03月02日 16:15, Zhang Chen wrote:



On 03/02/2017 03:51 PM, Jason Wang wrote:



On 2017年02月28日 14:11, Zhang Chen wrote:




OK, How about this?

+/*
+ * 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. we ignored all IP 
header

+ * include other field like TOS,TTL,IP Checksum.
+ */

Thanks
Zhang Chen


But this does not explain why we can ignore those fields.


+/*
+ * 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.
+ * COLO just concern the response net packet payload from primary 
guest
+ * and secondary guest are same or not, So we ignored all IP 
header include

+ * other field like TOS,TTL,IP Checksum. we only need to compare
+ * the ip payload here.
+ */

How about this fixed comments?

Thanks
Zhang Chen




Looks ok.

Thanks



Thanks

Re: [Qemu-devel] [PATCH V2 2/3] COLO-compare: Optimize compare_common and compare_tcp

[Zhang Chen]

On 03/02/2017 03:51 PM, Jason Wang wrote:



On 2017年02月28日 14:11, Zhang Chen wrote:




OK, How about this?

+/*
+ * 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. we ignored all IP header
+ * include other field like TOS,TTL,IP Checksum.
+ */

Thanks
Zhang Chen


But this does not explain why we can ignore those fields.


+/*
+ * 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.
+ * COLO just concern the response net packet payload from primary guest
+ * and secondary guest are same or not, So we ignored all IP header 
include

+ * other field like TOS,TTL,IP Checksum. we only need to compare
+ * the ip payload here.
+ */

How about this fixed comments?

Thanks
Zhang Chen




Thanks





--
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH V2 2/3] COLO-compare: Optimize compare_common and compare_tcp

[Jason Wang]

On 2017年02月28日 14:11, Zhang Chen wrote:




OK, How about this?

+/*
+ * 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. we ignored all IP header
+ * include other field like TOS,TTL,IP Checksum.
+ */

Thanks
Zhang Chen


But this does not explain why we can ignore those fields.

Thanks

Re: [Qemu-devel] [PATCH V2 2/3] COLO-compare: Optimize compare_common and compare_tcp

[Zhang Chen]

On 02/28/2017 01:47 PM, Jason Wang wrote:



On 2017年02月28日 13:18, 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. Before compare all tcp packet,
we compare tcp checksum firstly, this function can get
better performance.

Signed-off-by: Zhang Chen 
---
  net/colo-compare.c | 42 --
  1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 602a758..61fcdf2 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;
@@ -207,13 +208,6 @@ static int colo_packet_compare_tcp(Packet *spkt, 
Packet *ppkt)

trace_colo_compare_main("compare tcp");
  -if (ppkt->size != spkt->size) {
-if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
-trace_colo_compare_main("pkt size not same");
-}
-return -1;
-}
-
  ptcp = (struct tcphdr *)ppkt->transport_header;
  stcp = (struct tcphdr *)spkt->transport_header;
  @@ -231,8 +225,11 @@ 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));
+if (ptcp->th_sum == stcp->th_sum) {
+res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
+} else {
+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),
@@ -263,10 +260,18 @@ static int colo_packet_compare_tcp(Packet 
*spkt, Packet *ppkt)

  static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
  {
  int ret;
+int network_length = ppkt->ip->ip_hl * 4;


network_header_length looks better.


OK, I will rename it.




trace_colo_compare_main("compare udp");
  -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.
+ */


I'm afraid there're some other fields were ignored e.g TOS,TTL, and 
options. We'd better explain this in the comment too.


OK, How about this?

+/*
+ * 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. we ignored all IP header
+ * include other field like TOS,TTL,IP Checksum.
+ */

Thanks
Zhang Chen




+ret = colo_packet_compare_common(ppkt, spkt, network_length + 
ETH_HLEN);

if (ret) {
  trace_colo_compare_udp_miscompare("primary pkt size", 
ppkt->size);
@@ -284,9 +289,18 @@ static int colo_packet_compare_udp(Packet *spkt, 
Packet *ppkt)

   */
  static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
  {
+int network_length = ppkt->ip->ip_hl * 4;
+
  trace_colo_compare_main("compare icmp");
  -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",
@@ -312,7 +326,7 @@ static int 

Re: [Qemu-devel] [PATCH V2 2/3] COLO-compare: Optimize compare_common and compare_tcp

[Jason Wang]

On 2017年02月28日 13:18, 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. Before compare all tcp packet,
we compare tcp checksum firstly, this function can get
better performance.

Signed-off-by: Zhang Chen 
---
  net/colo-compare.c | 42 --
  1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 602a758..61fcdf2 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;
@@ -207,13 +208,6 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet 
*ppkt)
  
  trace_colo_compare_main("compare tcp");
  
-if (ppkt->size != spkt->size) {

-if (trace_event_get_state(TRACE_COLO_COMPARE_MISCOMPARE)) {
-trace_colo_compare_main("pkt size not same");
-}
-return -1;
-}
-
  ptcp = (struct tcphdr *)ppkt->transport_header;
  stcp = (struct tcphdr *)spkt->transport_header;
  
@@ -231,8 +225,11 @@ 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));
+if (ptcp->th_sum == stcp->th_sum) {
+res = colo_packet_compare_common(ppkt, spkt, ETH_HLEN);
+} else {
+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),
@@ -263,10 +260,18 @@ static int colo_packet_compare_tcp(Packet *spkt, Packet 
*ppkt)
  static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
  {
  int ret;
+int network_length = ppkt->ip->ip_hl * 4;


network_header_length looks better.

  
  trace_colo_compare_main("compare udp");
  
-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.
+ */


I'm afraid there're some other fields were ignored e.g TOS,TTL, and 
options. We'd better explain this in the comment too.



+ret = colo_packet_compare_common(ppkt, spkt, network_length + ETH_HLEN);
  
  if (ret) {

  trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
@@ -284,9 +289,18 @@ static int colo_packet_compare_udp(Packet *spkt, Packet 
*ppkt)
   */
  static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
  {
+int network_length = ppkt->ip->ip_hl * 4;
+
  trace_colo_compare_main("compare icmp");
  
-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",
@@ -312,7 +326,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)