qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net/filter: Enable the vnet_hdr_support by default
@ 2021-11-10  2:39 Zhang Chen
  2021-11-10  5:06 ` Markus Armbruster
  2021-11-19  3:17 ` Jason Wang
  0 siblings, 2 replies; 12+ messages in thread
From: Zhang Chen @ 2021-11-10  2:39 UTC (permalink / raw)
  To: Jason Wang; +Cc: Zhang Chen, qemu-dev, Li Zhijian, Markus Armbruster

This patch make filters and colo-compare module support vnet_hdr by
default. And also support -device non-virtio-net(like e1000.).
But it can't avoid user manual configuration error between
different filters when enable/disable virtio-net-pci.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/colo-compare.c    | 2 +-
 net/filter-mirror.c   | 4 ++--
 net/filter-rewriter.c | 2 +-
 qemu-options.hx       | 9 +++++----
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index b8876d7fd9..82d4d81710 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -1397,7 +1397,7 @@ static void colo_compare_init(Object *obj)
                         get_max_queue_size,
                         set_max_queue_size, NULL, NULL);
 
-    s->vnet_hdr = false;
+    s->vnet_hdr = true;
     object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
                              compare_set_vnet_hdr);
 }
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index f20240cc9f..adb0c6d89a 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -406,14 +406,14 @@ static void filter_mirror_init(Object *obj)
 {
     MirrorState *s = FILTER_MIRROR(obj);
 
-    s->vnet_hdr = false;
+    s->vnet_hdr = true;
 }
 
 static void filter_redirector_init(Object *obj)
 {
     MirrorState *s = FILTER_REDIRECTOR(obj);
 
-    s->vnet_hdr = false;
+    s->vnet_hdr = true;
 }
 
 static void filter_mirror_fini(Object *obj)
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index bf05023dc3..5698cd39d1 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -407,7 +407,7 @@ static void filter_rewriter_init(Object *obj)
 {
     RewriterState *s = FILTER_REWRITER(obj);
 
-    s->vnet_hdr = false;
+    s->vnet_hdr = true;
     s->failover_mode = FAILOVER_MODE_OFF;
 }
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 7749f59300..c40e385ede 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4967,13 +4967,13 @@ SRST
     ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,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.
+        filter-mirror will mirror packet with vnet\_hdr\_len(default: on).
 
     ``-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]``
         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
+        will redirect packet with vnet\_hdr\_len(default: on). 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.
@@ -4983,7 +4983,8 @@ SRST
         packet to secondary from primary to keep secondary tcp
         connection,and rewrite tcp packet to primary from secondary make
         tcp packet can be handled by client.if it has the
-        vnet\_hdr\_support flag, we can parse packet with vnet header.
+        vnet\_hdr\_support flag, we can parse packet with vnet
+        header(default: on).
 
         usage: colo secondary: -object
         filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 -object
@@ -5004,7 +5005,7 @@ SRST
         checkpoint and send primary packet to out\_dev. In order to
         improve efficiency, we need to put the task of comparison in
         another iothread. If it has the vnet\_hdr\_support flag,
-        colo compare will send/recv packet with vnet\_hdr\_len.
+        colo compare will send/recv packet with vnet\_hdr\_len(default: on).
         The compare\_timeout=@var{ms} determines the maximum time of the
         colo-compare hold the packet. The expired\_scan\_cycle=@var{ms}
         is to set the period of scanning expired primary node network packets.
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  2:39 [PATCH] net/filter: Enable the vnet_hdr_support by default Zhang Chen
@ 2021-11-10  5:06 ` Markus Armbruster
  2021-11-10  5:15   ` Zhang, Chen
  2021-11-19  3:17 ` Jason Wang
  1 sibling, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2021-11-10  5:06 UTC (permalink / raw)
  To: Zhang Chen; +Cc: Jason Wang, qemu-dev, Li Zhijian

Zhang Chen <chen.zhang@intel.com> writes:

> This patch make filters and colo-compare module support vnet_hdr by
> default. And also support -device non-virtio-net(like e1000.).
> But it can't avoid user manual configuration error between
> different filters when enable/disable virtio-net-pci.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>

Have you considered backward compatibility?  Can it break usage that now
works?



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  5:06 ` Markus Armbruster
@ 2021-11-10  5:15   ` Zhang, Chen
  2021-11-10  6:21     ` Markus Armbruster
  0 siblings, 1 reply; 12+ messages in thread
From: Zhang, Chen @ 2021-11-10  5:15 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Jason Wang, qemu-dev, Li Zhijian



> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, November 10, 2021 1:07 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
> 
> Zhang Chen <chen.zhang@intel.com> writes:
> 
> > This patch make filters and colo-compare module support vnet_hdr by
> > default. And also support -device non-virtio-net(like e1000.).
> > But it can't avoid user manual configuration error between different
> > filters when enable/disable virtio-net-pci.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> 
> Have you considered backward compatibility?  Can it break usage that now
> works?

Yes, this patch fully guarantees the compatibility as Jason's comments.
Original usage still works.

Thanks
Chen 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  5:15   ` Zhang, Chen
@ 2021-11-10  6:21     ` Markus Armbruster
  2021-11-10  6:53       ` Zhang, Chen
  0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2021-11-10  6:21 UTC (permalink / raw)
  To: Zhang, Chen; +Cc: Jason Wang, qemu-dev, Li Zhijian

"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Wednesday, November 10, 2021 1:07 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
>> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
>> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
>> 
>> Zhang Chen <chen.zhang@intel.com> writes:
>> 
>> > This patch make filters and colo-compare module support vnet_hdr by
>> > default. And also support -device non-virtio-net(like e1000.).
>> > But it can't avoid user manual configuration error between different
>> > filters when enable/disable virtio-net-pci.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> 
>> Have you considered backward compatibility?  Can it break usage that now
>> works?
>
> Yes, this patch fully guarantees the compatibility as Jason's comments.
> Original usage still works.

Worth a brief explanation in the commit message?



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  6:21     ` Markus Armbruster
@ 2021-11-10  6:53       ` Zhang, Chen
  2021-11-10  8:35         ` Markus Armbruster
  0 siblings, 1 reply; 12+ messages in thread
From: Zhang, Chen @ 2021-11-10  6:53 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Jason Wang, qemu-dev, Li Zhijian



> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, November 10, 2021 2:21 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Wednesday, November 10, 2021 1:07 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by
> >> default
> >>
> >> Zhang Chen <chen.zhang@intel.com> writes:
> >>
> >> > This patch make filters and colo-compare module support vnet_hdr by
> >> > default. And also support -device non-virtio-net(like e1000.).
> >> > But it can't avoid user manual configuration error between
> >> > different filters when enable/disable virtio-net-pci.
> >> >
> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >>
> >> Have you considered backward compatibility?  Can it break usage that
> >> now works?
> >
> > Yes, this patch fully guarantees the compatibility as Jason's comments.
> > Original usage still works.
> 
> Worth a brief explanation in the commit message?

OK. Add following statement to commit message:
This patch make filters and colo-compare module support vnet_hdr by
default. And also support -device non-virtio-net(like e1000.). Because
when enabled the support will make the vnet_hdr_len field become must-delivery
part of filter transfer protocol(even 0 in use -device e1000). It fully guarantees the
compatibility for management layer like libvirt.
But it still can't avoid user manual configuration error between
different filters connected when enable/disable vnet_hdr_support.

How about this explanation?

By the way, please let me know your comments on filter passthrough series:
https://mail.gnu.org/archive/html/qemu-devel/2021-08/msg01393.html
If OK, I will update it.

Thanks
Chen









^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  6:53       ` Zhang, Chen
@ 2021-11-10  8:35         ` Markus Armbruster
  2021-11-11  2:49           ` Jason Wang
  2021-11-12  3:24           ` Zhang, Chen
  0 siblings, 2 replies; 12+ messages in thread
From: Markus Armbruster @ 2021-11-10  8:35 UTC (permalink / raw)
  To: Zhang, Chen; +Cc: Jason Wang, qemu-dev, Li Zhijian

"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Wednesday, November 10, 2021 2:21 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
>> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
>> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
>> 
>> "Zhang, Chen" <chen.zhang@intel.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Markus Armbruster <armbru@redhat.com>
>> >> Sent: Wednesday, November 10, 2021 1:07 PM
>> >> To: Zhang, Chen <chen.zhang@intel.com>
>> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
>> >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
>> >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by
>> >> default
>> >>
>> >> Zhang Chen <chen.zhang@intel.com> writes:
>> >>
>> >> > This patch make filters and colo-compare module support vnet_hdr by
>> >> > default. And also support -device non-virtio-net(like e1000.).
>> >> > But it can't avoid user manual configuration error between
>> >> > different filters when enable/disable virtio-net-pci.
>> >> >
>> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> >>
>> >> Have you considered backward compatibility?  Can it break usage that
>> >> now works?
>> >
>> > Yes, this patch fully guarantees the compatibility as Jason's comments.
>> > Original usage still works.
>> 
>> Worth a brief explanation in the commit message?
>
> OK. Add following statement to commit message:
> This patch make filters and colo-compare module support vnet_hdr by
> default. And also support -device non-virtio-net(like e1000.). Because
> when enabled the support will make the vnet_hdr_len field become must-delivery
> part of filter transfer protocol(even 0 in use -device e1000). It fully guarantees the
> compatibility for management layer like libvirt.
> But it still can't avoid user manual configuration error between
> different filters connected when enable/disable vnet_hdr_support.
>
> How about this explanation?

I'm deferring to Jason, because I can't judge this for technical
accuracy.

> By the way, please let me know your comments on filter passthrough series:
> https://mail.gnu.org/archive/html/qemu-devel/2021-08/msg01393.html
> If OK, I will update it.

Uh, I was under the impression that you'd respin with my comments
addressed as per your reply to my review.

It's too late for 6.2 now.  Suggest to respin, and adjust the "since:"
tags to 7.0.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  8:35         ` Markus Armbruster
@ 2021-11-11  2:49           ` Jason Wang
  2021-11-11  3:10             ` Zhang, Chen
  2021-11-12  3:24           ` Zhang, Chen
  1 sibling, 1 reply; 12+ messages in thread
From: Jason Wang @ 2021-11-11  2:49 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Zhang, Chen, qemu-dev, Li Zhijian

On Wed, Nov 10, 2021 at 4:36 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> "Zhang, Chen" <chen.zhang@intel.com> writes:
>
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Wednesday, November 10, 2021 2:21 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
> >>
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> >> Sent: Wednesday, November 10, 2021 1:07 PM
> >> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> >> >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by
> >> >> default
> >> >>
> >> >> Zhang Chen <chen.zhang@intel.com> writes:
> >> >>
> >> >> > This patch make filters and colo-compare module support vnet_hdr by
> >> >> > default. And also support -device non-virtio-net(like e1000.).
> >> >> > But it can't avoid user manual configuration error between
> >> >> > different filters when enable/disable virtio-net-pci.
> >> >> >
> >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> >>
> >> >> Have you considered backward compatibility?  Can it break usage that
> >> >> now works?
> >> >
> >> > Yes, this patch fully guarantees the compatibility as Jason's comments.
> >> > Original usage still works.
> >>
> >> Worth a brief explanation in the commit message?
> >
> > OK. Add following statement to commit message:
> > This patch make filters and colo-compare module support vnet_hdr by
> > default. And also support -device non-virtio-net(like e1000.). Because
> > when enabled the support will make the vnet_hdr_len field become must-delivery
> > part of filter transfer protocol(even 0 in use -device e1000). It fully guarantees the
> > compatibility for management layer like libvirt.
> > But it still can't avoid user manual configuration error between
> > different filters connected when enable/disable vnet_hdr_support.
> >
> > How about this explanation?
>
> I'm deferring to Jason, because I can't judge this for technical
> accuracy.

I think it should be fine.

Thanks

>
> > By the way, please let me know your comments on filter passthrough series:
> > https://mail.gnu.org/archive/html/qemu-devel/2021-08/msg01393.html
> > If OK, I will update it.
>
> Uh, I was under the impression that you'd respin with my comments
> addressed as per your reply to my review.
>
> It's too late for 6.2 now.  Suggest to respin, and adjust the "since:"
> tags to 7.0.
>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-11  2:49           ` Jason Wang
@ 2021-11-11  3:10             ` Zhang, Chen
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Chen @ 2021-11-11  3:10 UTC (permalink / raw)
  To: Jason Wang, Markus Armbruster; +Cc: qemu-dev, Li Zhijian



> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, November 11, 2021 10:50 AM
> To: Markus Armbruster <armbru@redhat.com>
> Cc: Zhang, Chen <chen.zhang@intel.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
> 
> On Wed, Nov 10, 2021 at 4:36 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> > "Zhang, Chen" <chen.zhang@intel.com> writes:
> >
> > >> -----Original Message-----
> > >> From: Markus Armbruster <armbru@redhat.com>
> > >> Sent: Wednesday, November 10, 2021 2:21 PM
> > >> To: Zhang, Chen <chen.zhang@intel.com>
> > >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> > >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by
> > >> default
> > >>
> > >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> > >>
> > >> >> -----Original Message-----
> > >> >> From: Markus Armbruster <armbru@redhat.com>
> > >> >> Sent: Wednesday, November 10, 2021 1:07 PM
> > >> >> To: Zhang, Chen <chen.zhang@intel.com>
> > >> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> > >> >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > >> >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by
> > >> >> default
> > >> >>
> > >> >> Zhang Chen <chen.zhang@intel.com> writes:
> > >> >>
> > >> >> > This patch make filters and colo-compare module support
> > >> >> > vnet_hdr by default. And also support -device non-virtio-net(like
> e1000.).
> > >> >> > But it can't avoid user manual configuration error between
> > >> >> > different filters when enable/disable virtio-net-pci.
> > >> >> >
> > >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > >> >>
> > >> >> Have you considered backward compatibility?  Can it break usage
> > >> >> that now works?
> > >> >
> > >> > Yes, this patch fully guarantees the compatibility as Jason's comments.
> > >> > Original usage still works.
> > >>
> > >> Worth a brief explanation in the commit message?
> > >
> > > OK. Add following statement to commit message:
> > > This patch make filters and colo-compare module support vnet_hdr by
> > > default. And also support -device non-virtio-net(like e1000.).
> > > Because when enabled the support will make the vnet_hdr_len field
> > > become must-delivery part of filter transfer protocol(even 0 in use
> > > -device e1000). It fully guarantees the compatibility for management
> layer like libvirt.
> > > But it still can't avoid user manual configuration error between
> > > different filters connected when enable/disable vnet_hdr_support.
> > >
> > > How about this explanation?
> >
> > I'm deferring to Jason, because I can't judge this for technical
> > accuracy.
> 
> I think it should be fine.

OK, I will update the commit message.

Thanks
Chen

> 
> Thanks
> 
> >
> > > By the way, please let me know your comments on filter passthrough
> series:
> > > https://mail.gnu.org/archive/html/qemu-devel/2021-08/msg01393.html
> > > If OK, I will update it.
> >
> > Uh, I was under the impression that you'd respin with my comments
> > addressed as per your reply to my review.
> >
> > It's too late for 6.2 now.  Suggest to respin, and adjust the "since:"
> > tags to 7.0.
> >


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  8:35         ` Markus Armbruster
  2021-11-11  2:49           ` Jason Wang
@ 2021-11-12  3:24           ` Zhang, Chen
  1 sibling, 0 replies; 12+ messages in thread
From: Zhang, Chen @ 2021-11-12  3:24 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Jason Wang, qemu-dev, Li Zhijian



> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, November 10, 2021 4:36 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Wednesday, November 10, 2021 2:21 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by
> >> default
> >>
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> >> Sent: Wednesday, November 10, 2021 1:07 PM
> >> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> >> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> >> >> Subject: Re: [PATCH] net/filter: Enable the vnet_hdr_support by
> >> >> default
> >> >>
> >> >> Zhang Chen <chen.zhang@intel.com> writes:
> >> >>
> >> >> > This patch make filters and colo-compare module support vnet_hdr
> >> >> > by default. And also support -device non-virtio-net(like e1000.).
> >> >> > But it can't avoid user manual configuration error between
> >> >> > different filters when enable/disable virtio-net-pci.
> >> >> >
> >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> >>
> >> >> Have you considered backward compatibility?  Can it break usage
> >> >> that now works?
> >> >
> >> > Yes, this patch fully guarantees the compatibility as Jason's comments.
> >> > Original usage still works.
> >>
> >> Worth a brief explanation in the commit message?
> >
> > OK. Add following statement to commit message:
> > This patch make filters and colo-compare module support vnet_hdr by
> > default. And also support -device non-virtio-net(like e1000.). Because
> > when enabled the support will make the vnet_hdr_len field become
> > must-delivery part of filter transfer protocol(even 0 in use -device
> > e1000). It fully guarantees the compatibility for management layer like
> libvirt.
> > But it still can't avoid user manual configuration error between
> > different filters connected when enable/disable vnet_hdr_support.
> >
> > How about this explanation?
> 
> I'm deferring to Jason, because I can't judge this for technical accuracy.
> 
> > By the way, please let me know your comments on filter passthrough
> series:
> > https://mail.gnu.org/archive/html/qemu-devel/2021-08/msg01393.html
> > If OK, I will update it.
> 
> Uh, I was under the impression that you'd respin with my comments
> addressed as per your reply to my review.
> 
> It's too late for 6.2 now.  Suggest to respin, and adjust the "since:"
> tags to 7.0.


Already update the " [PATCH for 7.0 V10 0/6] Add passthrough support to object with network processing function"
I think this version already addressed your comments, please review again.

Thanks
Chen





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-10  2:39 [PATCH] net/filter: Enable the vnet_hdr_support by default Zhang Chen
  2021-11-10  5:06 ` Markus Armbruster
@ 2021-11-19  3:17 ` Jason Wang
  2021-11-19  3:36   ` Jason Wang
  1 sibling, 1 reply; 12+ messages in thread
From: Jason Wang @ 2021-11-19  3:17 UTC (permalink / raw)
  To: Zhang Chen; +Cc: qemu-dev, Li Zhijian, Markus Armbruster


在 2021/11/10 上午10:39, Zhang Chen 写道:
> This patch make filters and colo-compare module support vnet_hdr by
> default. And also support -device non-virtio-net(like e1000.).
> But it can't avoid user manual configuration error between
> different filters when enable/disable virtio-net-pci.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---


Applied.

Thanks


>   net/colo-compare.c    | 2 +-
>   net/filter-mirror.c   | 4 ++--
>   net/filter-rewriter.c | 2 +-
>   qemu-options.hx       | 9 +++++----
>   4 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index b8876d7fd9..82d4d81710 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -1397,7 +1397,7 @@ static void colo_compare_init(Object *obj)
>                           get_max_queue_size,
>                           set_max_queue_size, NULL, NULL);
>   
> -    s->vnet_hdr = false;
> +    s->vnet_hdr = true;
>       object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
>                                compare_set_vnet_hdr);
>   }
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index f20240cc9f..adb0c6d89a 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -406,14 +406,14 @@ static void filter_mirror_init(Object *obj)
>   {
>       MirrorState *s = FILTER_MIRROR(obj);
>   
> -    s->vnet_hdr = false;
> +    s->vnet_hdr = true;
>   }
>   
>   static void filter_redirector_init(Object *obj)
>   {
>       MirrorState *s = FILTER_REDIRECTOR(obj);
>   
> -    s->vnet_hdr = false;
> +    s->vnet_hdr = true;
>   }
>   
>   static void filter_mirror_fini(Object *obj)
> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> index bf05023dc3..5698cd39d1 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -407,7 +407,7 @@ static void filter_rewriter_init(Object *obj)
>   {
>       RewriterState *s = FILTER_REWRITER(obj);
>   
> -    s->vnet_hdr = false;
> +    s->vnet_hdr = true;
>       s->failover_mode = FAILOVER_MODE_OFF;
>   }
>   
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 7749f59300..c40e385ede 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4967,13 +4967,13 @@ SRST
>       ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,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.
> +        filter-mirror will mirror packet with vnet\_hdr\_len(default: on).
>   
>       ``-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]``
>           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
> +        will redirect packet with vnet\_hdr\_len(default: on). 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.
> @@ -4983,7 +4983,8 @@ SRST
>           packet to secondary from primary to keep secondary tcp
>           connection,and rewrite tcp packet to primary from secondary make
>           tcp packet can be handled by client.if it has the
> -        vnet\_hdr\_support flag, we can parse packet with vnet header.
> +        vnet\_hdr\_support flag, we can parse packet with vnet
> +        header(default: on).
>   
>           usage: colo secondary: -object
>           filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 -object
> @@ -5004,7 +5005,7 @@ SRST
>           checkpoint and send primary packet to out\_dev. In order to
>           improve efficiency, we need to put the task of comparison in
>           another iothread. If it has the vnet\_hdr\_support flag,
> -        colo compare will send/recv packet with vnet\_hdr\_len.
> +        colo compare will send/recv packet with vnet\_hdr\_len(default: on).
>           The compare\_timeout=@var{ms} determines the maximum time of the
>           colo-compare hold the packet. The expired\_scan\_cycle=@var{ms}
>           is to set the period of scanning expired primary node network packets.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-19  3:17 ` Jason Wang
@ 2021-11-19  3:36   ` Jason Wang
  2021-11-19  5:34     ` Zhang, Chen
  0 siblings, 1 reply; 12+ messages in thread
From: Jason Wang @ 2021-11-19  3:36 UTC (permalink / raw)
  To: Zhang Chen; +Cc: qemu-dev, Li Zhijian, Markus Armbruster

On Fri, Nov 19, 2021 at 11:17 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/11/10 上午10:39, Zhang Chen 写道:
> > This patch make filters and colo-compare module support vnet_hdr by
> > default. And also support -device non-virtio-net(like e1000.).
> > But it can't avoid user manual configuration error between
> > different filters when enable/disable virtio-net-pci.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
>
>
> Applied.
>
> Thanks

Actually not, it breaks filter-mirror test:


ERROR:../tests/qtest/test-filter-mirror.c:74:test_mirror: assertion
failed (recv_buf == send_buf): ("" == "Hello! filter-mirror~")
ERROR qtest-x86_64/test-filter-mirror - Bail out!
ERROR:../tests/qtest/test-filter-mirror.c:74:test_mirror: assertion
failed (recv_buf == send_buf): ("" == "Hello! filter-mirror~")
^Cmake[1]: *** [Makefile.mtest:784: run-test-96] Interrupt

Thanks

>
>
> >   net/colo-compare.c    | 2 +-
> >   net/filter-mirror.c   | 4 ++--
> >   net/filter-rewriter.c | 2 +-
> >   qemu-options.hx       | 9 +++++----
> >   4 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c
> > index b8876d7fd9..82d4d81710 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -1397,7 +1397,7 @@ static void colo_compare_init(Object *obj)
> >                           get_max_queue_size,
> >                           set_max_queue_size, NULL, NULL);
> >
> > -    s->vnet_hdr = false;
> > +    s->vnet_hdr = true;
> >       object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
> >                                compare_set_vnet_hdr);
> >   }
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> > index f20240cc9f..adb0c6d89a 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -406,14 +406,14 @@ static void filter_mirror_init(Object *obj)
> >   {
> >       MirrorState *s = FILTER_MIRROR(obj);
> >
> > -    s->vnet_hdr = false;
> > +    s->vnet_hdr = true;
> >   }
> >
> >   static void filter_redirector_init(Object *obj)
> >   {
> >       MirrorState *s = FILTER_REDIRECTOR(obj);
> >
> > -    s->vnet_hdr = false;
> > +    s->vnet_hdr = true;
> >   }
> >
> >   static void filter_mirror_fini(Object *obj)
> > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> > index bf05023dc3..5698cd39d1 100644
> > --- a/net/filter-rewriter.c
> > +++ b/net/filter-rewriter.c
> > @@ -407,7 +407,7 @@ static void filter_rewriter_init(Object *obj)
> >   {
> >       RewriterState *s = FILTER_REWRITER(obj);
> >
> > -    s->vnet_hdr = false;
> > +    s->vnet_hdr = true;
> >       s->failover_mode = FAILOVER_MODE_OFF;
> >   }
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 7749f59300..c40e385ede 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4967,13 +4967,13 @@ SRST
> >       ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,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.
> > +        filter-mirror will mirror packet with vnet\_hdr\_len(default: on).
> >
> >       ``-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]``
> >           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
> > +        will redirect packet with vnet\_hdr\_len(default: on). 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.
> > @@ -4983,7 +4983,8 @@ SRST
> >           packet to secondary from primary to keep secondary tcp
> >           connection,and rewrite tcp packet to primary from secondary make
> >           tcp packet can be handled by client.if it has the
> > -        vnet\_hdr\_support flag, we can parse packet with vnet header.
> > +        vnet\_hdr\_support flag, we can parse packet with vnet
> > +        header(default: on).
> >
> >           usage: colo secondary: -object
> >           filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 -object
> > @@ -5004,7 +5005,7 @@ SRST
> >           checkpoint and send primary packet to out\_dev. In order to
> >           improve efficiency, we need to put the task of comparison in
> >           another iothread. If it has the vnet\_hdr\_support flag,
> > -        colo compare will send/recv packet with vnet\_hdr\_len.
> > +        colo compare will send/recv packet with vnet\_hdr\_len(default: on).
> >           The compare\_timeout=@var{ms} determines the maximum time of the
> >           colo-compare hold the packet. The expired\_scan\_cycle=@var{ms}
> >           is to set the period of scanning expired primary node network packets.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] net/filter: Enable the vnet_hdr_support by default
  2021-11-19  3:36   ` Jason Wang
@ 2021-11-19  5:34     ` Zhang, Chen
  0 siblings, 0 replies; 12+ messages in thread
From: Zhang, Chen @ 2021-11-19  5:34 UTC (permalink / raw)
  To: Jason Wang; +Cc: qemu-dev, Li Zhijian, Markus Armbruster



> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, November 19, 2021 11:37 AM
> 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] net/filter: Enable the vnet_hdr_support by default
> 
> On Fri, Nov 19, 2021 at 11:17 AM Jason Wang <jasowang@redhat.com>
> wrote:
> >
> >
> > 在 2021/11/10 上午10:39, Zhang Chen 写道:
> > > This patch make filters and colo-compare module support vnet_hdr by
> > > default. And also support -device non-virtio-net(like e1000.).
> > > But it can't avoid user manual configuration error between different
> > > filters when enable/disable virtio-net-pci.
> > >
> > > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > > ---
> >
> >
> > Applied.
> >
> > Thanks
> 
> Actually not, it breaks filter-mirror test:
> 
> 
> ERROR:../tests/qtest/test-filter-mirror.c:74:test_mirror: assertion failed
> (recv_buf == send_buf): ("" == "Hello! filter-mirror~") ERROR qtest-
> x86_64/test-filter-mirror - Bail out!
> ERROR:../tests/qtest/test-filter-mirror.c:74:test_mirror: assertion failed
> (recv_buf == send_buf): ("" == "Hello! filter-mirror~")
> ^Cmake[1]: *** [Makefile.mtest:784: run-test-96] Interrupt

Oh, This test used default way to send filter data without vnet_hdr_len.
But we changed default filter transfer protocol. I will fix it in another patch.

Thanks
Chen

> 
> Thanks
> 
> >
> >
> > >   net/colo-compare.c    | 2 +-
> > >   net/filter-mirror.c   | 4 ++--
> > >   net/filter-rewriter.c | 2 +-
> > >   qemu-options.hx       | 9 +++++----
> > >   4 files changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > b8876d7fd9..82d4d81710 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -1397,7 +1397,7 @@ static void colo_compare_init(Object *obj)
> > >                           get_max_queue_size,
> > >                           set_max_queue_size, NULL, NULL);
> > >
> > > -    s->vnet_hdr = false;
> > > +    s->vnet_hdr = true;
> > >       object_property_add_bool(obj, "vnet_hdr_support",
> compare_get_vnet_hdr,
> > >                                compare_set_vnet_hdr);
> > >   }
> > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > f20240cc9f..adb0c6d89a 100644
> > > --- a/net/filter-mirror.c
> > > +++ b/net/filter-mirror.c
> > > @@ -406,14 +406,14 @@ static void filter_mirror_init(Object *obj)
> > >   {
> > >       MirrorState *s = FILTER_MIRROR(obj);
> > >
> > > -    s->vnet_hdr = false;
> > > +    s->vnet_hdr = true;
> > >   }
> > >
> > >   static void filter_redirector_init(Object *obj)
> > >   {
> > >       MirrorState *s = FILTER_REDIRECTOR(obj);
> > >
> > > -    s->vnet_hdr = false;
> > > +    s->vnet_hdr = true;
> > >   }
> > >
> > >   static void filter_mirror_fini(Object *obj) diff --git
> > > a/net/filter-rewriter.c b/net/filter-rewriter.c index
> > > bf05023dc3..5698cd39d1 100644
> > > --- a/net/filter-rewriter.c
> > > +++ b/net/filter-rewriter.c
> > > @@ -407,7 +407,7 @@ static void filter_rewriter_init(Object *obj)
> > >   {
> > >       RewriterState *s = FILTER_REWRITER(obj);
> > >
> > > -    s->vnet_hdr = false;
> > > +    s->vnet_hdr = true;
> > >       s->failover_mode = FAILOVER_MODE_OFF;
> > >   }
> > >
> > > diff --git a/qemu-options.hx b/qemu-options.hx index
> > > 7749f59300..c40e385ede 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -4967,13 +4967,13 @@ SRST
> > >       ``-object filter-
> mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr
> _support][,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.
> > > +        filter-mirror will mirror packet with vnet\_hdr\_len(default: on).
> > >
> > >       ``-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]``
> > >           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
> > > +        will redirect packet with vnet\_hdr\_len(default: on).
> > > + 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.
> > > @@ -4983,7 +4983,8 @@ SRST
> > >           packet to secondary from primary to keep secondary tcp
> > >           connection,and rewrite tcp packet to primary from secondary make
> > >           tcp packet can be handled by client.if it has the
> > > -        vnet\_hdr\_support flag, we can parse packet with vnet header.
> > > +        vnet\_hdr\_support flag, we can parse packet with vnet
> > > +        header(default: on).
> > >
> > >           usage: colo secondary: -object
> > >           filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
> > > -object @@ -5004,7 +5005,7 @@ SRST
> > >           checkpoint and send primary packet to out\_dev. In order to
> > >           improve efficiency, we need to put the task of comparison in
> > >           another iothread. If it has the vnet\_hdr\_support flag,
> > > -        colo compare will send/recv packet with vnet\_hdr\_len.
> > > +        colo compare will send/recv packet with vnet\_hdr\_len(default:
> on).
> > >           The compare\_timeout=@var{ms} determines the maximum time
> of the
> > >           colo-compare hold the packet. The expired\_scan\_cycle=@var{ms}
> > >           is to set the period of scanning expired primary node network
> packets.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-11-19  5:36 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10  2:39 [PATCH] net/filter: Enable the vnet_hdr_support by default Zhang Chen
2021-11-10  5:06 ` Markus Armbruster
2021-11-10  5:15   ` Zhang, Chen
2021-11-10  6:21     ` Markus Armbruster
2021-11-10  6:53       ` Zhang, Chen
2021-11-10  8:35         ` Markus Armbruster
2021-11-11  2:49           ` Jason Wang
2021-11-11  3:10             ` Zhang, Chen
2021-11-12  3:24           ` Zhang, Chen
2021-11-19  3:17 ` Jason Wang
2021-11-19  3:36   ` Jason Wang
2021-11-19  5:34     ` Zhang, Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).