[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 1/3] colo-compare: introduce colo compare ini
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH V2 1/3] colo-compare: introduce colo compare initlization |
Date: |
Thu, 31 Mar 2016 10:24:16 +0100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
* Zhang Chen (address@hidden) wrote:
>
>
> On 03/30/2016 05:25 PM, Dr. David Alan Gilbert wrote:
> >* Zhang Chen (address@hidden) wrote:
> >>packet come from primary char indev will be send to
> >>outdev - packet come from secondary char dev will be drop
> >Please put in the description an example of how you invoke
> >the filter on the primary and secondary.
>
> OK.
> colo-compare get packet from chardev(primary_in,secondary_in),
> and output to other chardev(outdev), so, we can use it with the
> help of filter-mirror and filter-redirector. like that:
Wow, this is a bit more complicated than I expected - I was expecting just one
socket. So let me draw this out; see comments below.
> 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
----> mirror0: socket:secondary:9003
|
mirror-m0 <-- e1000
|
v
redirector-redire1 ---> compare0 socket:primary:9001 (to compare0-0)
-----< compare0-0 socket:primary:9001 (to compare0)
| primary_in
|
compare-comp0 -----> compare_out0 socket:primary:9005
|
| secondary_in
-----< compare1 socket:secondary:9004
tap <-- redirector-redire0 <--- compare_out socket:primary:9005 (from
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
----> red0 socket::9003
|
tap <-- redirector-f1 <--
e1000
--> redirector-f2 -->
|
----< red1 socket::9004
OK, so I think we need to find a way to fix two things:
a) There must be an easier way of connecting two filters within the same
qemu than going up to the kernel's socket code, around it's TCP stack
and back. Is there a better type of chardev to use we can short circuit
with - it shouldn't need to leave QEMU (although I can see it's easier
for debugging like this). Jason/Dan - any ideas?
b) We should only need one socket for the connection between primary and
secondary - I'm not sure how to change it to let it do that.
c) You need to be able to turn off nagling (socket no delay) on the sockets;
I found a noticeable benefit of doing this on the connection between
primary
and secondary in my world.
Dave
>
>
>
> >
> >>Signed-off-by: Li Zhijian <address@hidden>
> >>Signed-off-by: Zhang Chen <address@hidden>
> >>Signed-off-by: Wen Congyang <address@hidden>
> >>---
> >> net/Makefile.objs | 1 +
> >> net/colo-compare.c | 344
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> vl.c | 3 +-
> >> 3 files changed, 347 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..62c66df
> >>--- /dev/null
> >>+++ b/net/colo-compare.c
> >>@@ -0,0 +1,344 @@
> >>+/*
> >>+ * Copyright (c) 2016 FUJITSU LIMITED
> >>+ * Author: Zhang Chen <address@hidden>
> >>+ *
> >>+ * 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 "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"
> >>+
> >>+#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 */
> >>+ unsigned int index;
> >>+ unsigned int packet_len;
> >Please make packet_len and index uint32_t, since they're sent over the wire
> >as 32bit.
> >
> >>+ 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;
> >>+
> >>+static int compare_chr_send(CharDriverState *out, const uint8_t *buf, int
> >>size)
> >>+{
> >>+ int ret = 0;
> >>+ uint32_t len = htonl(size);
> >>+
> >Similarly, make 'size' uint32_t - everything that gets converted into a
> >uint32_t
> >it's probably best to make a uint32_t.
> >
> >>+ 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;
> >>+ }
> >>+
> >You can make this slightly simpler and save the return 0;
> >
> >>+ return 0;
> >>+
> >>+err:
> >>+ return ret < 0 ? ret : -EIO;
> >err:
> > return ret <= 0 ? ret : -EIO;
> >
> >>+}
> >>+
> >>+static int compare_chr_can_read(void *opaque)
> >>+{
> >>+ return COMPARE_READ_LEN_MAX;
> >>+}
> >>+
> >>+/* 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("serious error: oversized packet received.");
> >Isn't it easier to do this check above in the 'got length' if ?
> >Instead of 'serious error' say where it's coming from;
> > 'colo-compare: Received oversized packet over socket'
> >
> >that makes it a lot easier when people see the error for the first time.
> >Also, should you check for the error case of receiving a packet where
> >packet_len == 0 ?
> >
> >>+ 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;
> >>+}
> >>+
> >>+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, buf, size);
> >>+ } else if (ret == -1) {
> >>+ qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> >>+ }
> >>+}
> >>+
> >>+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*/
> >>+ } 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);
> >>+}
> >>+
> >>+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_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> >I think error_set seems to be discouraged these days, just use error_setg
> >(see the comment in include/qapi/error.h just above error_set).
> >
> >>+ "IN Device '%s' not found", s->pri_indev);
> >>+ goto out;
> >>+ }
> >>+
> >>+ 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);
> >>+
> >>+ s->chr_sec_in = qemu_chr_find(s->sec_indev);
> >>+ if (s->chr_sec_in == NULL) {
> >>+ error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> >>+ "IN Device '%s' not found", s->sec_indev);
> >>+ goto out;
> >>+ }
> >Can you explain/give an example of the way you create one of these
> >filters?
> >Would you ever have a pri_indev and sec_indev on the same filter?
> >If not, then why not just have an 'indev' option rather than the
> >two separate configs.
> >If you cna have both then you need to change hte error 'IN Device'
> >to say either 'Primary IN device' or secondary.
> >
> >>+ 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);
> >>+
> >>+ s->chr_out = qemu_chr_find(s->outdev);
> >>+ if (s->chr_out == NULL) {
> >>+ error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> >>+ "OUT Device '%s' not found", s->outdev);
> >>+ goto out;
> >>+ }
> >>+ qemu_chr_fe_claim_no_fail(s->chr_out);
> >>+
> >>+ QTAILQ_INSERT_TAIL(&net_compares, s, next);
> >>+
> >>+ return;
> >>+
> >>+out:
> >>+ 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);
> >>+ s->chr_pri_in = NULL;
> >>+ }
> >>+ 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);
> >>+ s->chr_pri_in = NULL;
> >>+ }
> >Can't you avoid this by making the code:
> >
> > s->chr_pri_in = qemu_chr_find(...)
> > if (s->chr_pri_in == NULL) {
> > error (...)
> > }
> > s->chr_sec_in = qemu_chr_find(...)
> > if (s->chr_sec_in == NULL) {
> > error (...)
> > }
> > s->chr_out = qemu_chr_find(...)
> > if (s->chr_out == NULL) {
> > error (...)
> > }
> >
> > qemu_chr_fe_claim_no_fail(pri)
> > add_handlers(pri...)
> > qemu_chr_fe_claim_no_fail(sec)
> > add_handlers(sec...)
> > qemu_chr_fe_claim_no_fail(out)
> > add_handlers(out...)
> >
> >so you don't have to clean up the handlers/release in the out: ?
> >
> >>+}
> >>+
> >>+static void colo_compare_class_init(ObjectClass *oc, void *data)
> >>+{
> >>+ UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> >>+
> >>+ ucc->complete = colo_compare_complete;
> >>+}
> >>+
> >>+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(CompareState),
> >>+ .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/vl.c b/vl.c
> >>index dc6e63a..70064ad 100644
> >>--- a/vl.c
> >>+++ b/vl.c
> >>@@ -2842,7 +2842,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
> >>
> >>
> >>
> >--
> >Dr. David Alan Gilbert / address@hidden / Manchester, UK
> >
> >
> >.
> >
>
> --
> Thanks
> zhangchen
>
>
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
[Qemu-devel] [PATCH V2 3/3] colo-compare: introduce packet comparison thread, Zhang Chen, 2016/03/30