From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMTWy-0001G7-0A for qemu-devel@nongnu.org; Mon, 11 Jul 2016 01:13:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMTWs-0005MV-AT for qemu-devel@nongnu.org; Mon, 11 Jul 2016 01:13:18 -0400 Received: from [59.151.112.132] (port=18665 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMTWq-0005L1-3M for qemu-devel@nongnu.org; Mon, 11 Jul 2016 01:13:14 -0400 References: <1466681677-30487-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <1466681677-30487-2-git-send-email-zhangchen.fnst@cn.fujitsu.com> <577F20C7.7000205@redhat.com> <577F6297.3050607@cn.fujitsu.com> <577F6E7A.1040000@redhat.com> From: Zhang Chen Message-ID: <57832B1D.1080408@cn.fujitsu.com> Date: Mon, 11 Jul 2016 13:14:05 +0800 MIME-Version: 1.0 In-Reply-To: <577F6E7A.1040000@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC PATCH V5 1/4] colo-compare: introduce colo compare initialization List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu devel Cc: "Dr . David Alan Gilbert" , "eddie . dong" , Li Zhijian , zhanghailiang On 07/08/2016 05:12 PM, Jason Wang wrote: > > > On 2016年07月08日 16:21, Zhang Chen wrote: >> >> >> On 07/08/2016 11:40 AM, Jason Wang wrote: >>> >>> >>> On 2016年06月23日 19:34, Zhang Chen wrote: >>>> Packets coming from the primary char indev will be sent to outdev >>>> Packets coming from the secondary char dev will be dropped >>>> >>>> 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 >>> >>> Consider we finally want a non-rfc patch, it's better to have a some >>> explanations on the above configurations since it was not easy to >>> for starters at first glance.Maybe you can use either a ascii figure >>> or a paragraph. Also need to explain the parameter of colo-compare >>> in detail. >> >> Make sense,I will add a ascii figure and some comments to explain it. >> >>> >>>> >>>> Signed-off-by: Zhang Chen >>>> Signed-off-by: Li Zhijian >>>> Signed-off-by: Wen Congyang >>>> --- >>>> net/Makefile.objs | 1 + >>>> net/colo-compare.c | 231 >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> qemu-options.hx | 34 ++++++++ >>>> vl.c | 3 +- >>>> 4 files changed, 268 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..a3e1456 >>>> --- /dev/null >>>> +++ b/net/colo-compare.c >>>> @@ -0,0 +1,231 @@ >>>> +/* >>>> + * COarse-grain LOck-stepping Virtual Machines for Non-stop >>>> Service (COLO) >>>> + * (a.k.a. Fault Tolerance or Continuous Replication) >>>> + * >>>> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD. >>>> + * Copyright (c) 2016 FUJITSU LIMITED >>>> + * Copyright (c) 2016 Intel Corporation >>>> + * >>>> + * Author: Zhang Chen >>>> + * >>>> + * 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/error-report.h" >>>> +#include "qemu-common.h" >>>> +#include "qapi/qmp/qerror.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" >>> >>> Looks like trace were not really used in the patch, you can delay >>> the inclusion until is was really used. >> >> OK~~~ >> >>> >>>> + >>>> +#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); >>> >>> What's the usage of this? A comment would be better. >> >> If we need compare more than one netdev,we should use >> more than one colo-compare. we do checkpoint should flush >> all enqueued packet in colo-compare when work with colo-frame. >> we use this queue to find all colo-compare. >> So, look like no need here, I will move it to after patch. > > Yes unless you want a single colo comparing threads to do comparing > for all netdevs. (But I agree, looks not need). > >> >> >>> >>>> + >>>> +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; >>>> + SocketReadState pri_rs; >>>> + SocketReadState sec_rs; >>>> +} CompareState; >>>> + >>>> +typedef struct CompareClass { >>>> + ObjectClass parent_class; >>>> +} CompareClass; >>>> + >>>> +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); >>> >>> I think we need do something more than this, e.g release the orig >>> dev and get the new one? Or just forbid setting this property. >>> >> >> Do you means that: >> qemu_chr_fe_release(s->chr_pri_in); >> >> If yes,in here we just get/set char* pri_indev(chardev's name). >> We don't get or set CharDriverState, so I think we needn't do more. > > Maybe I miss something, but is there any usage for just changing > chardev's name here? Yes, colo-compare get the name of chardev and save it. like primary_in=xxx, secondary_in=xxxx,outdev=xxxxx. > >> >> >>> And looks like we have similar issues for sec_indev and outdev. >>> >>>> +} >>>> + >>>> > > [...] > >>>> + >>>> +static void colo_compare_finalize(Object *obj) >>>> +{ >>>> + CompareState *s = COLO_COMPARE(obj); >>>> + >>>> + if (s->chr_pri_in) { >>>> + qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL); >>> >>> Why need do this? >> >> more safty before do qemu_chr_fe_release() for chardev >> like backends/rng-egd.c > > Ok, but looks like we don't set any handlers in the patch, so I don't > get why need to clear it. > > For the things of eng-egd, I think we need fail if it was not a socket > chardev. Vhost-use did similar check in net_vhost_chardev_opts() which > maybe useful for here (we probably need this for mirror/redirector too). > > [...] > > > . > -- Thanks zhangchen