| Message ID | 20211026181730.3102184-2-chen.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net/filter: Optimize filters vnet_hdr support | expand |
在 2021/10/27 上午2:17, Zhang Chen 写道: > Make the vnet header a necessary part of filter transfer protocol. > So remove the module's vnet_hdr_support switch here. > It make other modules(like another filter-redirector,colo-compare...) > know how to parse net packet correctly. If local device is not the > virtio-net-pci, vnet_hdr_len will be 0. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > net/filter-mirror.c | 88 ++++++++++----------------------------------- > qemu-options.hx | 14 ++++---- > 2 files changed, 25 insertions(+), 77 deletions(-) > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c > index f20240cc9f..4f0e26cc92 100644 > --- a/net/filter-mirror.c > +++ b/net/filter-mirror.c > @@ -39,7 +39,6 @@ struct MirrorState { > CharBackend chr_in; > CharBackend chr_out; > SocketReadState rs; > - bool vnet_hdr; > }; > > static int filter_send(MirrorState *s, > @@ -48,7 +47,7 @@ static int filter_send(MirrorState *s, > { > NetFilterState *nf = NETFILTER(s); > int ret = 0; > - ssize_t size = 0; > + ssize_t size = 0, vnet_hdr_len = 0; > uint32_t len = 0; > char *buf; > > @@ -63,21 +62,18 @@ static int filter_send(MirrorState *s, > goto err; > } > > - if (s->vnet_hdr) { > - /* > - * If vnet_hdr = on, we send vnet header len to make other > - * module(like colo-compare) know how to parse net > - * packet correctly. > - */ > - ssize_t vnet_hdr_len; > - > - vnet_hdr_len = nf->netdev->vnet_hdr_len; > + /* > + * The vnet header is the necessary part of filter transfer protocol. > + * It make other module(like colo-compare) know how to parse net > + * packet correctly. If device is not the virtio-net-pci, > + * vnet_hdr_len will be 0. > + */ > + vnet_hdr_len = nf->netdev->vnet_hdr_len; > > - len = htonl(vnet_hdr_len); > - ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); > - if (ret != sizeof(len)) { > - goto err; > - } > + len = htonl(vnet_hdr_len); > + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); > + if (ret != sizeof(len)) { > + goto err; > } > > buf = g_malloc(size); > @@ -232,6 +228,12 @@ static void redirector_rs_finalize(SocketReadState *rs) > MirrorState *s = container_of(rs, MirrorState, rs); > NetFilterState *nf = NETFILTER(s); > > + /* Check remote vnet_hdr */ > + if (rs->vnet_hdr_len != nf->netdev->vnet_hdr_len) { > + error_report("filter redirector got a different packet vnet_hdr" > + " from local, please check the -device configuration"); > + } > + > redirector_to_filter(nf, rs->buf, rs->packet_len); > } > > @@ -252,7 +254,7 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp) > } > } > > - net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr); > + net_socket_rs_init(&s->rs, redirector_rs_finalize, true); > > if (s->indev) { > chr = qemu_chr_find(s->indev); > @@ -323,20 +325,6 @@ static void filter_mirror_set_outdev(Object *obj, > } > } > > -static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp) > -{ > - MirrorState *s = FILTER_MIRROR(obj); > - > - return s->vnet_hdr; > -} > - > -static void filter_mirror_set_vnet_hdr(Object *obj, bool value, Error **errp) > -{ > - MirrorState *s = FILTER_MIRROR(obj); > - > - s->vnet_hdr = value; > -} > - > static char *filter_redirector_get_outdev(Object *obj, Error **errp) > { > MirrorState *s = FILTER_REDIRECTOR(obj); > @@ -354,31 +342,12 @@ static void filter_redirector_set_outdev(Object *obj, > s->outdev = g_strdup(value); > } > > -static bool filter_redirector_get_vnet_hdr(Object *obj, Error **errp) > -{ > - MirrorState *s = FILTER_REDIRECTOR(obj); > - > - return s->vnet_hdr; > -} > - > -static void filter_redirector_set_vnet_hdr(Object *obj, > - bool value, > - Error **errp) > -{ > - MirrorState *s = FILTER_REDIRECTOR(obj); > - > - s->vnet_hdr = value; > -} > - > static void filter_mirror_class_init(ObjectClass *oc, void *data) > { > NetFilterClass *nfc = NETFILTER_CLASS(oc); > > object_class_property_add_str(oc, "outdev", filter_mirror_get_outdev, > filter_mirror_set_outdev); > - object_class_property_add_bool(oc, "vnet_hdr_support", > - filter_mirror_get_vnet_hdr, > - filter_mirror_set_vnet_hdr); > > nfc->setup = filter_mirror_setup; > nfc->cleanup = filter_mirror_cleanup; > @@ -393,29 +362,12 @@ static void filter_redirector_class_init(ObjectClass *oc, void *data) > filter_redirector_set_indev); > object_class_property_add_str(oc, "outdev", filter_redirector_get_outdev, > filter_redirector_set_outdev); > - object_class_property_add_bool(oc, "vnet_hdr_support", > - filter_redirector_get_vnet_hdr, > - filter_redirector_set_vnet_hdr); > > nfc->setup = filter_redirector_setup; > nfc->cleanup = filter_redirector_cleanup; > nfc->receive_iov = filter_redirector_receive_iov; > } > > -static void filter_mirror_init(Object *obj) > -{ > - MirrorState *s = FILTER_MIRROR(obj); > - > - s->vnet_hdr = false; > -} > - > -static void filter_redirector_init(Object *obj) > -{ > - MirrorState *s = FILTER_REDIRECTOR(obj); > - > - s->vnet_hdr = false; > -} > - > static void filter_mirror_fini(Object *obj) > { > MirrorState *s = FILTER_MIRROR(obj); > @@ -435,7 +387,6 @@ static const TypeInfo filter_redirector_info = { > .name = TYPE_FILTER_REDIRECTOR, > .parent = TYPE_NETFILTER, > .class_init = filter_redirector_class_init, > - .instance_init = filter_redirector_init, > .instance_finalize = filter_redirector_fini, > .instance_size = sizeof(MirrorState), > }; > @@ -444,7 +395,6 @@ static const TypeInfo filter_mirror_info = { > .name = TYPE_FILTER_MIRROR, > .parent = TYPE_NETFILTER, > .class_init = filter_mirror_class_init, > - .instance_init = filter_mirror_init, > .instance_finalize = filter_mirror_fini, > .instance_size = sizeof(MirrorState), > }; > diff --git a/qemu-options.hx b/qemu-options.hx > index 5f375bbfa6..38c03812a7 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -4946,18 +4946,16 @@ SRST > > ``behind``: insert behind the specified filter (default). > > - ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]`` > + ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]`` I wonder if we break management layer. If yes, maybe it's better to keep the vnet_hdr_support here. Thanks > filter-mirror on netdev netdevid,mirror net packet to > - chardevchardevid, if it has the vnet\_hdr\_support flag, > - filter-mirror will mirror packet with vnet\_hdr\_len. > + chardev chardevid, filter-mirror will mirror packet with vnet\_hdr\_len. > > - ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]`` > + ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]`` > filter-redirector on netdev netdevid,redirect filter's net > packet to chardev chardevid,and redirect indev's packet to > - filter.if it has the vnet\_hdr\_support flag, filter-redirector > - will redirect packet with vnet\_hdr\_len. Create a > - filter-redirector we need to differ outdev id from indev id, id > - can not be the same. we can just use indev or outdev, but at > + filter. filter-redirector will redirect packet with vnet\_hdr\_len. > + Create a filter-redirector we need to differ outdev id from indev id, > + id can not be the same. we can just use indev or outdev, but at > least one of indev or outdev need to be specified. > > ``-object filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx,[vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
> -----Original Message----- > From: Jason Wang <jasowang@redhat.com> > Sent: Wednesday, October 27, 2021 12:46 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster > <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com> > Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror > and filter-redirector > > > 在 2021/10/27 上午2:17, Zhang Chen 写道: > > Make the vnet header a necessary part of filter transfer protocol. > > So remove the module's vnet_hdr_support switch here. > > It make other modules(like another filter-redirector,colo-compare...) > > know how to parse net packet correctly. If local device is not the > > virtio-net-pci, vnet_hdr_len will be 0. > > > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > > --- > > net/filter-mirror.c | 88 ++++++++++----------------------------------- > > qemu-options.hx | 14 ++++---- > > 2 files changed, 25 insertions(+), 77 deletions(-) > > > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index > > f20240cc9f..4f0e26cc92 100644 > > --- a/net/filter-mirror.c > > +++ b/net/filter-mirror.c > > @@ -39,7 +39,6 @@ struct MirrorState { > > CharBackend chr_in; > > CharBackend chr_out; > > SocketReadState rs; > > - bool vnet_hdr; > > }; > > > > static int filter_send(MirrorState *s, @@ -48,7 +47,7 @@ static int > > filter_send(MirrorState *s, > > { > > NetFilterState *nf = NETFILTER(s); > > int ret = 0; > > - ssize_t size = 0; > > + ssize_t size = 0, vnet_hdr_len = 0; > > uint32_t len = 0; > > char *buf; > > > > @@ -63,21 +62,18 @@ static int filter_send(MirrorState *s, > > goto err; > > } > > > > - if (s->vnet_hdr) { > > - /* > > - * If vnet_hdr = on, we send vnet header len to make other > > - * module(like colo-compare) know how to parse net > > - * packet correctly. > > - */ > > - ssize_t vnet_hdr_len; > > - > > - vnet_hdr_len = nf->netdev->vnet_hdr_len; > > + /* > > + * The vnet header is the necessary part of filter transfer protocol. > > + * It make other module(like colo-compare) know how to parse net > > + * packet correctly. If device is not the virtio-net-pci, > > + * vnet_hdr_len will be 0. > > + */ > > + vnet_hdr_len = nf->netdev->vnet_hdr_len; > > > > - len = htonl(vnet_hdr_len); > > - ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, > sizeof(len)); > > - if (ret != sizeof(len)) { > > - goto err; > > - } > > + len = htonl(vnet_hdr_len); > > + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); > > + if (ret != sizeof(len)) { > > + goto err; > > } > > > > buf = g_malloc(size); > > @@ -232,6 +228,12 @@ static void redirector_rs_finalize(SocketReadState > *rs) > > MirrorState *s = container_of(rs, MirrorState, rs); > > NetFilterState *nf = NETFILTER(s); > > > > + /* Check remote vnet_hdr */ > > + if (rs->vnet_hdr_len != nf->netdev->vnet_hdr_len) { > > + error_report("filter redirector got a different packet vnet_hdr" > > + " from local, please check the -device configuration"); > > + } > > + > > redirector_to_filter(nf, rs->buf, rs->packet_len); > > } > > > > @@ -252,7 +254,7 @@ static void filter_redirector_setup(NetFilterState > *nf, Error **errp) > > } > > } > > > > - net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr); > > + net_socket_rs_init(&s->rs, redirector_rs_finalize, true); > > > > if (s->indev) { > > chr = qemu_chr_find(s->indev); @@ -323,20 +325,6 @@ static > > void filter_mirror_set_outdev(Object *obj, > > } > > } > > > > -static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp) -{ > > - MirrorState *s = FILTER_MIRROR(obj); > > - > > - return s->vnet_hdr; > > -} > > - > > -static void filter_mirror_set_vnet_hdr(Object *obj, bool value, Error > > **errp) -{ > > - MirrorState *s = FILTER_MIRROR(obj); > > - > > - s->vnet_hdr = value; > > -} > > - > > static char *filter_redirector_get_outdev(Object *obj, Error **errp) > > { > > MirrorState *s = FILTER_REDIRECTOR(obj); @@ -354,31 +342,12 @@ > > static void filter_redirector_set_outdev(Object *obj, > > s->outdev = g_strdup(value); > > } > > > > -static bool filter_redirector_get_vnet_hdr(Object *obj, Error **errp) > > -{ > > - MirrorState *s = FILTER_REDIRECTOR(obj); > > - > > - return s->vnet_hdr; > > -} > > - > > -static void filter_redirector_set_vnet_hdr(Object *obj, > > - bool value, > > - Error **errp) > > -{ > > - MirrorState *s = FILTER_REDIRECTOR(obj); > > - > > - s->vnet_hdr = value; > > -} > > - > > static void filter_mirror_class_init(ObjectClass *oc, void *data) > > { > > NetFilterClass *nfc = NETFILTER_CLASS(oc); > > > > object_class_property_add_str(oc, "outdev", filter_mirror_get_outdev, > > filter_mirror_set_outdev); > > - object_class_property_add_bool(oc, "vnet_hdr_support", > > - filter_mirror_get_vnet_hdr, > > - filter_mirror_set_vnet_hdr); > > > > nfc->setup = filter_mirror_setup; > > nfc->cleanup = filter_mirror_cleanup; @@ -393,29 +362,12 @@ > > static void filter_redirector_class_init(ObjectClass *oc, void *data) > > filter_redirector_set_indev); > > object_class_property_add_str(oc, "outdev", > filter_redirector_get_outdev, > > filter_redirector_set_outdev); > > - object_class_property_add_bool(oc, "vnet_hdr_support", > > - filter_redirector_get_vnet_hdr, > > - filter_redirector_set_vnet_hdr); > > > > nfc->setup = filter_redirector_setup; > > nfc->cleanup = filter_redirector_cleanup; > > nfc->receive_iov = filter_redirector_receive_iov; > > } > > > > -static void filter_mirror_init(Object *obj) -{ > > - MirrorState *s = FILTER_MIRROR(obj); > > - > > - s->vnet_hdr = false; > > -} > > - > > -static void filter_redirector_init(Object *obj) -{ > > - MirrorState *s = FILTER_REDIRECTOR(obj); > > - > > - s->vnet_hdr = false; > > -} > > - > > static void filter_mirror_fini(Object *obj) > > { > > MirrorState *s = FILTER_MIRROR(obj); @@ -435,7 +387,6 @@ static > > const TypeInfo filter_redirector_info = { > > .name = TYPE_FILTER_REDIRECTOR, > > .parent = TYPE_NETFILTER, > > .class_init = filter_redirector_class_init, > > - .instance_init = filter_redirector_init, > > .instance_finalize = filter_redirector_fini, > > .instance_size = sizeof(MirrorState), > > }; > > @@ -444,7 +395,6 @@ static const TypeInfo filter_mirror_info = { > > .name = TYPE_FILTER_MIRROR, > > .parent = TYPE_NETFILTER, > > .class_init = filter_mirror_class_init, > > - .instance_init = filter_mirror_init, > > .instance_finalize = filter_mirror_fini, > > .instance_size = sizeof(MirrorState), > > }; > > diff --git a/qemu-options.hx b/qemu-options.hx index > > 5f375bbfa6..38c03812a7 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -4946,18 +4946,16 @@ SRST > > > > ``behind``: insert behind the specified filter (default). > > > > - ``-object filter- > mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr > _support][,position=head|tail|id=<id>][,insert=behind|before]`` > > + ``-object > > + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx > > + [,position=head|tail|id=<id>][,insert=behind|before]`` > > > I wonder if we break management layer. If yes, maybe it's better to keep the > vnet_hdr_support here. Yes and no, With this series of patches, filters have ability to automatically Configure the appropriate vnet_hdr_support flag according to the current environment. And can report error when can't fixing the vnet_hdr(The user cannot fix it from the previous way ). So I think no need for the user to configure this option, some relevant background knowledge required. For the management layer, keep the vnet_hdr_support may be meaningless except for compatibility. In this situation, Do you think we still need to keep the vnet_hdr_support for management layer? Enable/disable it do the same things for filters. Thanks Chen > > Thanks > > > > filter-mirror on netdev netdevid,mirror net packet to > > - chardevchardevid, if it has the vnet\_hdr\_support flag, > > - filter-mirror will mirror packet with vnet\_hdr\_len. > > + chardev chardevid, filter-mirror will mirror packet with > vnet\_hdr\_len. > > > > - ``-object filter- > redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queu > e=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind > |before]`` > > + ``-object > > + filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chard > > + evid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|bef > > + ore]`` > > filter-redirector on netdev netdevid,redirect filter's net > > packet to chardev chardevid,and redirect indev's packet to > > - filter.if it has the vnet\_hdr\_support flag, filter-redirector > > - will redirect packet with vnet\_hdr\_len. Create a > > - filter-redirector we need to differ outdev id from indev id, id > > - can not be the same. we can just use indev or outdev, but at > > + filter. filter-redirector will redirect packet with vnet\_hdr\_len. > > + Create a filter-redirector we need to differ outdev id from indev id, > > + id can not be the same. we can just use indev or outdev, but > > + at > > least one of indev or outdev need to be specified. > > > > ``-object > > filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx,[vnet_hdr_suppor > > t][,position=head|tail|id=<id>][,insert=behind|before]``
在 2021/10/27 下午2:19, Zhang, Chen 写道: >> mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr >> _support][,position=head|tail|id=<id>][,insert=behind|before]`` >>> + ``-object >>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx >>> + [,position=head|tail|id=<id>][,insert=behind|before]`` >> I wonder if we break management layer. If yes, maybe it's better to keep the >> vnet_hdr_support here. > Yes and no, With this series of patches, filters have ability to automatically > Configure the appropriate vnet_hdr_support flag according to the current environment. > And can report error when can't fixing the vnet_hdr(The user cannot fix it from the previous way ). > So I think no need for the user to configure this option, some relevant background knowledge required. > > For the management layer, keep the vnet_hdr_support may be meaningless except for compatibility. > In this situation, Do you think we still need to keep the vnet_hdr_support for management layer? So it depends on whether management layer like libvirt has already supported this. If yes, we may get errors using new qemu with old libvirt? Thanks > Enable/disable it do the same things for filters. > > Thanks > Chen >
> -----Original Message----- > From: Jason Wang <jasowang@redhat.com> > Sent: Wednesday, October 27, 2021 2:24 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster > <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com> > Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror > and filter-redirector > > > 在 2021/10/27 下午2:19, Zhang, Chen 写道: > >> > mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_h > >> dr _support][,position=head|tail|id=<id>][,insert=behind|before]`` > >>> + ``-object > >>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx| > >>> + tx [,position=head|tail|id=<id>][,insert=behind|before]`` > >> I wonder if we break management layer. If yes, maybe it's better to > >> keep the vnet_hdr_support here. > > Yes and no, With this series of patches, filters have ability to automatically > > Configure the appropriate vnet_hdr_support flag according to the current > environment. > > And can report error when can't fixing the vnet_hdr(The user cannot fix it > from the previous way ). > > So I think no need for the user to configure this option, some relevant > background knowledge required. > > > > For the management layer, keep the vnet_hdr_support may be > meaningless except for compatibility. > > In this situation, Do you think we still need to keep the vnet_hdr_support > for management layer? > > > So it depends on whether management layer like libvirt has already > supported this. If yes, we may get errors using new qemu with old libvirt? As far as I know, Current management layer like upstream libvirt is no COLO official support yet. And some real CSPs use libvirt passthrough qmp command to Qemu for manage COLO VM. It is no harm to users to reduce some unnecessary parameters. But if you think compatibility is more important, I will restore this parameter in next version. Thanks Chen > > Thanks > > > Enable/disable it do the same things for filters. > > > > Thanks > > Chen > >
On Wed, Oct 27, 2021 at 2:40 PM Zhang, Chen <chen.zhang@intel.com> wrote: > > > > > -----Original Message----- > > From: Jason Wang <jasowang@redhat.com> > > Sent: Wednesday, October 27, 2021 2:24 PM > > To: Zhang, Chen <chen.zhang@intel.com> > > Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster > > <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com> > > Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror > > and filter-redirector > > > > > > 在 2021/10/27 下午2:19, Zhang, Chen 写道: > > >> > > mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_h > > >> dr _support][,position=head|tail|id=<id>][,insert=behind|before]`` > > >>> + ``-object > > >>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx| > > >>> + tx [,position=head|tail|id=<id>][,insert=behind|before]`` > > >> I wonder if we break management layer. If yes, maybe it's better to > > >> keep the vnet_hdr_support here. > > > Yes and no, With this series of patches, filters have ability to automatically > > > Configure the appropriate vnet_hdr_support flag according to the current > > environment. > > > And can report error when can't fixing the vnet_hdr(The user cannot fix it > > from the previous way ). > > > So I think no need for the user to configure this option, some relevant > > background knowledge required. > > > > > > For the management layer, keep the vnet_hdr_support may be > > meaningless except for compatibility. > > > In this situation, Do you think we still need to keep the vnet_hdr_support > > for management layer? > > > > > > So it depends on whether management layer like libvirt has already > > supported this. If yes, we may get errors using new qemu with old libvirt? > > As far as I know, Current management layer like upstream libvirt is no COLO official support yet. > And some real CSPs use libvirt passthrough qmp command to Qemu for manage COLO VM. So the question still, it looks to me it requires the modification of the layers on top of libvirt? If the answer is yes, we'd better keep that compatibility. > It is no harm to users to reduce some unnecessary parameters. But if you think compatibility is > more important, I will restore this parameter in next version. Thanks > > Thanks > Chen > > > > > > > > Thanks > > > > > Enable/disable it do the same things for filters. > > > > > > Thanks > > > Chen > > > >
> -----Original Message----- > From: Jason Wang <jasowang@redhat.com> > Sent: Wednesday, October 27, 2021 2:45 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster > <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com> > Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror > and filter-redirector > > On Wed, Oct 27, 2021 at 2:40 PM Zhang, Chen <chen.zhang@intel.com> > wrote: > > > > > > > > > -----Original Message----- > > > From: Jason Wang <jasowang@redhat.com> > > > Sent: Wednesday, October 27, 2021 2:24 PM > > > To: Zhang, Chen <chen.zhang@intel.com> > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster > > > <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com> > > > Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from > > > filter-mirror and filter-redirector > > > > > > > > > 在 2021/10/27 下午2:19, Zhang, Chen 写道: > > > >> > > > mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_ > > > h > > > >> dr > > > >> _support][,position=head|tail|id=<id>][,insert=behind|before]`` > > > >>> + ``-object > > > >>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all > > > >>> + |rx| tx [,position=head|tail|id=<id>][,insert=behind|before]`` > > > >> I wonder if we break management layer. If yes, maybe it's better > > > >> to keep the vnet_hdr_support here. > > > > Yes and no, With this series of patches, filters have ability to > automatically > > > > Configure the appropriate vnet_hdr_support flag according to the > > > > current > > > environment. > > > > And can report error when can't fixing the vnet_hdr(The user > > > > cannot fix it > > > from the previous way ). > > > > So I think no need for the user to configure this option, some > > > > relevant > > > background knowledge required. > > > > > > > > For the management layer, keep the vnet_hdr_support may be > > > meaningless except for compatibility. > > > > In this situation, Do you think we still need to keep the > > > > vnet_hdr_support > > > for management layer? > > > > > > > > > So it depends on whether management layer like libvirt has already > > > supported this. If yes, we may get errors using new qemu with old libvirt? > > > > As far as I know, Current management layer like upstream libvirt is no COLO > official support yet. > > And some real CSPs use libvirt passthrough qmp command to Qemu for > manage COLO VM. > > So the question still, it looks to me it requires the modification of the layers > on top of libvirt? If the answer is yes, we'd better keep that compatibility. > Yes, I will keep the vnet_hdr_support and add some comments to update it in next version. Thanks Chen > > It is no harm to users to reduce some unnecessary parameters. But if > > you think compatibility is more important, I will restore this parameter in > next version. > > Thanks > > > > > Thanks > > Chen > > > > > > > > > > > > > > Thanks > > > > > > > Enable/disable it do the same things for filters. > > > > > > > > Thanks > > > > Chen > > > > > >
Jason Wang <jasowang@redhat.com> writes: > On Wed, Oct 27, 2021 at 2:40 PM Zhang, Chen <chen.zhang@intel.com> wrote: [...] >> > >> I wonder if we break management layer. If yes, maybe it's better to >> > >> keep the vnet_hdr_support here. >> > > >> > > Yes and no, With this series of patches, filters have ability to automatically >> > > Configure the appropriate vnet_hdr_support flag according to the current environment. >> > > And can report error when can't fixing the vnet_hdr(The user cannot fix it from the previous way ). >> > > So I think no need for the user to configure this option, some relevant background knowledge required. >> > > >> > > For the management layer, keep the vnet_hdr_support may be meaningless except for compatibility. >> > > In this situation, Do you think we still need to keep the vnet_hdr_support for management layer? >> > >> > >> > So it depends on whether management layer like libvirt has already >> > supported this. If yes, we may get errors using new qemu with old libvirt? >> >> As far as I know, Current management layer like upstream libvirt is no COLO official support yet. >> And some real CSPs use libvirt passthrough qmp command to Qemu for manage COLO VM. > > So the question still, it looks to me it requires the modification of > the layers on top of libvirt? If the answer is yes, we'd better keep > that compatibility. When in doubt, maintain compatibility. We may want to deprecate parameters that have become unnecessary. [...]
diff --git a/net/filter-mirror.c b/net/filter-mirror.c index f20240cc9f..4f0e26cc92 100644 --- a/net/filter-mirror.c +++ b/net/filter-mirror.c @@ -39,7 +39,6 @@ struct MirrorState { CharBackend chr_in; CharBackend chr_out; SocketReadState rs; - bool vnet_hdr; }; static int filter_send(MirrorState *s, @@ -48,7 +47,7 @@ static int filter_send(MirrorState *s, { NetFilterState *nf = NETFILTER(s); int ret = 0; - ssize_t size = 0; + ssize_t size = 0, vnet_hdr_len = 0; uint32_t len = 0; char *buf; @@ -63,21 +62,18 @@ static int filter_send(MirrorState *s, goto err; } - if (s->vnet_hdr) { - /* - * If vnet_hdr = on, we send vnet header len to make other - * module(like colo-compare) know how to parse net - * packet correctly. - */ - ssize_t vnet_hdr_len; - - vnet_hdr_len = nf->netdev->vnet_hdr_len; + /* + * The vnet header is the necessary part of filter transfer protocol. + * It make other module(like colo-compare) know how to parse net + * packet correctly. If device is not the virtio-net-pci, + * vnet_hdr_len will be 0. + */ + vnet_hdr_len = nf->netdev->vnet_hdr_len; - len = htonl(vnet_hdr_len); - ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); - if (ret != sizeof(len)) { - goto err; - } + len = htonl(vnet_hdr_len); + ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len)); + if (ret != sizeof(len)) { + goto err; } buf = g_malloc(size); @@ -232,6 +228,12 @@ static void redirector_rs_finalize(SocketReadState *rs) MirrorState *s = container_of(rs, MirrorState, rs); NetFilterState *nf = NETFILTER(s); + /* Check remote vnet_hdr */ + if (rs->vnet_hdr_len != nf->netdev->vnet_hdr_len) { + error_report("filter redirector got a different packet vnet_hdr" + " from local, please check the -device configuration"); + } + redirector_to_filter(nf, rs->buf, rs->packet_len); } @@ -252,7 +254,7 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp) } } - net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr); + net_socket_rs_init(&s->rs, redirector_rs_finalize, true); if (s->indev) { chr = qemu_chr_find(s->indev); @@ -323,20 +325,6 @@ static void filter_mirror_set_outdev(Object *obj, } } -static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp) -{ - MirrorState *s = FILTER_MIRROR(obj); - - return s->vnet_hdr; -} - -static void filter_mirror_set_vnet_hdr(Object *obj, bool value, Error **errp) -{ - MirrorState *s = FILTER_MIRROR(obj); - - s->vnet_hdr = value; -} - static char *filter_redirector_get_outdev(Object *obj, Error **errp) { MirrorState *s = FILTER_REDIRECTOR(obj); @@ -354,31 +342,12 @@ static void filter_redirector_set_outdev(Object *obj, s->outdev = g_strdup(value); } -static bool filter_redirector_get_vnet_hdr(Object *obj, Error **errp) -{ - MirrorState *s = FILTER_REDIRECTOR(obj); - - return s->vnet_hdr; -} - -static void filter_redirector_set_vnet_hdr(Object *obj, - bool value, - Error **errp) -{ - MirrorState *s = FILTER_REDIRECTOR(obj); - - s->vnet_hdr = value; -} - static void filter_mirror_class_init(ObjectClass *oc, void *data) { NetFilterClass *nfc = NETFILTER_CLASS(oc); object_class_property_add_str(oc, "outdev", filter_mirror_get_outdev, filter_mirror_set_outdev); - object_class_property_add_bool(oc, "vnet_hdr_support", - filter_mirror_get_vnet_hdr, - filter_mirror_set_vnet_hdr); nfc->setup = filter_mirror_setup; nfc->cleanup = filter_mirror_cleanup; @@ -393,29 +362,12 @@ static void filter_redirector_class_init(ObjectClass *oc, void *data) filter_redirector_set_indev); object_class_property_add_str(oc, "outdev", filter_redirector_get_outdev, filter_redirector_set_outdev); - object_class_property_add_bool(oc, "vnet_hdr_support", - filter_redirector_get_vnet_hdr, - filter_redirector_set_vnet_hdr); nfc->setup = filter_redirector_setup; nfc->cleanup = filter_redirector_cleanup; nfc->receive_iov = filter_redirector_receive_iov; } -static void filter_mirror_init(Object *obj) -{ - MirrorState *s = FILTER_MIRROR(obj); - - s->vnet_hdr = false; -} - -static void filter_redirector_init(Object *obj) -{ - MirrorState *s = FILTER_REDIRECTOR(obj); - - s->vnet_hdr = false; -} - static void filter_mirror_fini(Object *obj) { MirrorState *s = FILTER_MIRROR(obj); @@ -435,7 +387,6 @@ static const TypeInfo filter_redirector_info = { .name = TYPE_FILTER_REDIRECTOR, .parent = TYPE_NETFILTER, .class_init = filter_redirector_class_init, - .instance_init = filter_redirector_init, .instance_finalize = filter_redirector_fini, .instance_size = sizeof(MirrorState), }; @@ -444,7 +395,6 @@ static const TypeInfo filter_mirror_info = { .name = TYPE_FILTER_MIRROR, .parent = TYPE_NETFILTER, .class_init = filter_mirror_class_init, - .instance_init = filter_mirror_init, .instance_finalize = filter_mirror_fini, .instance_size = sizeof(MirrorState), }; diff --git a/qemu-options.hx b/qemu-options.hx index 5f375bbfa6..38c03812a7 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4946,18 +4946,16 @@ SRST ``behind``: insert behind the specified filter (default). - ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]`` + ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]`` filter-mirror on netdev netdevid,mirror net packet to - chardevchardevid, if it has the vnet\_hdr\_support flag, - filter-mirror will mirror packet with vnet\_hdr\_len. + chardev chardevid, filter-mirror will mirror packet with vnet\_hdr\_len. - ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]`` + ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]`` filter-redirector on netdev netdevid,redirect filter's net packet to chardev chardevid,and redirect indev's packet to - filter.if it has the vnet\_hdr\_support flag, filter-redirector - will redirect packet with vnet\_hdr\_len. Create a - filter-redirector we need to differ outdev id from indev id, id - can not be the same. we can just use indev or outdev, but at + filter. filter-redirector will redirect packet with vnet\_hdr\_len. + Create a filter-redirector we need to differ outdev id from indev id, + id can not be the same. we can just use indev or outdev, but at least one of indev or outdev need to be specified. ``-object filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx,[vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
Make the vnet header a necessary part of filter transfer protocol. So remove the module's vnet_hdr_support switch here. It make other modules(like another filter-redirector,colo-compare...) know how to parse net packet correctly. If local device is not the virtio-net-pci, vnet_hdr_len will be 0. Signed-off-by: Zhang Chen <chen.zhang@intel.com> --- net/filter-mirror.c | 88 ++++++++++----------------------------------- qemu-options.hx | 14 ++++---- 2 files changed, 25 insertions(+), 77 deletions(-)