[PATCH V5 0/3] net/filter: Optimize filters vnet_hdr support

[PATCH V5 0/3] net/filter: Optimize filters vnet_hdr support

[Zhang Chen]

This series make filters and colo-compare module support vnet_hdr by
default. And also support -device non-virtio-net(like e1000.) at the same time.
It can adapt -device automatically to avoid wrong setting between
different filters when enable/disable virtio-net-pci.

Optimize the filter transfer protocol from:
1.size -----> 2.real network payload.
to:
1.size -----> 2.vnet_hdr_len. -----> 3.real network payload.

When receiving node get the network packet, it will compare with
the local vnet_hdr_len. If they are not the same, report a error.
because this kind of packet cannot be correctly parsed by receiving
node. For the colo-compare, it need to compare whether the two sides
vnet_hdr_len are equal.


v5:
    Keep the vnet_hdr_support from filters for the management layer compatibility.

v4:
    Rewrite patches to impliment it in filter transfer protocol payload.
    Remove filters and colo-compare's "vnet_hdr_support" flag.

v3:
    Fix some typos.
    Rebased for Qemu 6.2.

v2:
    Detect virtio-net driver and apply vnet_hdr_support
    automatically. (Jason)


Zhang Chen (3):
  net/filter: Optimize transfer protocol for filter-mirror/redirector
  net/filter: Optimize transfer protocol for filter-rewriter
  net/colo-compare.c: Optimize transfer protocol for colo-compare

 net/colo-compare.c    | 24 +++++++++++++++++-------
 net/filter-mirror.c   | 34 ++++++++++++++++------------------
 net/filter-rewriter.c |  6 ++----
 3 files changed, 35 insertions(+), 29 deletions(-)

-- 
2.25.1

[PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang Chen]

Make the vnet header a necessary part of filter transfer protocol.
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 | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index f20240cc9f..24d3e498e9 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -39,6 +39,7 @@ struct MirrorState {
     CharBackend chr_in;
     CharBackend chr_out;
     SocketReadState rs;
+    /* Keep compatibility for the management layer */
     bool vnet_hdr;
 };
 
@@ -48,7 +49,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 +64,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);
@@ -252,7 +250,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);
@@ -406,14 +404,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)
-- 
2.25.1

[PATCH V5 2/3] net/filter: Optimize transfer protocol for filter-rewriter

[Zhang Chen]

Make the vnet header a necessary part of filter transfer protocol.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/filter-rewriter.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index cb3a96cde1..70fa71583a 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -266,9 +266,7 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
 
     iov_to_buf(iov, iovcnt, 0, buf, size);
 
-    if (s->vnet_hdr) {
-        vnet_hdr_len = nf->netdev->vnet_hdr_len;
-    }
+    vnet_hdr_len = nf->netdev->vnet_hdr_len;
 
     pkt = packet_new_nocopy(buf, size, vnet_hdr_len);
 
@@ -415,7 +413,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;
 }
 
-- 
2.25.1

[PATCH V5 3/3] net/colo-compare.c: Optimize transfer protocol for colo-compare

[Zhang Chen]

Enable vnet_hdr in payload by default. Make it easier to find module
communication configuration errors.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/colo-compare.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index b100e7b51f..fb61b0eea9 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -119,7 +119,9 @@ struct CompareState {
     SocketReadState notify_rs;
     SendCo out_sendco;
     SendCo notify_sendco;
+    /* Keep compatibility for the management layer */
     bool vnet_hdr;
+    int local_vnet_hdr_len;
     uint64_t compare_timeout;
     uint32_t expired_scan_cycle;
 
@@ -725,7 +727,6 @@ static void colo_compare_connection(void *opaque, void *user_data)
 static void coroutine_fn _compare_chr_send(void *opaque)
 {
     SendCo *sendco = opaque;
-    CompareState *s = sendco->s;
     int ret = 0;
 
     while (!g_queue_is_empty(&sendco->send_list)) {
@@ -740,7 +741,7 @@ static void coroutine_fn _compare_chr_send(void *opaque)
             goto err;
         }
 
-        if (!sendco->notify_remote_frame && s->vnet_hdr) {
+        if (!sendco->notify_remote_frame) {
             /*
              * We send vnet header len make other module(like filter-redirector)
              * know how to parse net packet correctly.
@@ -1157,6 +1158,9 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
     CompareState *s = container_of(pri_rs, CompareState, pri_rs);
     Connection *conn = NULL;
 
+    /* Update colo-compare local vnet_hdr_len */
+    s->local_vnet_hdr_len = pri_rs->vnet_hdr_len;
+
     if (packet_enqueue(s, PRIMARY_IN, &conn)) {
         trace_colo_compare_main("primary: unsupported packet in");
         compare_chr_send(s,
@@ -1176,6 +1180,12 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs)
     CompareState *s = container_of(sec_rs, CompareState, sec_rs);
     Connection *conn = NULL;
 
+    /* Check the secondary vnet_hdr_len to ensure parse packet correctly */
+    if (s->local_vnet_hdr_len != sec_rs->vnet_hdr_len) {
+        error_report("colo-compare got a different packet vnet_hdr_len"
+        " from local, please check the nodes -device configuration");
+    }
+
     if (packet_enqueue(s, SECONDARY_IN, &conn)) {
         trace_colo_compare_main("secondary: unsupported packet in");
     } else {
@@ -1289,8 +1299,8 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
         return;
     }
 
-    net_socket_rs_init(&s->pri_rs, compare_pri_rs_finalize, s->vnet_hdr);
-    net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize, s->vnet_hdr);
+    net_socket_rs_init(&s->pri_rs, compare_pri_rs_finalize, true);
+    net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize, true);
 
     /* Try to enable remote notify chardev, currently just for Xen COLO */
     if (s->notify_dev) {
@@ -1299,8 +1309,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
             return;
         }
 
-        net_socket_rs_init(&s->notify_rs, compare_notify_rs_finalize,
-                           s->vnet_hdr);
+        net_socket_rs_init(&s->notify_rs, compare_notify_rs_finalize, false);
     }
 
     s->out_sendco.s = s;
@@ -1397,7 +1406,8 @@ 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;
+    s->local_vnet_hdr_len = 0;
     object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
                              compare_set_vnet_hdr);
 }
-- 
2.25.1

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

在 2021/10/28 下午5:05, Zhang Chen 写道:
> Make the vnet header a necessary part of filter transfer protocol.
> 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 | 34 ++++++++++++++++------------------
>   1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index f20240cc9f..24d3e498e9 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -39,6 +39,7 @@ struct MirrorState {
>       CharBackend chr_in;
>       CharBackend chr_out;
>       SocketReadState rs;
> +    /* Keep compatibility for the management layer */
>       bool vnet_hdr;
>   };
>   
> @@ -48,7 +49,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 +64,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));


I wonder if we need to introduce new parameter, e.g force_vnet_hdr here, 
then we can always send vnet_hdr when it is enabled.

Otherwise the "vnet_hdr_support" seems meaningless.

Thanks


> +    if (ret != sizeof(len)) {
> +        goto err;
>       }
>   
>       buf = g_malloc(size);
> @@ -252,7 +250,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);
> @@ -406,14 +404,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)

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, October 29, 2021 11:11 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 V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> 
> 在 2021/10/28 下午5:05, Zhang Chen 写道:
> > Make the vnet header a necessary part of filter transfer protocol.
> > 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 | 34 ++++++++++++++++------------------
> >   1 file changed, 16 insertions(+), 18 deletions(-)
> >
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > f20240cc9f..24d3e498e9 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -39,6 +39,7 @@ struct MirrorState {
> >       CharBackend chr_in;
> >       CharBackend chr_out;
> >       SocketReadState rs;
> > +    /* Keep compatibility for the management layer */
> >       bool vnet_hdr;
> >   };
> >
> > @@ -48,7 +49,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 +64,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));
> 
> 
> I wonder if we need to introduce new parameter, e.g force_vnet_hdr here,
> then we can always send vnet_hdr when it is enabled.
> 
> Otherwise the "vnet_hdr_support" seems meaningless.

Yes, Current "vnet_hdr_support"  default enabled, and vnet_hdr_len already forced from attached nf->netdev.
Maybe we can introduce a new parameter "force_no_vnet_hdr" here to make the vnet_hdr_len always keep 0.
If you think OK, I will update it in next version.

Thanks
Chen

> 
> Thanks
> 
> 
> > +    if (ret != sizeof(len)) {
> > +        goto err;
> >       }
> >
> >       buf = g_malloc(size);
> > @@ -252,7 +250,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); @@ -406,14 +404,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)

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

On Fri, Oct 29, 2021 at 4:08 PM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Friday, October 29, 2021 11:11 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 V5 1/3] net/filter: Optimize transfer protocol for filter-
> > mirror/redirector
> >
> >
> > 在 2021/10/28 下午5:05, Zhang Chen 写道:
> > > Make the vnet header a necessary part of filter transfer protocol.
> > > 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 | 34 ++++++++++++++++------------------
> > >   1 file changed, 16 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > f20240cc9f..24d3e498e9 100644
> > > --- a/net/filter-mirror.c
> > > +++ b/net/filter-mirror.c
> > > @@ -39,6 +39,7 @@ struct MirrorState {
> > >       CharBackend chr_in;
> > >       CharBackend chr_out;
> > >       SocketReadState rs;
> > > +    /* Keep compatibility for the management layer */
> > >       bool vnet_hdr;
> > >   };
> > >
> > > @@ -48,7 +49,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 +64,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));
> >
> >
> > I wonder if we need to introduce new parameter, e.g force_vnet_hdr here,
> > then we can always send vnet_hdr when it is enabled.
> >
> > Otherwise the "vnet_hdr_support" seems meaningless.
>
> Yes, Current "vnet_hdr_support"  default enabled, and vnet_hdr_len already forced from attached nf->netdev.
> Maybe we can introduce a new parameter "force_no_vnet_hdr" here to make the vnet_hdr_len always keep 0.
> If you think OK, I will update it in next version.

Let me explain, if I was not wrong:

"vnet_hdr_support" means whether or not to send vnet header length. If
vnet_hdr_support=false, we won't send the vnet header. This looks the
same as you "force_no_vent_hdr" above.

And my "force_vnet_hdr" seems duplicated with vnet_hdr_support=true.
So it looks to me we can leave the mirror code as is and just change
the compare? (depends on the mgmt to set a correct vnet_hdr_support)

Thanks

>
> Thanks
> Chen
>
> >
> > Thanks
> >
> >
> > > +    if (ret != sizeof(len)) {
> > > +        goto err;
> > >       }
> > >
> > >       buf = g_malloc(size);
> > > @@ -252,7 +250,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); @@ -406,14 +404,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)
>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, November 1, 2021 11:46 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 V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> On Fri, Oct 29, 2021 at 4:08 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Friday, October 29, 2021 11:11 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 V5 1/3] net/filter: Optimize transfer protocol
> > > for filter- mirror/redirector
> > >
> > >
> > > 在 2021/10/28 下午5:05, Zhang Chen 写道:
> > > > Make the vnet header a necessary part of filter transfer protocol.
> > > > 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 | 34 ++++++++++++++++------------------
> > > >   1 file changed, 16 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > > f20240cc9f..24d3e498e9 100644
> > > > --- a/net/filter-mirror.c
> > > > +++ b/net/filter-mirror.c
> > > > @@ -39,6 +39,7 @@ struct MirrorState {
> > > >       CharBackend chr_in;
> > > >       CharBackend chr_out;
> > > >       SocketReadState rs;
> > > > +    /* Keep compatibility for the management layer */
> > > >       bool vnet_hdr;
> > > >   };
> > > >
> > > > @@ -48,7 +49,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 +64,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));
> > >
> > >
> > > I wonder if we need to introduce new parameter, e.g force_vnet_hdr
> > > here, then we can always send vnet_hdr when it is enabled.
> > >
> > > Otherwise the "vnet_hdr_support" seems meaningless.
> >
> > Yes, Current "vnet_hdr_support"  default enabled, and vnet_hdr_len
> already forced from attached nf->netdev.
> > Maybe we can introduce a new parameter "force_no_vnet_hdr" here to
> make the vnet_hdr_len always keep 0.
> > If you think OK, I will update it in next version.
> 
> Let me explain, if I was not wrong:
> 
> "vnet_hdr_support" means whether or not to send vnet header length. If
> vnet_hdr_support=false, we won't send the vnet header. This looks the
> same as you "force_no_vent_hdr" above.

Yes, It was.  But this series changed it.
Current "vnet_hdr_support" can't decide whether send vnet header length, we always send it even 0.
It will avoid sender/receiver transfer protocol parse issues:
When sender data with the vnet header length, but receiver can't enable the "vnet_hdr_support".
Filters will auto setup vnet_hdr_len as local nf->netdev and found the issue when get different vnet_hdr_len from other filters.

> 
> And my "force_vnet_hdr" seems duplicated with vnet_hdr_support=true.
> So it looks to me we can leave the mirror code as is and just change the
> compare? (depends on the mgmt to set a correct vnet_hdr_support)

OK, I will keep the filter-mirror/filter-redirector/filter-rewriter same as this version.
For the colo-compare module, It will get primary node's filter data's vnet_hdr_len as the local value,
And compare with secondary node's, because it is not attached any nf->netdev.
So, it looks compare module's "vnet_hdr_support" been auto configuration from the filter transport protocol.  
If the "force_vnet_hdr" means hard code a compare's local vnet_hdr_len rather than come from input filter's data?

Thanks
Chen

> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> > >
> > > Thanks
> > >
> > >
> > > > +    if (ret != sizeof(len)) {
> > > > +        goto err;
> > > >       }
> > > >
> > > >       buf = g_malloc(size);
> > > > @@ -252,7 +250,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); @@ -406,14 +404,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)
> >

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> > > >
> > > >
> > > > I wonder if we need to introduce new parameter, e.g force_vnet_hdr
> > > > here, then we can always send vnet_hdr when it is enabled.
> > > >
> > > > Otherwise the "vnet_hdr_support" seems meaningless.
> > >
> > > Yes, Current "vnet_hdr_support"  default enabled, and vnet_hdr_len
> > already forced from attached nf->netdev.
> > > Maybe we can introduce a new parameter "force_no_vnet_hdr" here to
> > make the vnet_hdr_len always keep 0.
> > > If you think OK, I will update it in next version.
> >
> > Let me explain, if I was not wrong:
> >
> > "vnet_hdr_support" means whether or not to send vnet header length. If
> > vnet_hdr_support=false, we won't send the vnet header. This looks the
> > same as you "force_no_vent_hdr" above.
> 
> Yes, It was.  But this series changed it.
> Current "vnet_hdr_support" can't decide whether send vnet header length,
> we always send it even 0.
> It will avoid sender/receiver transfer protocol parse issues:
> When sender data with the vnet header length, but receiver can't enable the
> "vnet_hdr_support".
> Filters will auto setup vnet_hdr_len as local nf->netdev and found the issue
> when get different vnet_hdr_len from other filters.
> 
> >
> > And my "force_vnet_hdr" seems duplicated with vnet_hdr_support=true.
> > So it looks to me we can leave the mirror code as is and just change
> > the compare? (depends on the mgmt to set a correct vnet_hdr_support)
> 
> OK, I will keep the filter-mirror/filter-redirector/filter-rewriter same as this
> version.
> For the colo-compare module, It will get primary node's filter data's
> vnet_hdr_len as the local value, And compare with secondary node's,
> because it is not attached any nf->netdev.
> So, it looks compare module's "vnet_hdr_support" been auto configuration
> from the filter transport protocol.
> If the "force_vnet_hdr" means hard code a compare's local vnet_hdr_len
> rather than come from input filter's data?
> 
> Thanks
> Chen
> 

Hi Jason/Markus,

Rethink about it, How about keep the original "vnet_hdr_support" function, 
And add a new optional parameter "auto_vnet_hdr" for filters/compare?

Thanks
Chen 


> >
> > Thanks
> >
> > >
> > > Thanks
> > > Chen
> > >
> > > >
> > > > Thanks
> > > >
> > > >

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

在 2021/11/4 下午1:37, Zhang, Chen 写道:
>>>>>
>>>>> I wonder if we need to introduce new parameter, e.g force_vnet_hdr
>>>>> here, then we can always send vnet_hdr when it is enabled.
>>>>>
>>>>> Otherwise the "vnet_hdr_support" seems meaningless.
>>>> Yes, Current "vnet_hdr_support"  default enabled, and vnet_hdr_len
>>> already forced from attached nf->netdev.
>>>> Maybe we can introduce a new parameter "force_no_vnet_hdr" here to
>>> make the vnet_hdr_len always keep 0.
>>>> If you think OK, I will update it in next version.
>>> Let me explain, if I was not wrong:
>>>
>>> "vnet_hdr_support" means whether or not to send vnet header length. If
>>> vnet_hdr_support=false, we won't send the vnet header. This looks the
>>> same as you "force_no_vent_hdr" above.
>> Yes, It was.  But this series changed it.
>> Current "vnet_hdr_support" can't decide whether send vnet header length,
>> we always send it even 0.
>> It will avoid sender/receiver transfer protocol parse issues:
>> When sender data with the vnet header length, but receiver can't enable the
>> "vnet_hdr_support".
>> Filters will auto setup vnet_hdr_len as local nf->netdev and found the issue
>> when get different vnet_hdr_len from other filters.
>>
>>> And my "force_vnet_hdr" seems duplicated with vnet_hdr_support=true.
>>> So it looks to me we can leave the mirror code as is and just change
>>> the compare? (depends on the mgmt to set a correct vnet_hdr_support)
>> OK, I will keep the filter-mirror/filter-redirector/filter-rewriter same as this
>> version.
>> For the colo-compare module, It will get primary node's filter data's
>> vnet_hdr_len as the local value, And compare with secondary node's,
>> because it is not attached any nf->netdev.
>> So, it looks compare module's "vnet_hdr_support" been auto configuration
>> from the filter transport protocol.
>> If the "force_vnet_hdr" means hard code a compare's local vnet_hdr_len
>> rather than come from input filter's data?
>>
>> Thanks
>> Chen
>>
> Hi Jason/Markus,
>
> Rethink about it, How about keep the original "vnet_hdr_support" function,
> And add a new optional parameter "auto_vnet_hdr" for filters/compare?


It's a way but rethink of the whole thing. I wonder what if we just 
enable "vnet_hdr_support" by default for filter and colo-compare?

Thanks


>
> Thanks
> Chen
>
>
>>> Thanks
>>>
>>>> Thanks
>>>> Chen
>>>>
>>>>> Thanks
>>>>>
>>>>>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, November 5, 2021 11:17 AM
> To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> <armbru@redhat.com>
> Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> 
> 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> >>>>>
> >>>>> I wonder if we need to introduce new parameter, e.g force_vnet_hdr
> >>>>> here, then we can always send vnet_hdr when it is enabled.
> >>>>>
> >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> >>>> Yes, Current "vnet_hdr_support"  default enabled, and vnet_hdr_len
> >>> already forced from attached nf->netdev.
> >>>> Maybe we can introduce a new parameter "force_no_vnet_hdr" here
> to
> >>> make the vnet_hdr_len always keep 0.
> >>>> If you think OK, I will update it in next version.
> >>> Let me explain, if I was not wrong:
> >>>
> >>> "vnet_hdr_support" means whether or not to send vnet header length.
> >>> If vnet_hdr_support=false, we won't send the vnet header. This looks
> >>> the same as you "force_no_vent_hdr" above.
> >> Yes, It was.  But this series changed it.
> >> Current "vnet_hdr_support" can't decide whether send vnet header
> >> length, we always send it even 0.
> >> It will avoid sender/receiver transfer protocol parse issues:
> >> When sender data with the vnet header length, but receiver can't
> >> enable the "vnet_hdr_support".
> >> Filters will auto setup vnet_hdr_len as local nf->netdev and found
> >> the issue when get different vnet_hdr_len from other filters.
> >>
> >>> And my "force_vnet_hdr" seems duplicated with
> vnet_hdr_support=true.
> >>> So it looks to me we can leave the mirror code as is and just change
> >>> the compare? (depends on the mgmt to set a correct vnet_hdr_support)
> >> OK, I will keep the filter-mirror/filter-redirector/filter-rewriter
> >> same as this version.
> >> For the colo-compare module, It will get primary node's filter data's
> >> vnet_hdr_len as the local value, And compare with secondary node's,
> >> because it is not attached any nf->netdev.
> >> So, it looks compare module's "vnet_hdr_support" been auto
> >> configuration from the filter transport protocol.
> >> If the "force_vnet_hdr" means hard code a compare's local
> >> vnet_hdr_len rather than come from input filter's data?
> >>
> >> Thanks
> >> Chen
> >>
> > Hi Jason/Markus,
> >
> > Rethink about it, How about keep the original "vnet_hdr_support"
> > function, And add a new optional parameter "auto_vnet_hdr" for
> filters/compare?
> 
> 
> It's a way but rethink of the whole thing. I wonder what if we just enable
> "vnet_hdr_support" by default for filter and colo-compare?

It's works by default for user use -device virtio-net-pci and e1000...
But it can't resolve this series motivation, how to fix/check user configuration issue:
For example user enable " vnet_hdr_support " filter-mirror and disable " vnet_hdr_support" filter-redirector
And connect both filter modules by chardev socket.
In this case guest will get wrong network workload and filters didn’t perceive any abnormalities,
but in fact, the whole system is no longer working.
This series will report error and try to correct it.

Thanks
Chen

> 
> Thanks
> 
> 
> >
> > Thanks
> > Chen
> >
> >
> >>> Thanks
> >>>
> >>>> Thanks
> >>>> Chen
> >>>>
> >>>>> Thanks
> >>>>>
> >>>>>

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Friday, November 5, 2021 11:17 AM
> > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> > <armbru@redhat.com>
> > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>
> > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> > mirror/redirector
> >
> >
> > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > >>>>>
> > >>>>> I wonder if we need to introduce new parameter, e.g force_vnet_hdr
> > >>>>> here, then we can always send vnet_hdr when it is enabled.
> > >>>>>
> > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > >>>> Yes, Current "vnet_hdr_support"  default enabled, and vnet_hdr_len
> > >>> already forced from attached nf->netdev.
> > >>>> Maybe we can introduce a new parameter "force_no_vnet_hdr" here
> > to
> > >>> make the vnet_hdr_len always keep 0.
> > >>>> If you think OK, I will update it in next version.
> > >>> Let me explain, if I was not wrong:
> > >>>
> > >>> "vnet_hdr_support" means whether or not to send vnet header length.
> > >>> If vnet_hdr_support=false, we won't send the vnet header. This looks
> > >>> the same as you "force_no_vent_hdr" above.
> > >> Yes, It was.  But this series changed it.
> > >> Current "vnet_hdr_support" can't decide whether send vnet header
> > >> length, we always send it even 0.
> > >> It will avoid sender/receiver transfer protocol parse issues:
> > >> When sender data with the vnet header length, but receiver can't
> > >> enable the "vnet_hdr_support".
> > >> Filters will auto setup vnet_hdr_len as local nf->netdev and found
> > >> the issue when get different vnet_hdr_len from other filters.
> > >>
> > >>> And my "force_vnet_hdr" seems duplicated with
> > vnet_hdr_support=true.
> > >>> So it looks to me we can leave the mirror code as is and just change
> > >>> the compare? (depends on the mgmt to set a correct vnet_hdr_support)
> > >> OK, I will keep the filter-mirror/filter-redirector/filter-rewriter
> > >> same as this version.
> > >> For the colo-compare module, It will get primary node's filter data's
> > >> vnet_hdr_len as the local value, And compare with secondary node's,
> > >> because it is not attached any nf->netdev.
> > >> So, it looks compare module's "vnet_hdr_support" been auto
> > >> configuration from the filter transport protocol.
> > >> If the "force_vnet_hdr" means hard code a compare's local
> > >> vnet_hdr_len rather than come from input filter's data?
> > >>
> > >> Thanks
> > >> Chen
> > >>
> > > Hi Jason/Markus,
> > >
> > > Rethink about it, How about keep the original "vnet_hdr_support"
> > > function, And add a new optional parameter "auto_vnet_hdr" for
> > filters/compare?
> >
> >
> > It's a way but rethink of the whole thing. I wonder what if we just enable
> > "vnet_hdr_support" by default for filter and colo-compare?
>
> It's works by default for user use -device virtio-net-pci and e1000...
> But it can't resolve this series motivation, how to fix/check user configuration issue:
> For example user enable " vnet_hdr_support " filter-mirror and disable " vnet_hdr_support" filter-redirector
> And connect both filter modules by chardev socket.
> In this case guest will get wrong network workload and filters didn’t perceive any abnormalities,
> but in fact, the whole system is no longer working.
> This series will report error and try to correct it.

The problem is how "auto_vnet_hdr" help in this case. It's a new
parameter which may lead to more wrong configuration?

Thanks

>
> Thanks
> Chen
>
> >
> > Thanks
> >
> >
> > >
> > > Thanks
> > > Chen
> > >
> > >
> > >>> Thanks
> > >>>
> > >>>> Thanks
> > >>>> Chen
> > >>>>
> > >>>>> Thanks
> > >>>>>
> > >>>>>
>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, November 5, 2021 12:03 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen <chen.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Friday, November 5, 2021 11:17 AM
> > > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> > > <armbru@redhat.com>
> > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>
> > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > for filter- mirror/redirector
> > >
> > >
> > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > >>>>>
> > > >>>>> I wonder if we need to introduce new parameter, e.g
> > > >>>>> force_vnet_hdr here, then we can always send vnet_hdr when it
> is enabled.
> > > >>>>>
> > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > >>>> Yes, Current "vnet_hdr_support"  default enabled, and
> > > >>>> vnet_hdr_len
> > > >>> already forced from attached nf->netdev.
> > > >>>> Maybe we can introduce a new parameter "force_no_vnet_hdr"
> here
> > > to
> > > >>> make the vnet_hdr_len always keep 0.
> > > >>>> If you think OK, I will update it in next version.
> > > >>> Let me explain, if I was not wrong:
> > > >>>
> > > >>> "vnet_hdr_support" means whether or not to send vnet header
> length.
> > > >>> If vnet_hdr_support=false, we won't send the vnet header. This
> > > >>> looks the same as you "force_no_vent_hdr" above.
> > > >> Yes, It was.  But this series changed it.
> > > >> Current "vnet_hdr_support" can't decide whether send vnet header
> > > >> length, we always send it even 0.
> > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > >> When sender data with the vnet header length, but receiver can't
> > > >> enable the "vnet_hdr_support".
> > > >> Filters will auto setup vnet_hdr_len as local nf->netdev and
> > > >> found the issue when get different vnet_hdr_len from other filters.
> > > >>
> > > >>> And my "force_vnet_hdr" seems duplicated with
> > > vnet_hdr_support=true.
> > > >>> So it looks to me we can leave the mirror code as is and just
> > > >>> change the compare? (depends on the mgmt to set a correct
> > > >>> vnet_hdr_support)
> > > >> OK, I will keep the
> > > >> filter-mirror/filter-redirector/filter-rewriter
> > > >> same as this version.
> > > >> For the colo-compare module, It will get primary node's filter
> > > >> data's vnet_hdr_len as the local value, And compare with
> > > >> secondary node's, because it is not attached any nf->netdev.
> > > >> So, it looks compare module's "vnet_hdr_support" been auto
> > > >> configuration from the filter transport protocol.
> > > >> If the "force_vnet_hdr" means hard code a compare's local
> > > >> vnet_hdr_len rather than come from input filter's data?
> > > >>
> > > >> Thanks
> > > >> Chen
> > > >>
> > > > Hi Jason/Markus,
> > > >
> > > > Rethink about it, How about keep the original "vnet_hdr_support"
> > > > function, And add a new optional parameter "auto_vnet_hdr" for
> > > filters/compare?
> > >
> > >
> > > It's a way but rethink of the whole thing. I wonder what if we just
> > > enable "vnet_hdr_support" by default for filter and colo-compare?
> >
> > It's works by default for user use -device virtio-net-pci and e1000...
> > But it can't resolve this series motivation, how to fix/check user
> configuration issue:
> > For example user enable " vnet_hdr_support " filter-mirror and disable
> > " vnet_hdr_support" filter-redirector And connect both filter modules by
> chardev socket.
> > In this case guest will get wrong network workload and filters didn’t
> > perceive any abnormalities, but in fact, the whole system is no longer
> working.
> > This series will report error and try to correct it.
> 
> The problem is how "auto_vnet_hdr" help in this case. It's a new parameter
> which may lead to more wrong configuration?

No, the "auto_vnet_hdr" will fix most the wrong configuration issues as "vnet_hdr_support" correct setting.
When we enable the "auto_vnet_hdr", the original "vnet_hdr_support" will no effect.

Thanks
Chen

> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > Thanks
> > > > Chen
> > > >
> > > >
> > > >>> Thanks
> > > >>>
> > > >>>> Thanks
> > > >>>> Chen
> > > >>>>
> > > >>>>> Thanks
> > > >>>>>
> > > >>>>>
> >

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Friday, November 5, 2021 12:03 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
>> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
>> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector
>> 
>> On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen <chen.zhang@intel.com> wrote:
>> >
>> >
>> >
>> > > -----Original Message-----
>> > > From: Jason Wang <jasowang@redhat.com>
>> > > Sent: Friday, November 5, 2021 11:17 AM
>> > > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
>> > > <armbru@redhat.com>
>> > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
>> > > <lizhijian@cn.fujitsu.com>
>> > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
>> > > for filter- mirror/redirector
>> > >
>> > >
>> > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
>> > > > Hi Jason/Markus,
>> > > >
>> > > > Rethink about it, How about keep the original "vnet_hdr_support"
>> > > > function, And add a new optional parameter "auto_vnet_hdr" for filters/compare?
>> > >
>> > >
>> > > It's a way but rethink of the whole thing. I wonder what if we just
>> > > enable "vnet_hdr_support" by default for filter and colo-compare?
>> >
>> > It's works by default for user use -device virtio-net-pci and e1000...
>> > But it can't resolve this series motivation, how to fix/check user configuration issue:
>> > For example user enable " vnet_hdr_support " filter-mirror and disable
>> > " vnet_hdr_support" filter-redirector And connect both filter modules by chardev socket.
>> > In this case guest will get wrong network workload and filters didn’t
>> > perceive any abnormalities, but in fact, the whole system is no longer working.
>> > This series will report error and try to correct it.
>> 
>> The problem is how "auto_vnet_hdr" help in this case. It's a new parameter
>> which may lead to more wrong configuration?
>
> No, the "auto_vnet_hdr" will fix most the wrong configuration issues as "vnet_hdr_support" correct setting.
> When we enable the "auto_vnet_hdr", the original "vnet_hdr_support" will no effect.

I don't know enough to help much here.  What I do know: having to
specify an obscure parameter to get a nicer user interface is
backwards.  Is this the case here?

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Friday, November 5, 2021 12:03 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> > mirror/redirector
> >
> > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen <chen.zhang@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> > > > <armbru@redhat.com>
> > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > <lizhijian@cn.fujitsu.com>
> > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > > for filter- mirror/redirector
> > > >
> > > >
> > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > >>>>>
> > > > >>>>> I wonder if we need to introduce new parameter, e.g
> > > > >>>>> force_vnet_hdr here, then we can always send vnet_hdr when it
> > is enabled.
> > > > >>>>>
> > > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > > >>>> Yes, Current "vnet_hdr_support"  default enabled, and
> > > > >>>> vnet_hdr_len
> > > > >>> already forced from attached nf->netdev.
> > > > >>>> Maybe we can introduce a new parameter "force_no_vnet_hdr"
> > here
> > > > to
> > > > >>> make the vnet_hdr_len always keep 0.
> > > > >>>> If you think OK, I will update it in next version.
> > > > >>> Let me explain, if I was not wrong:
> > > > >>>
> > > > >>> "vnet_hdr_support" means whether or not to send vnet header
> > length.
> > > > >>> If vnet_hdr_support=false, we won't send the vnet header. This
> > > > >>> looks the same as you "force_no_vent_hdr" above.
> > > > >> Yes, It was.  But this series changed it.
> > > > >> Current "vnet_hdr_support" can't decide whether send vnet header
> > > > >> length, we always send it even 0.
> > > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > > >> When sender data with the vnet header length, but receiver can't
> > > > >> enable the "vnet_hdr_support".
> > > > >> Filters will auto setup vnet_hdr_len as local nf->netdev and
> > > > >> found the issue when get different vnet_hdr_len from other filters.
> > > > >>
> > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > vnet_hdr_support=true.
> > > > >>> So it looks to me we can leave the mirror code as is and just
> > > > >>> change the compare? (depends on the mgmt to set a correct
> > > > >>> vnet_hdr_support)
> > > > >> OK, I will keep the
> > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > >> same as this version.
> > > > >> For the colo-compare module, It will get primary node's filter
> > > > >> data's vnet_hdr_len as the local value, And compare with
> > > > >> secondary node's, because it is not attached any nf->netdev.
> > > > >> So, it looks compare module's "vnet_hdr_support" been auto
> > > > >> configuration from the filter transport protocol.
> > > > >> If the "force_vnet_hdr" means hard code a compare's local
> > > > >> vnet_hdr_len rather than come from input filter's data?
> > > > >>
> > > > >> Thanks
> > > > >> Chen
> > > > >>
> > > > > Hi Jason/Markus,
> > > > >
> > > > > Rethink about it, How about keep the original "vnet_hdr_support"
> > > > > function, And add a new optional parameter "auto_vnet_hdr" for
> > > > filters/compare?
> > > >
> > > >
> > > > It's a way but rethink of the whole thing. I wonder what if we just
> > > > enable "vnet_hdr_support" by default for filter and colo-compare?
> > >
> > > It's works by default for user use -device virtio-net-pci and e1000...
> > > But it can't resolve this series motivation, how to fix/check user
> > configuration issue:
> > > For example user enable " vnet_hdr_support " filter-mirror and disable
> > > " vnet_hdr_support" filter-redirector And connect both filter modules by
> > chardev socket.
> > > In this case guest will get wrong network workload and filters didn’t
> > > perceive any abnormalities, but in fact, the whole system is no longer
> > working.
> > > This series will report error and try to correct it.
> >
> > The problem is how "auto_vnet_hdr" help in this case. It's a new parameter
> > which may lead to more wrong configuration?
>
> No, the "auto_vnet_hdr" will fix most the wrong configuration issues as "vnet_hdr_support" correct setting.
> When we enable the "auto_vnet_hdr", the original "vnet_hdr_support" will no effect.

So it looks to me it still depends on the management to set
"auto_vnet_hdr" to be true? (or make it enabled by default)

If we can do that, isn't it much more simpler to make vnet_hdr_support
by default?

I think I may miss something.

Thanks

>
> Thanks
> Chen
>
> >
> > Thanks
> >
> > >
> > > Thanks
> > > Chen
> > >
> > > >
> > > > Thanks
> > > >
> > > >
> > > > >
> > > > > Thanks
> > > > > Chen
> > > > >
> > > > >
> > > > >>> Thanks
> > > > >>>
> > > > >>>> Thanks
> > > > >>>> Chen
> > > > >>>>
> > > > >>>>> Thanks
> > > > >>>>>
> > > > >>>>>
> > >
>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, November 5, 2021 4:30 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Friday, November 5, 2021 12:03 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > for filter- mirror/redirector
> > >
> > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen <chen.zhang@intel.com>
> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> > > > > <armbru@redhat.com>
> > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > <lizhijian@cn.fujitsu.com>
> > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > protocol for filter- mirror/redirector
> > > > >
> > > > >
> > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > >>>>>
> > > > > >>>>> I wonder if we need to introduce new parameter, e.g
> > > > > >>>>> force_vnet_hdr here, then we can always send vnet_hdr
> when
> > > > > >>>>> it
> > > is enabled.
> > > > > >>>>>
> > > > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > > > >>>> Yes, Current "vnet_hdr_support"  default enabled, and
> > > > > >>>> vnet_hdr_len
> > > > > >>> already forced from attached nf->netdev.
> > > > > >>>> Maybe we can introduce a new parameter
> "force_no_vnet_hdr"
> > > here
> > > > > to
> > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > >>>> If you think OK, I will update it in next version.
> > > > > >>> Let me explain, if I was not wrong:
> > > > > >>>
> > > > > >>> "vnet_hdr_support" means whether or not to send vnet header
> > > length.
> > > > > >>> If vnet_hdr_support=false, we won't send the vnet header.
> > > > > >>> This looks the same as you "force_no_vent_hdr" above.
> > > > > >> Yes, It was.  But this series changed it.
> > > > > >> Current "vnet_hdr_support" can't decide whether send vnet
> > > > > >> header length, we always send it even 0.
> > > > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > > > >> When sender data with the vnet header length, but receiver
> > > > > >> can't enable the "vnet_hdr_support".
> > > > > >> Filters will auto setup vnet_hdr_len as local nf->netdev and
> > > > > >> found the issue when get different vnet_hdr_len from other
> filters.
> > > > > >>
> > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > vnet_hdr_support=true.
> > > > > >>> So it looks to me we can leave the mirror code as is and
> > > > > >>> just change the compare? (depends on the mgmt to set a
> > > > > >>> correct
> > > > > >>> vnet_hdr_support)
> > > > > >> OK, I will keep the
> > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > >> same as this version.
> > > > > >> For the colo-compare module, It will get primary node's
> > > > > >> filter data's vnet_hdr_len as the local value, And compare
> > > > > >> with secondary node's, because it is not attached any nf->netdev.
> > > > > >> So, it looks compare module's "vnet_hdr_support" been auto
> > > > > >> configuration from the filter transport protocol.
> > > > > >> If the "force_vnet_hdr" means hard code a compare's local
> > > > > >> vnet_hdr_len rather than come from input filter's data?
> > > > > >>
> > > > > >> Thanks
> > > > > >> Chen
> > > > > >>
> > > > > > Hi Jason/Markus,
> > > > > >
> > > > > > Rethink about it, How about keep the original "vnet_hdr_support"
> > > > > > function, And add a new optional parameter "auto_vnet_hdr" for
> > > > > filters/compare?
> > > > >
> > > > >
> > > > > It's a way but rethink of the whole thing. I wonder what if we
> > > > > just enable "vnet_hdr_support" by default for filter and colo-
> compare?
> > > >
> > > > It's works by default for user use -device virtio-net-pci and e1000...
> > > > But it can't resolve this series motivation, how to fix/check user
> > > configuration issue:
> > > > For example user enable " vnet_hdr_support " filter-mirror and
> > > > disable " vnet_hdr_support" filter-redirector And connect both
> > > > filter modules by
> > > chardev socket.
> > > > In this case guest will get wrong network workload and filters
> > > > didn’t perceive any abnormalities, but in fact, the whole system
> > > > is no longer
> > > working.
> > > > This series will report error and try to correct it.
> > >
> > > The problem is how "auto_vnet_hdr" help in this case. It's a new
> > > parameter which may lead to more wrong configuration?
> >
> > No, the "auto_vnet_hdr" will fix most the wrong configuration issues as
> "vnet_hdr_support" correct setting.
> > When we enable the "auto_vnet_hdr", the original "vnet_hdr_support"
> will no effect.
> 
> So it looks to me it still depends on the management to set "auto_vnet_hdr"
> to be true? (or make it enabled by default)

Yes, I plan to make "auto_vnet_hdr" enabled by default in next version. 

> 
> If we can do that, isn't it much more simpler to make vnet_hdr_support by
> default?

Yes, For compatibility filters and COLO still work with the original "vnet_hdr_support".
For new users, they can enable the new "auto_vnet_hdr" to avoid some issues.

Thanks
Chen

> 
> I think I may miss something.
> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > > Chen
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > Chen
> > > > > >
> > > > > >
> > > > > >>> Thanks
> > > > > >>>
> > > > > >>>> Thanks
> > > > > >>>> Chen
> > > > > >>>>
> > > > > >>>>> Thanks
> > > > > >>>>>
> > > > > >>>>>
> > > >
> >

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Friday, November 5, 2021 4:30 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> > mirror/redirector
> >
> > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > > for filter- mirror/redirector
> > > >
> > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen <chen.zhang@intel.com>
> > wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> > > > > > <armbru@redhat.com>
> > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > protocol for filter- mirror/redirector
> > > > > >
> > > > > >
> > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > >>>>>
> > > > > > >>>>> I wonder if we need to introduce new parameter, e.g
> > > > > > >>>>> force_vnet_hdr here, then we can always send vnet_hdr
> > when
> > > > > > >>>>> it
> > > > is enabled.
> > > > > > >>>>>
> > > > > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > > > > >>>> Yes, Current "vnet_hdr_support"  default enabled, and
> > > > > > >>>> vnet_hdr_len
> > > > > > >>> already forced from attached nf->netdev.
> > > > > > >>>> Maybe we can introduce a new parameter
> > "force_no_vnet_hdr"
> > > > here
> > > > > > to
> > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > >>> Let me explain, if I was not wrong:
> > > > > > >>>
> > > > > > >>> "vnet_hdr_support" means whether or not to send vnet header
> > > > length.
> > > > > > >>> If vnet_hdr_support=false, we won't send the vnet header.
> > > > > > >>> This looks the same as you "force_no_vent_hdr" above.
> > > > > > >> Yes, It was.  But this series changed it.
> > > > > > >> Current "vnet_hdr_support" can't decide whether send vnet
> > > > > > >> header length, we always send it even 0.
> > > > > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > > > > >> When sender data with the vnet header length, but receiver
> > > > > > >> can't enable the "vnet_hdr_support".
> > > > > > >> Filters will auto setup vnet_hdr_len as local nf->netdev and
> > > > > > >> found the issue when get different vnet_hdr_len from other
> > filters.
> > > > > > >>
> > > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > > vnet_hdr_support=true.
> > > > > > >>> So it looks to me we can leave the mirror code as is and
> > > > > > >>> just change the compare? (depends on the mgmt to set a
> > > > > > >>> correct
> > > > > > >>> vnet_hdr_support)
> > > > > > >> OK, I will keep the
> > > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > > >> same as this version.
> > > > > > >> For the colo-compare module, It will get primary node's
> > > > > > >> filter data's vnet_hdr_len as the local value, And compare
> > > > > > >> with secondary node's, because it is not attached any nf->netdev.
> > > > > > >> So, it looks compare module's "vnet_hdr_support" been auto
> > > > > > >> configuration from the filter transport protocol.
> > > > > > >> If the "force_vnet_hdr" means hard code a compare's local
> > > > > > >> vnet_hdr_len rather than come from input filter's data?
> > > > > > >>
> > > > > > >> Thanks
> > > > > > >> Chen
> > > > > > >>
> > > > > > > Hi Jason/Markus,
> > > > > > >
> > > > > > > Rethink about it, How about keep the original "vnet_hdr_support"
> > > > > > > function, And add a new optional parameter "auto_vnet_hdr" for
> > > > > > filters/compare?
> > > > > >
> > > > > >
> > > > > > It's a way but rethink of the whole thing. I wonder what if we
> > > > > > just enable "vnet_hdr_support" by default for filter and colo-
> > compare?
> > > > >
> > > > > It's works by default for user use -device virtio-net-pci and e1000...
> > > > > But it can't resolve this series motivation, how to fix/check user
> > > > configuration issue:
> > > > > For example user enable " vnet_hdr_support " filter-mirror and
> > > > > disable " vnet_hdr_support" filter-redirector And connect both
> > > > > filter modules by
> > > > chardev socket.
> > > > > In this case guest will get wrong network workload and filters
> > > > > didn’t perceive any abnormalities, but in fact, the whole system
> > > > > is no longer
> > > > working.
> > > > > This series will report error and try to correct it.
> > > >
> > > > The problem is how "auto_vnet_hdr" help in this case. It's a new
> > > > parameter which may lead to more wrong configuration?
> > >
> > > No, the "auto_vnet_hdr" will fix most the wrong configuration issues as
> > "vnet_hdr_support" correct setting.
> > > When we enable the "auto_vnet_hdr", the original "vnet_hdr_support"
> > will no effect.
> >
> > So it looks to me it still depends on the management to set "auto_vnet_hdr"
> > to be true? (or make it enabled by default)
>
> Yes, I plan to make "auto_vnet_hdr" enabled by default in next version.
>
> >
> > If we can do that, isn't it much more simpler to make vnet_hdr_support by
> > default?
>
> Yes, For compatibility filters and COLO still work with the original "vnet_hdr_support".
> For new users, they can enable the new "auto_vnet_hdr" to avoid some issues.

Question still, so we have

1) enable vnet_hdr_support by default
2) add auto_vnet_hdr and enable it by default

Using 1) seems much more simpler and can solve this issue. If we
depends on the default behaviour, it should be almost the same. If we
want to teach the mgmt, both should work. Any other advantages of 2)?

Thanks

>
> Thanks
> Chen
>
> >
> > I think I may miss something.
> >
> > Thanks
> >
> > >
> > > Thanks
> > > Chen
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks
> > > > > Chen
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > > Chen
> > > > > > >
> > > > > > >
> > > > > > >>> Thanks
> > > > > > >>>
> > > > > > >>>> Thanks
> > > > > > >>>> Chen
> > > > > > >>>>
> > > > > > >>>>> Thanks
> > > > > > >>>>>
> > > > > > >>>>>
> > > > >
> > >
>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, November 8, 2021 10:42 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Friday, November 5, 2021 4:30 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > for filter- mirror/redirector
> > >
> > > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen <chen.zhang@intel.com>
> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > protocol for filter- mirror/redirector
> > > > >
> > > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen
> > > > > <chen.zhang@intel.com>
> > > wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> > > > > > > <armbru@redhat.com>
> > > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > > protocol for filter- mirror/redirector
> > > > > > >
> > > > > > >
> > > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > > >>>>>
> > > > > > > >>>>> I wonder if we need to introduce new parameter, e.g
> > > > > > > >>>>> force_vnet_hdr here, then we can always send vnet_hdr
> > > when
> > > > > > > >>>>> it
> > > > > is enabled.
> > > > > > > >>>>>
> > > > > > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > > > > > >>>> Yes, Current "vnet_hdr_support"  default enabled, and
> > > > > > > >>>> vnet_hdr_len
> > > > > > > >>> already forced from attached nf->netdev.
> > > > > > > >>>> Maybe we can introduce a new parameter
> > > "force_no_vnet_hdr"
> > > > > here
> > > > > > > to
> > > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > > >>> Let me explain, if I was not wrong:
> > > > > > > >>>
> > > > > > > >>> "vnet_hdr_support" means whether or not to send vnet
> > > > > > > >>> header
> > > > > length.
> > > > > > > >>> If vnet_hdr_support=false, we won't send the vnet header.
> > > > > > > >>> This looks the same as you "force_no_vent_hdr" above.
> > > > > > > >> Yes, It was.  But this series changed it.
> > > > > > > >> Current "vnet_hdr_support" can't decide whether send vnet
> > > > > > > >> header length, we always send it even 0.
> > > > > > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > > > > > >> When sender data with the vnet header length, but
> > > > > > > >> receiver can't enable the "vnet_hdr_support".
> > > > > > > >> Filters will auto setup vnet_hdr_len as local nf->netdev
> > > > > > > >> and found the issue when get different vnet_hdr_len from
> > > > > > > >> other
> > > filters.
> > > > > > > >>
> > > > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > > > vnet_hdr_support=true.
> > > > > > > >>> So it looks to me we can leave the mirror code as is and
> > > > > > > >>> just change the compare? (depends on the mgmt to set a
> > > > > > > >>> correct
> > > > > > > >>> vnet_hdr_support)
> > > > > > > >> OK, I will keep the
> > > > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > > > >> same as this version.
> > > > > > > >> For the colo-compare module, It will get primary node's
> > > > > > > >> filter data's vnet_hdr_len as the local value, And
> > > > > > > >> compare with secondary node's, because it is not attached any
> nf->netdev.
> > > > > > > >> So, it looks compare module's "vnet_hdr_support" been
> > > > > > > >> auto configuration from the filter transport protocol.
> > > > > > > >> If the "force_vnet_hdr" means hard code a compare's local
> > > > > > > >> vnet_hdr_len rather than come from input filter's data?
> > > > > > > >>
> > > > > > > >> Thanks
> > > > > > > >> Chen
> > > > > > > >>
> > > > > > > > Hi Jason/Markus,
> > > > > > > >
> > > > > > > > Rethink about it, How about keep the original
> "vnet_hdr_support"
> > > > > > > > function, And add a new optional parameter "auto_vnet_hdr"
> > > > > > > > for
> > > > > > > filters/compare?
> > > > > > >
> > > > > > >
> > > > > > > It's a way but rethink of the whole thing. I wonder what if
> > > > > > > we just enable "vnet_hdr_support" by default for filter and
> > > > > > > colo-
> > > compare?
> > > > > >
> > > > > > It's works by default for user use -device virtio-net-pci and e1000...
> > > > > > But it can't resolve this series motivation, how to fix/check
> > > > > > user
> > > > > configuration issue:
> > > > > > For example user enable " vnet_hdr_support " filter-mirror and
> > > > > > disable " vnet_hdr_support" filter-redirector And connect both
> > > > > > filter modules by
> > > > > chardev socket.
> > > > > > In this case guest will get wrong network workload and filters
> > > > > > didn’t perceive any abnormalities, but in fact, the whole
> > > > > > system is no longer
> > > > > working.
> > > > > > This series will report error and try to correct it.
> > > > >
> > > > > The problem is how "auto_vnet_hdr" help in this case. It's a new
> > > > > parameter which may lead to more wrong configuration?
> > > >
> > > > No, the "auto_vnet_hdr" will fix most the wrong configuration
> > > > issues as
> > > "vnet_hdr_support" correct setting.
> > > > When we enable the "auto_vnet_hdr", the original
> "vnet_hdr_support"
> > > will no effect.
> > >
> > > So it looks to me it still depends on the management to set
> "auto_vnet_hdr"
> > > to be true? (or make it enabled by default)
> >
> > Yes, I plan to make "auto_vnet_hdr" enabled by default in next version.
> >
> > >
> > > If we can do that, isn't it much more simpler to make
> > > vnet_hdr_support by default?
> >
> > Yes, For compatibility filters and COLO still work with the original
> "vnet_hdr_support".
> > For new users, they can enable the new "auto_vnet_hdr" to avoid some
> issues.
> 
> Question still, so we have
> 
> 1) enable vnet_hdr_support by default
> 2) add auto_vnet_hdr and enable it by default
> 
> Using 1) seems much more simpler and can solve this issue. If we depends
> on the default behaviour, it should be almost the same. If we want to teach
> the mgmt, both should work. Any other advantages of 2)?

Using 1) we can't ensure user configure parts of module by itself. (vnet_hdr_support = off).
In this case, filter data already wrong and the filters can't report any transfer error here.

Using 2) filters will find/report this issue and try to fix it from local vnet_hdr_len.

Thanks
Chen

> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> > >
> > > I think I may miss something.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > > Chen
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > Chen
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > Chen
> > > > > > > >
> > > > > > > >
> > > > > > > >>> Thanks
> > > > > > > >>>
> > > > > > > >>>> Thanks
> > > > > > > >>>> Chen
> > > > > > > >>>>
> > > > > > > >>>>> Thanks
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > >
> > > >
> >

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

On Mon, Nov 8, 2021 at 10:50 AM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Monday, November 8, 2021 10:42 AM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> > mirror/redirector
> >
> > On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Friday, November 5, 2021 4:30 PM
> > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > > for filter- mirror/redirector
> > > >
> > > > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen <chen.zhang@intel.com>
> > wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > protocol for filter- mirror/redirector
> > > > > >
> > > > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen
> > > > > > <chen.zhang@intel.com>
> > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus Armbruster
> > > > > > > > <armbru@redhat.com>
> > > > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > > > protocol for filter- mirror/redirector
> > > > > > > >
> > > > > > > >
> > > > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > > > >>>>>
> > > > > > > > >>>>> I wonder if we need to introduce new parameter, e.g
> > > > > > > > >>>>> force_vnet_hdr here, then we can always send vnet_hdr
> > > > when
> > > > > > > > >>>>> it
> > > > > > is enabled.
> > > > > > > > >>>>>
> > > > > > > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > > > > > > >>>> Yes, Current "vnet_hdr_support"  default enabled, and
> > > > > > > > >>>> vnet_hdr_len
> > > > > > > > >>> already forced from attached nf->netdev.
> > > > > > > > >>>> Maybe we can introduce a new parameter
> > > > "force_no_vnet_hdr"
> > > > > > here
> > > > > > > > to
> > > > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > > > >>> Let me explain, if I was not wrong:
> > > > > > > > >>>
> > > > > > > > >>> "vnet_hdr_support" means whether or not to send vnet
> > > > > > > > >>> header
> > > > > > length.
> > > > > > > > >>> If vnet_hdr_support=false, we won't send the vnet header.
> > > > > > > > >>> This looks the same as you "force_no_vent_hdr" above.
> > > > > > > > >> Yes, It was.  But this series changed it.
> > > > > > > > >> Current "vnet_hdr_support" can't decide whether send vnet
> > > > > > > > >> header length, we always send it even 0.
> > > > > > > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > > > > > > >> When sender data with the vnet header length, but
> > > > > > > > >> receiver can't enable the "vnet_hdr_support".
> > > > > > > > >> Filters will auto setup vnet_hdr_len as local nf->netdev
> > > > > > > > >> and found the issue when get different vnet_hdr_len from
> > > > > > > > >> other
> > > > filters.
> > > > > > > > >>
> > > > > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > > > > vnet_hdr_support=true.
> > > > > > > > >>> So it looks to me we can leave the mirror code as is and
> > > > > > > > >>> just change the compare? (depends on the mgmt to set a
> > > > > > > > >>> correct
> > > > > > > > >>> vnet_hdr_support)
> > > > > > > > >> OK, I will keep the
> > > > > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > > > > >> same as this version.
> > > > > > > > >> For the colo-compare module, It will get primary node's
> > > > > > > > >> filter data's vnet_hdr_len as the local value, And
> > > > > > > > >> compare with secondary node's, because it is not attached any
> > nf->netdev.
> > > > > > > > >> So, it looks compare module's "vnet_hdr_support" been
> > > > > > > > >> auto configuration from the filter transport protocol.
> > > > > > > > >> If the "force_vnet_hdr" means hard code a compare's local
> > > > > > > > >> vnet_hdr_len rather than come from input filter's data?
> > > > > > > > >>
> > > > > > > > >> Thanks
> > > > > > > > >> Chen
> > > > > > > > >>
> > > > > > > > > Hi Jason/Markus,
> > > > > > > > >
> > > > > > > > > Rethink about it, How about keep the original
> > "vnet_hdr_support"
> > > > > > > > > function, And add a new optional parameter "auto_vnet_hdr"
> > > > > > > > > for
> > > > > > > > filters/compare?
> > > > > > > >
> > > > > > > >
> > > > > > > > It's a way but rethink of the whole thing. I wonder what if
> > > > > > > > we just enable "vnet_hdr_support" by default for filter and
> > > > > > > > colo-
> > > > compare?
> > > > > > >
> > > > > > > It's works by default for user use -device virtio-net-pci and e1000...
> > > > > > > But it can't resolve this series motivation, how to fix/check
> > > > > > > user
> > > > > > configuration issue:
> > > > > > > For example user enable " vnet_hdr_support " filter-mirror and
> > > > > > > disable " vnet_hdr_support" filter-redirector And connect both
> > > > > > > filter modules by
> > > > > > chardev socket.
> > > > > > > In this case guest will get wrong network workload and filters
> > > > > > > didn’t perceive any abnormalities, but in fact, the whole
> > > > > > > system is no longer
> > > > > > working.
> > > > > > > This series will report error and try to correct it.
> > > > > >
> > > > > > The problem is how "auto_vnet_hdr" help in this case. It's a new
> > > > > > parameter which may lead to more wrong configuration?
> > > > >
> > > > > No, the "auto_vnet_hdr" will fix most the wrong configuration
> > > > > issues as
> > > > "vnet_hdr_support" correct setting.
> > > > > When we enable the "auto_vnet_hdr", the original
> > "vnet_hdr_support"
> > > > will no effect.
> > > >
> > > > So it looks to me it still depends on the management to set
> > "auto_vnet_hdr"
> > > > to be true? (or make it enabled by default)
> > >
> > > Yes, I plan to make "auto_vnet_hdr" enabled by default in next version.
> > >
> > > >
> > > > If we can do that, isn't it much more simpler to make
> > > > vnet_hdr_support by default?
> > >
> > > Yes, For compatibility filters and COLO still work with the original
> > "vnet_hdr_support".
> > > For new users, they can enable the new "auto_vnet_hdr" to avoid some
> > issues.
> >
> > Question still, so we have
> >
> > 1) enable vnet_hdr_support by default
> > 2) add auto_vnet_hdr and enable it by default
> >
> > Using 1) seems much more simpler and can solve this issue. If we depends
> > on the default behaviour, it should be almost the same. If we want to teach
> > the mgmt, both should work. Any other advantages of 2)?
>
> Using 1) we can't ensure user configure parts of module by itself. (vnet_hdr_support = off).
> In this case, filter data already wrong and the filters can't report any transfer error here.

So I think the point is we can't ensure the user configure parts of
module in any case even if auto_vnet_hdr is introduced. E.g user can
choose to disable it manually. That's why I think it should not differ
too much from making vnet_hdr_support enabled by default.

Thanks

>
> Using 2) filters will find/report this issue and try to fix it from local vnet_hdr_len.
>
> Thanks
> Chen
>
> >
> > Thanks
> >
> > >
> > > Thanks
> > > Chen
> > >
> > > >
> > > > I think I may miss something.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks
> > > > > Chen
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > > Chen
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > > Chen
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >>> Thanks
> > > > > > > > >>>
> > > > > > > > >>>> Thanks
> > > > > > > > >>>> Chen
> > > > > > > > >>>>
> > > > > > > > >>>>> Thanks
> > > > > > > > >>>>>
> > > > > > > > >>>>>
> > > > > > >
> > > > >
> > >
>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, November 9, 2021 2:42 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> On Mon, Nov 8, 2021 at 10:50 AM Zhang, Chen <chen.zhang@intel.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Monday, November 8, 2021 10:42 AM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > for filter- mirror/redirector
> > >
> > > On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen <chen.zhang@intel.com>
> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Friday, November 5, 2021 4:30 PM
> > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > protocol for filter- mirror/redirector
> > > > >
> > > > > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen
> > > > > <chen.zhang@intel.com>
> > > wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> <qemu-
> > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > > protocol for filter- mirror/redirector
> > > > > > >
> > > > > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen
> > > > > > > <chen.zhang@intel.com>
> > > > > wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus
> > > > > > > > > Armbruster <armbru@redhat.com>
> > > > > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > > > > >>>>>
> > > > > > > > > >>>>> I wonder if we need to introduce new parameter,
> > > > > > > > > >>>>> e.g force_vnet_hdr here, then we can always send
> > > > > > > > > >>>>> vnet_hdr
> > > > > when
> > > > > > > > > >>>>> it
> > > > > > > is enabled.
> > > > > > > > > >>>>>
> > > > > > > > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > > > > > > > >>>> Yes, Current "vnet_hdr_support"  default enabled,
> > > > > > > > > >>>> and vnet_hdr_len
> > > > > > > > > >>> already forced from attached nf->netdev.
> > > > > > > > > >>>> Maybe we can introduce a new parameter
> > > > > "force_no_vnet_hdr"
> > > > > > > here
> > > > > > > > > to
> > > > > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > > > > >>> Let me explain, if I was not wrong:
> > > > > > > > > >>>
> > > > > > > > > >>> "vnet_hdr_support" means whether or not to send vnet
> > > > > > > > > >>> header
> > > > > > > length.
> > > > > > > > > >>> If vnet_hdr_support=false, we won't send the vnet
> header.
> > > > > > > > > >>> This looks the same as you "force_no_vent_hdr" above.
> > > > > > > > > >> Yes, It was.  But this series changed it.
> > > > > > > > > >> Current "vnet_hdr_support" can't decide whether send
> > > > > > > > > >> vnet header length, we always send it even 0.
> > > > > > > > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > > > > > > > >> When sender data with the vnet header length, but
> > > > > > > > > >> receiver can't enable the "vnet_hdr_support".
> > > > > > > > > >> Filters will auto setup vnet_hdr_len as local
> > > > > > > > > >> nf->netdev and found the issue when get different
> > > > > > > > > >> vnet_hdr_len from other
> > > > > filters.
> > > > > > > > > >>
> > > > > > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > > > > > vnet_hdr_support=true.
> > > > > > > > > >>> So it looks to me we can leave the mirror code as is
> > > > > > > > > >>> and just change the compare? (depends on the mgmt to
> > > > > > > > > >>> set a correct
> > > > > > > > > >>> vnet_hdr_support)
> > > > > > > > > >> OK, I will keep the
> > > > > > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > > > > > >> same as this version.
> > > > > > > > > >> For the colo-compare module, It will get primary
> > > > > > > > > >> node's filter data's vnet_hdr_len as the local value,
> > > > > > > > > >> And compare with secondary node's, because it is not
> > > > > > > > > >> attached any
> > > nf->netdev.
> > > > > > > > > >> So, it looks compare module's "vnet_hdr_support" been
> > > > > > > > > >> auto configuration from the filter transport protocol.
> > > > > > > > > >> If the "force_vnet_hdr" means hard code a compare's
> > > > > > > > > >> local vnet_hdr_len rather than come from input filter's data?
> > > > > > > > > >>
> > > > > > > > > >> Thanks
> > > > > > > > > >> Chen
> > > > > > > > > >>
> > > > > > > > > > Hi Jason/Markus,
> > > > > > > > > >
> > > > > > > > > > Rethink about it, How about keep the original
> > > "vnet_hdr_support"
> > > > > > > > > > function, And add a new optional parameter
> "auto_vnet_hdr"
> > > > > > > > > > for
> > > > > > > > > filters/compare?
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > It's a way but rethink of the whole thing. I wonder what
> > > > > > > > > if we just enable "vnet_hdr_support" by default for
> > > > > > > > > filter and
> > > > > > > > > colo-
> > > > > compare?
> > > > > > > >
> > > > > > > > It's works by default for user use -device virtio-net-pci and
> e1000...
> > > > > > > > But it can't resolve this series motivation, how to
> > > > > > > > fix/check user
> > > > > > > configuration issue:
> > > > > > > > For example user enable " vnet_hdr_support " filter-mirror
> > > > > > > > and disable " vnet_hdr_support" filter-redirector And
> > > > > > > > connect both filter modules by
> > > > > > > chardev socket.
> > > > > > > > In this case guest will get wrong network workload and
> > > > > > > > filters didn’t perceive any abnormalities, but in fact,
> > > > > > > > the whole system is no longer
> > > > > > > working.
> > > > > > > > This series will report error and try to correct it.
> > > > > > >
> > > > > > > The problem is how "auto_vnet_hdr" help in this case. It's a
> > > > > > > new parameter which may lead to more wrong configuration?
> > > > > >
> > > > > > No, the "auto_vnet_hdr" will fix most the wrong configuration
> > > > > > issues as
> > > > > "vnet_hdr_support" correct setting.
> > > > > > When we enable the "auto_vnet_hdr", the original
> > > "vnet_hdr_support"
> > > > > will no effect.
> > > > >
> > > > > So it looks to me it still depends on the management to set
> > > "auto_vnet_hdr"
> > > > > to be true? (or make it enabled by default)
> > > >
> > > > Yes, I plan to make "auto_vnet_hdr" enabled by default in next version.
> > > >
> > > > >
> > > > > If we can do that, isn't it much more simpler to make
> > > > > vnet_hdr_support by default?
> > > >
> > > > Yes, For compatibility filters and COLO still work with the
> > > > original
> > > "vnet_hdr_support".
> > > > For new users, they can enable the new "auto_vnet_hdr" to avoid
> > > > some
> > > issues.
> > >
> > > Question still, so we have
> > >
> > > 1) enable vnet_hdr_support by default
> > > 2) add auto_vnet_hdr and enable it by default
> > >
> > > Using 1) seems much more simpler and can solve this issue. If we
> > > depends on the default behaviour, it should be almost the same. If
> > > we want to teach the mgmt, both should work. Any other advantages of
> 2)?
> >
> > Using 1) we can't ensure user configure parts of module by itself.
> (vnet_hdr_support = off).
> > In this case, filter data already wrong and the filters can't report any
> transfer error here.
> 
> So I think the point is we can't ensure the user configure parts of module in
> any case even if auto_vnet_hdr is introduced. E.g user can choose to disable
> it manually. That's why I think it should not differ too much from making
> vnet_hdr_support enabled by default.

Yes, you are right. The "auto_vnet_hdr" just can fix part of user configure issue.
And I think this series make the filters better, it make user know filters have some issues
when they have wrong manual configuration(current code not).

Thanks
Chen

> 
> Thanks
> 
> >
> > Using 2) filters will find/report this issue and try to fix it from local
> vnet_hdr_len.
> >
> > Thanks
> > Chen
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > > Chen
> > > >
> > > > >
> > > > > I think I may miss something.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > Chen
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > Chen
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > > Chen
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >>> Thanks
> > > > > > > > > >>>
> > > > > > > > > >>>> Thanks
> > > > > > > > > >>>> Chen
> > > > > > > > > >>>>
> > > > > > > > > >>>>> Thanks
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>
> > > > > > > >
> > > > > >
> > > >
> >

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

On Tue, Nov 9, 2021 at 3:20 PM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, November 9, 2021 2:42 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> > mirror/redirector
> >
> > On Mon, Nov 8, 2021 at 10:50 AM Zhang, Chen <chen.zhang@intel.com>
> > wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Monday, November 8, 2021 10:42 AM
> > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > > for filter- mirror/redirector
> > > >
> > > > On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen <chen.zhang@intel.com>
> > wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Friday, November 5, 2021 4:30 PM
> > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > protocol for filter- mirror/redirector
> > > > > >
> > > > > > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen
> > > > > > <chen.zhang@intel.com>
> > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> > <qemu-
> > > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > > > protocol for filter- mirror/redirector
> > > > > > > >
> > > > > > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen
> > > > > > > > <chen.zhang@intel.com>
> > > > > > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus
> > > > > > > > > > Armbruster <armbru@redhat.com>
> > > > > > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>> I wonder if we need to introduce new parameter,
> > > > > > > > > > >>>>> e.g force_vnet_hdr here, then we can always send
> > > > > > > > > > >>>>> vnet_hdr
> > > > > > when
> > > > > > > > > > >>>>> it
> > > > > > > > is enabled.
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>> Otherwise the "vnet_hdr_support" seems meaningless.
> > > > > > > > > > >>>> Yes, Current "vnet_hdr_support"  default enabled,
> > > > > > > > > > >>>> and vnet_hdr_len
> > > > > > > > > > >>> already forced from attached nf->netdev.
> > > > > > > > > > >>>> Maybe we can introduce a new parameter
> > > > > > "force_no_vnet_hdr"
> > > > > > > > here
> > > > > > > > > > to
> > > > > > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > > > > > >>> Let me explain, if I was not wrong:
> > > > > > > > > > >>>
> > > > > > > > > > >>> "vnet_hdr_support" means whether or not to send vnet
> > > > > > > > > > >>> header
> > > > > > > > length.
> > > > > > > > > > >>> If vnet_hdr_support=false, we won't send the vnet
> > header.
> > > > > > > > > > >>> This looks the same as you "force_no_vent_hdr" above.
> > > > > > > > > > >> Yes, It was.  But this series changed it.
> > > > > > > > > > >> Current "vnet_hdr_support" can't decide whether send
> > > > > > > > > > >> vnet header length, we always send it even 0.
> > > > > > > > > > >> It will avoid sender/receiver transfer protocol parse issues:
> > > > > > > > > > >> When sender data with the vnet header length, but
> > > > > > > > > > >> receiver can't enable the "vnet_hdr_support".
> > > > > > > > > > >> Filters will auto setup vnet_hdr_len as local
> > > > > > > > > > >> nf->netdev and found the issue when get different
> > > > > > > > > > >> vnet_hdr_len from other
> > > > > > filters.
> > > > > > > > > > >>
> > > > > > > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > > > > > > vnet_hdr_support=true.
> > > > > > > > > > >>> So it looks to me we can leave the mirror code as is
> > > > > > > > > > >>> and just change the compare? (depends on the mgmt to
> > > > > > > > > > >>> set a correct
> > > > > > > > > > >>> vnet_hdr_support)
> > > > > > > > > > >> OK, I will keep the
> > > > > > > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > > > > > > >> same as this version.
> > > > > > > > > > >> For the colo-compare module, It will get primary
> > > > > > > > > > >> node's filter data's vnet_hdr_len as the local value,
> > > > > > > > > > >> And compare with secondary node's, because it is not
> > > > > > > > > > >> attached any
> > > > nf->netdev.
> > > > > > > > > > >> So, it looks compare module's "vnet_hdr_support" been
> > > > > > > > > > >> auto configuration from the filter transport protocol.
> > > > > > > > > > >> If the "force_vnet_hdr" means hard code a compare's
> > > > > > > > > > >> local vnet_hdr_len rather than come from input filter's data?
> > > > > > > > > > >>
> > > > > > > > > > >> Thanks
> > > > > > > > > > >> Chen
> > > > > > > > > > >>
> > > > > > > > > > > Hi Jason/Markus,
> > > > > > > > > > >
> > > > > > > > > > > Rethink about it, How about keep the original
> > > > "vnet_hdr_support"
> > > > > > > > > > > function, And add a new optional parameter
> > "auto_vnet_hdr"
> > > > > > > > > > > for
> > > > > > > > > > filters/compare?
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > It's a way but rethink of the whole thing. I wonder what
> > > > > > > > > > if we just enable "vnet_hdr_support" by default for
> > > > > > > > > > filter and
> > > > > > > > > > colo-
> > > > > > compare?
> > > > > > > > >
> > > > > > > > > It's works by default for user use -device virtio-net-pci and
> > e1000...
> > > > > > > > > But it can't resolve this series motivation, how to
> > > > > > > > > fix/check user
> > > > > > > > configuration issue:
> > > > > > > > > For example user enable " vnet_hdr_support " filter-mirror
> > > > > > > > > and disable " vnet_hdr_support" filter-redirector And
> > > > > > > > > connect both filter modules by
> > > > > > > > chardev socket.
> > > > > > > > > In this case guest will get wrong network workload and
> > > > > > > > > filters didn’t perceive any abnormalities, but in fact,
> > > > > > > > > the whole system is no longer
> > > > > > > > working.
> > > > > > > > > This series will report error and try to correct it.
> > > > > > > >
> > > > > > > > The problem is how "auto_vnet_hdr" help in this case. It's a
> > > > > > > > new parameter which may lead to more wrong configuration?
> > > > > > >
> > > > > > > No, the "auto_vnet_hdr" will fix most the wrong configuration
> > > > > > > issues as
> > > > > > "vnet_hdr_support" correct setting.
> > > > > > > When we enable the "auto_vnet_hdr", the original
> > > > "vnet_hdr_support"
> > > > > > will no effect.
> > > > > >
> > > > > > So it looks to me it still depends on the management to set
> > > > "auto_vnet_hdr"
> > > > > > to be true? (or make it enabled by default)
> > > > >
> > > > > Yes, I plan to make "auto_vnet_hdr" enabled by default in next version.
> > > > >
> > > > > >
> > > > > > If we can do that, isn't it much more simpler to make
> > > > > > vnet_hdr_support by default?
> > > > >
> > > > > Yes, For compatibility filters and COLO still work with the
> > > > > original
> > > > "vnet_hdr_support".
> > > > > For new users, they can enable the new "auto_vnet_hdr" to avoid
> > > > > some
> > > > issues.
> > > >
> > > > Question still, so we have
> > > >
> > > > 1) enable vnet_hdr_support by default
> > > > 2) add auto_vnet_hdr and enable it by default
> > > >
> > > > Using 1) seems much more simpler and can solve this issue. If we
> > > > depends on the default behaviour, it should be almost the same. If
> > > > we want to teach the mgmt, both should work. Any other advantages of
> > 2)?
> > >
> > > Using 1) we can't ensure user configure parts of module by itself.
> > (vnet_hdr_support = off).
> > > In this case, filter data already wrong and the filters can't report any
> > transfer error here.
> >
> > So I think the point is we can't ensure the user configure parts of module in
> > any case even if auto_vnet_hdr is introduced. E.g user can choose to disable
> > it manually. That's why I think it should not differ too much from making
> > vnet_hdr_support enabled by default.
>
> Yes, you are right. The "auto_vnet_hdr" just can fix part of user configure issue.
> And I think this series make the filters better, it make user know filters have some issues
> when they have wrong manual configuration(current code not).

I think if you stick to the change, I wonder if something like
"vnet_hdr_support=auto" would be better? (not sure if it's too late to
change)

Thanks

>
> Thanks
> Chen
>
> >
> > Thanks
> >
> > >
> > > Using 2) filters will find/report this issue and try to fix it from local
> > vnet_hdr_len.
> > >
> > > Thanks
> > > Chen
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks
> > > > > Chen
> > > > >
> > > > > >
> > > > > > I think I may miss something.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > > Chen
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > > Chen
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > > Chen
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >>> Thanks
> > > > > > > > > > >>>
> > > > > > > > > > >>>> Thanks
> > > > > > > > > > >>>> Chen
> > > > > > > > > > >>>>
> > > > > > > > > > >>>>> Thanks
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>>
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, November 9, 2021 3:26 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> On Tue, Nov 9, 2021 at 3:20 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, November 9, 2021 2:42 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > for filter- mirror/redirector
> > >
> > > On Mon, Nov 8, 2021 at 10:50 AM Zhang, Chen <chen.zhang@intel.com>
> > > wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Monday, November 8, 2021 10:42 AM
> > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > protocol for filter- mirror/redirector
> > > > >
> > > > > On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen
> > > > > <chen.zhang@intel.com>
> > > wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Friday, November 5, 2021 4:30 PM
> > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> <qemu-
> > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > > protocol for filter- mirror/redirector
> > > > > > >
> > > > > > > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen
> > > > > > > <chen.zhang@intel.com>
> > > > > wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> > > <qemu-
> > > > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > >
> > > > > > > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen
> > > > > > > > > <chen.zhang@intel.com>
> > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus
> > > > > > > > > > > Armbruster <armbru@redhat.com>
> > > > > > > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>> I wonder if we need to introduce new
> > > > > > > > > > > >>>>> parameter, e.g force_vnet_hdr here, then we
> > > > > > > > > > > >>>>> can always send vnet_hdr
> > > > > > > when
> > > > > > > > > > > >>>>> it
> > > > > > > > > is enabled.
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>> Otherwise the "vnet_hdr_support" seems
> meaningless.
> > > > > > > > > > > >>>> Yes, Current "vnet_hdr_support"  default
> > > > > > > > > > > >>>> enabled, and vnet_hdr_len
> > > > > > > > > > > >>> already forced from attached nf->netdev.
> > > > > > > > > > > >>>> Maybe we can introduce a new parameter
> > > > > > > "force_no_vnet_hdr"
> > > > > > > > > here
> > > > > > > > > > > to
> > > > > > > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > > > > > > >>> Let me explain, if I was not wrong:
> > > > > > > > > > > >>>
> > > > > > > > > > > >>> "vnet_hdr_support" means whether or not to send
> > > > > > > > > > > >>> vnet header
> > > > > > > > > length.
> > > > > > > > > > > >>> If vnet_hdr_support=false, we won't send the
> > > > > > > > > > > >>> vnet
> > > header.
> > > > > > > > > > > >>> This looks the same as you "force_no_vent_hdr"
> above.
> > > > > > > > > > > >> Yes, It was.  But this series changed it.
> > > > > > > > > > > >> Current "vnet_hdr_support" can't decide whether
> > > > > > > > > > > >> send vnet header length, we always send it even 0.
> > > > > > > > > > > >> It will avoid sender/receiver transfer protocol parse
> issues:
> > > > > > > > > > > >> When sender data with the vnet header length, but
> > > > > > > > > > > >> receiver can't enable the "vnet_hdr_support".
> > > > > > > > > > > >> Filters will auto setup vnet_hdr_len as local
> > > > > > > > > > > >> nf->netdev and found the issue when get different
> > > > > > > > > > > >> vnet_hdr_len from other
> > > > > > > filters.
> > > > > > > > > > > >>
> > > > > > > > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > > > > > > > vnet_hdr_support=true.
> > > > > > > > > > > >>> So it looks to me we can leave the mirror code
> > > > > > > > > > > >>> as is and just change the compare? (depends on
> > > > > > > > > > > >>> the mgmt to set a correct
> > > > > > > > > > > >>> vnet_hdr_support)
> > > > > > > > > > > >> OK, I will keep the
> > > > > > > > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > > > > > > > >> same as this version.
> > > > > > > > > > > >> For the colo-compare module, It will get primary
> > > > > > > > > > > >> node's filter data's vnet_hdr_len as the local
> > > > > > > > > > > >> value, And compare with secondary node's, because
> > > > > > > > > > > >> it is not attached any
> > > > > nf->netdev.
> > > > > > > > > > > >> So, it looks compare module's "vnet_hdr_support"
> > > > > > > > > > > >> been auto configuration from the filter transport
> protocol.
> > > > > > > > > > > >> If the "force_vnet_hdr" means hard code a
> > > > > > > > > > > >> compare's local vnet_hdr_len rather than come from
> input filter's data?
> > > > > > > > > > > >>
> > > > > > > > > > > >> Thanks
> > > > > > > > > > > >> Chen
> > > > > > > > > > > >>
> > > > > > > > > > > > Hi Jason/Markus,
> > > > > > > > > > > >
> > > > > > > > > > > > Rethink about it, How about keep the original
> > > > > "vnet_hdr_support"
> > > > > > > > > > > > function, And add a new optional parameter
> > > "auto_vnet_hdr"
> > > > > > > > > > > > for
> > > > > > > > > > > filters/compare?
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > It's a way but rethink of the whole thing. I wonder
> > > > > > > > > > > what if we just enable "vnet_hdr_support" by default
> > > > > > > > > > > for filter and
> > > > > > > > > > > colo-
> > > > > > > compare?
> > > > > > > > > >
> > > > > > > > > > It's works by default for user use -device
> > > > > > > > > > virtio-net-pci and
> > > e1000...
> > > > > > > > > > But it can't resolve this series motivation, how to
> > > > > > > > > > fix/check user
> > > > > > > > > configuration issue:
> > > > > > > > > > For example user enable " vnet_hdr_support "
> > > > > > > > > > filter-mirror and disable " vnet_hdr_support"
> > > > > > > > > > filter-redirector And connect both filter modules by
> > > > > > > > > chardev socket.
> > > > > > > > > > In this case guest will get wrong network workload and
> > > > > > > > > > filters didn’t perceive any abnormalities, but in
> > > > > > > > > > fact, the whole system is no longer
> > > > > > > > > working.
> > > > > > > > > > This series will report error and try to correct it.
> > > > > > > > >
> > > > > > > > > The problem is how "auto_vnet_hdr" help in this case.
> > > > > > > > > It's a new parameter which may lead to more wrong
> configuration?
> > > > > > > >
> > > > > > > > No, the "auto_vnet_hdr" will fix most the wrong
> > > > > > > > configuration issues as
> > > > > > > "vnet_hdr_support" correct setting.
> > > > > > > > When we enable the "auto_vnet_hdr", the original
> > > > > "vnet_hdr_support"
> > > > > > > will no effect.
> > > > > > >
> > > > > > > So it looks to me it still depends on the management to set
> > > > > "auto_vnet_hdr"
> > > > > > > to be true? (or make it enabled by default)
> > > > > >
> > > > > > Yes, I plan to make "auto_vnet_hdr" enabled by default in next
> version.
> > > > > >
> > > > > > >
> > > > > > > If we can do that, isn't it much more simpler to make
> > > > > > > vnet_hdr_support by default?
> > > > > >
> > > > > > Yes, For compatibility filters and COLO still work with the
> > > > > > original
> > > > > "vnet_hdr_support".
> > > > > > For new users, they can enable the new "auto_vnet_hdr" to
> > > > > > avoid some
> > > > > issues.
> > > > >
> > > > > Question still, so we have
> > > > >
> > > > > 1) enable vnet_hdr_support by default
> > > > > 2) add auto_vnet_hdr and enable it by default
> > > > >
> > > > > Using 1) seems much more simpler and can solve this issue. If we
> > > > > depends on the default behaviour, it should be almost the same.
> > > > > If we want to teach the mgmt, both should work. Any other
> > > > > advantages of
> > > 2)?
> > > >
> > > > Using 1) we can't ensure user configure parts of module by itself.
> > > (vnet_hdr_support = off).
> > > > In this case, filter data already wrong and the filters can't
> > > > report any
> > > transfer error here.
> > >
> > > So I think the point is we can't ensure the user configure parts of
> > > module in any case even if auto_vnet_hdr is introduced. E.g user can
> > > choose to disable it manually. That's why I think it should not
> > > differ too much from making vnet_hdr_support enabled by default.
> >
> > Yes, you are right. The "auto_vnet_hdr" just can fix part of user configure
> issue.
> > And I think this series make the filters better, it make user know
> > filters have some issues when they have wrong manual
> configuration(current code not).
> 
> I think if you stick to the change, I wonder if something like
> "vnet_hdr_support=auto" would be better? (not sure if it's too late to
> change)

It's OK for me. I will update the V6.
By the way, have any update about the queued filter passthrough series?
Need I do something?

Thanks
Chen

> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Using 2) filters will find/report this issue and try to fix it
> > > > from local
> > > vnet_hdr_len.
> > > >
> > > > Thanks
> > > > Chen
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > Chen
> > > > > >
> > > > > > >
> > > > > > > I think I may miss something.
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > Chen
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > > Chen
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks
> > > > > > > > > > > > Chen
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >>> Thanks
> > > > > > > > > > > >>>
> > > > > > > > > > > >>>> Thanks
> > > > > > > > > > > >>>> Chen
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>>> Thanks
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>>
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >

Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Jason Wang]

On Tue, Nov 9, 2021 at 3:31 PM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, November 9, 2021 3:26 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> > mirror/redirector
> >
> > On Tue, Nov 9, 2021 at 3:20 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Tuesday, November 9, 2021 2:42 PM
> > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > > for filter- mirror/redirector
> > > >
> > > > On Mon, Nov 8, 2021 at 10:50 AM Zhang, Chen <chen.zhang@intel.com>
> > > > wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Monday, November 8, 2021 10:42 AM
> > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > protocol for filter- mirror/redirector
> > > > > >
> > > > > > On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen
> > > > > > <chen.zhang@intel.com>
> > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > Sent: Friday, November 5, 2021 4:30 PM
> > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> > <qemu-
> > > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > > > protocol for filter- mirror/redirector
> > > > > > > >
> > > > > > > > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen
> > > > > > > > <chen.zhang@intel.com>
> > > > > > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> > > > <qemu-
> > > > > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > > >
> > > > > > > > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen
> > > > > > > > > > <chen.zhang@intel.com>
> > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus
> > > > > > > > > > > > Armbruster <armbru@redhat.com>
> > > > > > > > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > > > > > > > >>>>>
> > > > > > > > > > > > >>>>> I wonder if we need to introduce new
> > > > > > > > > > > > >>>>> parameter, e.g force_vnet_hdr here, then we
> > > > > > > > > > > > >>>>> can always send vnet_hdr
> > > > > > > > when
> > > > > > > > > > > > >>>>> it
> > > > > > > > > > is enabled.
> > > > > > > > > > > > >>>>>
> > > > > > > > > > > > >>>>> Otherwise the "vnet_hdr_support" seems
> > meaningless.
> > > > > > > > > > > > >>>> Yes, Current "vnet_hdr_support"  default
> > > > > > > > > > > > >>>> enabled, and vnet_hdr_len
> > > > > > > > > > > > >>> already forced from attached nf->netdev.
> > > > > > > > > > > > >>>> Maybe we can introduce a new parameter
> > > > > > > > "force_no_vnet_hdr"
> > > > > > > > > > here
> > > > > > > > > > > > to
> > > > > > > > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > > > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > > > > > > > >>> Let me explain, if I was not wrong:
> > > > > > > > > > > > >>>
> > > > > > > > > > > > >>> "vnet_hdr_support" means whether or not to send
> > > > > > > > > > > > >>> vnet header
> > > > > > > > > > length.
> > > > > > > > > > > > >>> If vnet_hdr_support=false, we won't send the
> > > > > > > > > > > > >>> vnet
> > > > header.
> > > > > > > > > > > > >>> This looks the same as you "force_no_vent_hdr"
> > above.
> > > > > > > > > > > > >> Yes, It was.  But this series changed it.
> > > > > > > > > > > > >> Current "vnet_hdr_support" can't decide whether
> > > > > > > > > > > > >> send vnet header length, we always send it even 0.
> > > > > > > > > > > > >> It will avoid sender/receiver transfer protocol parse
> > issues:
> > > > > > > > > > > > >> When sender data with the vnet header length, but
> > > > > > > > > > > > >> receiver can't enable the "vnet_hdr_support".
> > > > > > > > > > > > >> Filters will auto setup vnet_hdr_len as local
> > > > > > > > > > > > >> nf->netdev and found the issue when get different
> > > > > > > > > > > > >> vnet_hdr_len from other
> > > > > > > > filters.
> > > > > > > > > > > > >>
> > > > > > > > > > > > >>> And my "force_vnet_hdr" seems duplicated with
> > > > > > > > > > > > vnet_hdr_support=true.
> > > > > > > > > > > > >>> So it looks to me we can leave the mirror code
> > > > > > > > > > > > >>> as is and just change the compare? (depends on
> > > > > > > > > > > > >>> the mgmt to set a correct
> > > > > > > > > > > > >>> vnet_hdr_support)
> > > > > > > > > > > > >> OK, I will keep the
> > > > > > > > > > > > >> filter-mirror/filter-redirector/filter-rewriter
> > > > > > > > > > > > >> same as this version.
> > > > > > > > > > > > >> For the colo-compare module, It will get primary
> > > > > > > > > > > > >> node's filter data's vnet_hdr_len as the local
> > > > > > > > > > > > >> value, And compare with secondary node's, because
> > > > > > > > > > > > >> it is not attached any
> > > > > > nf->netdev.
> > > > > > > > > > > > >> So, it looks compare module's "vnet_hdr_support"
> > > > > > > > > > > > >> been auto configuration from the filter transport
> > protocol.
> > > > > > > > > > > > >> If the "force_vnet_hdr" means hard code a
> > > > > > > > > > > > >> compare's local vnet_hdr_len rather than come from
> > input filter's data?
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> Thanks
> > > > > > > > > > > > >> Chen
> > > > > > > > > > > > >>
> > > > > > > > > > > > > Hi Jason/Markus,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Rethink about it, How about keep the original
> > > > > > "vnet_hdr_support"
> > > > > > > > > > > > > function, And add a new optional parameter
> > > > "auto_vnet_hdr"
> > > > > > > > > > > > > for
> > > > > > > > > > > > filters/compare?
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > It's a way but rethink of the whole thing. I wonder
> > > > > > > > > > > > what if we just enable "vnet_hdr_support" by default
> > > > > > > > > > > > for filter and
> > > > > > > > > > > > colo-
> > > > > > > > compare?
> > > > > > > > > > >
> > > > > > > > > > > It's works by default for user use -device
> > > > > > > > > > > virtio-net-pci and
> > > > e1000...
> > > > > > > > > > > But it can't resolve this series motivation, how to
> > > > > > > > > > > fix/check user
> > > > > > > > > > configuration issue:
> > > > > > > > > > > For example user enable " vnet_hdr_support "
> > > > > > > > > > > filter-mirror and disable " vnet_hdr_support"
> > > > > > > > > > > filter-redirector And connect both filter modules by
> > > > > > > > > > chardev socket.
> > > > > > > > > > > In this case guest will get wrong network workload and
> > > > > > > > > > > filters didn’t perceive any abnormalities, but in
> > > > > > > > > > > fact, the whole system is no longer
> > > > > > > > > > working.
> > > > > > > > > > > This series will report error and try to correct it.
> > > > > > > > > >
> > > > > > > > > > The problem is how "auto_vnet_hdr" help in this case.
> > > > > > > > > > It's a new parameter which may lead to more wrong
> > configuration?
> > > > > > > > >
> > > > > > > > > No, the "auto_vnet_hdr" will fix most the wrong
> > > > > > > > > configuration issues as
> > > > > > > > "vnet_hdr_support" correct setting.
> > > > > > > > > When we enable the "auto_vnet_hdr", the original
> > > > > > "vnet_hdr_support"
> > > > > > > > will no effect.
> > > > > > > >
> > > > > > > > So it looks to me it still depends on the management to set
> > > > > > "auto_vnet_hdr"
> > > > > > > > to be true? (or make it enabled by default)
> > > > > > >
> > > > > > > Yes, I plan to make "auto_vnet_hdr" enabled by default in next
> > version.
> > > > > > >
> > > > > > > >
> > > > > > > > If we can do that, isn't it much more simpler to make
> > > > > > > > vnet_hdr_support by default?
> > > > > > >
> > > > > > > Yes, For compatibility filters and COLO still work with the
> > > > > > > original
> > > > > > "vnet_hdr_support".
> > > > > > > For new users, they can enable the new "auto_vnet_hdr" to
> > > > > > > avoid some
> > > > > > issues.
> > > > > >
> > > > > > Question still, so we have
> > > > > >
> > > > > > 1) enable vnet_hdr_support by default
> > > > > > 2) add auto_vnet_hdr and enable it by default
> > > > > >
> > > > > > Using 1) seems much more simpler and can solve this issue. If we
> > > > > > depends on the default behaviour, it should be almost the same.
> > > > > > If we want to teach the mgmt, both should work. Any other
> > > > > > advantages of
> > > > 2)?
> > > > >
> > > > > Using 1) we can't ensure user configure parts of module by itself.
> > > > (vnet_hdr_support = off).
> > > > > In this case, filter data already wrong and the filters can't
> > > > > report any
> > > > transfer error here.
> > > >
> > > > So I think the point is we can't ensure the user configure parts of
> > > > module in any case even if auto_vnet_hdr is introduced. E.g user can
> > > > choose to disable it manually. That's why I think it should not
> > > > differ too much from making vnet_hdr_support enabled by default.
> > >
> > > Yes, you are right. The "auto_vnet_hdr" just can fix part of user configure
> > issue.
> > > And I think this series make the filters better, it make user know
> > > filters have some issues when they have wrong manual
> > configuration(current code not).
> >
> > I think if you stick to the change, I wonder if something like
> > "vnet_hdr_support=auto" would be better? (not sure if it's too late to
> > change)
>
> It's OK for me. I will update the V6.
> By the way, have any update about the queued filter passthrough series?
> Need I do something?

If I'm not wrong, Markus has some concern so I drop it from the queue.

Thanks

>
> Thanks
> Chen
>
> >
> > Thanks
> >
> > >
> > > Thanks
> > > Chen
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Using 2) filters will find/report this issue and try to fix it
> > > > > from local
> > > > vnet_hdr_len.
> > > > >
> > > > > Thanks
> > > > > Chen
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > > Chen
> > > > > > >
> > > > > > > >
> > > > > > > > I think I may miss something.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > > Chen
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > > Chen
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks
> > > > > > > > > > > > > Chen
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >>> Thanks
> > > > > > > > > > > > >>>
> > > > > > > > > > > > >>>> Thanks
> > > > > > > > > > > > >>>> Chen
> > > > > > > > > > > > >>>>
> > > > > > > > > > > > >>>>> Thanks
> > > > > > > > > > > > >>>>>
> > > > > > > > > > > > >>>>>
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, November 9, 2021 3:42 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-
> mirror/redirector
> 
> On Tue, Nov 9, 2021 at 3:31 PM Zhang, Chen <chen.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, November 9, 2021 3:26 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer protocol
> > > for filter- mirror/redirector
> > >
> > > On Tue, Nov 9, 2021 at 3:20 PM Zhang, Chen <chen.zhang@intel.com>
> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Tuesday, November 9, 2021 2:42 PM
> > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-
> > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > protocol for filter- mirror/redirector
> > > > >
> > > > > On Mon, Nov 8, 2021 at 10:50 AM Zhang, Chen
> > > > > <chen.zhang@intel.com>
> > > > > wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Monday, November 8, 2021 10:42 AM
> > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> <qemu-
> > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize transfer
> > > > > > > protocol for filter- mirror/redirector
> > > > > > >
> > > > > > > On Fri, Nov 5, 2021 at 4:43 PM Zhang, Chen
> > > > > > > <chen.zhang@intel.com>
> > > > > wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > Sent: Friday, November 5, 2021 4:30 PM
> > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> > > <qemu-
> > > > > > > > > devel@nongnu.org>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > >
> > > > > > > > > On Fri, Nov 5, 2021 at 1:29 PM Zhang, Chen
> > > > > > > > > <chen.zhang@intel.com>
> > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > > Sent: Friday, November 5, 2021 12:03 PM
> > > > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > > > > > > > Cc: Markus Armbruster <armbru@redhat.com>; qemu-dev
> > > > > <qemu-
> > > > > > > > > > > devel@nongnu.org>; Li Zhijian
> > > > > > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Nov 5, 2021 at 11:27 AM Zhang, Chen
> > > > > > > > > > > <chen.zhang@intel.com>
> > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > > > > Sent: Friday, November 5, 2021 11:17 AM
> > > > > > > > > > > > > To: Zhang, Chen <chen.zhang@intel.com>; Markus
> > > > > > > > > > > > > Armbruster <armbru@redhat.com>
> > > > > > > > > > > > > Cc: qemu-dev <qemu-devel@nongnu.org>; Li Zhijian
> > > > > > > > > > > > > <lizhijian@cn.fujitsu.com>
> > > > > > > > > > > > > Subject: Re: [PATCH V5 1/3] net/filter: Optimize
> > > > > > > > > > > > > transfer protocol for filter- mirror/redirector
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > 在 2021/11/4 下午1:37, Zhang, Chen 写道:
> > > > > > > > > > > > > >>>>>
> > > > > > > > > > > > > >>>>> I wonder if we need to introduce new
> > > > > > > > > > > > > >>>>> parameter, e.g force_vnet_hdr here, then
> > > > > > > > > > > > > >>>>> we can always send vnet_hdr
> > > > > > > > > when
> > > > > > > > > > > > > >>>>> it
> > > > > > > > > > > is enabled.
> > > > > > > > > > > > > >>>>>
> > > > > > > > > > > > > >>>>> Otherwise the "vnet_hdr_support" seems
> > > meaningless.
> > > > > > > > > > > > > >>>> Yes, Current "vnet_hdr_support"  default
> > > > > > > > > > > > > >>>> enabled, and vnet_hdr_len
> > > > > > > > > > > > > >>> already forced from attached nf->netdev.
> > > > > > > > > > > > > >>>> Maybe we can introduce a new parameter
> > > > > > > > > "force_no_vnet_hdr"
> > > > > > > > > > > here
> > > > > > > > > > > > > to
> > > > > > > > > > > > > >>> make the vnet_hdr_len always keep 0.
> > > > > > > > > > > > > >>>> If you think OK, I will update it in next version.
> > > > > > > > > > > > > >>> Let me explain, if I was not wrong:
> > > > > > > > > > > > > >>>
> > > > > > > > > > > > > >>> "vnet_hdr_support" means whether or not to
> > > > > > > > > > > > > >>> send vnet header
> > > > > > > > > > > length.
> > > > > > > > > > > > > >>> If vnet_hdr_support=false, we won't send the
> > > > > > > > > > > > > >>> vnet
> > > > > header.
> > > > > > > > > > > > > >>> This looks the same as you "force_no_vent_hdr"
> > > above.
> > > > > > > > > > > > > >> Yes, It was.  But this series changed it.
> > > > > > > > > > > > > >> Current "vnet_hdr_support" can't decide
> > > > > > > > > > > > > >> whether send vnet header length, we always send
> it even 0.
> > > > > > > > > > > > > >> It will avoid sender/receiver transfer
> > > > > > > > > > > > > >> protocol parse
> > > issues:
> > > > > > > > > > > > > >> When sender data with the vnet header length,
> > > > > > > > > > > > > >> but receiver can't enable the "vnet_hdr_support".
> > > > > > > > > > > > > >> Filters will auto setup vnet_hdr_len as local
> > > > > > > > > > > > > >> nf->netdev and found the issue when get
> > > > > > > > > > > > > >> nf->different
> > > > > > > > > > > > > >> vnet_hdr_len from other
> > > > > > > > > filters.
> > > > > > > > > > > > > >>
> > > > > > > > > > > > > >>> And my "force_vnet_hdr" seems duplicated
> > > > > > > > > > > > > >>> with
> > > > > > > > > > > > > vnet_hdr_support=true.
> > > > > > > > > > > > > >>> So it looks to me we can leave the mirror
> > > > > > > > > > > > > >>> code as is and just change the compare?
> > > > > > > > > > > > > >>> (depends on the mgmt to set a correct
> > > > > > > > > > > > > >>> vnet_hdr_support)
> > > > > > > > > > > > > >> OK, I will keep the
> > > > > > > > > > > > > >> filter-mirror/filter-redirector/filter-rewrit
> > > > > > > > > > > > > >> er same as this version.
> > > > > > > > > > > > > >> For the colo-compare module, It will get
> > > > > > > > > > > > > >> primary node's filter data's vnet_hdr_len as
> > > > > > > > > > > > > >> the local value, And compare with secondary
> > > > > > > > > > > > > >> node's, because it is not attached any
> > > > > > > nf->netdev.
> > > > > > > > > > > > > >> So, it looks compare module's "vnet_hdr_support"
> > > > > > > > > > > > > >> been auto configuration from the filter
> > > > > > > > > > > > > >> transport
> > > protocol.
> > > > > > > > > > > > > >> If the "force_vnet_hdr" means hard code a
> > > > > > > > > > > > > >> compare's local vnet_hdr_len rather than come
> > > > > > > > > > > > > >> from
> > > input filter's data?
> > > > > > > > > > > > > >>
> > > > > > > > > > > > > >> Thanks
> > > > > > > > > > > > > >> Chen
> > > > > > > > > > > > > >>
> > > > > > > > > > > > > > Hi Jason/Markus,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Rethink about it, How about keep the original
> > > > > > > "vnet_hdr_support"
> > > > > > > > > > > > > > function, And add a new optional parameter
> > > > > "auto_vnet_hdr"
> > > > > > > > > > > > > > for
> > > > > > > > > > > > > filters/compare?
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > It's a way but rethink of the whole thing. I
> > > > > > > > > > > > > wonder what if we just enable "vnet_hdr_support"
> > > > > > > > > > > > > by default for filter and
> > > > > > > > > > > > > colo-
> > > > > > > > > compare?
> > > > > > > > > > > >
> > > > > > > > > > > > It's works by default for user use -device
> > > > > > > > > > > > virtio-net-pci and
> > > > > e1000...
> > > > > > > > > > > > But it can't resolve this series motivation, how
> > > > > > > > > > > > to fix/check user
> > > > > > > > > > > configuration issue:
> > > > > > > > > > > > For example user enable " vnet_hdr_support "
> > > > > > > > > > > > filter-mirror and disable " vnet_hdr_support"
> > > > > > > > > > > > filter-redirector And connect both filter modules
> > > > > > > > > > > > by
> > > > > > > > > > > chardev socket.
> > > > > > > > > > > > In this case guest will get wrong network workload
> > > > > > > > > > > > and filters didn’t perceive any abnormalities, but
> > > > > > > > > > > > in fact, the whole system is no longer
> > > > > > > > > > > working.
> > > > > > > > > > > > This series will report error and try to correct it.
> > > > > > > > > > >
> > > > > > > > > > > The problem is how "auto_vnet_hdr" help in this case.
> > > > > > > > > > > It's a new parameter which may lead to more wrong
> > > configuration?
> > > > > > > > > >
> > > > > > > > > > No, the "auto_vnet_hdr" will fix most the wrong
> > > > > > > > > > configuration issues as
> > > > > > > > > "vnet_hdr_support" correct setting.
> > > > > > > > > > When we enable the "auto_vnet_hdr", the original
> > > > > > > "vnet_hdr_support"
> > > > > > > > > will no effect.
> > > > > > > > >
> > > > > > > > > So it looks to me it still depends on the management to
> > > > > > > > > set
> > > > > > > "auto_vnet_hdr"
> > > > > > > > > to be true? (or make it enabled by default)
> > > > > > > >
> > > > > > > > Yes, I plan to make "auto_vnet_hdr" enabled by default in
> > > > > > > > next
> > > version.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > If we can do that, isn't it much more simpler to make
> > > > > > > > > vnet_hdr_support by default?
> > > > > > > >
> > > > > > > > Yes, For compatibility filters and COLO still work with
> > > > > > > > the original
> > > > > > > "vnet_hdr_support".
> > > > > > > > For new users, they can enable the new "auto_vnet_hdr" to
> > > > > > > > avoid some
> > > > > > > issues.
> > > > > > >
> > > > > > > Question still, so we have
> > > > > > >
> > > > > > > 1) enable vnet_hdr_support by default
> > > > > > > 2) add auto_vnet_hdr and enable it by default
> > > > > > >
> > > > > > > Using 1) seems much more simpler and can solve this issue.
> > > > > > > If we depends on the default behaviour, it should be almost the
> same.
> > > > > > > If we want to teach the mgmt, both should work. Any other
> > > > > > > advantages of
> > > > > 2)?
> > > > > >
> > > > > > Using 1) we can't ensure user configure parts of module by itself.
> > > > > (vnet_hdr_support = off).
> > > > > > In this case, filter data already wrong and the filters can't
> > > > > > report any
> > > > > transfer error here.
> > > > >
> > > > > So I think the point is we can't ensure the user configure parts
> > > > > of module in any case even if auto_vnet_hdr is introduced. E.g
> > > > > user can choose to disable it manually. That's why I think it
> > > > > should not differ too much from making vnet_hdr_support enabled
> by default.
> > > >
> > > > Yes, you are right. The "auto_vnet_hdr" just can fix part of user
> > > > configure
> > > issue.
> > > > And I think this series make the filters better, it make user know
> > > > filters have some issues when they have wrong manual
> > > configuration(current code not).
> > >
> > > I think if you stick to the change, I wonder if something like
> > > "vnet_hdr_support=auto" would be better? (not sure if it's too late
> > > to
> > > change)
> >
> > It's OK for me. I will update the V6.
> > By the way, have any update about the queued filter passthrough series?
> > Need I do something?
> 
> If I'm not wrong, Markus has some concern so I drop it from the queue.

I remember I asked Markus should I update/fix the queued version, but no reply.
Hi Markus, any comments?

Thanks
Chen 

> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > > Chen
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Using 2) filters will find/report this issue and try to fix it
> > > > > > from local
> > > > > vnet_hdr_len.
> > > > > >
> > > > > > Thanks
> > > > > > Chen
> > > > > >
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > > Chen
> > > > > > > >
> > > > > > > > >
> > > > > > > > > I think I may miss something.
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > > Chen
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Thanks
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks
> > > > > > > > > > > > Chen
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks
> > > > > > > > > > > > > > Chen
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >>> Thanks
> > > > > > > > > > > > > >>>
> > > > > > > > > > > > > >>>> Thanks
> > > > > > > > > > > > > >>>> Chen
> > > > > > > > > > > > > >>>>
> > > > > > > > > > > > > >>>>> Thanks
> > > > > > > > > > > > > >>>>>
> > > > > > > > > > > > > >>>>>
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >

RE: [PATCH V5 1/3] net/filter: Optimize transfer protocol for filter-mirror/redirector

[Zhang, Chen]

> > > > > >
> > > > > > If we can do that, isn't it much more simpler to make
> > > > > > vnet_hdr_support by default?
> > > > >
> > > > > Yes, For compatibility filters and COLO still work with the
> > > > > original
> > > > "vnet_hdr_support".
> > > > > For new users, they can enable the new "auto_vnet_hdr" to avoid
> > > > > some
> > > > issues.
> > > >
> > > > Question still, so we have
> > > >
> > > > 1) enable vnet_hdr_support by default
> > > > 2) add auto_vnet_hdr and enable it by default
> > > >
> > > > Using 1) seems much more simpler and can solve this issue. If we
> > > > depends on the default behaviour, it should be almost the same. If
> > > > we want to teach the mgmt, both should work. Any other advantages
> > > > of
> > 2)?
> > >
> > > Using 1) we can't ensure user configure parts of module by itself.
> > (vnet_hdr_support = off).
> > > In this case, filter data already wrong and the filters can't report
> > > any
> > transfer error here.
> >
> > So I think the point is we can't ensure the user configure parts of
> > module in any case even if auto_vnet_hdr is introduced. E.g user can
> > choose to disable it manually. That's why I think it should not differ
> > too much from making vnet_hdr_support enabled by default.
> 

Rethink about it. Using 1) make things much more simpler except the user config it manually.
I will follow your comments make V6 to 1).

Thanks
Chen

>