* [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-18 11:11 [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare Zhang Chen
@ 2016-04-18 11:11 ` Zhang Chen
2016-04-28 6:53 ` Jason Wang
2016-04-28 20:55 ` Eric Blake
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet Zhang Chen
` (3 subsequent siblings)
4 siblings, 2 replies; 39+ messages in thread
From: Zhang Chen @ 2016-04-18 11:11 UTC (permalink / raw)
To: qemu devel, Jason Wang
Cc: Zhang Chen, Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
packet come from primary char indev will be send to
outdev - packet come from secondary char dev will be drop
usage:
primary:
-netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown
-device e1000,id=e0,netdev=hn0,mac=52:a4:00:12:78:66
-chardev socket,id=mirror0,host=3.3.3.3,port=9003,server,nowait
-chardev socket,id=compare1,host=3.3.3.3,port=9004,server,nowait
-chardev socket,id=compare0,host=3.3.3.3,port=9001,server,nowait
-chardev socket,id=compare0-0,host=3.3.3.3,port=9001
-chardev socket,id=compare_out,host=3.3.3.3,port=9005,server,nowait
-chardev socket,id=compare_out0,host=3.3.3.3,port=9005
-object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0
-object filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out
-object filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0
-object colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0
secondary:
-netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,down script=/etc/qemu-ifdown
-device e1000,netdev=hn0,mac=52:a4:00:12:78:66
-chardev socket,id=red0,host=3.3.3.3,port=9003
-chardev socket,id=red1,host=3.3.3.3,port=9004
-object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
-object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
net/Makefile.objs | 1 +
net/colo-compare.c | 361 +++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-options.hx | 6 +
vl.c | 3 +-
4 files changed, 370 insertions(+), 1 deletion(-)
create mode 100644 net/colo-compare.c
diff --git a/net/Makefile.objs b/net/Makefile.objs
index b7c22fd..ba92f73 100644
--- a/net/Makefile.objs
+++ b/net/Makefile.objs
@@ -16,3 +16,4 @@ common-obj-$(CONFIG_NETMAP) += netmap.o
common-obj-y += filter.o
common-obj-y += filter-buffer.o
common-obj-y += filter-mirror.o
+common-obj-y += colo-compare.o
diff --git a/net/colo-compare.c b/net/colo-compare.c
new file mode 100644
index 0000000..c45b132
--- /dev/null
+++ b/net/colo-compare.c
@@ -0,0 +1,361 @@
+/*
+ * Copyright (c) 2016 FUJITSU LIMITED
+ * Author: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "net/net.h"
+#include "net/vhost_net.h"
+#include "qom/object_interfaces.h"
+#include "qemu/iov.h"
+#include "qom/object.h"
+#include "qemu/typedefs.h"
+#include "net/queue.h"
+#include "sysemu/char.h"
+#include "qemu/sockets.h"
+#include "qapi-visit.h"
+#include "trace.h"
+
+#define TYPE_COLO_COMPARE "colo-compare"
+#define COLO_COMPARE(obj) \
+ OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
+
+#define COMPARE_READ_LEN_MAX NET_BUFSIZE
+
+static QTAILQ_HEAD(, CompareState) net_compares =
+ QTAILQ_HEAD_INITIALIZER(net_compares);
+
+typedef struct ReadState {
+ int state; /* 0 = getting length, 1 = getting data */
+ uint32_t index;
+ uint32_t packet_len;
+ uint8_t buf[COMPARE_READ_LEN_MAX];
+} ReadState;
+
+typedef struct CompareState {
+ Object parent;
+
+ char *pri_indev;
+ char *sec_indev;
+ char *outdev;
+ CharDriverState *chr_pri_in;
+ CharDriverState *chr_sec_in;
+ CharDriverState *chr_out;
+ QTAILQ_ENTRY(CompareState) next;
+ ReadState pri_rs;
+ ReadState sec_rs;
+} CompareState;
+
+typedef struct CompareClass {
+ ObjectClass parent_class;
+} CompareClass;
+
+static int compare_chr_send(CharDriverState *out,
+ const uint8_t *buf,
+ uint32_t size)
+{
+ int ret = 0;
+ uint32_t len = htonl(size);
+
+ if (!size) {
+ return 0;
+ }
+
+ ret = qemu_chr_fe_write_all(out, (uint8_t *)&len, sizeof(len));
+ if (ret != sizeof(len)) {
+ goto err;
+ }
+
+ ret = qemu_chr_fe_write_all(out, (uint8_t *)buf, size);
+ if (ret != size) {
+ goto err;
+ }
+
+ return 0;
+
+err:
+ return ret < 0 ? ret : -EIO;
+}
+
+static int compare_chr_can_read(void *opaque)
+{
+ return COMPARE_READ_LEN_MAX;
+}
+
+/*
+ * called from the main thread on the primary
+ * for get packets
+ * Returns
+ * 0: readstate is not ready
+ * 1: readstate is ready
+ * otherwise error occurs
+ */
+static int compare_chr_fill_rstate(ReadState *rs, const uint8_t *buf, int size)
+{
+ unsigned int l;
+ while (size > 0) {
+ /* reassemble a packet from the network */
+ switch (rs->state) { /* 0 = getting length, 1 = getting data */
+ case 0:
+ l = 4 - rs->index;
+ if (l > size) {
+ l = size;
+ }
+ memcpy(rs->buf + rs->index, buf, l);
+ buf += l;
+ size -= l;
+ rs->index += l;
+ if (rs->index == 4) {
+ /* got length */
+ rs->packet_len = ntohl(*(uint32_t *)rs->buf);
+ rs->index = 0;
+ rs->state = 1;
+ }
+ break;
+ case 1:
+ l = rs->packet_len - rs->index;
+ if (l > size) {
+ l = size;
+ }
+ if (rs->index + l <= sizeof(rs->buf)) {
+ memcpy(rs->buf + rs->index, buf, l);
+ } else {
+ error_report("colo-compare: "
+ "Received oversized packet over socket");
+ rs->index = rs->state = 0;
+ return -1;
+ }
+
+ rs->index += l;
+ buf += l;
+ size -= l;
+ if (rs->index >= rs->packet_len) {
+ rs->index = 0;
+ rs->state = 0;
+ return 1;
+ }
+ break;
+ }
+ }
+ return 0;
+}
+
+/*
+ * called from the main thread on the primary for packets
+ * arriving over the socket from the primary.
+ */
+static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
+{
+ CompareState *s = COLO_COMPARE(opaque);
+ int ret;
+
+ ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
+ if (ret == 1) {
+ /* FIXME: enqueue to primary packet list */
+ compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
+ } else if (ret == -1) {
+ qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
+ }
+}
+
+/*
+ * called from the main thread on the primary for packets
+ * arriving over the socket from the secondary.
+ */
+static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
+{
+ CompareState *s = COLO_COMPARE(opaque);
+ int ret;
+
+ ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
+ if (ret == 1) {
+ /* TODO: enqueue to secondary packet list*/
+ /* should we send sec arp pkt? */
+ compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
+ } else if (ret == -1) {
+ qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
+ }
+}
+
+static char *compare_get_pri_indev(Object *obj, Error **errp)
+{
+ CompareState *s = COLO_COMPARE(obj);
+
+ return g_strdup(s->pri_indev);
+}
+
+static void compare_set_pri_indev(Object *obj, const char *value, Error **errp)
+{
+ CompareState *s = COLO_COMPARE(obj);
+
+ g_free(s->pri_indev);
+ s->pri_indev = g_strdup(value);
+}
+
+static char *compare_get_sec_indev(Object *obj, Error **errp)
+{
+ CompareState *s = COLO_COMPARE(obj);
+
+ return g_strdup(s->sec_indev);
+}
+
+static void compare_set_sec_indev(Object *obj, const char *value, Error **errp)
+{
+ CompareState *s = COLO_COMPARE(obj);
+
+ g_free(s->sec_indev);
+ s->sec_indev = g_strdup(value);
+}
+
+static char *compare_get_outdev(Object *obj, Error **errp)
+{
+ CompareState *s = COLO_COMPARE(obj);
+
+ return g_strdup(s->outdev);
+}
+
+static void compare_set_outdev(Object *obj, const char *value, Error **errp)
+{
+ CompareState *s = COLO_COMPARE(obj);
+
+ g_free(s->outdev);
+ s->outdev = g_strdup(value);
+}
+
+/*
+ * called from the main thread on the primary
+ * to setup colo-compare.
+ */
+static void colo_compare_complete(UserCreatable *uc, Error **errp)
+{
+ CompareState *s = COLO_COMPARE(uc);
+
+ if (!s->pri_indev || !s->sec_indev || !s->outdev) {
+ error_setg(errp, "colo compare needs 'primary_in' ,"
+ "'secondary_in','outdev' property set");
+ return;
+ } else if (!strcmp(s->pri_indev, s->outdev) ||
+ !strcmp(s->sec_indev, s->outdev) ||
+ !strcmp(s->pri_indev, s->sec_indev)) {
+ error_setg(errp, "'indev' and 'outdev' could not be same "
+ "for compare module");
+ return;
+ }
+
+ s->chr_pri_in = qemu_chr_find(s->pri_indev);
+ if (s->chr_pri_in == NULL) {
+ error_setg(errp, "Primary IN Device '%s' not found",
+ s->pri_indev);
+ return;
+ }
+
+ s->chr_sec_in = qemu_chr_find(s->sec_indev);
+ if (s->chr_sec_in == NULL) {
+ error_setg(errp, "Secondary IN Device '%s' not found",
+ s->sec_indev);
+ return;
+ }
+
+ s->chr_out = qemu_chr_find(s->outdev);
+ if (s->chr_out == NULL) {
+ error_setg(errp, "OUT Device '%s' not found", s->outdev);
+ return;
+ }
+
+ qemu_chr_fe_claim_no_fail(s->chr_pri_in);
+ qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
+ compare_pri_chr_in, NULL, s);
+
+ qemu_chr_fe_claim_no_fail(s->chr_sec_in);
+ qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
+ compare_sec_chr_in, NULL, s);
+
+ qemu_chr_fe_claim_no_fail(s->chr_out);
+ QTAILQ_INSERT_TAIL(&net_compares, s, next);
+
+ return;
+}
+
+static void colo_compare_class_init(ObjectClass *oc, void *data)
+{
+ UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+
+ ucc->complete = colo_compare_complete;
+}
+
+/*
+ * called from the main thread on the primary
+ * to cleanup colo-compare.
+ */
+static void colo_compare_class_finalize(ObjectClass *oc, void *data)
+{
+ UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+ CompareState *s = COLO_COMPARE(ucc);
+
+ if (s->chr_pri_in) {
+ qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
+ qemu_chr_fe_release(s->chr_pri_in);
+ }
+ if (s->chr_sec_in) {
+ qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
+ qemu_chr_fe_release(s->chr_sec_in);
+ }
+ if (s->chr_out) {
+ qemu_chr_fe_release(s->chr_out);
+ }
+
+ if (!QTAILQ_EMPTY(&net_compares)) {
+ QTAILQ_REMOVE(&net_compares, s, next);
+ }
+}
+
+static void colo_compare_init(Object *obj)
+{
+ object_property_add_str(obj, "primary_in",
+ compare_get_pri_indev, compare_set_pri_indev,
+ NULL);
+ object_property_add_str(obj, "secondary_in",
+ compare_get_sec_indev, compare_set_sec_indev,
+ NULL);
+ object_property_add_str(obj, "outdev",
+ compare_get_outdev, compare_set_outdev,
+ NULL);
+}
+
+static void colo_compare_finalize(Object *obj)
+{
+ CompareState *s = COLO_COMPARE(obj);
+
+ g_free(s->pri_indev);
+ g_free(s->sec_indev);
+ g_free(s->outdev);
+}
+
+static const TypeInfo colo_compare_info = {
+ .name = TYPE_COLO_COMPARE,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(CompareState),
+ .instance_init = colo_compare_init,
+ .instance_finalize = colo_compare_finalize,
+ .class_size = sizeof(CompareClass),
+ .class_init = colo_compare_class_init,
+ .class_finalize = colo_compare_class_finalize,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_USER_CREATABLE },
+ { }
+ }
+};
+
+static void register_types(void)
+{
+ type_register_static(&colo_compare_info);
+}
+
+type_init(register_types);
diff --git a/qemu-options.hx b/qemu-options.hx
index 587de8f..38f58f7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
The file format is libpcap, so it can be analyzed with tools such as tcpdump
or Wireshark.
+@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
+outdev=@var{chardevid}
+
+Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
+and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
+
@item -object secret,id=@var{id},data=@var{string},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
@item -object secret,id=@var{id},file=@var{filename},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
diff --git a/vl.c b/vl.c
index cbe51ac..c6b9a6f 100644
--- a/vl.c
+++ b/vl.c
@@ -2865,7 +2865,8 @@ static bool object_create_initial(const char *type)
if (g_str_equal(type, "filter-buffer") ||
g_str_equal(type, "filter-dump") ||
g_str_equal(type, "filter-mirror") ||
- g_str_equal(type, "filter-redirector")) {
+ g_str_equal(type, "filter-redirector") ||
+ g_str_equal(type, "colo-compare")) {
return false;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization Zhang Chen
@ 2016-04-28 6:53 ` Jason Wang
2016-04-28 7:16 ` Jason Wang
2016-04-28 7:53 ` Zhang Chen
2016-04-28 20:55 ` Eric Blake
1 sibling, 2 replies; 39+ messages in thread
From: Jason Wang @ 2016-04-28 6:53 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/18/2016 07:11 PM, Zhang Chen wrote:
> packet come from primary char indev will be send to
> outdev - packet come from secondary char dev will be drop
>
> usage:
>
> primary:
> -netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown
> -device e1000,id=e0,netdev=hn0,mac=52:a4:00:12:78:66
> -chardev socket,id=mirror0,host=3.3.3.3,port=9003,server,nowait
> -chardev socket,id=compare1,host=3.3.3.3,port=9004,server,nowait
> -chardev socket,id=compare0,host=3.3.3.3,port=9001,server,nowait
> -chardev socket,id=compare0-0,host=3.3.3.3,port=9001
> -chardev socket,id=compare_out,host=3.3.3.3,port=9005,server,nowait
> -chardev socket,id=compare_out0,host=3.3.3.3,port=9005
> -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0
> -object filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out
> -object filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0
> -object colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0
>
> secondary:
> -netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,down script=/etc/qemu-ifdown
> -device e1000,netdev=hn0,mac=52:a4:00:12:78:66
> -chardev socket,id=red0,host=3.3.3.3,port=9003
> -chardev socket,id=red1,host=3.3.3.3,port=9004
> -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
> -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
> net/Makefile.objs | 1 +
> net/colo-compare.c | 361 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> qemu-options.hx | 6 +
> vl.c | 3 +-
> 4 files changed, 370 insertions(+), 1 deletion(-)
> create mode 100644 net/colo-compare.c
>
> diff --git a/net/Makefile.objs b/net/Makefile.objs
> index b7c22fd..ba92f73 100644
> --- a/net/Makefile.objs
> +++ b/net/Makefile.objs
> @@ -16,3 +16,4 @@ common-obj-$(CONFIG_NETMAP) += netmap.o
> common-obj-y += filter.o
> common-obj-y += filter-buffer.o
> common-obj-y += filter-mirror.o
> +common-obj-y += colo-compare.o
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> new file mode 100644
> index 0000000..c45b132
> --- /dev/null
> +++ b/net/colo-compare.c
> @@ -0,0 +1,361 @@
> +/*
> + * Copyright (c) 2016 FUJITSU LIMITED
> + * Author: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later. See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "net/net.h"
> +#include "net/vhost_net.h"
> +#include "qom/object_interfaces.h"
> +#include "qemu/iov.h"
> +#include "qom/object.h"
> +#include "qemu/typedefs.h"
> +#include "net/queue.h"
> +#include "sysemu/char.h"
> +#include "qemu/sockets.h"
> +#include "qapi-visit.h"
> +#include "trace.h"
> +
> +#define TYPE_COLO_COMPARE "colo-compare"
> +#define COLO_COMPARE(obj) \
> + OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
> +
> +#define COMPARE_READ_LEN_MAX NET_BUFSIZE
> +
> +static QTAILQ_HEAD(, CompareState) net_compares =
> + QTAILQ_HEAD_INITIALIZER(net_compares);
Don't see any usage of this, if it was used in the following patches,
better move this to that patch.
> +
> +typedef struct ReadState {
> + int state; /* 0 = getting length, 1 = getting data */
> + uint32_t index;
> + uint32_t packet_len;
> + uint8_t buf[COMPARE_READ_LEN_MAX];
> +} ReadState;
> +
> +typedef struct CompareState {
> + Object parent;
> +
> + char *pri_indev;
> + char *sec_indev;
> + char *outdev;
> + CharDriverState *chr_pri_in;
> + CharDriverState *chr_sec_in;
> + CharDriverState *chr_out;
> + QTAILQ_ENTRY(CompareState) next;
> + ReadState pri_rs;
> + ReadState sec_rs;
What did 'rs' short for, ReadState? :)
> +} CompareState;
> +
> +typedef struct CompareClass {
> + ObjectClass parent_class;
> +} CompareClass;
> +
> +static int compare_chr_send(CharDriverState *out,
> + const uint8_t *buf,
> + uint32_t size)
> +{
> + int ret = 0;
> + uint32_t len = htonl(size);
> +
> + if (!size) {
> + return 0;
> + }
> +
> + ret = qemu_chr_fe_write_all(out, (uint8_t *)&len, sizeof(len));
> + if (ret != sizeof(len)) {
> + goto err;
> + }
> +
> + ret = qemu_chr_fe_write_all(out, (uint8_t *)buf, size);
> + if (ret != size) {
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + return ret < 0 ? ret : -EIO;
> +}
> +
> +static int compare_chr_can_read(void *opaque)
> +{
> + return COMPARE_READ_LEN_MAX;
> +}
> +
> +/*
> + * called from the main thread on the primary
> + * for get packets
> + * Returns
> + * 0: readstate is not ready
> + * 1: readstate is ready
> + * otherwise error occurs
> + */
> +static int compare_chr_fill_rstate(ReadState *rs, const uint8_t *buf, int size)
> +{
> + unsigned int l;
> + while (size > 0) {
> + /* reassemble a packet from the network */
> + switch (rs->state) { /* 0 = getting length, 1 = getting data */
> + case 0:
> + l = 4 - rs->index;
> + if (l > size) {
> + l = size;
> + }
> + memcpy(rs->buf + rs->index, buf, l);
> + buf += l;
> + size -= l;
> + rs->index += l;
> + if (rs->index == 4) {
> + /* got length */
> + rs->packet_len = ntohl(*(uint32_t *)rs->buf);
> + rs->index = 0;
> + rs->state = 1;
> + }
> + break;
> + case 1:
> + l = rs->packet_len - rs->index;
> + if (l > size) {
> + l = size;
> + }
> + if (rs->index + l <= sizeof(rs->buf)) {
> + memcpy(rs->buf + rs->index, buf, l);
> + } else {
> + error_report("colo-compare: "
> + "Received oversized packet over socket");
> + rs->index = rs->state = 0;
> + return -1;
> + }
> +
> + rs->index += l;
> + buf += l;
> + size -= l;
> + if (rs->index >= rs->packet_len) {
> + rs->index = 0;
> + rs->state = 0;
> + return 1;
> + }
> + break;
> + }
> + }
> + return 0;
> +}
This looks rather similar to redirector_chr_read(), any chance to unify
the code?
> +
> +/*
> + * called from the main thread on the primary for packets
> + * arriving over the socket from the primary.
> + */
> +static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
> +{
> + CompareState *s = COLO_COMPARE(opaque);
> + int ret;
> +
> + ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
> + if (ret == 1) {
> + /* FIXME: enqueue to primary packet list */
Do you mean to use some internal data structure instead of chardev? If
yes, probably a "TODO", not "FIXME".
> + compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
> + } else if (ret == -1) {
> + qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
Should we add a warning here?
> + }
> +}
> +
> +/*
> + * called from the main thread on the primary for packets
> + * arriving over the socket from the secondary.
> + */
> +static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
> +{
> + CompareState *s = COLO_COMPARE(opaque);
> + int ret;
> +
> + ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
> + if (ret == 1) {
> + /* TODO: enqueue to secondary packet list*/
> + /* should we send sec arp pkt? */
What's the problem here?
> + compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
> + } else if (ret == -1) {
> + qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> + }
This function is similar to primary version, may need to unify the codes.
> +}
> +
> +static char *compare_get_pri_indev(Object *obj, Error **errp)
> +{
> + CompareState *s = COLO_COMPARE(obj);
> +
> + return g_strdup(s->pri_indev);
> +}
> +
> +static void compare_set_pri_indev(Object *obj, const char *value, Error **errp)
> +{
> + CompareState *s = COLO_COMPARE(obj);
> +
> + g_free(s->pri_indev);
> + s->pri_indev = g_strdup(value);
> +}
> +
> +static char *compare_get_sec_indev(Object *obj, Error **errp)
> +{
> + CompareState *s = COLO_COMPARE(obj);
> +
> + return g_strdup(s->sec_indev);
> +}
> +
> +static void compare_set_sec_indev(Object *obj, const char *value, Error **errp)
> +{
> + CompareState *s = COLO_COMPARE(obj);
> +
> + g_free(s->sec_indev);
> + s->sec_indev = g_strdup(value);
> +}
> +
> +static char *compare_get_outdev(Object *obj, Error **errp)
> +{
> + CompareState *s = COLO_COMPARE(obj);
> +
> + return g_strdup(s->outdev);
> +}
> +
> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
> +{
> + CompareState *s = COLO_COMPARE(obj);
> +
> + g_free(s->outdev);
> + s->outdev = g_strdup(value);
> +}
> +
> +/*
> + * called from the main thread on the primary
> + * to setup colo-compare.
> + */
> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
> +{
> + CompareState *s = COLO_COMPARE(uc);
> +
> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
> + error_setg(errp, "colo compare needs 'primary_in' ,"
> + "'secondary_in','outdev' property set");
> + return;
> + } else if (!strcmp(s->pri_indev, s->outdev) ||
> + !strcmp(s->sec_indev, s->outdev) ||
> + !strcmp(s->pri_indev, s->sec_indev)) {
> + error_setg(errp, "'indev' and 'outdev' could not be same "
> + "for compare module");
> + return;
> + }
> +
> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
> + if (s->chr_pri_in == NULL) {
> + error_setg(errp, "Primary IN Device '%s' not found",
> + s->pri_indev);
> + return;
> + }
> +
> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
> + if (s->chr_sec_in == NULL) {
> + error_setg(errp, "Secondary IN Device '%s' not found",
> + s->sec_indev);
> + return;
> + }
> +
> + s->chr_out = qemu_chr_find(s->outdev);
> + if (s->chr_out == NULL) {
> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
> + return;
> + }
> +
> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> + compare_pri_chr_in, NULL, s);
> +
> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> + compare_sec_chr_in, NULL, s);
> +
> + qemu_chr_fe_claim_no_fail(s->chr_out);
> + QTAILQ_INSERT_TAIL(&net_compares, s, next);
> +
> + return;
> +}
> +
> +static void colo_compare_class_init(ObjectClass *oc, void *data)
> +{
> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +
> + ucc->complete = colo_compare_complete;
> +}
> +
> +/*
> + * called from the main thread on the primary
> + * to cleanup colo-compare.
> + */
> +static void colo_compare_class_finalize(ObjectClass *oc, void *data)
> +{
> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> + CompareState *s = COLO_COMPARE(ucc);
> +
> + if (s->chr_pri_in) {
> + qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> + qemu_chr_fe_release(s->chr_pri_in);
> + }
> + if (s->chr_sec_in) {
> + qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> + qemu_chr_fe_release(s->chr_sec_in);
> + }
> + if (s->chr_out) {
> + qemu_chr_fe_release(s->chr_out);
> + }
> +
> + if (!QTAILQ_EMPTY(&net_compares)) {
> + QTAILQ_REMOVE(&net_compares, s, next);
> + }
> +}
> +
> +static void colo_compare_init(Object *obj)
> +{
> + object_property_add_str(obj, "primary_in",
> + compare_get_pri_indev, compare_set_pri_indev,
> + NULL);
> + object_property_add_str(obj, "secondary_in",
> + compare_get_sec_indev, compare_set_sec_indev,
> + NULL);
> + object_property_add_str(obj, "outdev",
> + compare_get_outdev, compare_set_outdev,
> + NULL);
> +}
> +
> +static void colo_compare_finalize(Object *obj)
> +{
> + CompareState *s = COLO_COMPARE(obj);
> +
> + g_free(s->pri_indev);
> + g_free(s->sec_indev);
> + g_free(s->outdev);
> +}
> +
> +static const TypeInfo colo_compare_info = {
> + .name = TYPE_COLO_COMPARE,
> + .parent = TYPE_OBJECT,
> + .instance_size = sizeof(CompareState),
> + .instance_init = colo_compare_init,
> + .instance_finalize = colo_compare_finalize,
> + .class_size = sizeof(CompareClass),
> + .class_init = colo_compare_class_init,
> + .class_finalize = colo_compare_class_finalize,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_USER_CREATABLE },
> + { }
> + }
> +};
> +
> +static void register_types(void)
> +{
> + type_register_static(&colo_compare_info);
> +}
> +
> +type_init(register_types);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 587de8f..38f58f7 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
> The file format is libpcap, so it can be analyzed with tools such as tcpdump
> or Wireshark.
>
> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
> +outdev=@var{chardevid}
> +
> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
> +
> @item -object secret,id=@var{id},data=@var{string},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
> @item -object secret,id=@var{id},file=@var{filename},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
>
> diff --git a/vl.c b/vl.c
> index cbe51ac..c6b9a6f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2865,7 +2865,8 @@ static bool object_create_initial(const char *type)
> if (g_str_equal(type, "filter-buffer") ||
> g_str_equal(type, "filter-dump") ||
> g_str_equal(type, "filter-mirror") ||
> - g_str_equal(type, "filter-redirector")) {
> + g_str_equal(type, "filter-redirector") ||
> + g_str_equal(type, "colo-compare")) {
> return false;
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 6:53 ` Jason Wang
@ 2016-04-28 7:16 ` Jason Wang
2016-04-28 7:55 ` Zhang Chen
2016-04-28 7:53 ` Zhang Chen
1 sibling, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-04-28 7:16 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, eddie.dong, zhanghailiang,
Dr. David Alan Gilbert, Yang Hongyang
On 04/28/2016 02:53 PM, Jason Wang wrote:
> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
> > +{
> > + CompareState *s = COLO_COMPARE(obj);
> > +
> > + g_free(s->outdev);
> > + s->outdev = g_strdup(value);
> > +}
> > +
> > +/*
> > + * called from the main thread on the primary
> > + * to setup colo-compare.
> > + */
> > +static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > +{
> > + CompareState *s = COLO_COMPARE(uc);
> > +
> > + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
> > + error_setg(errp, "colo compare needs 'primary_in' ,"
> > + "'secondary_in','outdev' property set");
> > + return;
> > + } else if (!strcmp(s->pri_indev, s->outdev) ||
> > + !strcmp(s->sec_indev, s->outdev) ||
> > + !strcmp(s->pri_indev, s->sec_indev)) {
> > + error_setg(errp, "'indev' and 'outdev' could not be same "
> > + "for compare module");
> > + return;
> > + }
> > +
> > + s->chr_pri_in = qemu_chr_find(s->pri_indev);
> > + if (s->chr_pri_in == NULL) {
> > + error_setg(errp, "Primary IN Device '%s' not found",
> > + s->pri_indev);
> > + return;
> > + }
> > +
> > + s->chr_sec_in = qemu_chr_find(s->sec_indev);
> > + if (s->chr_sec_in == NULL) {
> > + error_setg(errp, "Secondary IN Device '%s' not found",
> > + s->sec_indev);
> > + return;
> > + }
> > +
> > + s->chr_out = qemu_chr_find(s->outdev);
> > + if (s->chr_out == NULL) {
> > + error_setg(errp, "OUT Device '%s' not found", s->outdev);
> > + return;
> > + }
> > +
> > + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> > + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> > + compare_pri_chr_in, NULL, s);
> > +
> > + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> > + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> > + compare_sec_chr_in, NULL, s);
> > +
Btw, what's the reason of handling this in main loop? I thought it would
be better to do this in colo thread? Otherwise, you need lots of extra
synchronizations?
> > + qemu_chr_fe_claim_no_fail(s->chr_out);
> > + QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > +
> > + return;
> > +}
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 7:16 ` Jason Wang
@ 2016-04-28 7:55 ` Zhang Chen
2016-04-28 8:17 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-28 7:55 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: Li Zhijian, Gui jianfeng, eddie.dong, zhanghailiang,
Dr. David Alan Gilbert, Yang Hongyang
On 04/28/2016 03:16 PM, Jason Wang wrote:
>
> On 04/28/2016 02:53 PM, Jason Wang wrote:
>> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
>>> +{
>>> + CompareState *s = COLO_COMPARE(obj);
>>> +
>>> + g_free(s->outdev);
>>> + s->outdev = g_strdup(value);
>>> +}
>>> +
>>> +/*
>>> + * called from the main thread on the primary
>>> + * to setup colo-compare.
>>> + */
>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>> +{
>>> + CompareState *s = COLO_COMPARE(uc);
>>> +
>>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>>> + "'secondary_in','outdev' property set");
>>> + return;
>>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>>> + !strcmp(s->sec_indev, s->outdev) ||
>>> + !strcmp(s->pri_indev, s->sec_indev)) {
>>> + error_setg(errp, "'indev' and 'outdev' could not be same "
>>> + "for compare module");
>>> + return;
>>> + }
>>> +
>>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>> + if (s->chr_pri_in == NULL) {
>>> + error_setg(errp, "Primary IN Device '%s' not found",
>>> + s->pri_indev);
>>> + return;
>>> + }
>>> +
>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>> + if (s->chr_sec_in == NULL) {
>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>> + s->sec_indev);
>>> + return;
>>> + }
>>> +
>>> + s->chr_out = qemu_chr_find(s->outdev);
>>> + if (s->chr_out == NULL) {
>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>> + return;
>>> + }
>>> +
>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>> + compare_pri_chr_in, NULL, s);
>>> +
>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>> + compare_sec_chr_in, NULL, s);
>>> +
> Btw, what's the reason of handling this in main loop? I thought it would
> be better to do this in colo thread? Otherwise, you need lots of extra
> synchronizations?
Do you mean we should start/stop/do checkpoint it by colo-frame?
>
>>> + qemu_chr_fe_claim_no_fail(s->chr_out);
>>> + QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>> +
>>> + return;
>>> +}
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 7:55 ` Zhang Chen
@ 2016-04-28 8:17 ` Jason Wang
2016-04-28 9:04 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-04-28 8:17 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 04/28/2016 03:55 PM, Zhang Chen wrote:
>
>
> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>
>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>> +static void compare_set_outdev(Object *obj, const char *value,
>>> Error **errp)
>>>> +{
>>>> + CompareState *s = COLO_COMPARE(obj);
>>>> +
>>>> + g_free(s->outdev);
>>>> + s->outdev = g_strdup(value);
>>>> +}
>>>> +
>>>> +/*
>>>> + * called from the main thread on the primary
>>>> + * to setup colo-compare.
>>>> + */
>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>> +{
>>>> + CompareState *s = COLO_COMPARE(uc);
>>>> +
>>>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>>>> + "'secondary_in','outdev' property set");
>>>> + return;
>>>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>> + !strcmp(s->sec_indev, s->outdev) ||
>>>> + !strcmp(s->pri_indev, s->sec_indev)) {
>>>> + error_setg(errp, "'indev' and 'outdev' could not be same "
>>>> + "for compare module");
>>>> + return;
>>>> + }
>>>> +
>>>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>> + if (s->chr_pri_in == NULL) {
>>>> + error_setg(errp, "Primary IN Device '%s' not found",
>>>> + s->pri_indev);
>>>> + return;
>>>> + }
>>>> +
>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>> + if (s->chr_sec_in == NULL) {
>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>> + s->sec_indev);
>>>> + return;
>>>> + }
>>>> +
>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>> + if (s->chr_out == NULL) {
>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>> + return;
>>>> + }
>>>> +
>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>> + compare_pri_chr_in, NULL, s);
>>>> +
>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>> + compare_sec_chr_in, NULL, s);
>>>> +
>> Btw, what's the reason of handling this in main loop? I thought it would
>> be better to do this in colo thread? Otherwise, you need lots of extra
>> synchronizations?
>
> Do you mean we should start/stop/do checkpoint it by colo-frame?
I mean we probably want to handle pri_in and sec_in in colo compare
thread. Through this way, there's no need for extra synchronization with
main loop.
>
>>
>>>> + qemu_chr_fe_claim_no_fail(s->chr_out);
>>>> + QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>>> +
>>>> + return;
>>>> +}
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 8:17 ` Jason Wang
@ 2016-04-28 9:04 ` Zhang Chen
2016-04-29 2:03 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-28 9:04 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 04/28/2016 04:17 PM, Jason Wang wrote:
>
> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>
>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>> Error **errp)
>>>>> +{
>>>>> + CompareState *s = COLO_COMPARE(obj);
>>>>> +
>>>>> + g_free(s->outdev);
>>>>> + s->outdev = g_strdup(value);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * called from the main thread on the primary
>>>>> + * to setup colo-compare.
>>>>> + */
>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>> +{
>>>>> + CompareState *s = COLO_COMPARE(uc);
>>>>> +
>>>>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>> + "'secondary_in','outdev' property set");
>>>>> + return;
>>>>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>> + !strcmp(s->sec_indev, s->outdev) ||
>>>>> + !strcmp(s->pri_indev, s->sec_indev)) {
>>>>> + error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>> + "for compare module");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>> + if (s->chr_pri_in == NULL) {
>>>>> + error_setg(errp, "Primary IN Device '%s' not found",
>>>>> + s->pri_indev);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>> + if (s->chr_sec_in == NULL) {
>>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>>> + s->sec_indev);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>>> + if (s->chr_out == NULL) {
>>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>> + compare_pri_chr_in, NULL, s);
>>>>> +
>>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>> + compare_sec_chr_in, NULL, s);
>>>>> +
>>> Btw, what's the reason of handling this in main loop? I thought it would
>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>> synchronizations?
>> Do you mean we should start/stop/do checkpoint it by colo-frame?
> I mean we probably want to handle pri_in and sec_in in colo compare
> thread. Through this way, there's no need for extra synchronization with
> main loop.
I get your point, but how to do this.
Now, we use qemu_chr_add_handlers to do this job.
Thanks
zhangchen
>
>>>>> + qemu_chr_fe_claim_no_fail(s->chr_out);
>>>>> + QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>>>> +
>>>>> + return;
>>>>> +}
>>>
>>> .
>>>
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 9:04 ` Zhang Chen
@ 2016-04-29 2:03 ` Jason Wang
2016-04-29 2:08 ` Zhang Chen
2016-05-06 5:42 ` Zhang Chen
0 siblings, 2 replies; 39+ messages in thread
From: Jason Wang @ 2016-04-29 2:03 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 04/28/2016 05:04 PM, Zhang Chen wrote:
>
>
> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>
>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>
>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>> Error **errp)
>>>>>> +{
>>>>>> + CompareState *s = COLO_COMPARE(obj);
>>>>>> +
>>>>>> + g_free(s->outdev);
>>>>>> + s->outdev = g_strdup(value);
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * called from the main thread on the primary
>>>>>> + * to setup colo-compare.
>>>>>> + */
>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>>> +{
>>>>>> + CompareState *s = COLO_COMPARE(uc);
>>>>>> +
>>>>>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>> + "'secondary_in','outdev' property set");
>>>>>> + return;
>>>>>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>> + !strcmp(s->sec_indev, s->outdev) ||
>>>>>> + !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>> + error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>>> + "for compare module");
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>> + if (s->chr_pri_in == NULL) {
>>>>>> + error_setg(errp, "Primary IN Device '%s' not found",
>>>>>> + s->pri_indev);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>> + if (s->chr_sec_in == NULL) {
>>>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>> + s->sec_indev);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>>>> + if (s->chr_out == NULL) {
>>>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>> + compare_pri_chr_in, NULL, s);
>>>>>> +
>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>> + compare_sec_chr_in, NULL, s);
>>>>>> +
>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>> would
>>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>>> synchronizations?
>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>> I mean we probably want to handle pri_in and sec_in in colo compare
>> thread. Through this way, there's no need for extra synchronization with
>> main loop.
>
> I get your point, but how to do this.
> Now, we use qemu_chr_add_handlers to do this job.
You probably want to start a new main loop in colo comparing thread.
>
>
> Thanks
> zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-29 2:03 ` Jason Wang
@ 2016-04-29 2:08 ` Zhang Chen
2016-05-06 5:42 ` Zhang Chen
1 sibling, 0 replies; 39+ messages in thread
From: Zhang Chen @ 2016-04-29 2:08 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 04/29/2016 10:03 AM, Jason Wang wrote:
>
> On 04/28/2016 05:04 PM, Zhang Chen wrote:
>>
>> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>>> Error **errp)
>>>>>>> +{
>>>>>>> + CompareState *s = COLO_COMPARE(obj);
>>>>>>> +
>>>>>>> + g_free(s->outdev);
>>>>>>> + s->outdev = g_strdup(value);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * called from the main thread on the primary
>>>>>>> + * to setup colo-compare.
>>>>>>> + */
>>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>>>> +{
>>>>>>> + CompareState *s = COLO_COMPARE(uc);
>>>>>>> +
>>>>>>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>>> + "'secondary_in','outdev' property set");
>>>>>>> + return;
>>>>>>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>>> + !strcmp(s->sec_indev, s->outdev) ||
>>>>>>> + !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>>> + error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>>>> + "for compare module");
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>>> + if (s->chr_pri_in == NULL) {
>>>>>>> + error_setg(errp, "Primary IN Device '%s' not found",
>>>>>>> + s->pri_indev);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>>> + if (s->chr_sec_in == NULL) {
>>>>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>>> + s->sec_indev);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>>>>> + if (s->chr_out == NULL) {
>>>>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>>> + compare_pri_chr_in, NULL, s);
>>>>>>> +
>>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>>> + compare_sec_chr_in, NULL, s);
>>>>>>> +
>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>> would
>>>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>>>> synchronizations?
>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>> thread. Through this way, there's no need for extra synchronization with
>>> main loop.
>> I get your point, but how to do this.
>> Now, we use qemu_chr_add_handlers to do this job.
> You probably want to start a new main loop in colo comparing thread.
Get it~ I will fix it in next version~
Thanks~~
Zhang Chen
>
>>
>> Thanks
>> zhangchen
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-29 2:03 ` Jason Wang
2016-04-29 2:08 ` Zhang Chen
@ 2016-05-06 5:42 ` Zhang Chen
2016-05-06 6:37 ` Jason Wang
1 sibling, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-05-06 5:42 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 04/29/2016 10:03 AM, Jason Wang wrote:
>
> On 04/28/2016 05:04 PM, Zhang Chen wrote:
>>
>> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>>> Error **errp)
>>>>>>> +{
>>>>>>> + CompareState *s = COLO_COMPARE(obj);
>>>>>>> +
>>>>>>> + g_free(s->outdev);
>>>>>>> + s->outdev = g_strdup(value);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * called from the main thread on the primary
>>>>>>> + * to setup colo-compare.
>>>>>>> + */
>>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>>>> +{
>>>>>>> + CompareState *s = COLO_COMPARE(uc);
>>>>>>> +
>>>>>>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>>> + "'secondary_in','outdev' property set");
>>>>>>> + return;
>>>>>>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>>> + !strcmp(s->sec_indev, s->outdev) ||
>>>>>>> + !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>>> + error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>>>> + "for compare module");
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>>> + if (s->chr_pri_in == NULL) {
>>>>>>> + error_setg(errp, "Primary IN Device '%s' not found",
>>>>>>> + s->pri_indev);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>>> + if (s->chr_sec_in == NULL) {
>>>>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>>> + s->sec_indev);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>>>>> + if (s->chr_out == NULL) {
>>>>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>>> + compare_pri_chr_in, NULL, s);
>>>>>>> +
>>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>>> + compare_sec_chr_in, NULL, s);
>>>>>>> +
>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>> would
>>>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>>>> synchronizations?
>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>> thread. Through this way, there's no need for extra synchronization with
>>> main loop.
>> I get your point, but how to do this.
>> Now, we use qemu_chr_add_handlers to do this job.
> You probably want to start a new main loop in colo comparing thread.
IIUC, do you mean
- remove char device read_handler
↓at colo comparing thread↓
while (true) {
- blocking read packet from char device with select(2)/poll(2)...
- compare packet
}
This solution will lead comparing packet and reading packet in serial.
But i don't know if this will have a good performance.
>>
>> Thanks
>> zhangchen
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-06 5:42 ` Zhang Chen
@ 2016-05-06 6:37 ` Jason Wang
2016-05-09 10:49 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-05-06 6:37 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 05/06/2016 01:42 PM, Zhang Chen wrote:
>
>
> On 04/29/2016 10:03 AM, Jason Wang wrote:
>>
>> On 04/28/2016 05:04 PM, Zhang Chen wrote:
>>>
>>> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>>>> Error **errp)
>>>>>>>> +{
>>>>>>>> + CompareState *s = COLO_COMPARE(obj);
>>>>>>>> +
>>>>>>>> + g_free(s->outdev);
>>>>>>>> + s->outdev = g_strdup(value);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * called from the main thread on the primary
>>>>>>>> + * to setup colo-compare.
>>>>>>>> + */
>>>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error
>>>>>>>> **errp)
>>>>>>>> +{
>>>>>>>> + CompareState *s = COLO_COMPARE(uc);
>>>>>>>> +
>>>>>>>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>>>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>>>> + "'secondary_in','outdev' property set");
>>>>>>>> + return;
>>>>>>>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>>>> + !strcmp(s->sec_indev, s->outdev) ||
>>>>>>>> + !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>>>> + error_setg(errp, "'indev' and 'outdev' could not be
>>>>>>>> same "
>>>>>>>> + "for compare module");
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>>>> + if (s->chr_pri_in == NULL) {
>>>>>>>> + error_setg(errp, "Primary IN Device '%s' not found",
>>>>>>>> + s->pri_indev);
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>>>> + if (s->chr_sec_in == NULL) {
>>>>>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>>>> + s->sec_indev);
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>>>>>> + if (s->chr_out == NULL) {
>>>>>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>>>> + compare_pri_chr_in, NULL, s);
>>>>>>>> +
>>>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>>>> + compare_sec_chr_in, NULL, s);
>>>>>>>> +
>>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>>> would
>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>> extra
>>>>>> synchronizations?
>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>> thread. Through this way, there's no need for extra synchronization
>>>> with
>>>> main loop.
>>> I get your point, but how to do this.
>>> Now, we use qemu_chr_add_handlers to do this job.
>> You probably want to start a new main loop in colo comparing thread.
>
>
> IIUC, do you mean
> - remove char device read_handler
>
> ↓at colo comparing thread↓
> while (true) {
> - blocking read packet from char device with select(2)/poll(2)...
> - compare packet
> }
Yes, something like this.
>
> This solution will lead comparing packet and reading packet in serial.
> But i don't know if this will have a good performance.
This probably won't have the best performance but it simplify lots of
things. Actually doing it in main loop will slow down all other I/O
processing. Consider colo can only handling userspace network traffic
now, we could start from this. For performance, it needs lots of other
stuff: I think the most important thing is to add vhost support.
Thanks
>
>>>
>>> Thanks
>>> zhangchen
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-06 6:37 ` Jason Wang
@ 2016-05-09 10:49 ` Zhang Chen
2016-05-12 6:49 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-05-09 10:49 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
> +
> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
> + if (s->chr_sec_in == NULL) {
> + error_setg(errp, "Secondary IN Device '%s' not found",
> + s->sec_indev);
> + return;
> + }
> +
> + s->chr_out = qemu_chr_find(s->outdev);
> + if (s->chr_out == NULL) {
> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
> + return;
> + }
> +
> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> + compare_pri_chr_in, NULL, s);
> +
> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> + compare_sec_chr_in, NULL, s);
> +
>>>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>>>> would
>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>> extra
>>>>>>> synchronizations?
>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>>> thread. Through this way, there's no need for extra synchronization
>>>>> with
>>>>> main loop.
>>>> I get your point, but how to do this.
>>>> Now, we use qemu_chr_add_handlers to do this job.
>>> You probably want to start a new main loop in colo comparing thread.
>>
>> IIUC, do you mean
>> - remove char device read_handler
>>
>> ↓at colo comparing thread↓
>> while (true) {
>> - blocking read packet from char device with select(2)/poll(2)...
>> - compare packet
>> }
> Yes, something like this.
>
But remove qemu_chr_add_handlers I can't get fd to select/poll.
How to get fd from all kinds of chardev?
Thanks
Zhang Chen
>> This solution will lead comparing packet and reading packet in serial.
>> But i don't know if this will have a good performance.
> This probably won't have the best performance but it simplify lots of
> things. Actually doing it in main loop will slow down all other I/O
> processing. Consider colo can only handling userspace network traffic
> now, we could start from this. For performance, it needs lots of other
> stuff: I think the most important thing is to add vhost support.
>
> Thanks
>
>>>> Thanks
>>>> zhangchen
>>>
>>> .
>>>
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-09 10:49 ` Zhang Chen
@ 2016-05-12 6:49 ` Zhang Chen
2016-05-12 8:01 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-05-12 6:49 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 05/09/2016 06:49 PM, Zhang Chen wrote:
>
>> +
>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>> + if (s->chr_sec_in == NULL) {
>> + error_setg(errp, "Secondary IN Device '%s' not found",
>> + s->sec_indev);
>> + return;
>> + }
>> +
>> + s->chr_out = qemu_chr_find(s->outdev);
>> + if (s->chr_out == NULL) {
>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>> + return;
>> + }
>> +
>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>> + compare_pri_chr_in, NULL, s);
>> +
>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>> + compare_sec_chr_in, NULL, s);
>> +
>>>>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>>>>> would
>>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>>> extra
>>>>>>>> synchronizations?
>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>>>> thread. Through this way, there's no need for extra synchronization
>>>>>> with
>>>>>> main loop.
>>>>> I get your point, but how to do this.
>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>> You probably want to start a new main loop in colo comparing thread.
>>>
>>> IIUC, do you mean
>>> - remove char device read_handler
>>>
>>> ↓at colo comparing thread↓
>>> while (true) {
>>> - blocking read packet from char device with select(2)/poll(2)...
>>> - compare packet
>>> }
>> Yes, something like this.
>>
>
> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>
> How to get fd from all kinds of chardev?
>
Hi~ jason.
If we use chardev socket the fd save in QIOChannelSocket.
and if we use chardev file the fd save in QIOChannelFile.
Have any common method to get fd?
> Thanks
> Zhang Chen
>
>>> This solution will lead comparing packet and reading packet in serial.
>>> But i don't know if this will have a good performance.
>> This probably won't have the best performance but it simplify lots of
>> things. Actually doing it in main loop will slow down all other I/O
>> processing. Consider colo can only handling userspace network traffic
>> now, we could start from this. For performance, it needs lots of other
>> stuff: I think the most important thing is to add vhost support.
>>
>> Thanks
>>
>>>>> Thanks
>>>>> zhangchen
>>>>
>>>> .
>>>>
>>
>>
>> .
>>
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-12 6:49 ` Zhang Chen
@ 2016-05-12 8:01 ` Jason Wang
2016-05-12 8:16 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-05-12 8:01 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 2016年05月12日 14:49, Zhang Chen wrote:
>
>
> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>
>>> +
>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>> + if (s->chr_sec_in == NULL) {
>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>> + s->sec_indev);
>>> + return;
>>> + }
>>> +
>>> + s->chr_out = qemu_chr_find(s->outdev);
>>> + if (s->chr_out == NULL) {
>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>> + return;
>>> + }
>>> +
>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>> + compare_pri_chr_in, NULL, s);
>>> +
>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>> + compare_sec_chr_in, NULL, s);
>>> +
>>>>>>>>> Btw, what's the reason of handling this in main loop? I
>>>>>>>>> thought it
>>>>>>>>> would
>>>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>>>> extra
>>>>>>>>> synchronizations?
>>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>>>>> thread. Through this way, there's no need for extra synchronization
>>>>>>> with
>>>>>>> main loop.
>>>>>> I get your point, but how to do this.
>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>> You probably want to start a new main loop in colo comparing thread.
>>>>
>>>> IIUC, do you mean
>>>> - remove char device read_handler
>>>>
>>>> ↓at colo comparing thread↓
>>>> while (true) {
>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>> - compare packet
>>>> }
>>> Yes, something like this.
>>>
>>
>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>
>> How to get fd from all kinds of chardev?
>>
>
> Hi~ jason.
>
> If we use chardev socket the fd save in QIOChannelSocket.
>
> and if we use chardev file the fd save in QIOChannelFile.
>
> Have any common method to get fd?
I'm not sure I get the question. But you probably can call
qemu_chr_add_handlers() in colo comparing thread to solve this I think?
>
>> Thanks
>> Zhang Chen
>>
>>>> This solution will lead comparing packet and reading packet in serial.
>>>> But i don't know if this will have a good performance.
>>> This probably won't have the best performance but it simplify lots of
>>> things. Actually doing it in main loop will slow down all other I/O
>>> processing. Consider colo can only handling userspace network traffic
>>> now, we could start from this. For performance, it needs lots of other
>>> stuff: I think the most important thing is to add vhost support.
>>>
>>> Thanks
>>>
>>>>>> Thanks
>>>>>> zhangchen
>>>>>
>>>>> .
>>>>>
>>>
>>>
>>> .
>>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-12 8:01 ` Jason Wang
@ 2016-05-12 8:16 ` Zhang Chen
2016-05-13 3:48 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-05-12 8:16 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang
On 05/12/2016 04:01 PM, Jason Wang wrote:
>
>
> On 2016年05月12日 14:49, Zhang Chen wrote:
>>
>>
>> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>>
>>>> +
>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>> + if (s->chr_sec_in == NULL) {
>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>> + s->sec_indev);
>>>> + return;
>>>> + }
>>>> +
>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>> + if (s->chr_out == NULL) {
>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>> + return;
>>>> + }
>>>> +
>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>> + compare_pri_chr_in, NULL, s);
>>>> +
>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>> + compare_sec_chr_in, NULL, s);
>>>> +
>>>>>>>>>> Btw, what's the reason of handling this in main loop? I
>>>>>>>>>> thought it
>>>>>>>>>> would
>>>>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>>>>> extra
>>>>>>>>>> synchronizations?
>>>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>>>> I mean we probably want to handle pri_in and sec_in in colo
>>>>>>>> compare
>>>>>>>> thread. Through this way, there's no need for extra
>>>>>>>> synchronization
>>>>>>>> with
>>>>>>>> main loop.
>>>>>>> I get your point, but how to do this.
>>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>>> You probably want to start a new main loop in colo comparing thread.
>>>>>
>>>>> IIUC, do you mean
>>>>> - remove char device read_handler
>>>>>
>>>>> ↓at colo comparing thread↓
>>>>> while (true) {
>>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>>> - compare packet
>>>>> }
>>>> Yes, something like this.
>>>>
>>>
>>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>>
>>> How to get fd from all kinds of chardev?
>>>
>>
>> Hi~ jason.
>>
>> If we use chardev socket the fd save in QIOChannelSocket.
>>
>> and if we use chardev file the fd save in QIOChannelFile.
>>
>> Have any common method to get fd?
>
> I'm not sure I get the question. But you probably can call
> qemu_chr_add_handlers() in colo comparing thread to solve this I think?
>
I have tested call qemu_chr_add_handlers() in colo comparing thread, but
when data come,
the handler always running in main loop.
Thanks
Zhang Chen
>>
>>> Thanks
>>> Zhang Chen
>>>
>>>>> This solution will lead comparing packet and reading packet in
>>>>> serial.
>>>>> But i don't know if this will have a good performance.
>>>> This probably won't have the best performance but it simplify lots of
>>>> things. Actually doing it in main loop will slow down all other I/O
>>>> processing. Consider colo can only handling userspace network traffic
>>>> now, we could start from this. For performance, it needs lots of other
>>>> stuff: I think the most important thing is to add vhost support.
>>>>
>>>> Thanks
>>>>
>>>>>>> Thanks
>>>>>>> zhangchen
>>>>>>
>>>>>> .
>>>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-12 8:16 ` Zhang Chen
@ 2016-05-13 3:48 ` Jason Wang
2016-05-20 2:46 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-05-13 3:48 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Yang Hongyang, Amit Shah
On 2016年05月12日 16:16, Zhang Chen wrote:
>
>
> On 05/12/2016 04:01 PM, Jason Wang wrote:
>>
>>
>> On 2016年05月12日 14:49, Zhang Chen wrote:
>>>
>>>
>>> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>>>
>>>>> +
>>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>> + if (s->chr_sec_in == NULL) {
>>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>>> + s->sec_indev);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>>> + if (s->chr_out == NULL) {
>>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>> + compare_pri_chr_in, NULL, s);
>>>>> +
>>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>> + compare_sec_chr_in, NULL, s);
>>>>> +
>>>>>>>>>>> Btw, what's the reason of handling this in main loop? I
>>>>>>>>>>> thought it
>>>>>>>>>>> would
>>>>>>>>>>> be better to do this in colo thread? Otherwise, you need
>>>>>>>>>>> lots of
>>>>>>>>>>> extra
>>>>>>>>>>> synchronizations?
>>>>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>>>>> I mean we probably want to handle pri_in and sec_in in colo
>>>>>>>>> compare
>>>>>>>>> thread. Through this way, there's no need for extra
>>>>>>>>> synchronization
>>>>>>>>> with
>>>>>>>>> main loop.
>>>>>>>> I get your point, but how to do this.
>>>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>>>> You probably want to start a new main loop in colo comparing
>>>>>>> thread.
>>>>>>
>>>>>> IIUC, do you mean
>>>>>> - remove char device read_handler
>>>>>>
>>>>>> ↓at colo comparing thread↓
>>>>>> while (true) {
>>>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>>>> - compare packet
>>>>>> }
>>>>> Yes, something like this.
>>>>>
>>>>
>>>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>>>
>>>> How to get fd from all kinds of chardev?
>>>>
>>>
>>> Hi~ jason.
>>>
>>> If we use chardev socket the fd save in QIOChannelSocket.
>>>
>>> and if we use chardev file the fd save in QIOChannelFile.
>>>
>>> Have any common method to get fd?
>>
>> I'm not sure I get the question. But you probably can call
>> qemu_chr_add_handlers() in colo comparing thread to solve this I think?
>>
>
> I have tested call qemu_chr_add_handlers() in colo comparing thread,
> but when data come,
> the handler always running in main loop.
>
> Thanks
> Zhang Chen
Cc Amit for the help.
Amit, we want to poll and handle chardev in another thread other than
main loop. But looks like qemu_chr_add_handlers() can only work for
default context other than thread default context. Any other solution
for this?
Thanks
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-13 3:48 ` Jason Wang
@ 2016-05-20 2:46 ` Jason Wang
2016-05-20 6:52 ` Fam Zheng
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-05-20 2:46 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: zhanghailiang, Li Zhijian, Gui jianfeng, eddie.dong,
Dr. David Alan Gilbert, Amit Shah, Yang Hongyang, famz
On 2016年05月13日 11:48, Jason Wang wrote:
>
>
> On 2016年05月12日 16:16, Zhang Chen wrote:
>>
>>
>> On 05/12/2016 04:01 PM, Jason Wang wrote:
>>>
>>>
>>> On 2016年05月12日 14:49, Zhang Chen wrote:
>>>>
>>>>
>>>> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>>>>
>>>>>> +
>>>>>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>> + if (s->chr_sec_in == NULL) {
>>>>>> + error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>> + s->sec_indev);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + s->chr_out = qemu_chr_find(s->outdev);
>>>>>> + if (s->chr_out == NULL) {
>>>>>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>> + compare_pri_chr_in, NULL, s);
>>>>>> +
>>>>>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>> + compare_sec_chr_in, NULL, s);
>>>>>> +
>>>>>>>>>>>> Btw, what's the reason of handling this in main loop? I
>>>>>>>>>>>> thought it
>>>>>>>>>>>> would
>>>>>>>>>>>> be better to do this in colo thread? Otherwise, you need
>>>>>>>>>>>> lots of
>>>>>>>>>>>> extra
>>>>>>>>>>>> synchronizations?
>>>>>>>>>>> Do you mean we should start/stop/do checkpoint it by
>>>>>>>>>>> colo-frame?
>>>>>>>>>> I mean we probably want to handle pri_in and sec_in in colo
>>>>>>>>>> compare
>>>>>>>>>> thread. Through this way, there's no need for extra
>>>>>>>>>> synchronization
>>>>>>>>>> with
>>>>>>>>>> main loop.
>>>>>>>>> I get your point, but how to do this.
>>>>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>>>>> You probably want to start a new main loop in colo comparing
>>>>>>>> thread.
>>>>>>>
>>>>>>> IIUC, do you mean
>>>>>>> - remove char device read_handler
>>>>>>>
>>>>>>> ↓at colo comparing thread↓
>>>>>>> while (true) {
>>>>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>>>>> - compare packet
>>>>>>> }
>>>>>> Yes, something like this.
>>>>>>
>>>>>
>>>>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>>>>
>>>>> How to get fd from all kinds of chardev?
>>>>>
>>>>
>>>> Hi~ jason.
>>>>
>>>> If we use chardev socket the fd save in QIOChannelSocket.
>>>>
>>>> and if we use chardev file the fd save in QIOChannelFile.
>>>>
>>>> Have any common method to get fd?
>>>
>>> I'm not sure I get the question. But you probably can call
>>> qemu_chr_add_handlers() in colo comparing thread to solve this I think?
>>>
>>
>> I have tested call qemu_chr_add_handlers() in colo comparing thread,
>> but when data come,
>> the handler always running in main loop.
>>
>> Thanks
>> Zhang Chen
>
> Cc Amit for the help.
>
> Amit, we want to poll and handle chardev in another thread other than
> main loop. But looks like qemu_chr_add_handlers() can only work for
> default context other than thread default context. Any other solution
> for this?
>
> Thanks
>
Cc Fam for more thought.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-05-20 2:46 ` Jason Wang
@ 2016-05-20 6:52 ` Fam Zheng
0 siblings, 0 replies; 39+ messages in thread
From: Fam Zheng @ 2016-05-20 6:52 UTC (permalink / raw)
To: Jason Wang
Cc: Zhang Chen, qemu devel, zhanghailiang, Li Zhijian, Gui jianfeng,
eddie.dong, Dr. David Alan Gilbert, Amit Shah, Yang Hongyang
On Fri, 05/20 10:46, Jason Wang wrote:
>
>
> On 2016年05月13日 11:48, Jason Wang wrote:
> >
> >
> > On 2016年05月12日 16:16, Zhang Chen wrote:
> > >
> > >
> > > On 05/12/2016 04:01 PM, Jason Wang wrote:
> > > >
> > > >
> > > > On 2016年05月12日 14:49, Zhang Chen wrote:
> > > > >
> > > > >
> > > > > On 05/09/2016 06:49 PM, Zhang Chen wrote:
> > > > > >
> > > > > > > +
> > > > > > > + s->chr_sec_in = qemu_chr_find(s->sec_indev);
> > > > > > > + if (s->chr_sec_in == NULL) {
> > > > > > > + error_setg(errp, "Secondary IN Device '%s' not found",
> > > > > > > + s->sec_indev);
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + s->chr_out = qemu_chr_find(s->outdev);
> > > > > > > + if (s->chr_out == NULL) {
> > > > > > > + error_setg(errp, "OUT Device '%s' not found", s->outdev);
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> > > > > > > + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> > > > > > > + compare_pri_chr_in, NULL, s);
> > > > > > > +
> > > > > > > + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> > > > > > > + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> > > > > > > + compare_sec_chr_in, NULL, s);
> > > > > > > +
> > > > > > > > > > > > > Btw, what's the reason of
> > > > > > > > > > > > > handling this in main loop?
> > > > > > > > > > > > > I thought it
> > > > > > > > > > > > > would
> > > > > > > > > > > > > be better to do this in colo
> > > > > > > > > > > > > thread? Otherwise, you need
> > > > > > > > > > > > > lots of
> > > > > > > > > > > > > extra
> > > > > > > > > > > > > synchronizations?
> > > > > > > > > > > > Do you mean we should
> > > > > > > > > > > > start/stop/do checkpoint it by
> > > > > > > > > > > > colo-frame?
> > > > > > > > > > > I mean we probably want to handle
> > > > > > > > > > > pri_in and sec_in in colo compare
> > > > > > > > > > > thread. Through this way, there's no
> > > > > > > > > > > need for extra synchronization
> > > > > > > > > > > with
> > > > > > > > > > > main loop.
> > > > > > > > > > I get your point, but how to do this.
> > > > > > > > > > Now, we use qemu_chr_add_handlers to do this job.
> > > > > > > > > You probably want to start a new main loop
> > > > > > > > > in colo comparing thread.
> > > > > > > >
> > > > > > > > IIUC, do you mean
> > > > > > > > - remove char device read_handler
> > > > > > > >
> > > > > > > > ↓at colo comparing thread↓
> > > > > > > > while (true) {
> > > > > > > > - blocking read packet from char device with select(2)/poll(2)...
> > > > > > > > - compare packet
> > > > > > > > }
> > > > > > > Yes, something like this.
> > > > > > >
> > > > > >
> > > > > > But remove qemu_chr_add_handlers I can't get fd to select/poll.
> > > > > >
> > > > > > How to get fd from all kinds of chardev?
> > > > > >
> > > > >
> > > > > Hi~ jason.
> > > > >
> > > > > If we use chardev socket the fd save in QIOChannelSocket.
> > > > >
> > > > > and if we use chardev file the fd save in QIOChannelFile.
> > > > >
> > > > > Have any common method to get fd?
> > > >
> > > > I'm not sure I get the question. But you probably can call
> > > > qemu_chr_add_handlers() in colo comparing thread to solve this I
> > > > think?
> > > >
> > >
> > > I have tested call qemu_chr_add_handlers() in colo comparing thread,
> > > but when data come,
> > > the handler always running in main loop.
> > >
> > > Thanks
> > > Zhang Chen
> >
> > Cc Amit for the help.
> >
> > Amit, we want to poll and handle chardev in another thread other than
> > main loop. But looks like qemu_chr_add_handlers() can only work for
> > default context other than thread default context. Any other solution
> > for this?
> >
> > Thanks
> >
>
> Cc Fam for more thought.
>
Unfortunately QIOChannel in chardev uses GSource, so there is no easy way to
move that to another thread, at least I don't think any code in QEMU has ever
tried.
One possibility is in the colo compare thread, call
g_main_context_push_thread_default() before setting up the chr handler, but I'm
not sure how well that would work.
Fam
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 6:53 ` Jason Wang
2016-04-28 7:16 ` Jason Wang
@ 2016-04-28 7:53 ` Zhang Chen
2016-04-28 8:23 ` Jason Wang
1 sibling, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-28 7:53 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/28/2016 02:53 PM, Jason Wang wrote:
>
> On 04/18/2016 07:11 PM, Zhang Chen wrote:
>> packet come from primary char indev will be send to
>> outdev - packet come from secondary char dev will be drop
>>
>> usage:
>>
>> primary:
>> -netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown
>> -device e1000,id=e0,netdev=hn0,mac=52:a4:00:12:78:66
>> -chardev socket,id=mirror0,host=3.3.3.3,port=9003,server,nowait
>> -chardev socket,id=compare1,host=3.3.3.3,port=9004,server,nowait
>> -chardev socket,id=compare0,host=3.3.3.3,port=9001,server,nowait
>> -chardev socket,id=compare0-0,host=3.3.3.3,port=9001
>> -chardev socket,id=compare_out,host=3.3.3.3,port=9005,server,nowait
>> -chardev socket,id=compare_out0,host=3.3.3.3,port=9005
>> -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0
>> -object filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out
>> -object filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0
>> -object colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0
>>
>> secondary:
>> -netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,down script=/etc/qemu-ifdown
>> -device e1000,netdev=hn0,mac=52:a4:00:12:78:66
>> -chardev socket,id=red0,host=3.3.3.3,port=9003
>> -chardev socket,id=red1,host=3.3.3.3,port=9004
>> -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
>> -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
>>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>> net/Makefile.objs | 1 +
>> net/colo-compare.c | 361 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> qemu-options.hx | 6 +
>> vl.c | 3 +-
>> 4 files changed, 370 insertions(+), 1 deletion(-)
>> create mode 100644 net/colo-compare.c
>>
>> diff --git a/net/Makefile.objs b/net/Makefile.objs
>> index b7c22fd..ba92f73 100644
>> --- a/net/Makefile.objs
>> +++ b/net/Makefile.objs
>> @@ -16,3 +16,4 @@ common-obj-$(CONFIG_NETMAP) += netmap.o
>> common-obj-y += filter.o
>> common-obj-y += filter-buffer.o
>> common-obj-y += filter-mirror.o
>> +common-obj-y += colo-compare.o
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> new file mode 100644
>> index 0000000..c45b132
>> --- /dev/null
>> +++ b/net/colo-compare.c
>> @@ -0,0 +1,361 @@
>> +/*
>> + * Copyright (c) 2016 FUJITSU LIMITED
>> + * Author: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later. See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "qapi/qmp/qerror.h"
>> +#include "qemu/error-report.h"
>> +#include "qapi/error.h"
>> +#include "net/net.h"
>> +#include "net/vhost_net.h"
>> +#include "qom/object_interfaces.h"
>> +#include "qemu/iov.h"
>> +#include "qom/object.h"
>> +#include "qemu/typedefs.h"
>> +#include "net/queue.h"
>> +#include "sysemu/char.h"
>> +#include "qemu/sockets.h"
>> +#include "qapi-visit.h"
>> +#include "trace.h"
>> +
>> +#define TYPE_COLO_COMPARE "colo-compare"
>> +#define COLO_COMPARE(obj) \
>> + OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
>> +
>> +#define COMPARE_READ_LEN_MAX NET_BUFSIZE
>> +
>> +static QTAILQ_HEAD(, CompareState) net_compares =
>> + QTAILQ_HEAD_INITIALIZER(net_compares);
> Don't see any usage of this, if it was used in the following patches,
> better move this to that patch.
Hi~ jason~
Thanks for review~~
will fix it in next version.
>
>> +
>> +typedef struct ReadState {
>> + int state; /* 0 = getting length, 1 = getting data */
>> + uint32_t index;
>> + uint32_t packet_len;
>> + uint8_t buf[COMPARE_READ_LEN_MAX];
>> +} ReadState;
>> +
>> +typedef struct CompareState {
>> + Object parent;
>> +
>> + char *pri_indev;
>> + char *sec_indev;
>> + char *outdev;
>> + CharDriverState *chr_pri_in;
>> + CharDriverState *chr_sec_in;
>> + CharDriverState *chr_out;
>> + QTAILQ_ENTRY(CompareState) next;
>> + ReadState pri_rs;
>> + ReadState sec_rs;
> What did 'rs' short for, ReadState? :)
Yes,should I change the 'rs' to another name?
>
>> +} CompareState;
>> +
>> +typedef struct CompareClass {
>> + ObjectClass parent_class;
>> +} CompareClass;
>> +
>> +static int compare_chr_send(CharDriverState *out,
>> + const uint8_t *buf,
>> + uint32_t size)
>> +{
>> + int ret = 0;
>> + uint32_t len = htonl(size);
>> +
>> + if (!size) {
>> + return 0;
>> + }
>> +
>> + ret = qemu_chr_fe_write_all(out, (uint8_t *)&len, sizeof(len));
>> + if (ret != sizeof(len)) {
>> + goto err;
>> + }
>> +
>> + ret = qemu_chr_fe_write_all(out, (uint8_t *)buf, size);
>> + if (ret != size) {
>> + goto err;
>> + }
>> +
>> + return 0;
>> +
>> +err:
>> + return ret < 0 ? ret : -EIO;
>> +}
>> +
>> +static int compare_chr_can_read(void *opaque)
>> +{
>> + return COMPARE_READ_LEN_MAX;
>> +}
>> +
>> +/*
>> + * called from the main thread on the primary
>> + * for get packets
>> + * Returns
>> + * 0: readstate is not ready
>> + * 1: readstate is ready
>> + * otherwise error occurs
>> + */
>> +static int compare_chr_fill_rstate(ReadState *rs, const uint8_t *buf, int size)
>> +{
>> + unsigned int l;
>> + while (size > 0) {
>> + /* reassemble a packet from the network */
>> + switch (rs->state) { /* 0 = getting length, 1 = getting data */
>> + case 0:
>> + l = 4 - rs->index;
>> + if (l > size) {
>> + l = size;
>> + }
>> + memcpy(rs->buf + rs->index, buf, l);
>> + buf += l;
>> + size -= l;
>> + rs->index += l;
>> + if (rs->index == 4) {
>> + /* got length */
>> + rs->packet_len = ntohl(*(uint32_t *)rs->buf);
>> + rs->index = 0;
>> + rs->state = 1;
>> + }
>> + break;
>> + case 1:
>> + l = rs->packet_len - rs->index;
>> + if (l > size) {
>> + l = size;
>> + }
>> + if (rs->index + l <= sizeof(rs->buf)) {
>> + memcpy(rs->buf + rs->index, buf, l);
>> + } else {
>> + error_report("colo-compare: "
>> + "Received oversized packet over socket");
>> + rs->index = rs->state = 0;
>> + return -1;
>> + }
>> +
>> + rs->index += l;
>> + buf += l;
>> + size -= l;
>> + if (rs->index >= rs->packet_len) {
>> + rs->index = 0;
>> + rs->state = 0;
>> + return 1;
>> + }
>> + break;
>> + }
>> + }
>> + return 0;
>> +}
> This looks rather similar to redirector_chr_read(), any chance to unify
> the code?
Yes,I think we can create 'colo-base.c' and 'colo-base.h'
in net/ to share codes for colo-compare,filter-redirector
and filter-rewriter. how about it?
>
>> +
>> +/*
>> + * called from the main thread on the primary for packets
>> + * arriving over the socket from the primary.
>> + */
>> +static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
>> +{
>> + CompareState *s = COLO_COMPARE(opaque);
>> + int ret;
>> +
>> + ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
>> + if (ret == 1) {
>> + /* FIXME: enqueue to primary packet list */
> Do you mean to use some internal data structure instead of chardev? If
> yes, probably a "TODO", not "FIXME".
Yes,will change to "TODO"
>> + compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
>> + } else if (ret == -1) {
>> + qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> Should we add a warning here?
OK~ I will add error_report()
>
>> + }
>> +}
>> +
>> +/*
>> + * called from the main thread on the primary for packets
>> + * arriving over the socket from the secondary.
>> + */
>> +static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
>> +{
>> + CompareState *s = COLO_COMPARE(opaque);
>> + int ret;
>> +
>> + ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
>> + if (ret == 1) {
>> + /* TODO: enqueue to secondary packet list*/
>> + /* should we send sec arp pkt? */
> What's the problem here?
This comments will be move to patch 2/4
The reason is I send secondary guest's arp and ipv6
packet to ensure it can get mac addr for send other IP
packet for compare.but currently I don't know this job
may be affected to something?
>
>> + compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
>> + } else if (ret == -1) {
>> + qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>> + }
> This function is similar to primary version, may need to unify the codes.
Thanks
zhangchen
>> +}
>> +
>> +static char *compare_get_pri_indev(Object *obj, Error **errp)
>> +{
>> + CompareState *s = COLO_COMPARE(obj);
>> +
>> + return g_strdup(s->pri_indev);
>> +}
>> +
>> +static void compare_set_pri_indev(Object *obj, const char *value, Error **errp)
>> +{
>> + CompareState *s = COLO_COMPARE(obj);
>> +
>> + g_free(s->pri_indev);
>> + s->pri_indev = g_strdup(value);
>> +}
>> +
>> +static char *compare_get_sec_indev(Object *obj, Error **errp)
>> +{
>> + CompareState *s = COLO_COMPARE(obj);
>> +
>> + return g_strdup(s->sec_indev);
>> +}
>> +
>> +static void compare_set_sec_indev(Object *obj, const char *value, Error **errp)
>> +{
>> + CompareState *s = COLO_COMPARE(obj);
>> +
>> + g_free(s->sec_indev);
>> + s->sec_indev = g_strdup(value);
>> +}
>> +
>> +static char *compare_get_outdev(Object *obj, Error **errp)
>> +{
>> + CompareState *s = COLO_COMPARE(obj);
>> +
>> + return g_strdup(s->outdev);
>> +}
>> +
>> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
>> +{
>> + CompareState *s = COLO_COMPARE(obj);
>> +
>> + g_free(s->outdev);
>> + s->outdev = g_strdup(value);
>> +}
>> +
>> +/*
>> + * called from the main thread on the primary
>> + * to setup colo-compare.
>> + */
>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>> +{
>> + CompareState *s = COLO_COMPARE(uc);
>> +
>> + if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>> + error_setg(errp, "colo compare needs 'primary_in' ,"
>> + "'secondary_in','outdev' property set");
>> + return;
>> + } else if (!strcmp(s->pri_indev, s->outdev) ||
>> + !strcmp(s->sec_indev, s->outdev) ||
>> + !strcmp(s->pri_indev, s->sec_indev)) {
>> + error_setg(errp, "'indev' and 'outdev' could not be same "
>> + "for compare module");
>> + return;
>> + }
>> +
>> + s->chr_pri_in = qemu_chr_find(s->pri_indev);
>> + if (s->chr_pri_in == NULL) {
>> + error_setg(errp, "Primary IN Device '%s' not found",
>> + s->pri_indev);
>> + return;
>> + }
>> +
>> + s->chr_sec_in = qemu_chr_find(s->sec_indev);
>> + if (s->chr_sec_in == NULL) {
>> + error_setg(errp, "Secondary IN Device '%s' not found",
>> + s->sec_indev);
>> + return;
>> + }
>> +
>> + s->chr_out = qemu_chr_find(s->outdev);
>> + if (s->chr_out == NULL) {
>> + error_setg(errp, "OUT Device '%s' not found", s->outdev);
>> + return;
>> + }
>> +
>> + qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>> + qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>> + compare_pri_chr_in, NULL, s);
>> +
>> + qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>> + qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>> + compare_sec_chr_in, NULL, s);
>> +
>> + qemu_chr_fe_claim_no_fail(s->chr_out);
>> + QTAILQ_INSERT_TAIL(&net_compares, s, next);
>> +
>> + return;
>> +}
>> +
>> +static void colo_compare_class_init(ObjectClass *oc, void *data)
>> +{
>> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>> +
>> + ucc->complete = colo_compare_complete;
>> +}
>> +
>> +/*
>> + * called from the main thread on the primary
>> + * to cleanup colo-compare.
>> + */
>> +static void colo_compare_class_finalize(ObjectClass *oc, void *data)
>> +{
>> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>> + CompareState *s = COLO_COMPARE(ucc);
>> +
>> + if (s->chr_pri_in) {
>> + qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>> + qemu_chr_fe_release(s->chr_pri_in);
>> + }
>> + if (s->chr_sec_in) {
>> + qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>> + qemu_chr_fe_release(s->chr_sec_in);
>> + }
>> + if (s->chr_out) {
>> + qemu_chr_fe_release(s->chr_out);
>> + }
>> +
>> + if (!QTAILQ_EMPTY(&net_compares)) {
>> + QTAILQ_REMOVE(&net_compares, s, next);
>> + }
>> +}
>> +
>> +static void colo_compare_init(Object *obj)
>> +{
>> + object_property_add_str(obj, "primary_in",
>> + compare_get_pri_indev, compare_set_pri_indev,
>> + NULL);
>> + object_property_add_str(obj, "secondary_in",
>> + compare_get_sec_indev, compare_set_sec_indev,
>> + NULL);
>> + object_property_add_str(obj, "outdev",
>> + compare_get_outdev, compare_set_outdev,
>> + NULL);
>> +}
>> +
>> +static void colo_compare_finalize(Object *obj)
>> +{
>> + CompareState *s = COLO_COMPARE(obj);
>> +
>> + g_free(s->pri_indev);
>> + g_free(s->sec_indev);
>> + g_free(s->outdev);
>> +}
>> +
>> +static const TypeInfo colo_compare_info = {
>> + .name = TYPE_COLO_COMPARE,
>> + .parent = TYPE_OBJECT,
>> + .instance_size = sizeof(CompareState),
>> + .instance_init = colo_compare_init,
>> + .instance_finalize = colo_compare_finalize,
>> + .class_size = sizeof(CompareClass),
>> + .class_init = colo_compare_class_init,
>> + .class_finalize = colo_compare_class_finalize,
>> + .interfaces = (InterfaceInfo[]) {
>> + { TYPE_USER_CREATABLE },
>> + { }
>> + }
>> +};
>> +
>> +static void register_types(void)
>> +{
>> + type_register_static(&colo_compare_info);
>> +}
>> +
>> +type_init(register_types);
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 587de8f..38f58f7 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
>> The file format is libpcap, so it can be analyzed with tools such as tcpdump
>> or Wireshark.
>>
>> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
>> +outdev=@var{chardevid}
>> +
>> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
>> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
>> +
>> @item -object secret,id=@var{id},data=@var{string},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
>> @item -object secret,id=@var{id},file=@var{filename},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
>>
>> diff --git a/vl.c b/vl.c
>> index cbe51ac..c6b9a6f 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2865,7 +2865,8 @@ static bool object_create_initial(const char *type)
>> if (g_str_equal(type, "filter-buffer") ||
>> g_str_equal(type, "filter-dump") ||
>> g_str_equal(type, "filter-mirror") ||
>> - g_str_equal(type, "filter-redirector")) {
>> + g_str_equal(type, "filter-redirector") ||
>> + g_str_equal(type, "colo-compare")) {
>> return false;
>> }
>>
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 7:53 ` Zhang Chen
@ 2016-04-28 8:23 ` Jason Wang
0 siblings, 0 replies; 39+ messages in thread
From: Jason Wang @ 2016-04-28 8:23 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, eddie.dong, zhanghailiang,
Dr. David Alan Gilbert, Yang Hongyang
On 04/28/2016 03:53 PM, Zhang Chen wrote:
>
>
> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>
>> On 04/18/2016 07:11 PM, Zhang Chen wrote:
>>> packet come from primary char indev will be send to
>>> outdev - packet come from secondary char dev will be drop
>>>
>>> usage:
>>>
>>> primary:
>>> -netdev
>>> tap,id=hn0,vhost=off,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown
>>> -device e1000,id=e0,netdev=hn0,mac=52:a4:00:12:78:66
>>> -chardev socket,id=mirror0,host=3.3.3.3,port=9003,server,nowait
>>> -chardev socket,id=compare1,host=3.3.3.3,port=9004,server,nowait
>>> -chardev socket,id=compare0,host=3.3.3.3,port=9001,server,nowait
>>> -chardev socket,id=compare0-0,host=3.3.3.3,port=9001
>>> -chardev socket,id=compare_out,host=3.3.3.3,port=9005,server,nowait
>>> -chardev socket,id=compare_out0,host=3.3.3.3,port=9005
>>> -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0
>>> -object
>>> filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out
>>> -object
>>> filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0
>>> -object
>>> colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0
>>>
>>> secondary:
>>> -netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,down
>>> script=/etc/qemu-ifdown
>>> -device e1000,netdev=hn0,mac=52:a4:00:12:78:66
>>> -chardev socket,id=red0,host=3.3.3.3,port=9003
>>> -chardev socket,id=red1,host=3.3.3.3,port=9004
>>> -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
>>> -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
>>>
>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> ---
>>> net/Makefile.objs | 1 +
>>> net/colo-compare.c | 361
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> qemu-options.hx | 6 +
>>> vl.c | 3 +-
>>> 4 files changed, 370 insertions(+), 1 deletion(-)
>>> create mode 100644 net/colo-compare.c
>>>
>>> diff --git a/net/Makefile.objs b/net/Makefile.objs
>>> index b7c22fd..ba92f73 100644
>>> --- a/net/Makefile.objs
>>> +++ b/net/Makefile.objs
>>> @@ -16,3 +16,4 @@ common-obj-$(CONFIG_NETMAP) += netmap.o
>>> common-obj-y += filter.o
>>> common-obj-y += filter-buffer.o
>>> common-obj-y += filter-mirror.o
>>> +common-obj-y += colo-compare.o
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> new file mode 100644
>>> index 0000000..c45b132
>>> --- /dev/null
>>> +++ b/net/colo-compare.c
>>> @@ -0,0 +1,361 @@
>>> +/*
>>> + * Copyright (c) 2016 FUJITSU LIMITED
>>> + * Author: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> + * later. See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu-common.h"
>>> +#include "qapi/qmp/qerror.h"
>>> +#include "qemu/error-report.h"
>>> +#include "qapi/error.h"
>>> +#include "net/net.h"
>>> +#include "net/vhost_net.h"
>>> +#include "qom/object_interfaces.h"
>>> +#include "qemu/iov.h"
>>> +#include "qom/object.h"
>>> +#include "qemu/typedefs.h"
>>> +#include "net/queue.h"
>>> +#include "sysemu/char.h"
>>> +#include "qemu/sockets.h"
>>> +#include "qapi-visit.h"
>>> +#include "trace.h"
>>> +
>>> +#define TYPE_COLO_COMPARE "colo-compare"
>>> +#define COLO_COMPARE(obj) \
>>> + OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
>>> +
>>> +#define COMPARE_READ_LEN_MAX NET_BUFSIZE
>>> +
>>> +static QTAILQ_HEAD(, CompareState) net_compares =
>>> + QTAILQ_HEAD_INITIALIZER(net_compares);
>> Don't see any usage of this, if it was used in the following patches,
>> better move this to that patch.
>
> Hi~ jason~
>
> Thanks for review~~
>
> will fix it in next version.
>
>>
>>> +
>>> +typedef struct ReadState {
>>> + int state; /* 0 = getting length, 1 = getting data */
>>> + uint32_t index;
>>> + uint32_t packet_len;
>>> + uint8_t buf[COMPARE_READ_LEN_MAX];
>>> +} ReadState;
>>> +
>>> +typedef struct CompareState {
>>> + Object parent;
>>> +
>>> + char *pri_indev;
>>> + char *sec_indev;
>>> + char *outdev;
>>> + CharDriverState *chr_pri_in;
>>> + CharDriverState *chr_sec_in;
>>> + CharDriverState *chr_out;
>>> + QTAILQ_ENTRY(CompareState) next;
>>> + ReadState pri_rs;
>>> + ReadState sec_rs;
>> What did 'rs' short for, ReadState? :)
>
> Yes,should I change the 'rs' to another name?
Probably not.
>
>>
>>> +} CompareState;
>>> +
>>> +typedef struct CompareClass {
>>> + ObjectClass parent_class;
>>> +} CompareClass;
>>> +
>>> +static int compare_chr_send(CharDriverState *out,
>>> + const uint8_t *buf,
>>> + uint32_t size)
>>> +{
>>> + int ret = 0;
>>> + uint32_t len = htonl(size);
>>> +
>>> + if (!size) {
>>> + return 0;
>>> + }
>>> +
>>> + ret = qemu_chr_fe_write_all(out, (uint8_t *)&len, sizeof(len));
>>> + if (ret != sizeof(len)) {
>>> + goto err;
>>> + }
>>> +
>>> + ret = qemu_chr_fe_write_all(out, (uint8_t *)buf, size);
>>> + if (ret != size) {
>>> + goto err;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +err:
>>> + return ret < 0 ? ret : -EIO;
>>> +}
>>> +
>>> +static int compare_chr_can_read(void *opaque)
>>> +{
>>> + return COMPARE_READ_LEN_MAX;
>>> +}
>>> +
>>> +/*
>>> + * called from the main thread on the primary
>>> + * for get packets
>>> + * Returns
>>> + * 0: readstate is not ready
>>> + * 1: readstate is ready
>>> + * otherwise error occurs
>>> + */
>>> +static int compare_chr_fill_rstate(ReadState *rs, const uint8_t
>>> *buf, int size)
>>> +{
>>> + unsigned int l;
>>> + while (size > 0) {
>>> + /* reassemble a packet from the network */
>>> + switch (rs->state) { /* 0 = getting length, 1 = getting
>>> data */
>>> + case 0:
>>> + l = 4 - rs->index;
>>> + if (l > size) {
>>> + l = size;
>>> + }
>>> + memcpy(rs->buf + rs->index, buf, l);
>>> + buf += l;
>>> + size -= l;
>>> + rs->index += l;
>>> + if (rs->index == 4) {
>>> + /* got length */
>>> + rs->packet_len = ntohl(*(uint32_t *)rs->buf);
>>> + rs->index = 0;
>>> + rs->state = 1;
>>> + }
>>> + break;
>>> + case 1:
>>> + l = rs->packet_len - rs->index;
>>> + if (l > size) {
>>> + l = size;
>>> + }
>>> + if (rs->index + l <= sizeof(rs->buf)) {
>>> + memcpy(rs->buf + rs->index, buf, l);
>>> + } else {
>>> + error_report("colo-compare: "
>>> + "Received oversized packet over socket");
>>> + rs->index = rs->state = 0;
>>> + return -1;
>>> + }
>>> +
>>> + rs->index += l;
>>> + buf += l;
>>> + size -= l;
>>> + if (rs->index >= rs->packet_len) {
>>> + rs->index = 0;
>>> + rs->state = 0;
>>> + return 1;
>>> + }
>>> + break;
>>> + }
>>> + }
>>> + return 0;
>>> +}
>> This looks rather similar to redirector_chr_read(), any chance to unify
>> the code?
>
> Yes,I think we can create 'colo-base.c' and 'colo-base.h'
> in net/ to share codes for colo-compare,filter-redirector
> and filter-rewriter. how about it?
You can, but need a generic name since it would be used by redirector
too. Looking at net/socket.c, I wonder whether or not we could reuse
NetSocketState directly.
>
>
>>
>>> +
>>> +/*
>>> + * called from the main thread on the primary for packets
>>> + * arriving over the socket from the primary.
>>> + */
>>> +static void compare_pri_chr_in(void *opaque, const uint8_t *buf,
>>> int size)
>>> +{
>>> + CompareState *s = COLO_COMPARE(opaque);
>>> + int ret;
>>> +
>>> + ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
>>> + if (ret == 1) {
>>> + /* FIXME: enqueue to primary packet list */
>> Do you mean to use some internal data structure instead of chardev? If
>> yes, probably a "TODO", not "FIXME".
>
> Yes,will change to "TODO"
>
>>> + compare_chr_send(s->chr_out, s->pri_rs.buf,
>>> s->pri_rs.packet_len);
>>> + } else if (ret == -1) {
>>> + qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>> Should we add a warning here?
>
> OK~ I will add error_report()
>
>>
>>> + }
>>> +}
>>> +
>>> +/*
>>> + * called from the main thread on the primary for packets
>>> + * arriving over the socket from the secondary.
>>> + */
>>> +static void compare_sec_chr_in(void *opaque, const uint8_t *buf,
>>> int size)
>>> +{
>>> + CompareState *s = COLO_COMPARE(opaque);
>>> + int ret;
>>> +
>>> + ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
>>> + if (ret == 1) {
>>> + /* TODO: enqueue to secondary packet list*/
>>> + /* should we send sec arp pkt? */
>> What's the problem here?
>
> This comments will be move to patch 2/4
>
>
> The reason is I send secondary guest's arp and ipv6
> packet to ensure it can get mac addr for send other IP
> packet for compare.but currently I don't know this job
> may be affected to something?
Still not clear, I thought we don't need to do any trick for arp to work?
>
>>
>>> + compare_chr_send(s->chr_out, s->sec_rs.buf,
>>> s->sec_rs.packet_len);
>>> + } else if (ret == -1) {
>>> + qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>>> + }
>> This function is similar to primary version, may need to unify the
>> codes.
>
> Thanks
> zhangchen
[...]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization Zhang Chen
2016-04-28 6:53 ` Jason Wang
@ 2016-04-28 20:55 ` Eric Blake
2016-04-29 1:28 ` Zhang Chen
1 sibling, 1 reply; 39+ messages in thread
From: Eric Blake @ 2016-04-28 20:55 UTC (permalink / raw)
To: Zhang Chen, qemu devel, Jason Wang
Cc: Li Zhijian, Gui jianfeng, eddie.dong, zhanghailiang,
Dr. David Alan Gilbert, Yang Hongyang
[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]
On 04/18/2016 05:11 AM, Zhang Chen wrote:
s/initlization/initialization/ in the subject
> packet come from primary char indev will be send to
> outdev - packet come from secondary char dev will be drop
Grammar suggestion:
Packets coming from the primary char indev will be sent to outdev;
packets coming from the secondary char dev will be dropped
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
> +/*
> + * called from the main thread on the primary
> + * for get packets
s/get/getting/
> +++ b/qemu-options.hx
> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
> The file format is libpcap, so it can be analyzed with tools such as tcpdump
> or Wireshark.
>
> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
> +outdev=@var{chardevid}
> +
> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
s/get/gets/
> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
s/output/outputs/
Worth any better hints here about _how_ to use it with filter-mirror and
filter-redirector, or should the last phrase just be dropped?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization
2016-04-28 20:55 ` Eric Blake
@ 2016-04-29 1:28 ` Zhang Chen
0 siblings, 0 replies; 39+ messages in thread
From: Zhang Chen @ 2016-04-29 1:28 UTC (permalink / raw)
To: Eric Blake, qemu devel, Jason Wang
Cc: Li Zhijian, Gui jianfeng, eddie.dong, zhanghailiang,
Dr. David Alan Gilbert, Yang Hongyang
On 04/29/2016 04:55 AM, Eric Blake wrote:
> On 04/18/2016 05:11 AM, Zhang Chen wrote:
>
> s/initlization/initialization/ in the subject
OK~
>> packet come from primary char indev will be send to
>> outdev - packet come from secondary char dev will be drop
> Grammar suggestion:
>
> Packets coming from the primary char indev will be sent to outdev;
> packets coming from the secondary char dev will be dropped
>
I will fix it in next version.
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>> +/*
>> + * called from the main thread on the primary
>> + * for get packets
> s/get/getting/
OK
>> +++ b/qemu-options.hx
>> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
>> The file format is libpcap, so it can be analyzed with tools such as tcpdump
>> or Wireshark.
>>
>> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
>> +outdev=@var{chardevid}
>> +
>> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
> s/get/gets/
OK
>> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
> s/output/outputs/
>
> Worth any better hints here about _how_ to use it with filter-mirror and
> filter-redirector, or should the last phrase just be dropped?
I will add how_to in next version.
Thanks
Zhang Chen
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet
2016-04-18 11:11 [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare Zhang Chen
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization Zhang Chen
@ 2016-04-18 11:11 ` Zhang Chen
2016-04-28 7:47 ` Jason Wang
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread Zhang Chen
` (2 subsequent siblings)
4 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-18 11:11 UTC (permalink / raw)
To: qemu devel, Jason Wang
Cc: Zhang Chen, Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
In this patch we use kernel jhash table to track
connection, and then enqueue net packet like this:
+ CompareState ++
| |
+---------------+ +---------------+ +---------------+
|conn list +--->conn +--------->conn |
+---------------+ +---------------+ +---------------+
| | | | | |
+---------------+ +---v----+ +---v----+ +---v----+ +---v----+
|primary | |secondary |primary | |secondary
|packet | |packet + |packet | |packet +
+--------+ +--------+ +--------+ +--------+
| | | |
+---v----+ +---v----+ +---v----+ +---v----+
|primary | |secondary |primary | |secondary
|packet | |packet + |packet | |packet +
+--------+ +--------+ +--------+ +--------+
| | | |
+---v----+ +---v----+ +---v----+ +---v----+
|primary | |secondary |primary | |secondary
|packet | |packet + |packet | |packet +
+--------+ +--------+ +--------+ +--------+
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
include/qemu/jhash.h | 59 ++++++++++
net/colo-compare.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++++++-
trace-events | 3 +
3 files changed, 360 insertions(+), 5 deletions(-)
create mode 100644 include/qemu/jhash.h
diff --git a/include/qemu/jhash.h b/include/qemu/jhash.h
new file mode 100644
index 0000000..8a8ff0f
--- /dev/null
+++ b/include/qemu/jhash.h
@@ -0,0 +1,59 @@
+/* jhash.h: Jenkins hash support.
+ *
+ * Copyright (C) 2006. Bob Jenkins (bob_jenkins@burtleburtle.net)
+ *
+ * http://burtleburtle.net/bob/hash/
+ *
+ * These are the credits from Bob's sources:
+ *
+ * lookup3.c, by Bob Jenkins, May 2006, Public Domain.
+ *
+ * These are functions for producing 32-bit hashes for hash table lookup.
+ * hashword(), hashlittle(), hashlittle2(), hashbig(), mix(), and final()
+ * are externally useful functions. Routines to test the hash are included
+ * if SELF_TEST is defined. You can use this free for any purpose.It's in
+ * the public domain. It has no warranty.
+ *
+ * Copyright (C) 2009-2010 Jozsef Kadlecsik (kadlec@blackhole.kfki.hu)
+ *
+ * I've modified Bob's hash to be useful in the Linux kernel, and
+ * any bugs present are my fault.
+ * Jozsef
+ */
+
+#ifndef QEMU_JHASH_H__
+#define QEMU_JHASH_H__
+
+#include "qemu/bitops.h"
+
+/*
+ * hashtable related is copied from linux kernel jhash
+ */
+
+/* __jhash_mix -- mix 3 32-bit values reversibly. */
+#define __jhash_mix(a, b, c) \
+{ \
+ a -= c; a ^= rol32(c, 4); c += b; \
+ b -= a; b ^= rol32(a, 6); a += c; \
+ c -= b; c ^= rol32(b, 8); b += a; \
+ a -= c; a ^= rol32(c, 16); c += b; \
+ b -= a; b ^= rol32(a, 19); a += c; \
+ c -= b; c ^= rol32(b, 4); b += a; \
+}
+
+/* __jhash_final - final mixing of 3 32-bit values (a,b,c) into c */
+#define __jhash_final(a, b, c) \
+{ \
+ c ^= b; c -= rol32(b, 14); \
+ a ^= c; a -= rol32(c, 11); \
+ b ^= a; b -= rol32(a, 25); \
+ c ^= b; c -= rol32(b, 16); \
+ a ^= c; a -= rol32(c, 4); \
+ b ^= a; b -= rol32(a, 14); \
+ c ^= b; c -= rol32(b, 24); \
+}
+
+/* An arbitrary initial parameter */
+#define JHASH_INITVAL 0xdeadbeef
+
+#endif /* QEMU_JHASH_H__ */
diff --git a/net/colo-compare.c b/net/colo-compare.c
index c45b132..dc57eac 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -22,12 +22,16 @@
#include "qemu/sockets.h"
#include "qapi-visit.h"
#include "trace.h"
+#include "slirp/slirp.h"
+#include "qemu/jhash.h"
+#include "net/eth.h"
#define TYPE_COLO_COMPARE "colo-compare"
#define COLO_COMPARE(obj) \
OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
#define COMPARE_READ_LEN_MAX NET_BUFSIZE
+#define HASHTABLE_MAX_SIZE 16384
static QTAILQ_HEAD(, CompareState) net_compares =
QTAILQ_HEAD_INITIALIZER(net_compares);
@@ -39,6 +43,28 @@ typedef struct ReadState {
uint8_t buf[COMPARE_READ_LEN_MAX];
} ReadState;
+/*
+ + CompareState ++
+ | |
+ +---------------+ +---------------+ +---------------+
+ |conn list +--->conn +--------->conn |
+ +---------------+ +---------------+ +---------------+
+ | | | | | |
+ +---------------+ +---v----+ +---v----+ +---v----+ +---v----+
+ |primary | |secondary |primary | |secondary
+ |packet | |packet + |packet | |packet +
+ +--------+ +--------+ +--------+ +--------+
+ | | | |
+ +---v----+ +---v----+ +---v----+ +---v----+
+ |primary | |secondary |primary | |secondary
+ |packet | |packet + |packet | |packet +
+ +--------+ +--------+ +--------+ +--------+
+ | | | |
+ +---v----+ +---v----+ +---v----+ +---v----+
+ |primary | |secondary |primary | |secondary
+ |packet | |packet + |packet | |packet +
+ +--------+ +--------+ +--------+ +--------+
+*/
typedef struct CompareState {
Object parent;
@@ -51,12 +77,265 @@ typedef struct CompareState {
QTAILQ_ENTRY(CompareState) next;
ReadState pri_rs;
ReadState sec_rs;
+
+ /* connection list: the connections belonged to this NIC could be found
+ * in this list.
+ * element type: Connection
+ */
+ GQueue conn_list;
+ QemuMutex conn_list_lock; /* to protect conn_list */
+ /* hashtable to save connection */
+ GHashTable *connection_track_table;
+ /* to save unprocessed_connections */
+ GQueue unprocessed_connections;
+ /* proxy current hash size */
+ uint32_t hashtable_size;
} CompareState;
typedef struct CompareClass {
ObjectClass parent_class;
} CompareClass;
+typedef struct Packet {
+ void *data;
+ union {
+ uint8_t *network_layer;
+ struct ip *ip;
+ };
+ uint8_t *transport_layer;
+ int size;
+ CompareState *s;
+} Packet;
+
+typedef struct ConnectionKey {
+ /* (src, dst) must be grouped, in the same way than in IP header */
+ struct in_addr src;
+ struct in_addr dst;
+ uint16_t src_port;
+ uint16_t dst_port;
+ uint8_t ip_proto;
+} QEMU_PACKED ConnectionKey;
+
+typedef struct Connection {
+ QemuMutex list_lock;
+ /* connection primary send queue: element type: Packet */
+ GQueue primary_list;
+ /* connection secondary send queue: element type: Packet */
+ GQueue secondary_list;
+ /* flag to enqueue unprocessed_connections */
+ bool processing;
+ uint8_t ip_proto;
+} Connection;
+
+enum {
+ PRIMARY_IN = 0,
+ SECONDARY_IN,
+};
+
+static void packet_destroy(void *opaque, void *user_data);
+static int compare_chr_send(CharDriverState *out,
+ const uint8_t *buf,
+ uint32_t size);
+
+static uint32_t connection_key_hash(const void *opaque)
+{
+ const ConnectionKey *key = opaque;
+ uint32_t a, b, c;
+
+ /* Jenkins hash */
+ a = b = c = JHASH_INITVAL + sizeof(*key);
+ a += key->src.s_addr;
+ b += key->dst.s_addr;
+ c += (key->src_port | key->dst_port << 16);
+ __jhash_mix(a, b, c);
+
+ a += key->ip_proto;
+ __jhash_final(a, b, c);
+
+ return c;
+}
+
+static int connection_key_equal(const void *opaque1, const void *opaque2)
+{
+ return memcmp(opaque1, opaque2, sizeof(ConnectionKey)) == 0;
+}
+
+/*
+ * initialize connecon_key for packet
+ * Return 0 on success, if return 1 the pkt will be sent later
+ */
+static int connection_key_init(Packet *pkt, ConnectionKey *key)
+{
+ int network_length;
+ uint8_t *data = pkt->data;
+ uint16_t l3_proto;
+ uint32_t tmp_ports;
+ ssize_t l2hdr_len = eth_get_l2_hdr_length(data);
+
+ pkt->network_layer = data + ETH_HLEN;
+ l3_proto = eth_get_l3_proto(data, l2hdr_len);
+ if (l3_proto != ETH_P_IP) {
+ return 1;
+ }
+
+ network_length = pkt->ip->ip_hl * 4;
+ pkt->transport_layer = pkt->network_layer + network_length;
+ if (!pkt->transport_layer) {
+ error_report("pkt->transport_layer is valid");
+ return 1;
+ }
+ key->ip_proto = pkt->ip->ip_p;
+ key->src = pkt->ip->ip_src;
+ key->dst = pkt->ip->ip_dst;
+
+ switch (key->ip_proto) {
+ case IPPROTO_TCP:
+ case IPPROTO_UDP:
+ case IPPROTO_DCCP:
+ case IPPROTO_ESP:
+ case IPPROTO_SCTP:
+ case IPPROTO_UDPLITE:
+ tmp_ports = *(uint32_t *)(pkt->transport_layer);
+ key->src_port = ntohs(tmp_ports & 0xffff);
+ key->dst_port = ntohs(tmp_ports >> 16);
+ break;
+ case IPPROTO_AH:
+ tmp_ports = *(uint32_t *)(pkt->transport_layer + 4);
+ key->src_port = ntohs(tmp_ports & 0xffff);
+ key->dst_port = ntohs(tmp_ports >> 16);
+ break;
+ default:
+ key->src_port = 0;
+ key->dst_port = 0;
+ break;
+ }
+
+ return 0;
+}
+
+static Connection *connection_new(ConnectionKey *key)
+{
+ Connection *conn = g_slice_new(Connection);
+
+ qemu_mutex_init(&conn->list_lock);
+ conn->ip_proto = key->ip_proto;
+ conn->processing = false;
+ g_queue_init(&conn->primary_list);
+ g_queue_init(&conn->secondary_list);
+
+ return conn;
+}
+
+/*
+ * Clear hashtable, stop this hash growing really huge
+ */
+static void connection_hashtable_reset(CompareState *s)
+{
+ s->hashtable_size = 0;
+ g_hash_table_remove_all(s->connection_track_table);
+}
+
+/* if not found, create a new connection and add to hash table */
+static Connection *connection_get(CompareState *s, ConnectionKey *key)
+{
+ /* FIXME: protect connection_track_table */
+ Connection *conn = g_hash_table_lookup(s->connection_track_table, key);
+
+ if (conn == NULL) {
+ ConnectionKey *new_key = g_memdup(key, sizeof(*key));
+
+ conn = connection_new(key);
+
+ s->hashtable_size++;
+ if (s->hashtable_size > HASHTABLE_MAX_SIZE) {
+ error_report("colo proxy connection hashtable full, clear it");
+ connection_hashtable_reset(s);
+ /* TODO:clear conn_list */
+ }
+
+ g_hash_table_insert(s->connection_track_table, new_key, conn);
+ }
+
+ return conn;
+}
+
+static void connection_destroy(void *opaque)
+{
+ Connection *conn = opaque;
+
+ qemu_mutex_lock(&conn->list_lock);
+ g_queue_foreach(&conn->primary_list, packet_destroy, NULL);
+ g_queue_free(&conn->primary_list);
+ g_queue_foreach(&conn->secondary_list, packet_destroy, NULL);
+ g_queue_free(&conn->secondary_list);
+ qemu_mutex_unlock(&conn->list_lock);
+ qemu_mutex_destroy(&conn->list_lock);
+ g_slice_free(Connection, conn);
+}
+
+static Packet *packet_new(CompareState *s, const void *data,
+ int size, ConnectionKey *key)
+{
+ Packet *pkt = g_slice_new(Packet);
+
+ pkt->data = g_memdup(data, size);
+ pkt->size = size;
+ pkt->s = s;
+
+ if (connection_key_init(pkt, key)) {
+ packet_destroy(pkt, NULL);
+ pkt = NULL;
+ }
+
+ return pkt;
+}
+
+/*
+ * Return 0 on success, if return -1 means the pkt
+ * is unsupported(arp and ipv6) and will be sent later
+ */
+static int packet_enqueue(CompareState *s, int mode)
+{
+ ConnectionKey key = {{ 0 } };
+ Packet *pkt = NULL;
+ Connection *conn;
+
+ if (mode == PRIMARY_IN) {
+ pkt = packet_new(s, s->pri_rs.buf, s->pri_rs.packet_len, &key);
+ } else {
+ pkt = packet_new(s, s->sec_rs.buf, s->sec_rs.packet_len, &key);
+ }
+ if (!pkt) {
+ return -1;
+ }
+
+ conn = connection_get(s, &key);
+ if (!conn->processing) {
+ qemu_mutex_lock(&s->conn_list_lock);
+ g_queue_push_tail(&s->conn_list, conn);
+ qemu_mutex_unlock(&s->conn_list_lock);
+ conn->processing = true;
+ }
+
+ qemu_mutex_lock(&conn->list_lock);
+ if (mode == PRIMARY_IN) {
+ g_queue_push_tail(&conn->primary_list, pkt);
+ } else {
+ g_queue_push_tail(&conn->secondary_list, pkt);
+ }
+ qemu_mutex_unlock(&conn->list_lock);
+
+ return 0;
+}
+
+static void packet_destroy(void *opaque, void *user_data)
+{
+ Packet *pkt = opaque;
+
+ g_free(pkt->data);
+ g_slice_free(Packet, pkt);
+}
+
static int compare_chr_send(CharDriverState *out,
const uint8_t *buf,
uint32_t size)
@@ -158,8 +437,10 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
if (ret == 1) {
- /* FIXME: enqueue to primary packet list */
- compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
+ if (packet_enqueue(s, PRIMARY_IN)) {
+ trace_colo_compare_main("primary: unsupported packet in");
+ compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
+ }
} else if (ret == -1) {
qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
}
@@ -176,9 +457,11 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
if (ret == 1) {
- /* TODO: enqueue to secondary packet list*/
- /* should we send sec arp pkt? */
- compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
+ if (packet_enqueue(s, SECONDARY_IN)) {
+ trace_colo_compare_main("secondary: unsupported packet in");
+ /* should we send sec arp pkt? */
+ compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
+ }
} else if (ret == -1) {
qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
}
@@ -280,6 +563,15 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
qemu_chr_fe_claim_no_fail(s->chr_out);
QTAILQ_INSERT_TAIL(&net_compares, s, next);
+ g_queue_init(&s->conn_list);
+ qemu_mutex_init(&s->conn_list_lock);
+ s->hashtable_size = 0;
+
+ s->connection_track_table = g_hash_table_new_full(connection_key_hash,
+ connection_key_equal,
+ g_free,
+ connection_destroy);
+
return;
}
@@ -314,6 +606,7 @@ static void colo_compare_class_finalize(ObjectClass *oc, void *data)
if (!QTAILQ_EMPTY(&net_compares)) {
QTAILQ_REMOVE(&net_compares, s, next);
}
+ qemu_mutex_destroy(&s->conn_list_lock);
}
static void colo_compare_init(Object *obj)
diff --git a/trace-events b/trace-events
index ca7211b..8862288 100644
--- a/trace-events
+++ b/trace-events
@@ -1916,3 +1916,6 @@ aspeed_vic_update_fiq(int flags) "Raising FIQ: %d"
aspeed_vic_update_irq(int flags) "Raising IRQ: %d"
aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "From 0x%" PRIx64 " of size %u: 0x%" PRIx32
aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
+
+# net/colo-compare.c
+colo_compare_main(const char *chr) "chr: %s"
--
1.9.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet Zhang Chen
@ 2016-04-28 7:47 ` Jason Wang
2016-04-28 10:25 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-04-28 7:47 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/18/2016 07:11 PM, Zhang Chen wrote:
> In this patch we use kernel jhash table to track
> connection, and then enqueue net packet like this:
>
> + CompareState ++
> | |
> +---------------+ +---------------+ +---------------+
> |conn list +--->conn +--------->conn |
> +---------------+ +---------------+ +---------------+
> | | | | | |
> +---------------+ +---v----+ +---v----+ +---v----+ +---v----+
> |primary | |secondary |primary | |secondary
> |packet | |packet + |packet | |packet +
> +--------+ +--------+ +--------+ +--------+
> | | | |
> +---v----+ +---v----+ +---v----+ +---v----+
> |primary | |secondary |primary | |secondary
> |packet | |packet + |packet | |packet +
> +--------+ +--------+ +--------+ +--------+
> | | | |
> +---v----+ +---v----+ +---v----+ +---v----+
> |primary | |secondary |primary | |secondary
> |packet | |packet + |packet | |packet +
> +--------+ +--------+ +--------+ +--------+
>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
> include/qemu/jhash.h | 59 ++++++++++
> net/colo-compare.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> trace-events | 3 +
> 3 files changed, 360 insertions(+), 5 deletions(-)
> create mode 100644 include/qemu/jhash.h
>
> diff --git a/include/qemu/jhash.h b/include/qemu/jhash.h
> new file mode 100644
> index 0000000..8a8ff0f
> --- /dev/null
> +++ b/include/qemu/jhash.h
> @@ -0,0 +1,59 @@
> +/* jhash.h: Jenkins hash support.
> + *
> + * Copyright (C) 2006. Bob Jenkins (bob_jenkins@burtleburtle.net)
> + *
> + * http://burtleburtle.net/bob/hash/
> + *
> + * These are the credits from Bob's sources:
> + *
> + * lookup3.c, by Bob Jenkins, May 2006, Public Domain.
> + *
> + * These are functions for producing 32-bit hashes for hash table lookup.
> + * hashword(), hashlittle(), hashlittle2(), hashbig(), mix(), and final()
> + * are externally useful functions. Routines to test the hash are included
> + * if SELF_TEST is defined. You can use this free for any purpose.It's in
> + * the public domain. It has no warranty.
> + *
> + * Copyright (C) 2009-2010 Jozsef Kadlecsik (kadlec@blackhole.kfki.hu)
> + *
> + * I've modified Bob's hash to be useful in the Linux kernel, and
> + * any bugs present are my fault.
> + * Jozsef
> + */
> +
> +#ifndef QEMU_JHASH_H__
> +#define QEMU_JHASH_H__
> +
> +#include "qemu/bitops.h"
> +
> +/*
> + * hashtable related is copied from linux kernel jhash
> + */
> +
> +/* __jhash_mix -- mix 3 32-bit values reversibly. */
> +#define __jhash_mix(a, b, c) \
> +{ \
> + a -= c; a ^= rol32(c, 4); c += b; \
> + b -= a; b ^= rol32(a, 6); a += c; \
> + c -= b; c ^= rol32(b, 8); b += a; \
> + a -= c; a ^= rol32(c, 16); c += b; \
> + b -= a; b ^= rol32(a, 19); a += c; \
> + c -= b; c ^= rol32(b, 4); b += a; \
> +}
> +
> +/* __jhash_final - final mixing of 3 32-bit values (a,b,c) into c */
> +#define __jhash_final(a, b, c) \
> +{ \
> + c ^= b; c -= rol32(b, 14); \
> + a ^= c; a -= rol32(c, 11); \
> + b ^= a; b -= rol32(a, 25); \
> + c ^= b; c -= rol32(b, 16); \
> + a ^= c; a -= rol32(c, 4); \
> + b ^= a; b -= rol32(a, 14); \
> + c ^= b; c -= rol32(b, 24); \
> +}
> +
> +/* An arbitrary initial parameter */
> +#define JHASH_INITVAL 0xdeadbeef
> +
> +#endif /* QEMU_JHASH_H__ */
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index c45b132..dc57eac 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -22,12 +22,16 @@
> #include "qemu/sockets.h"
> #include "qapi-visit.h"
> #include "trace.h"
> +#include "slirp/slirp.h"
> +#include "qemu/jhash.h"
> +#include "net/eth.h"
>
> #define TYPE_COLO_COMPARE "colo-compare"
> #define COLO_COMPARE(obj) \
> OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
>
> #define COMPARE_READ_LEN_MAX NET_BUFSIZE
> +#define HASHTABLE_MAX_SIZE 16384
>
> static QTAILQ_HEAD(, CompareState) net_compares =
> QTAILQ_HEAD_INITIALIZER(net_compares);
> @@ -39,6 +43,28 @@ typedef struct ReadState {
> uint8_t buf[COMPARE_READ_LEN_MAX];
> } ReadState;
>
> +/*
> + + CompareState ++
> + | |
> + +---------------+ +---------------+ +---------------+
> + |conn list +--->conn +--------->conn |
> + +---------------+ +---------------+ +---------------+
> + | | | | | |
> + +---------------+ +---v----+ +---v----+ +---v----+ +---v----+
> + |primary | |secondary |primary | |secondary
> + |packet | |packet + |packet | |packet +
> + +--------+ +--------+ +--------+ +--------+
> + | | | |
> + +---v----+ +---v----+ +---v----+ +---v----+
> + |primary | |secondary |primary | |secondary
> + |packet | |packet + |packet | |packet +
> + +--------+ +--------+ +--------+ +--------+
> + | | | |
> + +---v----+ +---v----+ +---v----+ +---v----+
> + |primary | |secondary |primary | |secondary
> + |packet | |packet + |packet | |packet +
> + +--------+ +--------+ +--------+ +--------+
> +*/
> typedef struct CompareState {
> Object parent;
>
> @@ -51,12 +77,265 @@ typedef struct CompareState {
> QTAILQ_ENTRY(CompareState) next;
> ReadState pri_rs;
> ReadState sec_rs;
> +
> + /* connection list: the connections belonged to this NIC could be found
> + * in this list.
> + * element type: Connection
> + */
> + GQueue conn_list;
> + QemuMutex conn_list_lock; /* to protect conn_list */
> + /* hashtable to save connection */
> + GHashTable *connection_track_table;
> + /* to save unprocessed_connections */
> + GQueue unprocessed_connections;
> + /* proxy current hash size */
> + uint32_t hashtable_size;
> } CompareState;
>
> typedef struct CompareClass {
> ObjectClass parent_class;
> } CompareClass;
>
> +typedef struct Packet {
> + void *data;
> + union {
> + uint8_t *network_layer;
> + struct ip *ip;
Does this mean ipv6 is not supported?
> + };
> + uint8_t *transport_layer;
> + int size;
> + CompareState *s;
> +} Packet;
> +
> +typedef struct ConnectionKey {
> + /* (src, dst) must be grouped, in the same way than in IP header */
> + struct in_addr src;
> + struct in_addr dst;
> + uint16_t src_port;
> + uint16_t dst_port;
> + uint8_t ip_proto;
> +} QEMU_PACKED ConnectionKey;
> +
> +typedef struct Connection {
> + QemuMutex list_lock;
> + /* connection primary send queue: element type: Packet */
> + GQueue primary_list;
> + /* connection secondary send queue: element type: Packet */
> + GQueue secondary_list;
> + /* flag to enqueue unprocessed_connections */
> + bool processing;
> + uint8_t ip_proto;
> +} Connection;
> +
> +enum {
> + PRIMARY_IN = 0,
> + SECONDARY_IN,
> +};
> +
> +static void packet_destroy(void *opaque, void *user_data);
> +static int compare_chr_send(CharDriverState *out,
> + const uint8_t *buf,
> + uint32_t size);
> +
> +static uint32_t connection_key_hash(const void *opaque)
> +{
> + const ConnectionKey *key = opaque;
> + uint32_t a, b, c;
> +
> + /* Jenkins hash */
> + a = b = c = JHASH_INITVAL + sizeof(*key);
> + a += key->src.s_addr;
> + b += key->dst.s_addr;
> + c += (key->src_port | key->dst_port << 16);
> + __jhash_mix(a, b, c);
> +
> + a += key->ip_proto;
> + __jhash_final(a, b, c);
> +
> + return c;
> +}
> +
> +static int connection_key_equal(const void *opaque1, const void *opaque2)
> +{
> + return memcmp(opaque1, opaque2, sizeof(ConnectionKey)) == 0;
So why not useing ConnectionKey * consider we're sure of the type?
> +}
> +
> +/*
> + * initialize connecon_key for packet
> + * Return 0 on success, if return 1 the pkt will be sent later
> + */
> +static int connection_key_init(Packet *pkt, ConnectionKey *key)
> +{
> + int network_length;
> + uint8_t *data = pkt->data;
> + uint16_t l3_proto;
> + uint32_t tmp_ports;
> + ssize_t l2hdr_len = eth_get_l2_hdr_length(data);
> +
> + pkt->network_layer = data + ETH_HLEN;
Can the length of data be shorter than ETH_HELN?
> + l3_proto = eth_get_l3_proto(data, l2hdr_len);
> + if (l3_proto != ETH_P_IP) {
> + return 1;
> + }
> +
> + network_length = pkt->ip->ip_hl * 4;
> + pkt->transport_layer = pkt->network_layer + network_length;
Do we need sanity check to make sure there's no evil network_length here?
> + if (!pkt->transport_layer) {
> + error_report("pkt->transport_layer is valid");
invalid? And if this is caused by the bad packet it self, there's no
need for a error_report.
> + return 1;
> + }
> + key->ip_proto = pkt->ip->ip_p;
> + key->src = pkt->ip->ip_src;
> + key->dst = pkt->ip->ip_dst;
> +
> + switch (key->ip_proto) {
> + case IPPROTO_TCP:
> + case IPPROTO_UDP:
> + case IPPROTO_DCCP:
> + case IPPROTO_ESP:
> + case IPPROTO_SCTP:
> + case IPPROTO_UDPLITE:
> + tmp_ports = *(uint32_t *)(pkt->transport_layer);
> + key->src_port = ntohs(tmp_ports & 0xffff);
> + key->dst_port = ntohs(tmp_ports >> 16);
> + break;
> + case IPPROTO_AH:
> + tmp_ports = *(uint32_t *)(pkt->transport_layer + 4);
> + key->src_port = ntohs(tmp_ports & 0xffff);
> + key->dst_port = ntohs(tmp_ports >> 16);
> + break;
> + default:
> + key->src_port = 0;
> + key->dst_port = 0;
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static Connection *connection_new(ConnectionKey *key)
> +{
> + Connection *conn = g_slice_new(Connection);
> +
> + qemu_mutex_init(&conn->list_lock);
> + conn->ip_proto = key->ip_proto;
> + conn->processing = false;
> + g_queue_init(&conn->primary_list);
> + g_queue_init(&conn->secondary_list);
> +
> + return conn;
> +}
> +
> +/*
> + * Clear hashtable, stop this hash growing really huge
> + */
> +static void connection_hashtable_reset(CompareState *s)
> +{
> + s->hashtable_size = 0;
> + g_hash_table_remove_all(s->connection_track_table);
> +}
> +
> +/* if not found, create a new connection and add to hash table */
> +static Connection *connection_get(CompareState *s, ConnectionKey *key)
> +{
> + /* FIXME: protect connection_track_table */
> + Connection *conn = g_hash_table_lookup(s->connection_track_table, key);
> +
> + if (conn == NULL) {
> + ConnectionKey *new_key = g_memdup(key, sizeof(*key));
> +
> + conn = connection_new(key);
> +
> + s->hashtable_size++;
> + if (s->hashtable_size > HASHTABLE_MAX_SIZE) {
> + error_report("colo proxy connection hashtable full, clear it");
> + connection_hashtable_reset(s);
> + /* TODO:clear conn_list */
> + }
> +
> + g_hash_table_insert(s->connection_track_table, new_key, conn);
> + }
> +
> + return conn;
> +}
> +
> +static void connection_destroy(void *opaque)
> +{
> + Connection *conn = opaque;
> +
> + qemu_mutex_lock(&conn->list_lock);
Like I said in previous patch, if you do all the processing in colo
compare thread, you can avoid almost all synchronization (e.g mutex).
> + g_queue_foreach(&conn->primary_list, packet_destroy, NULL);
> + g_queue_free(&conn->primary_list);
> + g_queue_foreach(&conn->secondary_list, packet_destroy, NULL);
> + g_queue_free(&conn->secondary_list);
> + qemu_mutex_unlock(&conn->list_lock);
> + qemu_mutex_destroy(&conn->list_lock);
> + g_slice_free(Connection, conn);
> +}
> +
> +static Packet *packet_new(CompareState *s, const void *data,
> + int size, ConnectionKey *key)
> +{
> + Packet *pkt = g_slice_new(Packet);
> +
> + pkt->data = g_memdup(data, size);
> + pkt->size = size;
> + pkt->s = s;
> +
> + if (connection_key_init(pkt, key)) {
> + packet_destroy(pkt, NULL);
> + pkt = NULL;
> + }
Can we do connection_key_init() first, this can avoid packet_desctory()
if it fails.
> +
> + return pkt;
> +}
> +
> +/*
> + * Return 0 on success, if return -1 means the pkt
> + * is unsupported(arp and ipv6) and will be sent later
> + */
> +static int packet_enqueue(CompareState *s, int mode)
> +{
> + ConnectionKey key = {{ 0 } };
> + Packet *pkt = NULL;
> + Connection *conn;
> +
> + if (mode == PRIMARY_IN) {
> + pkt = packet_new(s, s->pri_rs.buf, s->pri_rs.packet_len, &key);
> + } else {
> + pkt = packet_new(s, s->sec_rs.buf, s->sec_rs.packet_len, &key);
> + }
> + if (!pkt) {
> + return -1;
> + }
> +
> + conn = connection_get(s, &key);
> + if (!conn->processing) {
> + qemu_mutex_lock(&s->conn_list_lock);
> + g_queue_push_tail(&s->conn_list, conn);
> + qemu_mutex_unlock(&s->conn_list_lock);
> + conn->processing = true;
> + }
> +
> + qemu_mutex_lock(&conn->list_lock);
> + if (mode == PRIMARY_IN) {
> + g_queue_push_tail(&conn->primary_list, pkt);
> + } else {
> + g_queue_push_tail(&conn->secondary_list, pkt);
> + }
> + qemu_mutex_unlock(&conn->list_lock);
> +
> + return 0;
> +}
> +
> +static void packet_destroy(void *opaque, void *user_data)
> +{
> + Packet *pkt = opaque;
> +
> + g_free(pkt->data);
> + g_slice_free(Packet, pkt);
> +}
> +
> static int compare_chr_send(CharDriverState *out,
> const uint8_t *buf,
> uint32_t size)
> @@ -158,8 +437,10 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
>
> ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
> if (ret == 1) {
> - /* FIXME: enqueue to primary packet list */
> - compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
> + if (packet_enqueue(s, PRIMARY_IN)) {
> + trace_colo_compare_main("primary: unsupported packet in");
> + compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
Looks like if a packet was not recognized by connection_key_init(), it
will be sent directly without comparing it with the packet sent from
secondary? Is this expected?
> + }
> } else if (ret == -1) {
> qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> }
> @@ -176,9 +457,11 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
>
> ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
> if (ret == 1) {
> - /* TODO: enqueue to secondary packet list*/
> - /* should we send sec arp pkt? */
> - compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
> + if (packet_enqueue(s, SECONDARY_IN)) {
> + trace_colo_compare_main("secondary: unsupported packet in");
> + /* should we send sec arp pkt? */
> + compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
> + }
> } else if (ret == -1) {
> qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> }
> @@ -280,6 +563,15 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> qemu_chr_fe_claim_no_fail(s->chr_out);
> QTAILQ_INSERT_TAIL(&net_compares, s, next);
>
> + g_queue_init(&s->conn_list);
> + qemu_mutex_init(&s->conn_list_lock);
> + s->hashtable_size = 0;
> +
> + s->connection_track_table = g_hash_table_new_full(connection_key_hash,
> + connection_key_equal,
> + g_free,
> + connection_destroy);
> +
> return;
> }
>
> @@ -314,6 +606,7 @@ static void colo_compare_class_finalize(ObjectClass *oc, void *data)
> if (!QTAILQ_EMPTY(&net_compares)) {
> QTAILQ_REMOVE(&net_compares, s, next);
> }
> + qemu_mutex_destroy(&s->conn_list_lock);
> }
>
> static void colo_compare_init(Object *obj)
> diff --git a/trace-events b/trace-events
> index ca7211b..8862288 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1916,3 +1916,6 @@ aspeed_vic_update_fiq(int flags) "Raising FIQ: %d"
> aspeed_vic_update_irq(int flags) "Raising IRQ: %d"
> aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "From 0x%" PRIx64 " of size %u: 0x%" PRIx32
> aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
> +
> +# net/colo-compare.c
> +colo_compare_main(const char *chr) "chr: %s"
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet
2016-04-28 7:47 ` Jason Wang
@ 2016-04-28 10:25 ` Zhang Chen
2016-04-29 2:05 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-28 10:25 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/28/2016 03:47 PM, Jason Wang wrote:
>
> On 04/18/2016 07:11 PM, Zhang Chen wrote:
>> In this patch we use kernel jhash table to track
>> connection, and then enqueue net packet like this:
>>
>> + CompareState ++
>> | |
>> +---------------+ +---------------+ +---------------+
>> |conn list +--->conn +--------->conn |
>> +---------------+ +---------------+ +---------------+
>> | | | | | |
>> +---------------+ +---v----+ +---v----+ +---v----+ +---v----+
>> |primary | |secondary |primary | |secondary
>> |packet | |packet + |packet | |packet +
>> +--------+ +--------+ +--------+ +--------+
>> | | | |
>> +---v----+ +---v----+ +---v----+ +---v----+
>> |primary | |secondary |primary | |secondary
>> |packet | |packet + |packet | |packet +
>> +--------+ +--------+ +--------+ +--------+
>> | | | |
>> +---v----+ +---v----+ +---v----+ +---v----+
>> |primary | |secondary |primary | |secondary
>> |packet | |packet + |packet | |packet +
>> +--------+ +--------+ +--------+ +--------+
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>> include/qemu/jhash.h | 59 ++++++++++
>> net/colo-compare.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> trace-events | 3 +
>> 3 files changed, 360 insertions(+), 5 deletions(-)
>> create mode 100644 include/qemu/jhash.h
>>
>> diff --git a/include/qemu/jhash.h b/include/qemu/jhash.h
>> new file mode 100644
>> index 0000000..8a8ff0f
>> --- /dev/null
>> +++ b/include/qemu/jhash.h
>> @@ -0,0 +1,59 @@
>> +/* jhash.h: Jenkins hash support.
>> + *
>> + * Copyright (C) 2006. Bob Jenkins (bob_jenkins@burtleburtle.net)
>> + *
>> + * http://burtleburtle.net/bob/hash/
>> + *
>> + * These are the credits from Bob's sources:
>> + *
>> + * lookup3.c, by Bob Jenkins, May 2006, Public Domain.
>> + *
>> + * These are functions for producing 32-bit hashes for hash table lookup.
>> + * hashword(), hashlittle(), hashlittle2(), hashbig(), mix(), and final()
>> + * are externally useful functions. Routines to test the hash are included
>> + * if SELF_TEST is defined. You can use this free for any purpose.It's in
>> + * the public domain. It has no warranty.
>> + *
>> + * Copyright (C) 2009-2010 Jozsef Kadlecsik (kadlec@blackhole.kfki.hu)
>> + *
>> + * I've modified Bob's hash to be useful in the Linux kernel, and
>> + * any bugs present are my fault.
>> + * Jozsef
>> + */
>> +
>> +#ifndef QEMU_JHASH_H__
>> +#define QEMU_JHASH_H__
>> +
>> +#include "qemu/bitops.h"
>> +
>> +/*
>> + * hashtable related is copied from linux kernel jhash
>> + */
>> +
>> +/* __jhash_mix -- mix 3 32-bit values reversibly. */
>> +#define __jhash_mix(a, b, c) \
>> +{ \
>> + a -= c; a ^= rol32(c, 4); c += b; \
>> + b -= a; b ^= rol32(a, 6); a += c; \
>> + c -= b; c ^= rol32(b, 8); b += a; \
>> + a -= c; a ^= rol32(c, 16); c += b; \
>> + b -= a; b ^= rol32(a, 19); a += c; \
>> + c -= b; c ^= rol32(b, 4); b += a; \
>> +}
>> +
>> +/* __jhash_final - final mixing of 3 32-bit values (a,b,c) into c */
>> +#define __jhash_final(a, b, c) \
>> +{ \
>> + c ^= b; c -= rol32(b, 14); \
>> + a ^= c; a -= rol32(c, 11); \
>> + b ^= a; b -= rol32(a, 25); \
>> + c ^= b; c -= rol32(b, 16); \
>> + a ^= c; a -= rol32(c, 4); \
>> + b ^= a; b -= rol32(a, 14); \
>> + c ^= b; c -= rol32(b, 24); \
>> +}
>> +
>> +/* An arbitrary initial parameter */
>> +#define JHASH_INITVAL 0xdeadbeef
>> +
>> +#endif /* QEMU_JHASH_H__ */
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index c45b132..dc57eac 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -22,12 +22,16 @@
>> #include "qemu/sockets.h"
>> #include "qapi-visit.h"
>> #include "trace.h"
>> +#include "slirp/slirp.h"
>> +#include "qemu/jhash.h"
>> +#include "net/eth.h"
>>
>> #define TYPE_COLO_COMPARE "colo-compare"
>> #define COLO_COMPARE(obj) \
>> OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
>>
>> #define COMPARE_READ_LEN_MAX NET_BUFSIZE
>> +#define HASHTABLE_MAX_SIZE 16384
>>
>> static QTAILQ_HEAD(, CompareState) net_compares =
>> QTAILQ_HEAD_INITIALIZER(net_compares);
>> @@ -39,6 +43,28 @@ typedef struct ReadState {
>> uint8_t buf[COMPARE_READ_LEN_MAX];
>> } ReadState;
>>
>> +/*
>> + + CompareState ++
>> + | |
>> + +---------------+ +---------------+ +---------------+
>> + |conn list +--->conn +--------->conn |
>> + +---------------+ +---------------+ +---------------+
>> + | | | | | |
>> + +---------------+ +---v----+ +---v----+ +---v----+ +---v----+
>> + |primary | |secondary |primary | |secondary
>> + |packet | |packet + |packet | |packet +
>> + +--------+ +--------+ +--------+ +--------+
>> + | | | |
>> + +---v----+ +---v----+ +---v----+ +---v----+
>> + |primary | |secondary |primary | |secondary
>> + |packet | |packet + |packet | |packet +
>> + +--------+ +--------+ +--------+ +--------+
>> + | | | |
>> + +---v----+ +---v----+ +---v----+ +---v----+
>> + |primary | |secondary |primary | |secondary
>> + |packet | |packet + |packet | |packet +
>> + +--------+ +--------+ +--------+ +--------+
>> +*/
>> typedef struct CompareState {
>> Object parent;
>>
>> @@ -51,12 +77,265 @@ typedef struct CompareState {
>> QTAILQ_ENTRY(CompareState) next;
>> ReadState pri_rs;
>> ReadState sec_rs;
>> +
>> + /* connection list: the connections belonged to this NIC could be found
>> + * in this list.
>> + * element type: Connection
>> + */
>> + GQueue conn_list;
>> + QemuMutex conn_list_lock; /* to protect conn_list */
>> + /* hashtable to save connection */
>> + GHashTable *connection_track_table;
>> + /* to save unprocessed_connections */
>> + GQueue unprocessed_connections;
>> + /* proxy current hash size */
>> + uint32_t hashtable_size;
>> } CompareState;
>>
>> typedef struct CompareClass {
>> ObjectClass parent_class;
>> } CompareClass;
>>
>> +typedef struct Packet {
>> + void *data;
>> + union {
>> + uint8_t *network_layer;
>> + struct ip *ip;
> Does this mean ipv6 is not supported?
Yes,currently not support.
>
>> + };
>> + uint8_t *transport_layer;
>> + int size;
>> + CompareState *s;
>> +} Packet;
>> +
>> +typedef struct ConnectionKey {
>> + /* (src, dst) must be grouped, in the same way than in IP header */
>> + struct in_addr src;
>> + struct in_addr dst;
>> + uint16_t src_port;
>> + uint16_t dst_port;
>> + uint8_t ip_proto;
>> +} QEMU_PACKED ConnectionKey;
>> +
>> +typedef struct Connection {
>> + QemuMutex list_lock;
>> + /* connection primary send queue: element type: Packet */
>> + GQueue primary_list;
>> + /* connection secondary send queue: element type: Packet */
>> + GQueue secondary_list;
>> + /* flag to enqueue unprocessed_connections */
>> + bool processing;
>> + uint8_t ip_proto;
>> +} Connection;
>> +
>> +enum {
>> + PRIMARY_IN = 0,
>> + SECONDARY_IN,
>> +};
>> +
>> +static void packet_destroy(void *opaque, void *user_data);
>> +static int compare_chr_send(CharDriverState *out,
>> + const uint8_t *buf,
>> + uint32_t size);
>> +
>> +static uint32_t connection_key_hash(const void *opaque)
>> +{
>> + const ConnectionKey *key = opaque;
>> + uint32_t a, b, c;
>> +
>> + /* Jenkins hash */
>> + a = b = c = JHASH_INITVAL + sizeof(*key);
>> + a += key->src.s_addr;
>> + b += key->dst.s_addr;
>> + c += (key->src_port | key->dst_port << 16);
>> + __jhash_mix(a, b, c);
>> +
>> + a += key->ip_proto;
>> + __jhash_final(a, b, c);
>> +
>> + return c;
>> +}
>> +
>> +static int connection_key_equal(const void *opaque1, const void *opaque2)
>> +{
>> + return memcmp(opaque1, opaque2, sizeof(ConnectionKey)) == 0;
> So why not useing ConnectionKey * consider we're sure of the type?
OK, will fix it in next version.
>
>> +}
>> +
>> +/*
>> + * initialize connecon_key for packet
>> + * Return 0 on success, if return 1 the pkt will be sent later
>> + */
>> +static int connection_key_init(Packet *pkt, ConnectionKey *key)
>> +{
>> + int network_length;
>> + uint8_t *data = pkt->data;
>> + uint16_t l3_proto;
>> + uint32_t tmp_ports;
>> + ssize_t l2hdr_len = eth_get_l2_hdr_length(data);
>> +
>> + pkt->network_layer = data + ETH_HLEN;
> Can the length of data be shorter than ETH_HELN?
Thanks,I will check pkt->size first.
>
>> + l3_proto = eth_get_l3_proto(data, l2hdr_len);
>> + if (l3_proto != ETH_P_IP) {
>> + return 1;
>> + }
>> +
>> + network_length = pkt->ip->ip_hl * 4;
>> + pkt->transport_layer = pkt->network_layer + network_length;
> Do we need sanity check to make sure there's no evil network_length here?
Yes,I will fix.
>
>> + if (!pkt->transport_layer) {
>> + error_report("pkt->transport_layer is valid");
> invalid? And if this is caused by the bad packet it self, there's no
> need for a error_report.
OK
>
>> + return 1;
>> + }
>> + key->ip_proto = pkt->ip->ip_p;
>> + key->src = pkt->ip->ip_src;
>> + key->dst = pkt->ip->ip_dst;
>> +
>> + switch (key->ip_proto) {
>> + case IPPROTO_TCP:
>> + case IPPROTO_UDP:
>> + case IPPROTO_DCCP:
>> + case IPPROTO_ESP:
>> + case IPPROTO_SCTP:
>> + case IPPROTO_UDPLITE:
>> + tmp_ports = *(uint32_t *)(pkt->transport_layer);
>> + key->src_port = ntohs(tmp_ports & 0xffff);
>> + key->dst_port = ntohs(tmp_ports >> 16);
>> + break;
>> + case IPPROTO_AH:
>> + tmp_ports = *(uint32_t *)(pkt->transport_layer + 4);
>> + key->src_port = ntohs(tmp_ports & 0xffff);
>> + key->dst_port = ntohs(tmp_ports >> 16);
>> + break;
>> + default:
>> + key->src_port = 0;
>> + key->dst_port = 0;
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static Connection *connection_new(ConnectionKey *key)
>> +{
>> + Connection *conn = g_slice_new(Connection);
>> +
>> + qemu_mutex_init(&conn->list_lock);
>> + conn->ip_proto = key->ip_proto;
>> + conn->processing = false;
>> + g_queue_init(&conn->primary_list);
>> + g_queue_init(&conn->secondary_list);
>> +
>> + return conn;
>> +}
>> +
>> +/*
>> + * Clear hashtable, stop this hash growing really huge
>> + */
>> +static void connection_hashtable_reset(CompareState *s)
>> +{
>> + s->hashtable_size = 0;
>> + g_hash_table_remove_all(s->connection_track_table);
>> +}
>> +
>> +/* if not found, create a new connection and add to hash table */
>> +static Connection *connection_get(CompareState *s, ConnectionKey *key)
>> +{
>> + /* FIXME: protect connection_track_table */
>> + Connection *conn = g_hash_table_lookup(s->connection_track_table, key);
>> +
>> + if (conn == NULL) {
>> + ConnectionKey *new_key = g_memdup(key, sizeof(*key));
>> +
>> + conn = connection_new(key);
>> +
>> + s->hashtable_size++;
>> + if (s->hashtable_size > HASHTABLE_MAX_SIZE) {
>> + error_report("colo proxy connection hashtable full, clear it");
>> + connection_hashtable_reset(s);
>> + /* TODO:clear conn_list */
>> + }
>> +
>> + g_hash_table_insert(s->connection_track_table, new_key, conn);
>> + }
>> +
>> + return conn;
>> +}
>> +
>> +static void connection_destroy(void *opaque)
>> +{
>> + Connection *conn = opaque;
>> +
>> + qemu_mutex_lock(&conn->list_lock);
> Like I said in previous patch, if you do all the processing in colo
> compare thread, you can avoid almost all synchronization (e.g mutex).
>
>> + g_queue_foreach(&conn->primary_list, packet_destroy, NULL);
>> + g_queue_free(&conn->primary_list);
>> + g_queue_foreach(&conn->secondary_list, packet_destroy, NULL);
>> + g_queue_free(&conn->secondary_list);
>> + qemu_mutex_unlock(&conn->list_lock);
>> + qemu_mutex_destroy(&conn->list_lock);
>> + g_slice_free(Connection, conn);
>> +}
>> +
>> +static Packet *packet_new(CompareState *s, const void *data,
>> + int size, ConnectionKey *key)
>> +{
>> + Packet *pkt = g_slice_new(Packet);
>> +
>> + pkt->data = g_memdup(data, size);
>> + pkt->size = size;
>> + pkt->s = s;
>> +
>> + if (connection_key_init(pkt, key)) {
>> + packet_destroy(pkt, NULL);
>> + pkt = NULL;
>> + }
> Can we do connection_key_init() first, this can avoid packet_desctory()
> if it fails.
Do you mean we should call connection_key_init() first
and then call packet_new()?
>
>> +
>> + return pkt;
>> +}
>> +
>> +/*
>> + * Return 0 on success, if return -1 means the pkt
>> + * is unsupported(arp and ipv6) and will be sent later
>> + */
>> +static int packet_enqueue(CompareState *s, int mode)
>> +{
>> + ConnectionKey key = {{ 0 } };
>> + Packet *pkt = NULL;
>> + Connection *conn;
>> +
>> + if (mode == PRIMARY_IN) {
>> + pkt = packet_new(s, s->pri_rs.buf, s->pri_rs.packet_len, &key);
>> + } else {
>> + pkt = packet_new(s, s->sec_rs.buf, s->sec_rs.packet_len, &key);
>> + }
>> + if (!pkt) {
>> + return -1;
>> + }
>> +
>> + conn = connection_get(s, &key);
>> + if (!conn->processing) {
>> + qemu_mutex_lock(&s->conn_list_lock);
>> + g_queue_push_tail(&s->conn_list, conn);
>> + qemu_mutex_unlock(&s->conn_list_lock);
>> + conn->processing = true;
>> + }
>> +
>> + qemu_mutex_lock(&conn->list_lock);
>> + if (mode == PRIMARY_IN) {
>> + g_queue_push_tail(&conn->primary_list, pkt);
>> + } else {
>> + g_queue_push_tail(&conn->secondary_list, pkt);
>> + }
>> + qemu_mutex_unlock(&conn->list_lock);
>> +
>> + return 0;
>> +}
>> +
>> +static void packet_destroy(void *opaque, void *user_data)
>> +{
>> + Packet *pkt = opaque;
>> +
>> + g_free(pkt->data);
>> + g_slice_free(Packet, pkt);
>> +}
>> +
>> static int compare_chr_send(CharDriverState *out,
>> const uint8_t *buf,
>> uint32_t size)
>> @@ -158,8 +437,10 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
>>
>> ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
>> if (ret == 1) {
>> - /* FIXME: enqueue to primary packet list */
>> - compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
>> + if (packet_enqueue(s, PRIMARY_IN)) {
>> + trace_colo_compare_main("primary: unsupported packet in");
>> + compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
> Looks like if a packet was not recognized by connection_key_init(), it
> will be sent directly without comparing it with the packet sent from
> secondary? Is this expected?
Yes,we will send primary's arp packet to get mac first.
Thanks
zhangchen
>
>> + }
>> } else if (ret == -1) {
>> qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>> }
>> @@ -176,9 +457,11 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
>>
>> ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
>> if (ret == 1) {
>> - /* TODO: enqueue to secondary packet list*/
>> - /* should we send sec arp pkt? */
>> - compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
>> + if (packet_enqueue(s, SECONDARY_IN)) {
>> + trace_colo_compare_main("secondary: unsupported packet in");
>> + /* should we send sec arp pkt? */
>> + compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
>> + }
>> } else if (ret == -1) {
>> qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>> }
>> @@ -280,6 +563,15 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>> qemu_chr_fe_claim_no_fail(s->chr_out);
>> QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>
>> + g_queue_init(&s->conn_list);
>> + qemu_mutex_init(&s->conn_list_lock);
>> + s->hashtable_size = 0;
>> +
>> + s->connection_track_table = g_hash_table_new_full(connection_key_hash,
>> + connection_key_equal,
>> + g_free,
>> + connection_destroy);
>> +
>> return;
>> }
>>
>> @@ -314,6 +606,7 @@ static void colo_compare_class_finalize(ObjectClass *oc, void *data)
>> if (!QTAILQ_EMPTY(&net_compares)) {
>> QTAILQ_REMOVE(&net_compares, s, next);
>> }
>> + qemu_mutex_destroy(&s->conn_list_lock);
>> }
>>
>> static void colo_compare_init(Object *obj)
>> diff --git a/trace-events b/trace-events
>> index ca7211b..8862288 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1916,3 +1916,6 @@ aspeed_vic_update_fiq(int flags) "Raising FIQ: %d"
>> aspeed_vic_update_irq(int flags) "Raising IRQ: %d"
>> aspeed_vic_read(uint64_t offset, unsigned size, uint32_t value) "From 0x%" PRIx64 " of size %u: 0x%" PRIx32
>> aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
>> +
>> +# net/colo-compare.c
>> +colo_compare_main(const char *chr) "chr: %s"
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet
2016-04-28 10:25 ` Zhang Chen
@ 2016-04-29 2:05 ` Jason Wang
2016-04-29 7:24 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-04-29 2:05 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/28/2016 06:25 PM, Zhang Chen wrote:
>>> +static Packet *packet_new(CompareState *s, const void *data,
>>> + int size, ConnectionKey *key)
>>> +{
>>> + Packet *pkt = g_slice_new(Packet);
>>> +
>>> + pkt->data = g_memdup(data, size);
>>> + pkt->size = size;
>>> + pkt->s = s;
>>> +
>>> + if (connection_key_init(pkt, key)) {
>>> + packet_destroy(pkt, NULL);
>>> + pkt = NULL;
>>> + }
>> Can we do connection_key_init() first, this can avoid packet_desctory()
>> if it fails.
>
> Do you mean we should call connection_key_init() first
> and then call packet_new()?
Yes, only when connection_key_init() succeed.
>
>
>>
>>> +
>>> + return pkt;
>>> +}
>>> +
>>> +/*
>>> + * Return 0 on success, if return -1 means the pkt
>>> + * is unsupported(arp and ipv6) and will be sent later
>>> + */
>>> +static int packet_enqueue(CompareState *s, int mode)
>>> +{
>>> + ConnectionKey key = {{ 0 } };
>>> + Packet *pkt = NULL;
>>> + Connection *conn;
>>> +
>>> + if (mode == PRIMARY_IN) {
>>> + pkt = packet_new(s, s->pri_rs.buf, s->pri_rs.packet_len,
>>> &key);
>>> + } else {
>>> + pkt = packet_new(s, s->sec_rs.buf, s->sec_rs.packet_len,
>>> &key);
>>> + }
>>> + if (!pkt) {
>>> + return -1;
>>> + }
>>> +
>>> + conn = connection_get(s, &key);
>>> + if (!conn->processing) {
>>> + qemu_mutex_lock(&s->conn_list_lock);
>>> + g_queue_push_tail(&s->conn_list, conn);
>>> + qemu_mutex_unlock(&s->conn_list_lock);
>>> + conn->processing = true;
>>> + }
>>> +
>>> + qemu_mutex_lock(&conn->list_lock);
>>> + if (mode == PRIMARY_IN) {
>>> + g_queue_push_tail(&conn->primary_list, pkt);
>>> + } else {
>>> + g_queue_push_tail(&conn->secondary_list, pkt);
>>> + }
>>> + qemu_mutex_unlock(&conn->list_lock);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void packet_destroy(void *opaque, void *user_data)
>>> +{
>>> + Packet *pkt = opaque;
>>> +
>>> + g_free(pkt->data);
>>> + g_slice_free(Packet, pkt);
>>> +}
>>> +
>>> static int compare_chr_send(CharDriverState *out,
>>> const uint8_t *buf,
>>> uint32_t size)
>>> @@ -158,8 +437,10 @@ static void compare_pri_chr_in(void *opaque,
>>> const uint8_t *buf, int size)
>>> ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
>>> if (ret == 1) {
>>> - /* FIXME: enqueue to primary packet list */
>>> - compare_chr_send(s->chr_out, s->pri_rs.buf,
>>> s->pri_rs.packet_len);
>>> + if (packet_enqueue(s, PRIMARY_IN)) {
>>> + trace_colo_compare_main("primary: unsupported packet in");
>>> + compare_chr_send(s->chr_out, s->pri_rs.buf,
>>> s->pri_rs.packet_len);
>> Looks like if a packet was not recognized by connection_key_init(), it
>> will be sent directly without comparing it with the packet sent from
>> secondary? Is this expected?
>
> Yes,we will send primary's arp packet to get mac first.
>
> Thanks
> zhangchen
But what if the packet was not arp?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet
2016-04-29 2:05 ` Jason Wang
@ 2016-04-29 7:24 ` Zhang Chen
0 siblings, 0 replies; 39+ messages in thread
From: Zhang Chen @ 2016-04-29 7:24 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/29/2016 10:05 AM, Jason Wang wrote:
> On 04/28/2016 06:25 PM, Zhang Chen wrote:
>>>> +static Packet *packet_new(CompareState *s, const void *data,
>>>> + int size, ConnectionKey *key)
>>>> +{
>>>> + Packet *pkt = g_slice_new(Packet);
>>>> +
>>>> + pkt->data = g_memdup(data, size);
>>>> + pkt->size = size;
>>>> + pkt->s = s;
>>>> +
>>>> + if (connection_key_init(pkt, key)) {
>>>> + packet_destroy(pkt, NULL);
>>>> + pkt = NULL;
>>>> + }
>>> Can we do connection_key_init() first, this can avoid packet_desctory()
>>> if it fails.
>> Do you mean we should call connection_key_init() first
>> and then call packet_new()?
> Yes, only when connection_key_init() succeed.
OK~ will fix in next.
>>>> +
>>>> + return pkt;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Return 0 on success, if return -1 means the pkt
>>>> + * is unsupported(arp and ipv6) and will be sent later
>>>> + */
>>>> +static int packet_enqueue(CompareState *s, int mode)
>>>> +{
>>>> + ConnectionKey key = {{ 0 } };
>>>> + Packet *pkt = NULL;
>>>> + Connection *conn;
>>>> +
>>>> + if (mode == PRIMARY_IN) {
>>>> + pkt = packet_new(s, s->pri_rs.buf, s->pri_rs.packet_len,
>>>> &key);
>>>> + } else {
>>>> + pkt = packet_new(s, s->sec_rs.buf, s->sec_rs.packet_len,
>>>> &key);
>>>> + }
>>>> + if (!pkt) {
>>>> + return -1;
>>>> + }
>>>> +
>>>> + conn = connection_get(s, &key);
>>>> + if (!conn->processing) {
>>>> + qemu_mutex_lock(&s->conn_list_lock);
>>>> + g_queue_push_tail(&s->conn_list, conn);
>>>> + qemu_mutex_unlock(&s->conn_list_lock);
>>>> + conn->processing = true;
>>>> + }
>>>> +
>>>> + qemu_mutex_lock(&conn->list_lock);
>>>> + if (mode == PRIMARY_IN) {
>>>> + g_queue_push_tail(&conn->primary_list, pkt);
>>>> + } else {
>>>> + g_queue_push_tail(&conn->secondary_list, pkt);
>>>> + }
>>>> + qemu_mutex_unlock(&conn->list_lock);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void packet_destroy(void *opaque, void *user_data)
>>>> +{
>>>> + Packet *pkt = opaque;
>>>> +
>>>> + g_free(pkt->data);
>>>> + g_slice_free(Packet, pkt);
>>>> +}
>>>> +
>>>> static int compare_chr_send(CharDriverState *out,
>>>> const uint8_t *buf,
>>>> uint32_t size)
>>>> @@ -158,8 +437,10 @@ static void compare_pri_chr_in(void *opaque,
>>>> const uint8_t *buf, int size)
>>>> ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
>>>> if (ret == 1) {
>>>> - /* FIXME: enqueue to primary packet list */
>>>> - compare_chr_send(s->chr_out, s->pri_rs.buf,
>>>> s->pri_rs.packet_len);
>>>> + if (packet_enqueue(s, PRIMARY_IN)) {
>>>> + trace_colo_compare_main("primary: unsupported packet in");
>>>> + compare_chr_send(s->chr_out, s->pri_rs.buf,
>>>> s->pri_rs.packet_len);
>>> Looks like if a packet was not recognized by connection_key_init(), it
>>> will be sent directly without comparing it with the packet sent from
>>> secondary? Is this expected?
>> Yes,we will send primary's arp packet to get mac first.
>>
>> Thanks
>> zhangchen
> But what if the packet was not arp?
>
>
> .
rarp packet will be sent, ip packet will be enqueue.
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread
2016-04-18 11:11 [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare Zhang Chen
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization Zhang Chen
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 2/4] colo-compare: track connection and enqueue packet Zhang Chen
@ 2016-04-18 11:11 ` Zhang Chen
2016-04-28 7:58 ` Jason Wang
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison Zhang Chen
2016-04-27 11:54 ` [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare Zhang Chen
4 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-18 11:11 UTC (permalink / raw)
To: qemu devel, Jason Wang
Cc: Zhang Chen, Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
if packets are same, we send primary packet and drop secondary
packet, otherwise notify COLO do checkpoint.
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
net/colo-compare.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++++
trace-events | 2 +
2 files changed, 128 insertions(+)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index dc57eac..4b5a2d4 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -26,6 +26,7 @@
#include "qemu/jhash.h"
#include "net/eth.h"
+#define DEBUG_TCP_COMPARE 1
#define TYPE_COLO_COMPARE "colo-compare"
#define COLO_COMPARE(obj) \
OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
@@ -90,6 +91,13 @@ typedef struct CompareState {
GQueue unprocessed_connections;
/* proxy current hash size */
uint32_t hashtable_size;
+
+ /* notify compare thread */
+ QemuEvent event;
+ /* compare thread, a thread for each NIC */
+ QemuThread thread;
+ int thread_status;
+
} CompareState;
typedef struct CompareClass {
@@ -132,6 +140,15 @@ enum {
SECONDARY_IN,
};
+enum {
+ /* compare thread isn't started */
+ COMPARE_THREAD_NONE,
+ /* compare thread is running */
+ COMPARE_THREAD_RUNNING,
+ /* compare thread exit */
+ COMPARE_THREAD_EXIT,
+};
+
static void packet_destroy(void *opaque, void *user_data);
static int compare_chr_send(CharDriverState *out,
const uint8_t *buf,
@@ -336,6 +353,94 @@ static void packet_destroy(void *opaque, void *user_data)
g_slice_free(Packet, pkt);
}
+static inline void colo_dump_packet(Packet *pkt)
+{
+ int i;
+ for (i = 0; i < pkt->size; i++) {
+ printf("%02x ", ((uint8_t *)pkt->data)[i]);
+ }
+ printf("\n");
+}
+
+/*
+ * The IP packets sent by primary and secondary
+ * will be compared in here
+ * TODO support ip fragment, Out-Of-Order
+ * return: 0 means packet same
+ * > 0 || < 0 means packet different
+ */
+static int colo_packet_compare(Packet *ppkt, Packet *spkt)
+{
+ trace_colo_compare_with_int("ppkt size", ppkt->size);
+ trace_colo_compare_with_char("ppkt ip_src", inet_ntoa(ppkt->ip->ip_src));
+ trace_colo_compare_with_char("ppkt ip_dst", inet_ntoa(ppkt->ip->ip_dst));
+ trace_colo_compare_with_int("spkt size", spkt->size);
+ trace_colo_compare_with_char("spkt ip_src", inet_ntoa(spkt->ip->ip_src));
+ trace_colo_compare_with_char("spkt ip_dst", inet_ntoa(spkt->ip->ip_dst));
+
+ if (ppkt->size == spkt->size) {
+ return memcmp(ppkt->data, spkt->data, spkt->size);
+ } else {
+ return -1;
+ }
+}
+
+static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
+{
+ trace_colo_compare_main("compare all");
+ return colo_packet_compare(ppkt, spkt);
+}
+
+/*
+ * called from the compare thread on the primary
+ * for compare connection
+ */
+static void colo_compare_connection(void *opaque, void *user_data)
+{
+ Connection *conn = opaque;
+ Packet *pkt = NULL;
+ GList *result = NULL;
+ int ret;
+
+ qemu_mutex_lock(&conn->list_lock);
+ while (!g_queue_is_empty(&conn->primary_list) &&
+ !g_queue_is_empty(&conn->secondary_list)) {
+ pkt = g_queue_pop_head(&conn->primary_list);
+ result = g_queue_find_custom(&conn->secondary_list,
+ pkt, (GCompareFunc)colo_packet_compare_all);
+
+ if (result) {
+ ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
+ if (ret < 0) {
+ error_report("colo_send_primary_packet failed");
+ }
+ trace_colo_compare_main("packet same and release packet");
+ g_queue_remove(&conn->secondary_list, result->data);
+ } else {
+ trace_colo_compare_main("packet different");
+ g_queue_push_head(&conn->primary_list, pkt);
+ /* TODO: colo_notify_checkpoint();*/
+ break;
+ }
+ }
+ qemu_mutex_unlock(&conn->list_lock);
+}
+
+static void *colo_compare_thread(void *opaque)
+{
+ CompareState *s = opaque;
+
+ while (s->thread_status == COMPARE_THREAD_RUNNING) {
+ qemu_event_wait(&s->event);
+ qemu_event_reset(&s->event);
+ qemu_mutex_lock(&s->conn_list_lock);
+ g_queue_foreach(&s->conn_list, colo_compare_connection, NULL);
+ qemu_mutex_unlock(&s->conn_list_lock);
+ }
+
+ return NULL;
+}
+
static int compare_chr_send(CharDriverState *out,
const uint8_t *buf,
uint32_t size)
@@ -440,6 +545,8 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
if (packet_enqueue(s, PRIMARY_IN)) {
trace_colo_compare_main("primary: unsupported packet in");
compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
+ } else {
+ qemu_event_set(&s->event);
}
} else if (ret == -1) {
qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
@@ -461,6 +568,8 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
trace_colo_compare_main("secondary: unsupported packet in");
/* should we send sec arp pkt? */
compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
+ } else {
+ qemu_event_set(&s->event);
}
} else if (ret == -1) {
qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
@@ -519,6 +628,8 @@ static void compare_set_outdev(Object *obj, const char *value, Error **errp)
static void colo_compare_complete(UserCreatable *uc, Error **errp)
{
CompareState *s = COLO_COMPARE(uc);
+ char thread_name[64];
+ static int compare_id;
if (!s->pri_indev || !s->sec_indev || !s->outdev) {
error_setg(errp, "colo compare needs 'primary_in' ,"
@@ -564,6 +675,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
QTAILQ_INSERT_TAIL(&net_compares, s, next);
g_queue_init(&s->conn_list);
+ qemu_event_init(&s->event, false);
qemu_mutex_init(&s->conn_list_lock);
s->hashtable_size = 0;
@@ -572,6 +684,13 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
g_free,
connection_destroy);
+ s->thread_status = COMPARE_THREAD_RUNNING;
+ sprintf(thread_name, "compare %d", compare_id);
+ qemu_thread_create(&s->thread, thread_name,
+ colo_compare_thread, s,
+ QEMU_THREAD_JOINABLE);
+ compare_id++;
+
return;
}
@@ -607,6 +726,13 @@ static void colo_compare_class_finalize(ObjectClass *oc, void *data)
QTAILQ_REMOVE(&net_compares, s, next);
}
qemu_mutex_destroy(&s->conn_list_lock);
+
+ if (s->thread.thread) {
+ s->thread_status = COMPARE_THREAD_EXIT;
+ qemu_event_set(&s->event);
+ qemu_thread_join(&s->thread);
+ }
+ qemu_event_destroy(&s->event);
}
static void colo_compare_init(Object *obj)
diff --git a/trace-events b/trace-events
index 8862288..978c47f 100644
--- a/trace-events
+++ b/trace-events
@@ -1919,3 +1919,5 @@ aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64
# net/colo-compare.c
colo_compare_main(const char *chr) "chr: %s"
+colo_compare_with_int(const char *sta, int size) ": %s = %d"
+colo_compare_with_char(const char *sta, const char *stb) ": %s = %s"
--
1.9.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread Zhang Chen
@ 2016-04-28 7:58 ` Jason Wang
2016-04-28 10:31 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-04-28 7:58 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/18/2016 07:11 PM, Zhang Chen wrote:
> if packets are same, we send primary packet and drop secondary
> packet, otherwise notify COLO do checkpoint.
>
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
> net/colo-compare.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> trace-events | 2 +
> 2 files changed, 128 insertions(+)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index dc57eac..4b5a2d4 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -26,6 +26,7 @@
> #include "qemu/jhash.h"
> #include "net/eth.h"
>
> +#define DEBUG_TCP_COMPARE 1
> #define TYPE_COLO_COMPARE "colo-compare"
> #define COLO_COMPARE(obj) \
> OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
> @@ -90,6 +91,13 @@ typedef struct CompareState {
> GQueue unprocessed_connections;
> /* proxy current hash size */
> uint32_t hashtable_size;
> +
> + /* notify compare thread */
> + QemuEvent event;
> + /* compare thread, a thread for each NIC */
> + QemuThread thread;
> + int thread_status;
> +
> } CompareState;
>
> typedef struct CompareClass {
> @@ -132,6 +140,15 @@ enum {
> SECONDARY_IN,
> };
>
> +enum {
> + /* compare thread isn't started */
> + COMPARE_THREAD_NONE,
> + /* compare thread is running */
> + COMPARE_THREAD_RUNNING,
> + /* compare thread exit */
> + COMPARE_THREAD_EXIT,
> +};
> +
> static void packet_destroy(void *opaque, void *user_data);
> static int compare_chr_send(CharDriverState *out,
> const uint8_t *buf,
> @@ -336,6 +353,94 @@ static void packet_destroy(void *opaque, void *user_data)
> g_slice_free(Packet, pkt);
> }
>
> +static inline void colo_dump_packet(Packet *pkt)
> +{
> + int i;
> + for (i = 0; i < pkt->size; i++) {
> + printf("%02x ", ((uint8_t *)pkt->data)[i]);
> + }
> + printf("\n");
Can we use something like qemu_hexdump() here?
> +}
> +
> +/*
> + * The IP packets sent by primary and secondary
> + * will be compared in here
> + * TODO support ip fragment, Out-Of-Order
> + * return: 0 means packet same
> + * > 0 || < 0 means packet different
> + */
> +static int colo_packet_compare(Packet *ppkt, Packet *spkt)
> +{
> + trace_colo_compare_with_int("ppkt size", ppkt->size);
> + trace_colo_compare_with_char("ppkt ip_src", inet_ntoa(ppkt->ip->ip_src));
> + trace_colo_compare_with_char("ppkt ip_dst", inet_ntoa(ppkt->ip->ip_dst));
> + trace_colo_compare_with_int("spkt size", spkt->size);
> + trace_colo_compare_with_char("spkt ip_src", inet_ntoa(spkt->ip->ip_src));
> + trace_colo_compare_with_char("spkt ip_dst", inet_ntoa(spkt->ip->ip_dst));
Can we use a single tracepoint here instead?
> +
> + if (ppkt->size == spkt->size) {
> + return memcmp(ppkt->data, spkt->data, spkt->size);
> + } else {
> + return -1;
> + }
> +}
> +
> +static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
> +{
> + trace_colo_compare_main("compare all");
> + return colo_packet_compare(ppkt, spkt);
Why need this?
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare connection
> + */
> +static void colo_compare_connection(void *opaque, void *user_data)
> +{
> + Connection *conn = opaque;
> + Packet *pkt = NULL;
> + GList *result = NULL;
> + int ret;
> +
> + qemu_mutex_lock(&conn->list_lock);
> + while (!g_queue_is_empty(&conn->primary_list) &&
> + !g_queue_is_empty(&conn->secondary_list)) {
> + pkt = g_queue_pop_head(&conn->primary_list);
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_all);
> +
> + if (result) {
> + ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
> + if (ret < 0) {
> + error_report("colo_send_primary_packet failed");
> + }
> + trace_colo_compare_main("packet same and release packet");
> + g_queue_remove(&conn->secondary_list, result->data);
> + } else {
> + trace_colo_compare_main("packet different");
> + g_queue_push_head(&conn->primary_list, pkt);
Is this possible that the packet from secondary has not been arrived on
time? If yes, do we still need to notify the checkpoint here?
> + /* TODO: colo_notify_checkpoint();*/
> + break;
> + }
> + }
> + qemu_mutex_unlock(&conn->list_lock);
> +}
> +
> +static void *colo_compare_thread(void *opaque)
> +{
> + CompareState *s = opaque;
> +
> + while (s->thread_status == COMPARE_THREAD_RUNNING) {
> + qemu_event_wait(&s->event);
> + qemu_event_reset(&s->event);
> + qemu_mutex_lock(&s->conn_list_lock);
> + g_queue_foreach(&s->conn_list, colo_compare_connection, NULL);
> + qemu_mutex_unlock(&s->conn_list_lock);
> + }
> +
> + return NULL;
> +}
> +
> static int compare_chr_send(CharDriverState *out,
> const uint8_t *buf,
> uint32_t size)
> @@ -440,6 +545,8 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
> if (packet_enqueue(s, PRIMARY_IN)) {
> trace_colo_compare_main("primary: unsupported packet in");
> compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
> + } else {
> + qemu_event_set(&s->event);
> }
> } else if (ret == -1) {
> qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> @@ -461,6 +568,8 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
> trace_colo_compare_main("secondary: unsupported packet in");
> /* should we send sec arp pkt? */
> compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
> + } else {
> + qemu_event_set(&s->event);
> }
> } else if (ret == -1) {
> qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> @@ -519,6 +628,8 @@ static void compare_set_outdev(Object *obj, const char *value, Error **errp)
> static void colo_compare_complete(UserCreatable *uc, Error **errp)
> {
> CompareState *s = COLO_COMPARE(uc);
> + char thread_name[64];
> + static int compare_id;
>
> if (!s->pri_indev || !s->sec_indev || !s->outdev) {
> error_setg(errp, "colo compare needs 'primary_in' ,"
> @@ -564,6 +675,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> QTAILQ_INSERT_TAIL(&net_compares, s, next);
>
> g_queue_init(&s->conn_list);
> + qemu_event_init(&s->event, false);
> qemu_mutex_init(&s->conn_list_lock);
> s->hashtable_size = 0;
>
> @@ -572,6 +684,13 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> g_free,
> connection_destroy);
>
> + s->thread_status = COMPARE_THREAD_RUNNING;
> + sprintf(thread_name, "compare %d", compare_id);
> + qemu_thread_create(&s->thread, thread_name,
> + colo_compare_thread, s,
> + QEMU_THREAD_JOINABLE);
> + compare_id++;
> +
> return;
> }
>
> @@ -607,6 +726,13 @@ static void colo_compare_class_finalize(ObjectClass *oc, void *data)
> QTAILQ_REMOVE(&net_compares, s, next);
> }
> qemu_mutex_destroy(&s->conn_list_lock);
> +
> + if (s->thread.thread) {
> + s->thread_status = COMPARE_THREAD_EXIT;
> + qemu_event_set(&s->event);
> + qemu_thread_join(&s->thread);
> + }
> + qemu_event_destroy(&s->event);
> }
>
> static void colo_compare_init(Object *obj)
> diff --git a/trace-events b/trace-events
> index 8862288..978c47f 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1919,3 +1919,5 @@ aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64
>
> # net/colo-compare.c
> colo_compare_main(const char *chr) "chr: %s"
> +colo_compare_with_int(const char *sta, int size) ": %s = %d"
> +colo_compare_with_char(const char *sta, const char *stb) ": %s = %s"
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread
2016-04-28 7:58 ` Jason Wang
@ 2016-04-28 10:31 ` Zhang Chen
2016-04-29 2:07 ` Jason Wang
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-28 10:31 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/28/2016 03:58 PM, Jason Wang wrote:
>
> On 04/18/2016 07:11 PM, Zhang Chen wrote:
>> if packets are same, we send primary packet and drop secondary
>> packet, otherwise notify COLO do checkpoint.
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>> net/colo-compare.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> trace-events | 2 +
>> 2 files changed, 128 insertions(+)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index dc57eac..4b5a2d4 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -26,6 +26,7 @@
>> #include "qemu/jhash.h"
>> #include "net/eth.h"
>>
>> +#define DEBUG_TCP_COMPARE 1
>> #define TYPE_COLO_COMPARE "colo-compare"
>> #define COLO_COMPARE(obj) \
>> OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
>> @@ -90,6 +91,13 @@ typedef struct CompareState {
>> GQueue unprocessed_connections;
>> /* proxy current hash size */
>> uint32_t hashtable_size;
>> +
>> + /* notify compare thread */
>> + QemuEvent event;
>> + /* compare thread, a thread for each NIC */
>> + QemuThread thread;
>> + int thread_status;
>> +
>> } CompareState;
>>
>> typedef struct CompareClass {
>> @@ -132,6 +140,15 @@ enum {
>> SECONDARY_IN,
>> };
>>
>> +enum {
>> + /* compare thread isn't started */
>> + COMPARE_THREAD_NONE,
>> + /* compare thread is running */
>> + COMPARE_THREAD_RUNNING,
>> + /* compare thread exit */
>> + COMPARE_THREAD_EXIT,
>> +};
>> +
>> static void packet_destroy(void *opaque, void *user_data);
>> static int compare_chr_send(CharDriverState *out,
>> const uint8_t *buf,
>> @@ -336,6 +353,94 @@ static void packet_destroy(void *opaque, void *user_data)
>> g_slice_free(Packet, pkt);
>> }
>>
>> +static inline void colo_dump_packet(Packet *pkt)
>> +{
>> + int i;
>> + for (i = 0; i < pkt->size; i++) {
>> + printf("%02x ", ((uint8_t *)pkt->data)[i]);
>> + }
>> + printf("\n");
> Can we use something like qemu_hexdump() here?
Thanks~~
I will change it to qemu_hexdump
>
>> +}
>> +
>> +/*
>> + * The IP packets sent by primary and secondary
>> + * will be compared in here
>> + * TODO support ip fragment, Out-Of-Order
>> + * return: 0 means packet same
>> + * > 0 || < 0 means packet different
>> + */
>> +static int colo_packet_compare(Packet *ppkt, Packet *spkt)
>> +{
>> + trace_colo_compare_with_int("ppkt size", ppkt->size);
>> + trace_colo_compare_with_char("ppkt ip_src", inet_ntoa(ppkt->ip->ip_src));
>> + trace_colo_compare_with_char("ppkt ip_dst", inet_ntoa(ppkt->ip->ip_dst));
>> + trace_colo_compare_with_int("spkt size", spkt->size);
>> + trace_colo_compare_with_char("spkt ip_src", inet_ntoa(spkt->ip->ip_src));
>> + trace_colo_compare_with_char("spkt ip_dst", inet_ntoa(spkt->ip->ip_dst));
> Can we use a single tracepoint here instead?
Yes,fix in next.
>
>> +
>> + if (ppkt->size == spkt->size) {
>> + return memcmp(ppkt->data, spkt->data, spkt->size);
>> + } else {
>> + return -1;
>> + }
>> +}
>> +
>> +static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
>> +{
>> + trace_colo_compare_main("compare all");
>> + return colo_packet_compare(ppkt, spkt);
> Why need this?
just temp name,will change in patch 4/4
>
>> +}
>> +
>> +/*
>> + * called from the compare thread on the primary
>> + * for compare connection
>> + */
>> +static void colo_compare_connection(void *opaque, void *user_data)
>> +{
>> + Connection *conn = opaque;
>> + Packet *pkt = NULL;
>> + GList *result = NULL;
>> + int ret;
>> +
>> + qemu_mutex_lock(&conn->list_lock);
>> + while (!g_queue_is_empty(&conn->primary_list) &&
>> + !g_queue_is_empty(&conn->secondary_list)) {
>> + pkt = g_queue_pop_head(&conn->primary_list);
>> + result = g_queue_find_custom(&conn->secondary_list,
>> + pkt, (GCompareFunc)colo_packet_compare_all);
>> +
>> + if (result) {
>> + ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
>> + if (ret < 0) {
>> + error_report("colo_send_primary_packet failed");
>> + }
>> + trace_colo_compare_main("packet same and release packet");
>> + g_queue_remove(&conn->secondary_list, result->data);
>> + } else {
>> + trace_colo_compare_main("packet different");
>> + g_queue_push_head(&conn->primary_list, pkt);
> Is this possible that the packet from secondary has not been arrived on
> time? If yes, do we still need to notify the checkpoint here?
Yes,the packet of secondary may not arrived.
we will hold primary packet to next periodic checkpoint
to flush it. and more, I consider to set a timer
to flush timeout(200ms???) packet like Dave's branch.
Thanks
zhangchen
>
>> + /* TODO: colo_notify_checkpoint();*/
>> + break;
>> + }
>> + }
>> + qemu_mutex_unlock(&conn->list_lock);
>> +}
>> +
>> +static void *colo_compare_thread(void *opaque)
>> +{
>> + CompareState *s = opaque;
>> +
>> + while (s->thread_status == COMPARE_THREAD_RUNNING) {
>> + qemu_event_wait(&s->event);
>> + qemu_event_reset(&s->event);
>> + qemu_mutex_lock(&s->conn_list_lock);
>> + g_queue_foreach(&s->conn_list, colo_compare_connection, NULL);
>> + qemu_mutex_unlock(&s->conn_list_lock);
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> static int compare_chr_send(CharDriverState *out,
>> const uint8_t *buf,
>> uint32_t size)
>> @@ -440,6 +545,8 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
>> if (packet_enqueue(s, PRIMARY_IN)) {
>> trace_colo_compare_main("primary: unsupported packet in");
>> compare_chr_send(s->chr_out, s->pri_rs.buf, s->pri_rs.packet_len);
>> + } else {
>> + qemu_event_set(&s->event);
>> }
>> } else if (ret == -1) {
>> qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>> @@ -461,6 +568,8 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
>> trace_colo_compare_main("secondary: unsupported packet in");
>> /* should we send sec arp pkt? */
>> compare_chr_send(s->chr_out, s->sec_rs.buf, s->sec_rs.packet_len);
>> + } else {
>> + qemu_event_set(&s->event);
>> }
>> } else if (ret == -1) {
>> qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>> @@ -519,6 +628,8 @@ static void compare_set_outdev(Object *obj, const char *value, Error **errp)
>> static void colo_compare_complete(UserCreatable *uc, Error **errp)
>> {
>> CompareState *s = COLO_COMPARE(uc);
>> + char thread_name[64];
>> + static int compare_id;
>>
>> if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>> error_setg(errp, "colo compare needs 'primary_in' ,"
>> @@ -564,6 +675,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>> QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>
>> g_queue_init(&s->conn_list);
>> + qemu_event_init(&s->event, false);
>> qemu_mutex_init(&s->conn_list_lock);
>> s->hashtable_size = 0;
>>
>> @@ -572,6 +684,13 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>> g_free,
>> connection_destroy);
>>
>> + s->thread_status = COMPARE_THREAD_RUNNING;
>> + sprintf(thread_name, "compare %d", compare_id);
>> + qemu_thread_create(&s->thread, thread_name,
>> + colo_compare_thread, s,
>> + QEMU_THREAD_JOINABLE);
>> + compare_id++;
>> +
>> return;
>> }
>>
>> @@ -607,6 +726,13 @@ static void colo_compare_class_finalize(ObjectClass *oc, void *data)
>> QTAILQ_REMOVE(&net_compares, s, next);
>> }
>> qemu_mutex_destroy(&s->conn_list_lock);
>> +
>> + if (s->thread.thread) {
>> + s->thread_status = COMPARE_THREAD_EXIT;
>> + qemu_event_set(&s->event);
>> + qemu_thread_join(&s->thread);
>> + }
>> + qemu_event_destroy(&s->event);
>> }
>>
>> static void colo_compare_init(Object *obj)
>> diff --git a/trace-events b/trace-events
>> index 8862288..978c47f 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1919,3 +1919,5 @@ aspeed_vic_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64
>>
>> # net/colo-compare.c
>> colo_compare_main(const char *chr) "chr: %s"
>> +colo_compare_with_int(const char *sta, int size) ": %s = %d"
>> +colo_compare_with_char(const char *sta, const char *stb) ": %s = %s"
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread
2016-04-28 10:31 ` Zhang Chen
@ 2016-04-29 2:07 ` Jason Wang
2016-04-29 8:28 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Jason Wang @ 2016-04-29 2:07 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/28/2016 06:31 PM, Zhang Chen wrote:
>>> +/*
>>> + * called from the compare thread on the primary
>>> + * for compare connection
>>> + */
>>> +static void colo_compare_connection(void *opaque, void *user_data)
>>> +{
>>> + Connection *conn = opaque;
>>> + Packet *pkt = NULL;
>>> + GList *result = NULL;
>>> + int ret;
>>> +
>>> + qemu_mutex_lock(&conn->list_lock);
>>> + while (!g_queue_is_empty(&conn->primary_list) &&
>>> + !g_queue_is_empty(&conn->secondary_list)) {
>>> + pkt = g_queue_pop_head(&conn->primary_list);
>>> + result = g_queue_find_custom(&conn->secondary_list,
>>> + pkt,
>>> (GCompareFunc)colo_packet_compare_all);
>>> +
>>> + if (result) {
>>> + ret = compare_chr_send(pkt->s->chr_out, pkt->data,
>>> pkt->size);
>>> + if (ret < 0) {
>>> + error_report("colo_send_primary_packet failed");
>>> + }
>>> + trace_colo_compare_main("packet same and release packet");
>>> + g_queue_remove(&conn->secondary_list, result->data);
>>> + } else {
>>> + trace_colo_compare_main("packet different");
>>> + g_queue_push_head(&conn->primary_list, pkt);
>> Is this possible that the packet from secondary has not been arrived on
>> time? If yes, do we still need to notify the checkpoint here?
>
> Yes,the packet of secondary may not arrived.
> we will hold primary packet to next periodic checkpoint
> to flush it. and more, I consider to set a timer
> to flush timeout(200ms???) packet like Dave's branch.
>
>
> Thanks
> zhangchen
I was wondering maybe you can merge or unify all other changes from
Dave's branch?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread
2016-04-29 2:07 ` Jason Wang
@ 2016-04-29 8:28 ` Zhang Chen
2016-04-29 11:20 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-04-29 8:28 UTC (permalink / raw)
To: Jason Wang, qemu devel
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
On 04/29/2016 10:07 AM, Jason Wang wrote:
>
> On 04/28/2016 06:31 PM, Zhang Chen wrote:
>>>> +/*
>>>> + * called from the compare thread on the primary
>>>> + * for compare connection
>>>> + */
>>>> +static void colo_compare_connection(void *opaque, void *user_data)
>>>> +{
>>>> + Connection *conn = opaque;
>>>> + Packet *pkt = NULL;
>>>> + GList *result = NULL;
>>>> + int ret;
>>>> +
>>>> + qemu_mutex_lock(&conn->list_lock);
>>>> + while (!g_queue_is_empty(&conn->primary_list) &&
>>>> + !g_queue_is_empty(&conn->secondary_list)) {
>>>> + pkt = g_queue_pop_head(&conn->primary_list);
>>>> + result = g_queue_find_custom(&conn->secondary_list,
>>>> + pkt,
>>>> (GCompareFunc)colo_packet_compare_all);
>>>> +
>>>> + if (result) {
>>>> + ret = compare_chr_send(pkt->s->chr_out, pkt->data,
>>>> pkt->size);
>>>> + if (ret < 0) {
>>>> + error_report("colo_send_primary_packet failed");
>>>> + }
>>>> + trace_colo_compare_main("packet same and release packet");
>>>> + g_queue_remove(&conn->secondary_list, result->data);
>>>> + } else {
>>>> + trace_colo_compare_main("packet different");
>>>> + g_queue_push_head(&conn->primary_list, pkt);
>>> Is this possible that the packet from secondary has not been arrived on
>>> time? If yes, do we still need to notify the checkpoint here?
>> Yes,the packet of secondary may not arrived.
>> we will hold primary packet to next periodic checkpoint
>> to flush it. and more, I consider to set a timer
>> to flush timeout(200ms???) packet like Dave's branch.
>>
>>
>> Thanks
>> zhangchen
> I was wondering maybe you can merge or unify all other changes from
> Dave's branch?
>
Yes, I will unify some codes from Dave's colo-proxy branch.
Thanks
Zhang Chen
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread
2016-04-29 8:28 ` Zhang Chen
@ 2016-04-29 11:20 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 39+ messages in thread
From: Dr. David Alan Gilbert @ 2016-04-29 11:20 UTC (permalink / raw)
To: Zhang Chen
Cc: Jason Wang, qemu devel, Li Zhijian, Gui jianfeng, Wen Congyang,
zhanghailiang, Yang Hongyang, eddie.dong
* Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>
>
> On 04/29/2016 10:07 AM, Jason Wang wrote:
> >
> >On 04/28/2016 06:31 PM, Zhang Chen wrote:
> >>>>+/*
> >>>>+ * called from the compare thread on the primary
> >>>>+ * for compare connection
> >>>>+ */
> >>>>+static void colo_compare_connection(void *opaque, void *user_data)
> >>>>+{
> >>>>+ Connection *conn = opaque;
> >>>>+ Packet *pkt = NULL;
> >>>>+ GList *result = NULL;
> >>>>+ int ret;
> >>>>+
> >>>>+ qemu_mutex_lock(&conn->list_lock);
> >>>>+ while (!g_queue_is_empty(&conn->primary_list) &&
> >>>>+ !g_queue_is_empty(&conn->secondary_list)) {
> >>>>+ pkt = g_queue_pop_head(&conn->primary_list);
> >>>>+ result = g_queue_find_custom(&conn->secondary_list,
> >>>>+ pkt,
> >>>>(GCompareFunc)colo_packet_compare_all);
> >>>>+
> >>>>+ if (result) {
> >>>>+ ret = compare_chr_send(pkt->s->chr_out, pkt->data,
> >>>>pkt->size);
> >>>>+ if (ret < 0) {
> >>>>+ error_report("colo_send_primary_packet failed");
> >>>>+ }
> >>>>+ trace_colo_compare_main("packet same and release packet");
> >>>>+ g_queue_remove(&conn->secondary_list, result->data);
> >>>>+ } else {
> >>>>+ trace_colo_compare_main("packet different");
> >>>>+ g_queue_push_head(&conn->primary_list, pkt);
> >>>Is this possible that the packet from secondary has not been arrived on
> >>>time? If yes, do we still need to notify the checkpoint here?
> >>Yes,the packet of secondary may not arrived.
> >>we will hold primary packet to next periodic checkpoint
> >>to flush it. and more, I consider to set a timer
> >>to flush timeout(200ms???) packet like Dave's branch.
> >>
> >>
> >>Thanks
> >>zhangchen
> >I was wondering maybe you can merge or unify all other changes from
> >Dave's branch?
> >
>
> Yes, I will unify some codes from Dave's colo-proxy branch.
Of course always check what I've written; some of that branch
was quite hacky itself so don't just assume it's good!
Dave
>
> Thanks
> Zhang Chen
>
> >.
> >
>
> --
> Thanks
> zhangchen
>
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison
2016-04-18 11:11 [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare Zhang Chen
` (2 preceding siblings ...)
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 3/4] colo-compare: introduce packet comparison thread Zhang Chen
@ 2016-04-18 11:11 ` Zhang Chen
2016-04-28 8:15 ` Jason Wang
2016-04-28 19:44 ` Dr. David Alan Gilbert
2016-04-27 11:54 ` [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare Zhang Chen
4 siblings, 2 replies; 39+ messages in thread
From: Zhang Chen @ 2016-04-18 11:11 UTC (permalink / raw)
To: qemu devel, Jason Wang
Cc: Zhang Chen, Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
net/colo-compare.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 154 insertions(+), 4 deletions(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 4b5a2d4..3dad461 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -385,9 +385,148 @@ static int colo_packet_compare(Packet *ppkt, Packet *spkt)
}
}
-static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
+/*
+ * called from the compare thread on the primary
+ * for compare tcp packet
+ * compare_tcp copied from Dr. David Alan Gilbert's branch
+ */
+static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
+{
+ struct tcphdr *ptcp, *stcp;
+ int res;
+ char *sdebug, *ddebug;
+ ptrdiff_t offset;
+
+ trace_colo_compare_main("compare tcp");
+ ptcp = (struct tcphdr *)ppkt->transport_layer;
+ stcp = (struct tcphdr *)spkt->transport_layer;
+
+ /* Initial is compare the whole packet */
+ offset = 12; /* Hack! Skip virtio header */
+
+ if (ptcp->th_flags == stcp->th_flags &&
+ ((ptcp->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
+ /* This is the syn/ack response from the guest to an incoming
+ * connection; the secondary won't have matched the sequence number
+ * Note: We should probably compare the IP level?
+ * Note hack: This already has the virtio offset
+ */
+ offset = sizeof(ptcp->th_ack) + (void *)&ptcp->th_ack - ppkt->data;
+ }
+ /* Note - we want to compare everything as long as it's not the syn/ack? */
+ assert(offset > 0);
+ assert(spkt->size > offset);
+
+ /* The 'identification' field in the IP header is *very* random
+ * it almost never matches. Fudge this by ignoring differences in
+ * unfragmented packets; they'll normally sort themselves out if different
+ * anyway, and it should recover at the TCP level.
+ * An alternative would be to get both the primary and secondary to rewrite
+ * somehow; but that would need some sync traffic to sync the state
+ */
+ if (ntohs(ppkt->ip->ip_off) & IP_DF) {
+ spkt->ip->ip_id = ppkt->ip->ip_id;
+ /* and the sum will be different if the IDs were different */
+ spkt->ip->ip_sum = ppkt->ip->ip_sum;
+ }
+
+ res = memcmp(ppkt->data + offset, spkt->data + offset,
+ (spkt->size - offset));
+
+ if (res && DEBUG_TCP_COMPARE) {
+ sdebug = strdup(inet_ntoa(ppkt->ip->ip_src));
+ ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst));
+ fprintf(stderr, "%s: src/dst: %s/%s offset=%zd p: seq/ack=%u/%u"
+ " s: seq/ack=%u/%u res=%d flags=%x/%x\n", __func__,
+ sdebug, ddebug, offset,
+ ntohl(ptcp->th_seq), ntohl(ptcp->th_ack),
+ ntohl(stcp->th_seq), ntohl(stcp->th_ack),
+ res, ptcp->th_flags, stcp->th_flags);
+ if (res && (ptcp->th_seq == stcp->th_seq)) {
+ trace_colo_compare_with_int("Primary len", ppkt->size);
+ colo_dump_packet(ppkt);
+ trace_colo_compare_with_int("Secondary len", spkt->size);
+ colo_dump_packet(spkt);
+ }
+ g_free(sdebug);
+ g_free(ddebug);
+ }
+
+ return res;
+}
+
+/*
+ * called from the compare thread on the primary
+ * for compare udp packet
+ */
+static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
+{
+ int ret = 1;
+
+ trace_colo_compare_main("compare udp");
+ ret = colo_packet_compare(ppkt, spkt);
+
+ if (ret) {
+ trace_colo_compare_main("primary udp");
+ colo_dump_packet(ppkt);
+ trace_colo_compare_main("secondary udp");
+ colo_dump_packet(spkt);
+ }
+
+ return ret;
+}
+
+/*
+ * called from the compare thread on the primary
+ * for compare icmp packet
+ */
+static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
+{
+ int network_length;
+ struct icmp *icmp_ppkt, *icmp_spkt;
+
+ trace_colo_compare_main("compare icmp");
+ network_length = (ppkt->ip->ip_hl & 0x0F) * 4;
+ icmp_ppkt = (struct icmp *)(ppkt->data + network_length + ETH_HLEN);
+ icmp_spkt = (struct icmp *)(spkt->data + network_length + ETH_HLEN);
+
+ if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) &&
+ (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) {
+ if (icmp_ppkt->icmp_type == ICMP_REDIRECT) {
+ if (icmp_ppkt->icmp_gwaddr.s_addr !=
+ icmp_spkt->icmp_gwaddr.s_addr) {
+ trace_colo_compare_main("icmp_gwaddr.s_addr not same");
+ trace_colo_compare_with_char("ppkt s_addr",
+ inet_ntoa(icmp_ppkt->icmp_gwaddr));
+ trace_colo_compare_with_char("spkt s_addr",
+ inet_ntoa(icmp_spkt->icmp_gwaddr));
+ return -1;
+ }
+ } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) &&
+ (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) {
+ if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) {
+ trace_colo_compare_main("icmp_nextmtu not same");
+ trace_colo_compare_with_int("ppkt s_addr",
+ icmp_ppkt->icmp_nextmtu);
+ trace_colo_compare_with_int("spkt s_addr",
+ icmp_spkt->icmp_nextmtu);
+ return -1;
+ }
+ }
+ } else {
+ return -1;
+ }
+
+ return 0;
+}
+
+/*
+ * called from the compare thread on the primary
+ * for compare other packet
+ */
+static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
{
- trace_colo_compare_main("compare all");
+ trace_colo_compare_main("compare other");
return colo_packet_compare(ppkt, spkt);
}
@@ -406,8 +545,19 @@ static void colo_compare_connection(void *opaque, void *user_data)
while (!g_queue_is_empty(&conn->primary_list) &&
!g_queue_is_empty(&conn->secondary_list)) {
pkt = g_queue_pop_head(&conn->primary_list);
- result = g_queue_find_custom(&conn->secondary_list,
- pkt, (GCompareFunc)colo_packet_compare_all);
+ if (conn->ip_proto == IPPROTO_TCP) {
+ result = g_queue_find_custom(&conn->secondary_list,
+ pkt, (GCompareFunc)colo_packet_compare_tcp);
+ } else if (conn->ip_proto == IPPROTO_UDP) {
+ result = g_queue_find_custom(&conn->secondary_list,
+ pkt, (GCompareFunc)colo_packet_compare_udp);
+ } else if (conn->ip_proto == IPPROTO_ICMP) {
+ result = g_queue_find_custom(&conn->secondary_list,
+ pkt, (GCompareFunc)colo_packet_compare_icmp);
+ } else {
+ result = g_queue_find_custom(&conn->secondary_list,
+ pkt, (GCompareFunc)colo_packet_compare_other);
+ }
if (result) {
ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
--
1.9.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison Zhang Chen
@ 2016-04-28 8:15 ` Jason Wang
2016-04-28 19:44 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 39+ messages in thread
From: Jason Wang @ 2016-04-28 8:15 UTC (permalink / raw)
To: Zhang Chen, qemu devel
Cc: Li Zhijian, Gui jianfeng, eddie.dong, zhanghailiang,
Dr. David Alan Gilbert, Yang Hongyang
On 04/18/2016 07:11 PM, Zhang Chen wrote:
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
> net/colo-compare.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 154 insertions(+), 4 deletions(-)
I'm not a tcp expert, this patch may need some guys who are good at TCP
to review.
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 4b5a2d4..3dad461 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -385,9 +385,148 @@ static int colo_packet_compare(Packet *ppkt, Packet *spkt)
> }
> }
>
> -static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
> +/*
> + * called from the compare thread on the primary
> + * for compare tcp packet
> + * compare_tcp copied from Dr. David Alan Gilbert's branch
> + */
> +static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
> +{
> + struct tcphdr *ptcp, *stcp;
> + int res;
> + char *sdebug, *ddebug;
> + ptrdiff_t offset;
> +
> + trace_colo_compare_main("compare tcp");
> + ptcp = (struct tcphdr *)ppkt->transport_layer;
> + stcp = (struct tcphdr *)spkt->transport_layer;
> +
> + /* Initial is compare the whole packet */
> + offset = 12; /* Hack! Skip virtio header */
So this won't work for e1000 I believe?
> +
> + if (ptcp->th_flags == stcp->th_flags &&
> + ((ptcp->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
> + /* This is the syn/ack response from the guest to an incoming
> + * connection; the secondary won't have matched the sequence number
> + * Note: We should probably compare the IP level?
> + * Note hack: This already has the virtio offset
> + */
> + offset = sizeof(ptcp->th_ack) + (void *)&ptcp->th_ack - ppkt->data;
> + }
> + /* Note - we want to compare everything as long as it's not the syn/ack? */
> + assert(offset > 0);
> + assert(spkt->size > offset);
> +
> + /* The 'identification' field in the IP header is *very* random
> + * it almost never matches. Fudge this by ignoring differences in
> + * unfragmented packets; they'll normally sort themselves out if different
> + * anyway, and it should recover at the TCP level.
> + * An alternative would be to get both the primary and secondary to rewrite
> + * somehow; but that would need some sync traffic to sync the state
This is very tricky I believe, I wonder this will work. Would it be
easier if we reassembly the packet?
And I believe we should check for the length before all the other
examinations?
> + */
> + if (ntohs(ppkt->ip->ip_off) & IP_DF) {
> + spkt->ip->ip_id = ppkt->ip->ip_id;
> + /* and the sum will be different if the IDs were different */
> + spkt->ip->ip_sum = ppkt->ip->ip_sum;
> + }
> +
> + res = memcmp(ppkt->data + offset, spkt->data + offset,
> + (spkt->size - offset));
> +
> + if (res && DEBUG_TCP_COMPARE) {
> + sdebug = strdup(inet_ntoa(ppkt->ip->ip_src));
> + ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst));
> + fprintf(stderr, "%s: src/dst: %s/%s offset=%zd p: seq/ack=%u/%u"
> + " s: seq/ack=%u/%u res=%d flags=%x/%x\n", __func__,
> + sdebug, ddebug, offset,
> + ntohl(ptcp->th_seq), ntohl(ptcp->th_ack),
> + ntohl(stcp->th_seq), ntohl(stcp->th_ack),
> + res, ptcp->th_flags, stcp->th_flags);
> + if (res && (ptcp->th_seq == stcp->th_seq)) {
> + trace_colo_compare_with_int("Primary len", ppkt->size);
> + colo_dump_packet(ppkt);
> + trace_colo_compare_with_int("Secondary len", spkt->size);
> + colo_dump_packet(spkt);
> + }
> + g_free(sdebug);
> + g_free(ddebug);
> + }
> +
> + return res;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare udp packet
> + */
> +static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
> +{
> + int ret = 1;
> +
> + trace_colo_compare_main("compare udp");
> + ret = colo_packet_compare(ppkt, spkt);
> +
> + if (ret) {
> + trace_colo_compare_main("primary udp");
> + colo_dump_packet(ppkt);
> + trace_colo_compare_main("secondary udp");
> + colo_dump_packet(spkt);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare icmp packet
> + */
> +static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
> +{
> + int network_length;
> + struct icmp *icmp_ppkt, *icmp_spkt;
> +
> + trace_colo_compare_main("compare icmp");
> + network_length = (ppkt->ip->ip_hl & 0x0F) * 4;
> + icmp_ppkt = (struct icmp *)(ppkt->data + network_length + ETH_HLEN);
> + icmp_spkt = (struct icmp *)(spkt->data + network_length + ETH_HLEN);
> +
> + if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) &&
> + (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) {
> + if (icmp_ppkt->icmp_type == ICMP_REDIRECT) {
> + if (icmp_ppkt->icmp_gwaddr.s_addr !=
> + icmp_spkt->icmp_gwaddr.s_addr) {
> + trace_colo_compare_main("icmp_gwaddr.s_addr not same");
> + trace_colo_compare_with_char("ppkt s_addr",
> + inet_ntoa(icmp_ppkt->icmp_gwaddr));
> + trace_colo_compare_with_char("spkt s_addr",
> + inet_ntoa(icmp_spkt->icmp_gwaddr));
> + return -1;
> + }
> + } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) &&
> + (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) {
> + if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) {
> + trace_colo_compare_main("icmp_nextmtu not same");
> + trace_colo_compare_with_int("ppkt s_addr",
> + icmp_ppkt->icmp_nextmtu);
> + trace_colo_compare_with_int("spkt s_addr",
> + icmp_spkt->icmp_nextmtu);
> + return -1;
> + }
> + }
> + } else {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare other packet
> + */
> +static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
> {
> - trace_colo_compare_main("compare all");
> + trace_colo_compare_main("compare other");
> return colo_packet_compare(ppkt, spkt);
> }
>
> @@ -406,8 +545,19 @@ static void colo_compare_connection(void *opaque, void *user_data)
> while (!g_queue_is_empty(&conn->primary_list) &&
> !g_queue_is_empty(&conn->secondary_list)) {
> pkt = g_queue_pop_head(&conn->primary_list);
> - result = g_queue_find_custom(&conn->secondary_list,
> - pkt, (GCompareFunc)colo_packet_compare_all);
> + if (conn->ip_proto == IPPROTO_TCP) {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_tcp);
> + } else if (conn->ip_proto == IPPROTO_UDP) {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_udp);
> + } else if (conn->ip_proto == IPPROTO_ICMP) {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_icmp);
> + } else {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_other);
> + }
>
A switch...case looks better.
> if (result) {
> ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison Zhang Chen
2016-04-28 8:15 ` Jason Wang
@ 2016-04-28 19:44 ` Dr. David Alan Gilbert
2016-05-05 3:03 ` Zhang Chen
1 sibling, 1 reply; 39+ messages in thread
From: Dr. David Alan Gilbert @ 2016-04-28 19:44 UTC (permalink / raw)
To: Zhang Chen
Cc: qemu devel, Jason Wang, Li Zhijian, Gui jianfeng, Wen Congyang,
zhanghailiang, Yang Hongyang, eddie.dong
* Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
> net/colo-compare.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 154 insertions(+), 4 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 4b5a2d4..3dad461 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -385,9 +385,148 @@ static int colo_packet_compare(Packet *ppkt, Packet *spkt)
> }
> }
>
> -static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
> +/*
> + * called from the compare thread on the primary
> + * for compare tcp packet
> + * compare_tcp copied from Dr. David Alan Gilbert's branch
> + */
> +static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
> +{
> + struct tcphdr *ptcp, *stcp;
> + int res;
> + char *sdebug, *ddebug;
> + ptrdiff_t offset;
> +
> + trace_colo_compare_main("compare tcp");
> + ptcp = (struct tcphdr *)ppkt->transport_layer;
> + stcp = (struct tcphdr *)spkt->transport_layer;
> +
> + /* Initial is compare the whole packet */
> + offset = 12; /* Hack! Skip virtio header */
So, when I post a set of patches and mark it saying that I know they've
got a lot of hacks in them, it's good for those reusing those patches
to check they need the hacks!
In my world I found I needed to skip over that header and I didn't understand
why; but hadn't figured out the details yet, and I'd added the 12 everywhere -
I think this is the only place you've got it, so it's almost certainly wrong.
> + if (ptcp->th_flags == stcp->th_flags &&
> + ((ptcp->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
> + /* This is the syn/ack response from the guest to an incoming
> + * connection; the secondary won't have matched the sequence number
> + * Note: We should probably compare the IP level?
> + * Note hack: This already has the virtio offset
> + */
> + offset = sizeof(ptcp->th_ack) + (void *)&ptcp->th_ack - ppkt->data;
> + }
> + /* Note - we want to compare everything as long as it's not the syn/ack? */
> + assert(offset > 0);
> + assert(spkt->size > offset);
> +
> + /* The 'identification' field in the IP header is *very* random
> + * it almost never matches. Fudge this by ignoring differences in
> + * unfragmented packets; they'll normally sort themselves out if different
> + * anyway, and it should recover at the TCP level.
> + * An alternative would be to get both the primary and secondary to rewrite
> + * somehow; but that would need some sync traffic to sync the state
> + */
> + if (ntohs(ppkt->ip->ip_off) & IP_DF) {
> + spkt->ip->ip_id = ppkt->ip->ip_id;
> + /* and the sum will be different if the IDs were different */
> + spkt->ip->ip_sum = ppkt->ip->ip_sum;
> + }
> +
> + res = memcmp(ppkt->data + offset, spkt->data + offset,
> + (spkt->size - offset));
> +
> + if (res && DEBUG_TCP_COMPARE) {
> + sdebug = strdup(inet_ntoa(ppkt->ip->ip_src));
> + ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst));
> + fprintf(stderr, "%s: src/dst: %s/%s offset=%zd p: seq/ack=%u/%u"
> + " s: seq/ack=%u/%u res=%d flags=%x/%x\n", __func__,
> + sdebug, ddebug, offset,
> + ntohl(ptcp->th_seq), ntohl(ptcp->th_ack),
> + ntohl(stcp->th_seq), ntohl(stcp->th_ack),
> + res, ptcp->th_flags, stcp->th_flags);
> + if (res && (ptcp->th_seq == stcp->th_seq)) {
> + trace_colo_compare_with_int("Primary len", ppkt->size);
> + colo_dump_packet(ppkt);
> + trace_colo_compare_with_int("Secondary len", spkt->size);
> + colo_dump_packet(spkt);
> + }
Try and use meaningful traceing for this - don't use a 'compare_with_int'
trace; but use a name that says what you're doing - for example
trace_colo_tcp_miscompare ; that way if you're running COLO and just
want to see why you're getting so many miscompares, you can look
at this without turning on all the rest of the debug.
Also, in my version instead of using a DEBUG_TCP macro, I again used
the trace system, so, my code here was:
if (trace_event_get_state(TRACE_COLO_PROXY_MISCOMPARE) && res) {
that means you can switch it on and off at runtime using the
trace system. Then just as it's running I can get to the (qemu) prompt
and do:
trace-event colo_proxy_miscompare on
and see what's happening without recompiling.
> + g_free(sdebug);
> + g_free(ddebug);
> + }
> +
> + return res;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare udp packet
> + */
> +static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
> +{
> + int ret = 1;
> +
> + trace_colo_compare_main("compare udp");
> + ret = colo_packet_compare(ppkt, spkt);
> +
> + if (ret) {
> + trace_colo_compare_main("primary udp");
> + colo_dump_packet(ppkt);
> + trace_colo_compare_main("secondary udp");
> + colo_dump_packet(spkt);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare icmp packet
> + */
> +static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
> +{
> + int network_length;
> + struct icmp *icmp_ppkt, *icmp_spkt;
> +
> + trace_colo_compare_main("compare icmp");
> + network_length = (ppkt->ip->ip_hl & 0x0F) * 4;
Do you need that & 0x0f - the definition in ip.h is ip_hl:4 ?
> + icmp_ppkt = (struct icmp *)(ppkt->data + network_length + ETH_HLEN);
> + icmp_spkt = (struct icmp *)(spkt->data + network_length + ETH_HLEN);
> +
> + if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) &&
> + (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) {
> + if (icmp_ppkt->icmp_type == ICMP_REDIRECT) {
Do you need to check the lengths again before reading any of these fields?
> + if (icmp_ppkt->icmp_gwaddr.s_addr !=
> + icmp_spkt->icmp_gwaddr.s_addr) {
> + trace_colo_compare_main("icmp_gwaddr.s_addr not same");
> + trace_colo_compare_with_char("ppkt s_addr",
> + inet_ntoa(icmp_ppkt->icmp_gwaddr));
> + trace_colo_compare_with_char("spkt s_addr",
> + inet_ntoa(icmp_spkt->icmp_gwaddr));
> + return -1;
> + }
> + } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) &&
> + (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) {
> + if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) {
> + trace_colo_compare_main("icmp_nextmtu not same");
> + trace_colo_compare_with_int("ppkt s_addr",
> + icmp_ppkt->icmp_nextmtu);
> + trace_colo_compare_with_int("spkt s_addr",
> + icmp_spkt->icmp_nextmtu);
> + return -1;
> + }
> + }
> + } else {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * called from the compare thread on the primary
> + * for compare other packet
> + */
> +static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
> {
> - trace_colo_compare_main("compare all");
> + trace_colo_compare_main("compare other");
Try and make the traces give you all the information you're likely to need - for
example, adding ip_proto here would be really useful for when you find you've
suddenly got a lot of miscompare compare others and want to figure out why.
> return colo_packet_compare(ppkt, spkt);
> }
>
> @@ -406,8 +545,19 @@ static void colo_compare_connection(void *opaque, void *user_data)
> while (!g_queue_is_empty(&conn->primary_list) &&
> !g_queue_is_empty(&conn->secondary_list)) {
> pkt = g_queue_pop_head(&conn->primary_list);
> - result = g_queue_find_custom(&conn->secondary_list,
> - pkt, (GCompareFunc)colo_packet_compare_all);
> + if (conn->ip_proto == IPPROTO_TCP) {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_tcp);
> + } else if (conn->ip_proto == IPPROTO_UDP) {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_udp);
> + } else if (conn->ip_proto == IPPROTO_ICMP) {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_icmp);
> + } else {
> + result = g_queue_find_custom(&conn->secondary_list,
> + pkt, (GCompareFunc)colo_packet_compare_other);
> + }
>
> if (result) {
> ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
> --
> 1.9.1
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison
2016-04-28 19:44 ` Dr. David Alan Gilbert
@ 2016-05-05 3:03 ` Zhang Chen
2016-05-05 3:10 ` Zhang Chen
0 siblings, 1 reply; 39+ messages in thread
From: Zhang Chen @ 2016-05-05 3:03 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu devel, Jason Wang, Li Zhijian, Gui jianfeng, Wen Congyang,
zhanghailiang, Yang Hongyang, eddie.dong
On 04/29/2016 03:44 AM, Dr. David Alan Gilbert wrote:
> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>> net/colo-compare.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 154 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index 4b5a2d4..3dad461 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -385,9 +385,148 @@ static int colo_packet_compare(Packet *ppkt, Packet *spkt)
>> }
>> }
>>
>> -static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
>> +/*
>> + * called from the compare thread on the primary
>> + * for compare tcp packet
>> + * compare_tcp copied from Dr. David Alan Gilbert's branch
>> + */
>> +static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
>> +{
>> + struct tcphdr *ptcp, *stcp;
>> + int res;
>> + char *sdebug, *ddebug;
>> + ptrdiff_t offset;
>> +
>> + trace_colo_compare_main("compare tcp");
>> + ptcp = (struct tcphdr *)ppkt->transport_layer;
>> + stcp = (struct tcphdr *)spkt->transport_layer;
>> +
>> + /* Initial is compare the whole packet */
>> + offset = 12; /* Hack! Skip virtio header */
> So, when I post a set of patches and mark it saying that I know they've
> got a lot of hacks in them, it's good for those reusing those patches
> to check they need the hacks!
>
> In my world I found I needed to skip over that header and I didn't understand
> why; but hadn't figured out the details yet, and I'd added the 12 everywhere -
> I think this is the only place you've got it, so it's almost certainly wrong.
I test in my world it hadn't that header,so if I remove the
12 offset,then the function is almost OK?
>
>> + if (ptcp->th_flags == stcp->th_flags &&
>> + ((ptcp->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
>> + /* This is the syn/ack response from the guest to an incoming
>> + * connection; the secondary won't have matched the sequence number
>> + * Note: We should probably compare the IP level?
>> + * Note hack: This already has the virtio offset
>> + */
>> + offset = sizeof(ptcp->th_ack) + (void *)&ptcp->th_ack - ppkt->data;
>> + }
>> + /* Note - we want to compare everything as long as it's not the syn/ack? */
>> + assert(offset > 0);
>> + assert(spkt->size > offset);
>> +
>> + /* The 'identification' field in the IP header is *very* random
>> + * it almost never matches. Fudge this by ignoring differences in
>> + * unfragmented packets; they'll normally sort themselves out if different
>> + * anyway, and it should recover at the TCP level.
>> + * An alternative would be to get both the primary and secondary to rewrite
>> + * somehow; but that would need some sync traffic to sync the state
>> + */
>> + if (ntohs(ppkt->ip->ip_off) & IP_DF) {
>> + spkt->ip->ip_id = ppkt->ip->ip_id;
>> + /* and the sum will be different if the IDs were different */
>> + spkt->ip->ip_sum = ppkt->ip->ip_sum;
>> + }
>> +
>> + res = memcmp(ppkt->data + offset, spkt->data + offset,
>> + (spkt->size - offset));
>> +
>> + if (res && DEBUG_TCP_COMPARE) {
>> + sdebug = strdup(inet_ntoa(ppkt->ip->ip_src));
>> + ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst));
>> + fprintf(stderr, "%s: src/dst: %s/%s offset=%zd p: seq/ack=%u/%u"
>> + " s: seq/ack=%u/%u res=%d flags=%x/%x\n", __func__,
>> + sdebug, ddebug, offset,
>> + ntohl(ptcp->th_seq), ntohl(ptcp->th_ack),
>> + ntohl(stcp->th_seq), ntohl(stcp->th_ack),
>> + res, ptcp->th_flags, stcp->th_flags);
>> + if (res && (ptcp->th_seq == stcp->th_seq)) {
>> + trace_colo_compare_with_int("Primary len", ppkt->size);
>> + colo_dump_packet(ppkt);
>> + trace_colo_compare_with_int("Secondary len", spkt->size);
>> + colo_dump_packet(spkt);
>> + }
> Try and use meaningful traceing for this - don't use a 'compare_with_int'
> trace; but use a name that says what you're doing - for example
> trace_colo_tcp_miscompare ; that way if you're running COLO and just
> want to see why you're getting so many miscompares, you can look
> at this without turning on all the rest of the debug.
OK,I will fix in next version.
>
> Also, in my version instead of using a DEBUG_TCP macro, I again used
> the trace system, so, my code here was:
>
> if (trace_event_get_state(TRACE_COLO_PROXY_MISCOMPARE) && res) {
>
> that means you can switch it on and off at runtime using the
> trace system. Then just as it's running I can get to the (qemu) prompt
> and do:
> trace-event colo_proxy_miscompare on
>
> and see what's happening without recompiling.
OK,I will fix.
>
>> + g_free(sdebug);
>> + g_free(ddebug);
>> + }
>> +
>> + return res;
>> +}
>> +
>> +/*
>> + * called from the compare thread on the primary
>> + * for compare udp packet
>> + */
>> +static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
>> +{
>> + int ret = 1;
>> +
>> + trace_colo_compare_main("compare udp");
>> + ret = colo_packet_compare(ppkt, spkt);
>> +
>> + if (ret) {
>> + trace_colo_compare_main("primary udp");
>> + colo_dump_packet(ppkt);
>> + trace_colo_compare_main("secondary udp");
>> + colo_dump_packet(spkt);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * called from the compare thread on the primary
>> + * for compare icmp packet
>> + */
>> +static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
>> +{
>> + int network_length;
>> + struct icmp *icmp_ppkt, *icmp_spkt;
>> +
>> + trace_colo_compare_main("compare icmp");
>> + network_length = (ppkt->ip->ip_hl & 0x0F) * 4;
> Do you need that & 0x0f - the definition in ip.h is ip_hl:4 ?
I will fix it in next version.
>
>> + icmp_ppkt = (struct icmp *)(ppkt->data + network_length + ETH_HLEN);
>> + icmp_spkt = (struct icmp *)(spkt->data + network_length + ETH_HLEN);
>> +
>> + if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) &&
>> + (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) {
>> + if (icmp_ppkt->icmp_type == ICMP_REDIRECT) {
> Do you need to check the lengths again before reading any of these fields?
OK, I will check it.
Thanks
Zhang Chen
>
>> + if (icmp_ppkt->icmp_gwaddr.s_addr !=
>> + icmp_spkt->icmp_gwaddr.s_addr) {
>> + trace_colo_compare_main("icmp_gwaddr.s_addr not same");
>> + trace_colo_compare_with_char("ppkt s_addr",
>> + inet_ntoa(icmp_ppkt->icmp_gwaddr));
>> + trace_colo_compare_with_char("spkt s_addr",
>> + inet_ntoa(icmp_spkt->icmp_gwaddr));
>> + return -1;
>> + }
>> + } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) &&
>> + (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) {
>> + if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) {
>> + trace_colo_compare_main("icmp_nextmtu not same");
>> + trace_colo_compare_with_int("ppkt s_addr",
>> + icmp_ppkt->icmp_nextmtu);
>> + trace_colo_compare_with_int("spkt s_addr",
>> + icmp_spkt->icmp_nextmtu);
>> + return -1;
>> + }
>> + }
>> + } else {
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * called from the compare thread on the primary
>> + * for compare other packet
>> + */
>> +static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
>> {
>> - trace_colo_compare_main("compare all");
>> + trace_colo_compare_main("compare other");
> Try and make the traces give you all the information you're likely to need - for
> example, adding ip_proto here would be really useful for when you find you've
> suddenly got a lot of miscompare compare others and want to figure out why.
>
>> return colo_packet_compare(ppkt, spkt);
>> }
>>
>> @@ -406,8 +545,19 @@ static void colo_compare_connection(void *opaque, void *user_data)
>> while (!g_queue_is_empty(&conn->primary_list) &&
>> !g_queue_is_empty(&conn->secondary_list)) {
>> pkt = g_queue_pop_head(&conn->primary_list);
>> - result = g_queue_find_custom(&conn->secondary_list,
>> - pkt, (GCompareFunc)colo_packet_compare_all);
>> + if (conn->ip_proto == IPPROTO_TCP) {
>> + result = g_queue_find_custom(&conn->secondary_list,
>> + pkt, (GCompareFunc)colo_packet_compare_tcp);
>> + } else if (conn->ip_proto == IPPROTO_UDP) {
>> + result = g_queue_find_custom(&conn->secondary_list,
>> + pkt, (GCompareFunc)colo_packet_compare_udp);
>> + } else if (conn->ip_proto == IPPROTO_ICMP) {
>> + result = g_queue_find_custom(&conn->secondary_list,
>> + pkt, (GCompareFunc)colo_packet_compare_icmp);
>> + } else {
>> + result = g_queue_find_custom(&conn->secondary_list,
>> + pkt, (GCompareFunc)colo_packet_compare_other);
>> + }
>>
>> if (result) {
>> ret = compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
>> --
>> 1.9.1
> Dave
>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
> .
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison
2016-05-05 3:03 ` Zhang Chen
@ 2016-05-05 3:10 ` Zhang Chen
0 siblings, 0 replies; 39+ messages in thread
From: Zhang Chen @ 2016-05-05 3:10 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Li Zhijian, Gui jianfeng, Jason Wang, eddie.dong, qemu devel,
Yang Hongyang, zhanghailiang
On 05/05/2016 11:03 AM, Zhang Chen wrote:
>
>
> On 04/29/2016 03:44 AM, Dr. David Alan Gilbert wrote:
>> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> ---
>>> net/colo-compare.c | 158
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 154 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> index 4b5a2d4..3dad461 100644
>>> --- a/net/colo-compare.c
>>> +++ b/net/colo-compare.c
>>> @@ -385,9 +385,148 @@ static int colo_packet_compare(Packet *ppkt,
>>> Packet *spkt)
>>> }
>>> }
>>> -static int colo_packet_compare_all(Packet *spkt, Packet *ppkt)
>>> +/*
>>> + * called from the compare thread on the primary
>>> + * for compare tcp packet
>>> + * compare_tcp copied from Dr. David Alan Gilbert's branch
>>> + */
>>> +static int colo_packet_compare_tcp(Packet *spkt, Packet *ppkt)
>>> +{
>>> + struct tcphdr *ptcp, *stcp;
>>> + int res;
>>> + char *sdebug, *ddebug;
>>> + ptrdiff_t offset;
>>> +
>>> + trace_colo_compare_main("compare tcp");
>>> + ptcp = (struct tcphdr *)ppkt->transport_layer;
>>> + stcp = (struct tcphdr *)spkt->transport_layer;
>>> +
>>> + /* Initial is compare the whole packet */
>>> + offset = 12; /* Hack! Skip virtio header */
>> So, when I post a set of patches and mark it saying that I know they've
>> got a lot of hacks in them, it's good for those reusing those patches
>> to check they need the hacks!
>>
>> In my world I found I needed to skip over that header and I didn't
>> understand
>> why; but hadn't figured out the details yet, and I'd added the 12
>> everywhere -
>> I think this is the only place you've got it, so it's almost
>> certainly wrong.
>
> I test in my world it hadn't that header,so if I remove the
> 12 offset,then the function is almost OK?
>
>>
>>> + if (ptcp->th_flags == stcp->th_flags &&
>>> + ((ptcp->th_flags & (TH_ACK | TH_SYN)) == (TH_ACK | TH_SYN))) {
>>> + /* This is the syn/ack response from the guest to an incoming
>>> + * connection; the secondary won't have matched the
>>> sequence number
>>> + * Note: We should probably compare the IP level?
>>> + * Note hack: This already has the virtio offset
>>> + */
>>> + offset = sizeof(ptcp->th_ack) + (void *)&ptcp->th_ack -
>>> ppkt->data;
>>> + }
>>> + /* Note - we want to compare everything as long as it's not the
>>> syn/ack? */
>>> + assert(offset > 0);
>>> + assert(spkt->size > offset);
>>> +
>>> + /* The 'identification' field in the IP header is *very* random
>>> + * it almost never matches. Fudge this by ignoring differences in
>>> + * unfragmented packets; they'll normally sort themselves out
>>> if different
>>> + * anyway, and it should recover at the TCP level.
>>> + * An alternative would be to get both the primary and
>>> secondary to rewrite
>>> + * somehow; but that would need some sync traffic to sync the
>>> state
>>> + */
>>> + if (ntohs(ppkt->ip->ip_off) & IP_DF) {
>>> + spkt->ip->ip_id = ppkt->ip->ip_id;
>>> + /* and the sum will be different if the IDs were different */
>>> + spkt->ip->ip_sum = ppkt->ip->ip_sum;
>>> + }
>>> +
>>> + res = memcmp(ppkt->data + offset, spkt->data + offset,
>>> + (spkt->size - offset));
>>> +
>>> + if (res && DEBUG_TCP_COMPARE) {
>>> + sdebug = strdup(inet_ntoa(ppkt->ip->ip_src));
>>> + ddebug = strdup(inet_ntoa(ppkt->ip->ip_dst));
>>> + fprintf(stderr, "%s: src/dst: %s/%s offset=%zd p:
>>> seq/ack=%u/%u"
>>> + " s: seq/ack=%u/%u res=%d flags=%x/%x\n", __func__,
>>> + sdebug, ddebug, offset,
>>> + ntohl(ptcp->th_seq), ntohl(ptcp->th_ack),
>>> + ntohl(stcp->th_seq), ntohl(stcp->th_ack),
>>> + res, ptcp->th_flags, stcp->th_flags);
>>> + if (res && (ptcp->th_seq == stcp->th_seq)) {
>>> + trace_colo_compare_with_int("Primary len", ppkt->size);
>>> + colo_dump_packet(ppkt);
>>> + trace_colo_compare_with_int("Secondary len", spkt->size);
>>> + colo_dump_packet(spkt);
>>> + }
>> Try and use meaningful traceing for this - don't use a
>> 'compare_with_int'
>> trace; but use a name that says what you're doing - for example
>> trace_colo_tcp_miscompare ; that way if you're running COLO and just
>> want to see why you're getting so many miscompares, you can look
>> at this without turning on all the rest of the debug.
>
> OK,I will fix in next version.
>
>>
>> Also, in my version instead of using a DEBUG_TCP macro, I again used
>> the trace system, so, my code here was:
>>
>> if (trace_event_get_state(TRACE_COLO_PROXY_MISCOMPARE) && res) {
>>
>> that means you can switch it on and off at runtime using the
>> trace system. Then just as it's running I can get to the (qemu) prompt
>> and do:
>> trace-event colo_proxy_miscompare on
>>
>> and see what's happening without recompiling.
>
> OK,I will fix.
>
>>
>>> + g_free(sdebug);
>>> + g_free(ddebug);
>>> + }
>>> +
>>> + return res;
>>> +}
>>> +
>>> +/*
>>> + * called from the compare thread on the primary
>>> + * for compare udp packet
>>> + */
>>> +static int colo_packet_compare_udp(Packet *spkt, Packet *ppkt)
>>> +{
>>> + int ret = 1;
>>> +
>>> + trace_colo_compare_main("compare udp");
>>> + ret = colo_packet_compare(ppkt, spkt);
>>> +
>>> + if (ret) {
>>> + trace_colo_compare_main("primary udp");
>>> + colo_dump_packet(ppkt);
>>> + trace_colo_compare_main("secondary udp");
>>> + colo_dump_packet(spkt);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +/*
>>> + * called from the compare thread on the primary
>>> + * for compare icmp packet
>>> + */
>>> +static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
>>> +{
>>> + int network_length;
>>> + struct icmp *icmp_ppkt, *icmp_spkt;
>>> +
>>> + trace_colo_compare_main("compare icmp");
>>> + network_length = (ppkt->ip->ip_hl & 0x0F) * 4;
>> Do you need that & 0x0f - the definition in ip.h is ip_hl:4 ?
>
> I will fix it in next version.
>
>>
>>> + icmp_ppkt = (struct icmp *)(ppkt->data + network_length +
>>> ETH_HLEN);
>>> + icmp_spkt = (struct icmp *)(spkt->data + network_length +
>>> ETH_HLEN);
>>> +
>>> + if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) &&
>>> + (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) {
>>> + if (icmp_ppkt->icmp_type == ICMP_REDIRECT) {
>> Do you need to check the lengths again before reading any of these
>> fields?
>
> OK, I will check it.
>
> Thanks
> Zhang Chen
>
>>
>>> + if (icmp_ppkt->icmp_gwaddr.s_addr !=
>>> + icmp_spkt->icmp_gwaddr.s_addr) {
>>> + trace_colo_compare_main("icmp_gwaddr.s_addr not
>>> same");
>>> + trace_colo_compare_with_char("ppkt s_addr",
>>> + inet_ntoa(icmp_ppkt->icmp_gwaddr));
>>> + trace_colo_compare_with_char("spkt s_addr",
>>> + inet_ntoa(icmp_spkt->icmp_gwaddr));
>>> + return -1;
>>> + }
>>> + } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) &&
>>> + (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) {
>>> + if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) {
>>> + trace_colo_compare_main("icmp_nextmtu not same");
>>> + trace_colo_compare_with_int("ppkt s_addr",
>>> + icmp_ppkt->icmp_nextmtu);
>>> + trace_colo_compare_with_int("spkt s_addr",
>>> + icmp_spkt->icmp_nextmtu);
>>> + return -1;
>>> + }
>>> + }
>>> + } else {
>>> + return -1;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * called from the compare thread on the primary
>>> + * for compare other packet
>>> + */
>>> +static int colo_packet_compare_other(Packet *spkt, Packet *ppkt)
>>> {
>>> - trace_colo_compare_main("compare all");
>>> + trace_colo_compare_main("compare other");
>> Try and make the traces give you all the information you're likely to
>> need - for
>> example, adding ip_proto here would be really useful for when you
>> find you've
>> suddenly got a lot of miscompare compare others and want to figure
>> out why.
OK,I will add more info.
Thanks
Zhang Chen
>>
>>> return colo_packet_compare(ppkt, spkt);
>>> }
>>> @@ -406,8 +545,19 @@ static void colo_compare_connection(void
>>> *opaque, void *user_data)
>>> while (!g_queue_is_empty(&conn->primary_list) &&
>>> !g_queue_is_empty(&conn->secondary_list)) {
>>> pkt = g_queue_pop_head(&conn->primary_list);
>>> - result = g_queue_find_custom(&conn->secondary_list,
>>> - pkt,
>>> (GCompareFunc)colo_packet_compare_all);
>>> + if (conn->ip_proto == IPPROTO_TCP) {
>>> + result = g_queue_find_custom(&conn->secondary_list,
>>> + pkt, (GCompareFunc)colo_packet_compare_tcp);
>>> + } else if (conn->ip_proto == IPPROTO_UDP) {
>>> + result = g_queue_find_custom(&conn->secondary_list,
>>> + pkt, (GCompareFunc)colo_packet_compare_udp);
>>> + } else if (conn->ip_proto == IPPROTO_ICMP) {
>>> + result = g_queue_find_custom(&conn->secondary_list,
>>> + pkt, (GCompareFunc)colo_packet_compare_icmp);
>>> + } else {
>>> + result = g_queue_find_custom(&conn->secondary_list,
>>> + pkt, (GCompareFunc)colo_packet_compare_other);
>>> + }
>>> if (result) {
>>> ret = compare_chr_send(pkt->s->chr_out, pkt->data,
>>> pkt->size);
>>> --
>>> 1.9.1
>> Dave
>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>>
>> .
>>
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare
2016-04-18 11:11 [Qemu-devel] [RFC PATCH V3 0/4] Introduce COLO-compare Zhang Chen
` (3 preceding siblings ...)
2016-04-18 11:11 ` [Qemu-devel] [RFC PATCH V3 4/4] colo-compare: add TCP, UDP, ICMP packet comparison Zhang Chen
@ 2016-04-27 11:54 ` Zhang Chen
4 siblings, 0 replies; 39+ messages in thread
From: Zhang Chen @ 2016-04-27 11:54 UTC (permalink / raw)
To: qemu devel, Jason Wang
Cc: Li Zhijian, Gui jianfeng, Wen Congyang, zhanghailiang,
Yang Hongyang, eddie.dong, Dr. David Alan Gilbert
Ping...
This patch has been no news for a week~~~
Thanks
zhang chen
On 04/18/2016 07:11 PM, Zhang Chen wrote:
> COLO-compare is a part of COLO project. It is used
> to compare the network package to help COLO decide
> whether to do checkpoint.
>
> We rebase colo-compare to colo-frame v2.7
> you can get the codes from github:
>
> https://github.com/zhangckid/qemu/tree/colo-v2.7-proxy-mode-apr18
>
>
>
> v3:
> - rebase colo-compare to colo-frame v2.7
> - fix most of Dave's comments
> (except RCU)
> - add TCP,UDP,ICMP and other packet comparison
> - add trace-event
> - add some comments
> - other bug fix
> - add RFC index
> - add usage in patch 1/4
>
> v2:
> - add jhash.h
>
> v1:
> - initial patch
>
>
> Zhang Chen (4):
> colo-compare: introduce colo compare initlization
> colo-compare: track connection and enqueue packet
> colo-compare: introduce packet comparison thread
> colo-compare: add TCP,UDP,ICMP packet comparison
>
> include/qemu/jhash.h | 59 ++++
> net/Makefile.objs | 1 +
> net/colo-compare.c | 930 +++++++++++++++++++++++++++++++++++++++++++++++++++
> qemu-options.hx | 6 +
> trace-events | 5 +
> vl.c | 3 +-
> 6 files changed, 1003 insertions(+), 1 deletion(-)
> create mode 100644 include/qemu/jhash.h
> create mode 100644 net/colo-compare.c
>
--
Thanks
zhangchen
^ permalink raw reply [flat|nested] 39+ messages in thread