Re: [Qemu-devel] [PATCH V2 1/3] colo-compare: introduce colo compare initlization

[Dr. David Alan Gilbert]

* 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