From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44471) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLRpv-0007Lf-89 for qemu-devel@nongnu.org; Fri, 08 Jul 2016 05:12:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLRpq-0005rH-3B for qemu-devel@nongnu.org; Fri, 08 Jul 2016 05:12:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53915) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLRpp-0005qw-Qh for qemu-devel@nongnu.org; Fri, 08 Jul 2016 05:12:34 -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> From: Jason Wang Message-ID: <577F6E7A.1040000@redhat.com> Date: Fri, 8 Jul 2016 17:12:26 +0800 MIME-Version: 1.0 In-Reply-To: <577F6297.3050607@cn.fujitsu.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Zhang Chen , qemu devel Cc: "Dr . David Alan Gilbert" , "eddie . dong" , Li Zhijian , zhanghailiang On 2016=E5=B9=B407=E6=9C=8808=E6=97=A5 16:21, Zhang Chen wrote: > > > On 07/08/2016 11:40 AM, Jason Wang wrote: >> >> >> On 2016=E5=B9=B406=E6=9C=8823=E6=97=A5 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=20 >>> tap,id=3Dhn0,vhost=3Doff,script=3D/etc/qemu-ifup,downscript=3D/etc/qe= mu-ifdown >>> -device e1000,id=3De0,netdev=3Dhn0,mac=3D52:a4:00:12:78:66 >>> -chardev socket,id=3Dmirror0,host=3D3.3.3.3,port=3D9003,server,nowait >>> -chardev socket,id=3Dcompare1,host=3D3.3.3.3,port=3D9004,server,nowai= t >>> -chardev socket,id=3Dcompare0,host=3D3.3.3.3,port=3D9001,server,nowai= t >>> -chardev socket,id=3Dcompare0-0,host=3D3.3.3.3,port=3D9001 >>> -chardev socket,id=3Dcompare_out,host=3D3.3.3.3,port=3D9005,server,no= wait >>> -chardev socket,id=3Dcompare_out0,host=3D3.3.3.3,port=3D9005 >>> -object filter-mirror,id=3Dm0,netdev=3Dhn0,queue=3Dtx,outdev=3Dmirror= 0 >>> -object=20 >>> filter-redirector,netdev=3Dhn0,id=3Dredire0,queue=3Drx,indev=3Dcompar= e_out >>> -object=20 >>> filter-redirector,netdev=3Dhn0,id=3Dredire1,queue=3Drx,outdev=3Dcompa= re0 >>> -object=20 >>> colo-compare,id=3Dcomp0,primary_in=3Dcompare0-0,secondary_in=3Dcompar= e1,outdev=3Dcompare_out0 >>> >>> secondary: >>> -netdev tap,id=3Dhn0,vhost=3Doff,script=3D/etc/qemu-ifup,down=20 >>> script=3D/etc/qemu-ifdown >>> -device e1000,netdev=3Dhn0,mac=3D52:a4:00:12:78:66 >>> -chardev socket,id=3Dred0,host=3D3.3.3.3,port=3D9003 >>> -chardev socket,id=3Dred1,host=3D3.3.3.3,port=3D9004 >>> -object filter-redirector,id=3Df1,netdev=3Dhn0,queue=3Dtx,indev=3Dred= 0 >>> -object filter-redirector,id=3Df2,netdev=3Dhn0,queue=3Drx,outdev=3Dre= d1 >> >> Consider we finally want a non-rfc patch, it's better to have a some=20 >> explanations on the above configurations since it was not easy to for=20 >> starters at first glance.Maybe you can use either a ascii figure or a=20 >> paragraph. Also need to explain the parameter of colo-compare in detai= l. > > 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=20 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 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) +=3D netmap.o >>> common-obj-y +=3D filter.o >>> common-obj-y +=3D filter-buffer.o >>> common-obj-y +=3D filter-mirror.o >>> +common-obj-y +=3D 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=20 >>> (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 o= r >>> + * 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=20 >> 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 =3D >>> + 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=20 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 =3D COLO_COMPARE(obj); >>> + >>> + return g_strdup(s->pri_indev); >>> +} >>> + >>> +static void compare_set_pri_indev(Object *obj, const char *value,=20 >>> Error **errp) >>> +{ >>> + CompareState *s =3D COLO_COMPARE(obj); >>> + >>> + g_free(s->pri_indev); >>> + s->pri_indev =3D g_strdup(value); >> >> I think we need do something more than this, e.g release the orig dev=20 >> 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=20 chardev's name here? > > >> And looks like we have similar issues for sec_indev and outdev. >> >>> +} >>> + >>> [...] >>> + >>> +static void colo_compare_finalize(Object *obj) >>> +{ >>> + CompareState *s =3D 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=20 get why need to clear it. For the things of eng-egd, I think we need fail if it was not a socket=20 chardev. Vhost-use did similar check in net_vhost_chardev_opts() which=20 maybe useful for here (we probably need this for mirror/redirector too). [...]