[V3] net/colo: check vnet_hdr_support flag when using virtio-net

Commit Message

Zhang Chen Sept. 18, 2021, 2:04 a.m. UTC

When COLO use only one vnet_hdr_support parameter between
COLO network filter(filter-mirror, filter-redirector or
filter-rewriter and colo-compare, packet will not be parsed
correctly. Acquire network driver related to COLO, if it is
nirtio-net, check vnet_hdr_support flag of COLO network filter
and colo-compare.

Signed-off-by: Tao Xu <[e-mail]>
Signed-off-by: Zhang Chen <[e-mail]>
---

Changelog:
v3:
    Fix some typos.
    Rebased for Qemu 6.2.

v2:
    Detect virtio-net driver and apply vnet_hdr_support
    automatically. (Jason)
---
 net/colo-compare.c    | 57 +++++++++++++++++++++++++++++++++++++++++++
 net/colo.c            | 20 +++++++++++++++
 net/colo.h            |  4 +++
 net/filter-mirror.c   | 21 ++++++++++++++++
 net/filter-rewriter.c | 10 ++++++++
 qapi/qom.json         |  6 +++++
 qemu-options.hx       |  6 +++--
 7 files changed, 122 insertions(+), 2 deletions(-)

Comments

Markus Armbruster Oct. 12, 2021, 9:31 a.m. UTC | #1

Jason, did this fall through the cracks?

Zhang Chen <[e-mail]> writes:

> When COLO use only one vnet_hdr_support parameter between
> COLO network filter(filter-mirror, filter-redirector or
> filter-rewriter and colo-compare, packet will not be parsed
> correctly. Acquire network driver related to COLO, if it is
> nirtio-net, check vnet_hdr_support flag of COLO network filter
> and colo-compare.
>
> Signed-off-by: Tao Xu <[e-mail]>
> Signed-off-by: Zhang Chen <[e-mail]>
> ---
>
> Changelog:
> v3:
>     Fix some typos.
>     Rebased for Qemu 6.2.
>
> v2:
>     Detect virtio-net driver and apply vnet_hdr_support
>     automatically. (Jason)
> ---
>  net/colo-compare.c    | 57 +++++++++++++++++++++++++++++++++++++++++++
>  net/colo.c            | 20 +++++++++++++++
>  net/colo.h            |  4 +++
>  net/filter-mirror.c   | 21 ++++++++++++++++
>  net/filter-rewriter.c | 10 ++++++++
>  qapi/qom.json         |  6 +++++
>  qemu-options.hx       |  6 +++--
>  7 files changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index b100e7b51f..870bd05a41 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -110,6 +110,7 @@ struct CompareState {
>      char *sec_indev;
>      char *outdev;
>      char *notify_dev;
> +    char *netdev;
>      CharBackend chr_pri_in;
>      CharBackend chr_sec_in;
>      CharBackend chr_out;
> @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
>      return COMPARE_READ_LEN_MAX;
>  }
>  
> +static int colo_set_default_netdev(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    const char *colo_obj_type, *netdev_from_filter;
> +    char **netdev = (char **)opaque;
> +
> +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> +
> +    if (colo_obj_type &&
> +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> +        if (*netdev == NULL) {
> +            *netdev = g_strdup(netdev_from_filter);
> +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> +            warn_report("%s is using a different netdev from other COLO "
> +                        "component", colo_obj_type);
> +        }
> +    }
> +    return 0;
> +}
> +
>  /*
>   * Called from the main thread on the primary for packets
>   * arriving over the socket from the primary.
> @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object *obj,
>      s->vnet_hdr = value;
>  }
>  
> +static char *compare_get_netdev(Object *obj, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    return g_strdup(s->netdev);
> +}
> +
> +static void compare_set_netdev(Object *obj, const char *value, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    g_free(s->netdev);
> +    s->netdev = g_strdup(value);
> +}
> +
>  static char *compare_get_notify_dev(Object *obj, Error **errp)
>  {
>      CompareState *s = COLO_COMPARE(obj);
> @@ -1274,6 +1312,12 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>          max_queue_size = MAX_QUEUE_SIZE;
>      }
>  
> +    if (!s->netdev) {
> +        /* Set default netdev as the first colo netfilter found */
> +        qemu_opts_foreach(qemu_find_opts("object"),
> +                          colo_set_default_netdev, &s->netdev, NULL);
> +    }
> +
>      if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
>          !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
>          return;
> @@ -1289,6 +1333,16 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>          return;
>      }
>  
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, s->netdev, NULL)) {
> +        /*
> +         * colo compare needs 'vnet_hdr_support' when it works on virtio-net,
> +         * add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>      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);
>  
> @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
>      s->vnet_hdr = false;
>      object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
>                               compare_set_vnet_hdr);
> +    /* colo compare can't varify that netdev is correct */
> +    object_property_add_str(obj, "netdev", compare_get_netdev,
> +                            compare_set_netdev);
>  }
>  
>  void colo_compare_cleanup(void)
> diff --git a/net/colo.c b/net/colo.c
> index 3a3e6e89a0..4a03780f45 100644
> --- a/net/colo.c
> +++ b/net/colo.c
> @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable *connection_track_table,
>  
>      return conn ? true : false;
>  }
> +
> +/* check the network driver related to COLO, return 1 if it is virtio-net */
> +int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    const char *driver_type, *netdev_from_driver;
> +    char *netdev_from_filter = (char *)opaque;
> +
> +    driver_type = qemu_opt_get(opts, "driver");
> +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> +
> +    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
> +        return 0;
> +    }
> +
> +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> +        return 1;
> +    }
> +    return 0;
> +}
> diff --git a/net/colo.h b/net/colo.h
> index d91cd245c4..d401fc76b6 100644
> --- a/net/colo.h
> +++ b/net/colo.h
> @@ -18,6 +18,9 @@
>  #include "qemu/jhash.h"
>  #include "qemu/timer.h"
>  #include "net/eth.h"
> +#include "qemu/option.h"
> +#include "qemu/option_int.h"
> +#include "qemu/config-file.h"
>  
>  #define HASHTABLE_MAX_SIZE 16384
>  
> @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int size, int vnet_hdr_len);
>  Packet *packet_new_nocopy(void *data, int size, int vnet_hdr_len);
>  void packet_destroy(void *opaque, void *user_data);
>  void packet_destroy_partial(void *opaque, void *user_data);
> +int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp);
>  
>  #endif /* NET_COLO_H */
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index f20240cc9f..69ca9c9839 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -12,6 +12,7 @@
>  #include "qemu/osdep.h"
>  #include "net/filter.h"
>  #include "net/net.h"
> +#include "net/colo.h"
>  #include "qapi/error.h"
>  #include "qom/object.h"
>  #include "qemu/main-loop.h"
> @@ -224,6 +225,16 @@ static void filter_mirror_setup(NetFilterState *nf, Error **errp)
>          return;
>      }
>  
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, nf->netdev_id, NULL)) {
> +        /*
> +         * filter mirror needs 'vnet_hdr_support' when colo filter modules
> +         * work on virtio-net, add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>      qemu_chr_fe_init(&s->chr_out, chr, errp);
>  }
>  
> @@ -252,6 +263,16 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
>          }
>      }
>  
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, nf->netdev_id, NULL)) {
> +        /*
> +         * filter redirector needs 'vnet_hdr_support' when colo filter modules
> +         * work on virtio-net, add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>      net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
>  
>      if (s->indev) {
> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> index cb3a96cde1..637ef4ce71 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -388,6 +388,16 @@ static void colo_rewriter_setup(NetFilterState *nf, Error **errp)
>  {
>      RewriterState *s = FILTER_REWRITER(nf);
>  
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, nf->netdev_id, NULL)) {
> +        /*
> +         * filter rewriter needs 'vnet_hdr_support' when colo filter modules
> +         * work on virtio-net, add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>      s->connection_track_table = g_hash_table_new_full(connection_key_hash,
>                                                        connection_key_equal,
>                                                        g_free,
> diff --git a/qapi/qom.json b/qapi/qom.json
> index a25616bc7a..5760107160 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -241,6 +241,11 @@
>  # @notify_dev: name of the character device backend to be used to communicate
>  #              with the remote colo-frame (only for Xen COLO)
>  #
> +# @netdev: id of the network device backend to colo-compare. Although
> +#          colo-compare doesn't depend on network device directly, this
> +#          parameter helps colo-compare know what network driver it is
> +#          working on.(since 6.2)
> +#
>  # @compare_timeout: the maximum time to hold a packet from @primary_in for
>  #                   comparison with an incoming packet on @secondary_in in
>  #                   milliseconds (default: 3000)
> @@ -264,6 +269,7 @@
>              'outdev': 'str',
>              'iothread': 'str',
>              '*notify_dev': 'str',
> +            '*netdev': 'str',
>              '*compare_timeout': 'uint64',
>              '*expired_scan_cycle': 'uint32',
>              '*max_queue_size': 'uint32',
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8f603cc7e6..250937fbbf 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4964,12 +4964,14 @@ SRST
>          stored. The file format is libpcap, so it can be analyzed with
>          tools such as tcpdump or Wireshark.
>  
> -    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> +    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
>          Colo-compare gets packet from primary\_in chardevid and
>          secondary\_in, then compare whether the payload of primary packet
>          and secondary packet are the same. If same, it will output
>          primary packet to out\_dev, else it will notify COLO-framework to do
> -        checkpoint and send primary packet to out\_dev. In order to
> +        checkpoint and send primary packet to out\_dev. Although colo-compare
> +        does not depend on network device directly, netdevid helps
> +        colo-compare know what network driver it is working on. In order to
>          improve efficiency, we need to put the task of comparison in
>          another iothread. If it has the vnet\_hdr\_support flag,
>          colo compare will send/recv packet with vnet\_hdr\_len.

Zhang Chen Oct. 18, 2021, 9:03 a.m. UTC | #2

Hi Jason~

Have any comments or updates for this patch?

Thanks
Chen

> -----Original Message-----
> From: Markus Armbruster <[e-mail]>
> Sent: Tuesday, October 12, 2021 5:31 PM
> To: Zhang, Chen <[e-mail]>
> Cc: Jason Wang <[e-mail]>; Eric Blake <[e-mail]>;
> qemu-dev <[e-mail]>; Li Zhijian <[e-mail]>;
> Lukas Straub <[e-mail]>; Tao Xu <[e-mail]>
> Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when using
> virtio-net
> 
> Jason, did this fall through the cracks?
> 
> Zhang Chen <[e-mail]> writes:
> 
> > When COLO use only one vnet_hdr_support parameter between COLO
> network
> > filter(filter-mirror, filter-redirector or filter-rewriter and
> > colo-compare, packet will not be parsed correctly. Acquire network
> > driver related to COLO, if it is nirtio-net, check vnet_hdr_support
> > flag of COLO network filter and colo-compare.
> >
> > Signed-off-by: Tao Xu <[e-mail]>
> > Signed-off-by: Zhang Chen <[e-mail]>
> > ---
> >
> > Changelog:
> > v3:
> >     Fix some typos.
> >     Rebased for Qemu 6.2.
> >
> > v2:
> >     Detect virtio-net driver and apply vnet_hdr_support
> >     automatically. (Jason)
> > ---
> >  net/colo-compare.c    | 57
> +++++++++++++++++++++++++++++++++++++++++++
> >  net/colo.c            | 20 +++++++++++++++
> >  net/colo.h            |  4 +++
> >  net/filter-mirror.c   | 21 ++++++++++++++++
> >  net/filter-rewriter.c | 10 ++++++++
> >  qapi/qom.json         |  6 +++++
> >  qemu-options.hx       |  6 +++--
> >  7 files changed, 122 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > b100e7b51f..870bd05a41 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -110,6 +110,7 @@ struct CompareState {
> >      char *sec_indev;
> >      char *outdev;
> >      char *notify_dev;
> > +    char *netdev;
> >      CharBackend chr_pri_in;
> >      CharBackend chr_sec_in;
> >      CharBackend chr_out;
> > @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
> >      return COMPARE_READ_LEN_MAX;
> >  }
> >
> > +static int colo_set_default_netdev(void *opaque, QemuOpts *opts,
> > +Error **errp) {
> > +    const char *colo_obj_type, *netdev_from_filter;
> > +    char **netdev = (char **)opaque;
> > +
> > +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> > +
> > +    if (colo_obj_type &&
> > +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> > +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> > +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> > +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> > +        if (*netdev == NULL) {
> > +            *netdev = g_strdup(netdev_from_filter);
> > +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> > +            warn_report("%s is using a different netdev from other COLO "
> > +                        "component", colo_obj_type);
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Called from the main thread on the primary for packets
> >   * arriving over the socket from the primary.
> > @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object *obj,
> >      s->vnet_hdr = value;
> >  }
> >
> > +static char *compare_get_netdev(Object *obj, Error **errp) {
> > +    CompareState *s = COLO_COMPARE(obj);
> > +
> > +    return g_strdup(s->netdev);
> > +}
> > +
> > +static void compare_set_netdev(Object *obj, const char *value, Error
> > +**errp) {
> > +    CompareState *s = COLO_COMPARE(obj);
> > +
> > +    g_free(s->netdev);
> > +    s->netdev = g_strdup(value);
> > +}
> > +
> >  static char *compare_get_notify_dev(Object *obj, Error **errp)  {
> >      CompareState *s = COLO_COMPARE(obj); @@ -1274,6 +1312,12 @@
> > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> >          max_queue_size = MAX_QUEUE_SIZE;
> >      }
> >
> > +    if (!s->netdev) {
> > +        /* Set default netdev as the first colo netfilter found */
> > +        qemu_opts_foreach(qemu_find_opts("object"),
> > +                          colo_set_default_netdev, &s->netdev, NULL);
> > +    }
> > +
> >      if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
> >          !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
> >          return;
> > @@ -1289,6 +1333,16 @@ static void
> colo_compare_complete(UserCreatable *uc, Error **errp)
> >          return;
> >      }
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, s->netdev, NULL)) {
> > +        /*
> > +         * colo compare needs 'vnet_hdr_support' when it works on virtio-net,
> > +         * add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >      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);
> >
> > @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
> >      s->vnet_hdr = false;
> >      object_property_add_bool(obj, "vnet_hdr_support",
> compare_get_vnet_hdr,
> >                               compare_set_vnet_hdr);
> > +    /* colo compare can't varify that netdev is correct */
> > +    object_property_add_str(obj, "netdev", compare_get_netdev,
> > +                            compare_set_netdev);
> >  }
> >
> >  void colo_compare_cleanup(void)
> > diff --git a/net/colo.c b/net/colo.c
> > index 3a3e6e89a0..4a03780f45 100644
> > --- a/net/colo.c
> > +++ b/net/colo.c
> > @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable
> > *connection_track_table,
> >
> >      return conn ? true : false;
> >  }
> > +
> > +/* check the network driver related to COLO, return 1 if it is
> > +virtio-net */ int vnet_driver_check(void *opaque, QemuOpts *opts,
> > +Error **errp) {
> > +    const char *driver_type, *netdev_from_driver;
> > +    char *netdev_from_filter = (char *)opaque;
> > +
> > +    driver_type = qemu_opt_get(opts, "driver");
> > +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> > +
> > +    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
> > +        return 0;
> > +    }
> > +
> > +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> > +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > diff --git a/net/colo.h b/net/colo.h
> > index d91cd245c4..d401fc76b6 100644
> > --- a/net/colo.h
> > +++ b/net/colo.h
> > @@ -18,6 +18,9 @@
> >  #include "qemu/jhash.h"
> >  #include "qemu/timer.h"
> >  #include "net/eth.h"
> > +#include "qemu/option.h"
> > +#include "qemu/option_int.h"
> > +#include "qemu/config-file.h"
> >
> >  #define HASHTABLE_MAX_SIZE 16384
> >
> > @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int size, int
> > vnet_hdr_len);  Packet *packet_new_nocopy(void *data, int size, int
> > vnet_hdr_len);  void packet_destroy(void *opaque, void *user_data);
> > void packet_destroy_partial(void *opaque, void *user_data);
> > +int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp);
> >
> >  #endif /* NET_COLO_H */
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > f20240cc9f..69ca9c9839 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -12,6 +12,7 @@
> >  #include "qemu/osdep.h"
> >  #include "net/filter.h"
> >  #include "net/net.h"
> > +#include "net/colo.h"
> >  #include "qapi/error.h"
> >  #include "qom/object.h"
> >  #include "qemu/main-loop.h"
> > @@ -224,6 +225,16 @@ static void filter_mirror_setup(NetFilterState *nf,
> Error **errp)
> >          return;
> >      }
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > +        /*
> > +         * filter mirror needs 'vnet_hdr_support' when colo filter modules
> > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >      qemu_chr_fe_init(&s->chr_out, chr, errp);  }
> >
> > @@ -252,6 +263,16 @@ static void filter_redirector_setup(NetFilterState
> *nf, Error **errp)
> >          }
> >      }
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > +        /*
> > +         * filter redirector needs 'vnet_hdr_support' when colo filter modules
> > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >      net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
> >
> >      if (s->indev) {
> > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c index
> > cb3a96cde1..637ef4ce71 100644
> > --- a/net/filter-rewriter.c
> > +++ b/net/filter-rewriter.c
> > @@ -388,6 +388,16 @@ static void colo_rewriter_setup(NetFilterState
> > *nf, Error **errp)  {
> >      RewriterState *s = FILTER_REWRITER(nf);
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > +        /*
> > +         * filter rewriter needs 'vnet_hdr_support' when colo filter modules
> > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >      s->connection_track_table =
> g_hash_table_new_full(connection_key_hash,
> >                                                        connection_key_equal,
> >                                                        g_free, diff
> > --git a/qapi/qom.json b/qapi/qom.json index a25616bc7a..5760107160
> > 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -241,6 +241,11 @@
> >  # @notify_dev: name of the character device backend to be used to
> communicate
> >  #              with the remote colo-frame (only for Xen COLO)
> >  #
> > +# @netdev: id of the network device backend to colo-compare. Although
> > +#          colo-compare doesn't depend on network device directly, this
> > +#          parameter helps colo-compare know what network driver it is
> > +#          working on.(since 6.2)
> > +#
> >  # @compare_timeout: the maximum time to hold a packet from
> @primary_in for
> >  #                   comparison with an incoming packet on @secondary_in in
> >  #                   milliseconds (default: 3000)
> > @@ -264,6 +269,7 @@
> >              'outdev': 'str',
> >              'iothread': 'str',
> >              '*notify_dev': 'str',
> > +            '*netdev': 'str',
> >              '*compare_timeout': 'uint64',
> >              '*expired_scan_cycle': 'uint32',
> >              '*max_queue_size': 'uint32', diff --git a/qemu-options.hx
> > b/qemu-options.hx index 8f603cc7e6..250937fbbf 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4964,12 +4964,14 @@ SRST
> >          stored. The file format is libpcap, so it can be analyzed with
> >          tools such as tcpdump or Wireshark.
> >
> > -    ``-object colo-
> compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chard
> evid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@
> var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> > +    ``-object
> > + colo-
> compare,id=id,primary_in=chardevid,secondary_in=chardevid,outde
> > + v=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,notify
> > +
> _dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,
> m
> > + ax_queue_size=@var{size}]``
> >          Colo-compare gets packet from primary\_in chardevid and
> >          secondary\_in, then compare whether the payload of primary packet
> >          and secondary packet are the same. If same, it will output
> >          primary packet to out\_dev, else it will notify COLO-framework to do
> > -        checkpoint and send primary packet to out\_dev. In order to
> > +        checkpoint and send primary packet to out\_dev. Although colo-
> compare
> > +        does not depend on network device directly, netdevid helps
> > +        colo-compare know what network driver it is working on. In
> > + order to
> >          improve efficiency, we need to put the task of comparison in
> >          another iothread. If it has the vnet\_hdr\_support flag,
> >          colo compare will send/recv packet with vnet\_hdr\_len.

Jason Wang Oct. 19, 2021, 7:38 a.m. UTC | #3

在 2021/9/18 上午10:04, Zhang Chen 写道:
> When COLO use only one vnet_hdr_support parameter between
> COLO network filter(filter-mirror, filter-redirector or
> filter-rewriter and colo-compare, packet will not be parsed
> correctly. Acquire network driver related to COLO, if it is
> nirtio-net,


Typo.


>   check vnet_hdr_support flag of COLO network filter
> and colo-compare.
>
> Signed-off-by: Tao Xu <[e-mail]>
> Signed-off-by: Zhang Chen <[e-mail]>
> ---
>
> Changelog:
> v3:
>      Fix some typos.
>      Rebased for Qemu 6.2.
>
> v2:
>      Detect virtio-net driver and apply vnet_hdr_support
>      automatically. (Jason)
> ---
>   net/colo-compare.c    | 57 +++++++++++++++++++++++++++++++++++++++++++
>   net/colo.c            | 20 +++++++++++++++
>   net/colo.h            |  4 +++
>   net/filter-mirror.c   | 21 ++++++++++++++++
>   net/filter-rewriter.c | 10 ++++++++
>   qapi/qom.json         |  6 +++++
>   qemu-options.hx       |  6 +++--
>   7 files changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index b100e7b51f..870bd05a41 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -110,6 +110,7 @@ struct CompareState {
>       char *sec_indev;
>       char *outdev;
>       char *notify_dev;
> +    char *netdev;
>       CharBackend chr_pri_in;
>       CharBackend chr_sec_in;
>       CharBackend chr_out;
> @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
>       return COMPARE_READ_LEN_MAX;
>   }
>   
> +static int colo_set_default_netdev(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    const char *colo_obj_type, *netdev_from_filter;
> +    char **netdev = (char **)opaque;
> +
> +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> +
> +    if (colo_obj_type &&
> +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> +        if (*netdev == NULL) {
> +            *netdev = g_strdup(netdev_from_filter);
> +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> +            warn_report("%s is using a different netdev from other COLO "
> +                        "component", colo_obj_type);
> +        }
> +    }
> +    return 0;
> +}
> +
>   /*
>    * Called from the main thread on the primary for packets
>    * arriving over the socket from the primary.
> @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object *obj,
>       s->vnet_hdr = value;
>   }
>   
> +static char *compare_get_netdev(Object *obj, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    return g_strdup(s->netdev);
> +}
> +
> +static void compare_set_netdev(Object *obj, const char *value, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    g_free(s->netdev);
> +    s->netdev = g_strdup(value);
> +}
> +
>   static char *compare_get_notify_dev(Object *obj, Error **errp)
>   {
>       CompareState *s = COLO_COMPARE(obj);
> @@ -1274,6 +1312,12 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>           max_queue_size = MAX_QUEUE_SIZE;
>       }
>   
> +    if (!s->netdev) {
> +        /* Set default netdev as the first colo netfilter found */
> +        qemu_opts_foreach(qemu_find_opts("object"),
> +                          colo_set_default_netdev, &s->netdev, NULL);
> +    }
> +
>       if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
>           !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
>           return;
> @@ -1289,6 +1333,16 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>           return;
>       }
>   
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, s->netdev, NULL)) {
> +        /*
> +         * colo compare needs 'vnet_hdr_support' when it works on virtio-net,
> +         * add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>       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);
>   
> @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
>       s->vnet_hdr = false;
>       object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
>                                compare_set_vnet_hdr);
> +    /* colo compare can't varify that netdev is correct */
> +    object_property_add_str(obj, "netdev", compare_get_netdev,
> +                            compare_set_netdev);
>   }
>   
>   void colo_compare_cleanup(void)
> diff --git a/net/colo.c b/net/colo.c
> index 3a3e6e89a0..4a03780f45 100644
> --- a/net/colo.c
> +++ b/net/colo.c
> @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable *connection_track_table,
>   
>       return conn ? true : false;
>   }
> +
> +/* check the network driver related to COLO, return 1 if it is virtio-net */
> +int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    const char *driver_type, *netdev_from_driver;
> +    char *netdev_from_filter = (char *)opaque;
> +
> +    driver_type = qemu_opt_get(opts, "driver");
> +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> +
> +    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
> +        return 0;
> +    }
> +
> +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> +        return 1;
> +    }
> +    return 0;
> +}
> diff --git a/net/colo.h b/net/colo.h
> index d91cd245c4..d401fc76b6 100644
> --- a/net/colo.h
> +++ b/net/colo.h
> @@ -18,6 +18,9 @@
>   #include "qemu/jhash.h"
>   #include "qemu/timer.h"
>   #include "net/eth.h"
> +#include "qemu/option.h"
> +#include "qemu/option_int.h"
> +#include "qemu/config-file.h"
>   
>   #define HASHTABLE_MAX_SIZE 16384
>   
> @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int size, int vnet_hdr_len);
>   Packet *packet_new_nocopy(void *data, int size, int vnet_hdr_len);
>   void packet_destroy(void *opaque, void *user_data);
>   void packet_destroy_partial(void *opaque, void *user_data);
> +int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp);
>   
>   #endif /* NET_COLO_H */
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index f20240cc9f..69ca9c9839 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -12,6 +12,7 @@
>   #include "qemu/osdep.h"
>   #include "net/filter.h"
>   #include "net/net.h"
> +#include "net/colo.h"
>   #include "qapi/error.h"
>   #include "qom/object.h"
>   #include "qemu/main-loop.h"
> @@ -224,6 +225,16 @@ static void filter_mirror_setup(NetFilterState *nf, Error **errp)
>           return;
>       }
>   
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, nf->netdev_id, NULL)) {
> +        /*
> +         * filter mirror needs 'vnet_hdr_support' when colo filter modules
> +         * work on virtio-net, add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>       qemu_chr_fe_init(&s->chr_out, chr, errp);
>   }
>   
> @@ -252,6 +263,16 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
>           }
>       }
>   
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, nf->netdev_id, NULL)) {
> +        /*
> +         * filter redirector needs 'vnet_hdr_support' when colo filter modules
> +         * work on virtio-net, add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>       net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
>   
>       if (s->indev) {
> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> index cb3a96cde1..637ef4ce71 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -388,6 +388,16 @@ static void colo_rewriter_setup(NetFilterState *nf, Error **errp)
>   {
>       RewriterState *s = FILTER_REWRITER(nf);
>   
> +    if (!s->vnet_hdr &&
> +        qemu_opts_foreach(qemu_find_opts("device"),
> +                          vnet_driver_check, nf->netdev_id, NULL)) {
> +        /*
> +         * filter rewriter needs 'vnet_hdr_support' when colo filter modules
> +         * work on virtio-net, add 'vnet_hdr_support' automatically
> +         */
> +        s->vnet_hdr = true;
> +    }
> +
>       s->connection_track_table = g_hash_table_new_full(connection_key_hash,
>                                                         connection_key_equal,
>                                                         g_free,
> diff --git a/qapi/qom.json b/qapi/qom.json
> index a25616bc7a..5760107160 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -241,6 +241,11 @@
>   # @notify_dev: name of the character device backend to be used to communicate
>   #              with the remote colo-frame (only for Xen COLO)
>   #
> +# @netdev: id of the network device backend to colo-compare. Although
> +#          colo-compare doesn't depend on network device directly, this
> +#          parameter helps colo-compare know what network driver it is
> +#          working on.(since 6.2)


This looks kind of not elegant. We don't want to couple COLO with netdev.

I wonder if we can do this without adding netdev.

Thanks


> +#
>   # @compare_timeout: the maximum time to hold a packet from @primary_in for
>   #                   comparison with an incoming packet on @secondary_in in
>   #                   milliseconds (default: 3000)
> @@ -264,6 +269,7 @@
>               'outdev': 'str',
>               'iothread': 'str',
>               '*notify_dev': 'str',
> +            '*netdev': 'str',
>               '*compare_timeout': 'uint64',
>               '*expired_scan_cycle': 'uint32',
>               '*max_queue_size': 'uint32',
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8f603cc7e6..250937fbbf 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4964,12 +4964,14 @@ SRST
>           stored. The file format is libpcap, so it can be analyzed with
>           tools such as tcpdump or Wireshark.
>   
> -    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> +    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
>           Colo-compare gets packet from primary\_in chardevid and
>           secondary\_in, then compare whether the payload of primary packet
>           and secondary packet are the same. If same, it will output
>           primary packet to out\_dev, else it will notify COLO-framework to do
> -        checkpoint and send primary packet to out\_dev. In order to
> +        checkpoint and send primary packet to out\_dev. Although colo-compare
> +        does not depend on network device directly, netdevid helps
> +        colo-compare know what network driver it is working on. In order to
>           improve efficiency, we need to put the task of comparison in
>           another iothread. If it has the vnet\_hdr\_support flag,
>           colo compare will send/recv packet with vnet\_hdr\_len.

Zhang Chen Oct. 20, 2021, 2:52 a.m. UTC | #4

> -----Original Message-----
> From: Jason Wang <[e-mail]>
> Sent: Tuesday, October 19, 2021 3:39 PM
> To: Zhang, Chen <[e-mail]>; Eric Blake <[e-mail]>;
> Markus Armbruster <[e-mail]>
> Cc: qemu-dev <[e-mail]>; Li Zhijian
> <[e-mail]>; Lukas Straub <[e-mail]>; Tao Xu
> <[e-mail]>
> Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when using
> virtio-net
> 
> 
> 在 2021/9/18 上午10:04, Zhang Chen 写道:
> > When COLO use only one vnet_hdr_support parameter between COLO
> network
> > filter(filter-mirror, filter-redirector or filter-rewriter and
> > colo-compare, packet will not be parsed correctly. Acquire network
> > driver related to COLO, if it is nirtio-net,
> 
> 
> Typo.

Oh~ will fix in next version.

> 
> 
> >   check vnet_hdr_support flag of COLO network filter and colo-compare.
> >
> > Signed-off-by: Tao Xu <[e-mail]>
> > Signed-off-by: Zhang Chen <[e-mail]>
> > ---
> >
> > Changelog:
> > v3:
> >      Fix some typos.
> >      Rebased for Qemu 6.2.
> >
> > v2:
> >      Detect virtio-net driver and apply vnet_hdr_support
> >      automatically. (Jason)
> > ---
> >   net/colo-compare.c    | 57
> +++++++++++++++++++++++++++++++++++++++++++
> >   net/colo.c            | 20 +++++++++++++++
> >   net/colo.h            |  4 +++
> >   net/filter-mirror.c   | 21 ++++++++++++++++
> >   net/filter-rewriter.c | 10 ++++++++
> >   qapi/qom.json         |  6 +++++
> >   qemu-options.hx       |  6 +++--
> >   7 files changed, 122 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > b100e7b51f..870bd05a41 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -110,6 +110,7 @@ struct CompareState {
> >       char *sec_indev;
> >       char *outdev;
> >       char *notify_dev;
> > +    char *netdev;
> >       CharBackend chr_pri_in;
> >       CharBackend chr_sec_in;
> >       CharBackend chr_out;
> > @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
> >       return COMPARE_READ_LEN_MAX;
> >   }
> >
> > +static int colo_set_default_netdev(void *opaque, QemuOpts *opts,
> > +Error **errp) {
> > +    const char *colo_obj_type, *netdev_from_filter;
> > +    char **netdev = (char **)opaque;
> > +
> > +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> > +
> > +    if (colo_obj_type &&
> > +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> > +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> > +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> > +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> > +        if (*netdev == NULL) {
> > +            *netdev = g_strdup(netdev_from_filter);
> > +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> > +            warn_report("%s is using a different netdev from other COLO "
> > +                        "component", colo_obj_type);
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> >   /*
> >    * Called from the main thread on the primary for packets
> >    * arriving over the socket from the primary.
> > @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object *obj,
> >       s->vnet_hdr = value;
> >   }
> >
> > +static char *compare_get_netdev(Object *obj, Error **errp) {
> > +    CompareState *s = COLO_COMPARE(obj);
> > +
> > +    return g_strdup(s->netdev);
> > +}
> > +
> > +static void compare_set_netdev(Object *obj, const char *value, Error
> > +**errp) {
> > +    CompareState *s = COLO_COMPARE(obj);
> > +
> > +    g_free(s->netdev);
> > +    s->netdev = g_strdup(value);
> > +}
> > +
> >   static char *compare_get_notify_dev(Object *obj, Error **errp)
> >   {
> >       CompareState *s = COLO_COMPARE(obj); @@ -1274,6 +1312,12 @@
> > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> >           max_queue_size = MAX_QUEUE_SIZE;
> >       }
> >
> > +    if (!s->netdev) {
> > +        /* Set default netdev as the first colo netfilter found */
> > +        qemu_opts_foreach(qemu_find_opts("object"),
> > +                          colo_set_default_netdev, &s->netdev, NULL);
> > +    }
> > +
> >       if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
> >           !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
> >           return;
> > @@ -1289,6 +1333,16 @@ static void
> colo_compare_complete(UserCreatable *uc, Error **errp)
> >           return;
> >       }
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, s->netdev, NULL)) {
> > +        /*
> > +         * colo compare needs 'vnet_hdr_support' when it works on virtio-net,
> > +         * add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >       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);
> >
> > @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
> >       s->vnet_hdr = false;
> >       object_property_add_bool(obj, "vnet_hdr_support",
> compare_get_vnet_hdr,
> >                                compare_set_vnet_hdr);
> > +    /* colo compare can't varify that netdev is correct */
> > +    object_property_add_str(obj, "netdev", compare_get_netdev,
> > +                            compare_set_netdev);
> >   }
> >
> >   void colo_compare_cleanup(void)
> > diff --git a/net/colo.c b/net/colo.c
> > index 3a3e6e89a0..4a03780f45 100644
> > --- a/net/colo.c
> > +++ b/net/colo.c
> > @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable
> > *connection_track_table,
> >
> >       return conn ? true : false;
> >   }
> > +
> > +/* check the network driver related to COLO, return 1 if it is
> > +virtio-net */ int vnet_driver_check(void *opaque, QemuOpts *opts,
> > +Error **errp) {
> > +    const char *driver_type, *netdev_from_driver;
> > +    char *netdev_from_filter = (char *)opaque;
> > +
> > +    driver_type = qemu_opt_get(opts, "driver");
> > +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> > +
> > +    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
> > +        return 0;
> > +    }
> > +
> > +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> > +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > diff --git a/net/colo.h b/net/colo.h
> > index d91cd245c4..d401fc76b6 100644
> > --- a/net/colo.h
> > +++ b/net/colo.h
> > @@ -18,6 +18,9 @@
> >   #include "qemu/jhash.h"
> >   #include "qemu/timer.h"
> >   #include "net/eth.h"
> > +#include "qemu/option.h"
> > +#include "qemu/option_int.h"
> > +#include "qemu/config-file.h"
> >
> >   #define HASHTABLE_MAX_SIZE 16384
> >
> > @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int size, int
> vnet_hdr_len);
> >   Packet *packet_new_nocopy(void *data, int size, int vnet_hdr_len);
> >   void packet_destroy(void *opaque, void *user_data);
> >   void packet_destroy_partial(void *opaque, void *user_data);
> > +int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp);
> >
> >   #endif /* NET_COLO_H */
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > f20240cc9f..69ca9c9839 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -12,6 +12,7 @@
> >   #include "qemu/osdep.h"
> >   #include "net/filter.h"
> >   #include "net/net.h"
> > +#include "net/colo.h"
> >   #include "qapi/error.h"
> >   #include "qom/object.h"
> >   #include "qemu/main-loop.h"
> > @@ -224,6 +225,16 @@ static void filter_mirror_setup(NetFilterState *nf,
> Error **errp)
> >           return;
> >       }
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > +        /*
> > +         * filter mirror needs 'vnet_hdr_support' when colo filter modules
> > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >       qemu_chr_fe_init(&s->chr_out, chr, errp);
> >   }
> >
> > @@ -252,6 +263,16 @@ static void filter_redirector_setup(NetFilterState
> *nf, Error **errp)
> >           }
> >       }
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > +        /*
> > +         * filter redirector needs 'vnet_hdr_support' when colo filter modules
> > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >       net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
> >
> >       if (s->indev) {
> > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c index
> > cb3a96cde1..637ef4ce71 100644
> > --- a/net/filter-rewriter.c
> > +++ b/net/filter-rewriter.c
> > @@ -388,6 +388,16 @@ static void colo_rewriter_setup(NetFilterState *nf,
> Error **errp)
> >   {
> >       RewriterState *s = FILTER_REWRITER(nf);
> >
> > +    if (!s->vnet_hdr &&
> > +        qemu_opts_foreach(qemu_find_opts("device"),
> > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > +        /*
> > +         * filter rewriter needs 'vnet_hdr_support' when colo filter modules
> > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > +         */
> > +        s->vnet_hdr = true;
> > +    }
> > +
> >       s->connection_track_table =
> g_hash_table_new_full(connection_key_hash,
> >                                                         connection_key_equal,
> >                                                         g_free, diff
> > --git a/qapi/qom.json b/qapi/qom.json index a25616bc7a..5760107160
> > 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -241,6 +241,11 @@
> >   # @notify_dev: name of the character device backend to be used to
> communicate
> >   #              with the remote colo-frame (only for Xen COLO)
> >   #
> > +# @netdev: id of the network device backend to colo-compare. Although
> > +#          colo-compare doesn't depend on network device directly, this
> > +#          parameter helps colo-compare know what network driver it is
> > +#          working on.(since 6.2)
> 
> 
> This looks kind of not elegant. We don't want to couple COLO with netdev.
> 
> I wonder if we can do this without adding netdev.

Yes, current COLO is not bind to netdev, but here we want to auto fix configuration issue.
To avoid wrong setting other non-colo related existing filter,  COLO need know the colo-effect netdev to setup.
And this parameter is optional, this patch will setup default colo-effect netdev in colo_set_default_netdev().
Or any good idea?

Thanks
Chen

> 
> Thanks
> 
> 
> > +#
> >   # @compare_timeout: the maximum time to hold a packet from
> @primary_in for
> >   #                   comparison with an incoming packet on @secondary_in in
> >   #                   milliseconds (default: 3000)
> > @@ -264,6 +269,7 @@
> >               'outdev': 'str',
> >               'iothread': 'str',
> >               '*notify_dev': 'str',
> > +            '*netdev': 'str',
> >               '*compare_timeout': 'uint64',
> >               '*expired_scan_cycle': 'uint32',
> >               '*max_queue_size': 'uint32', diff --git
> > a/qemu-options.hx b/qemu-options.hx index 8f603cc7e6..250937fbbf
> > 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4964,12 +4964,14 @@ SRST
> >           stored. The file format is libpcap, so it can be analyzed with
> >           tools such as tcpdump or Wireshark.
> >
> > -    ``-object colo-
> compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chard
> evid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@
> var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> > +    ``-object
> > + colo-
> compare,id=id,primary_in=chardevid,secondary_in=chardevid,outde
> > + v=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,notify
> > +
> _dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,
> m
> > + ax_queue_size=@var{size}]``
> >           Colo-compare gets packet from primary\_in chardevid and
> >           secondary\_in, then compare whether the payload of primary packet
> >           and secondary packet are the same. If same, it will output
> >           primary packet to out\_dev, else it will notify COLO-framework to do
> > -        checkpoint and send primary packet to out\_dev. In order to
> > +        checkpoint and send primary packet to out\_dev. Although colo-
> compare
> > +        does not depend on network device directly, netdevid helps
> > +        colo-compare know what network driver it is working on. In
> > + order to
> >           improve efficiency, we need to put the task of comparison in
> >           another iothread. If it has the vnet\_hdr\_support flag,
> >           colo compare will send/recv packet with vnet\_hdr\_len.

Jason Wang Oct. 20, 2021, 3:13 a.m. UTC | #5

On Wed, Oct 20, 2021 at 10:53 AM Zhang, Chen <[e-mail]> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <[e-mail]>
> > Sent: Tuesday, October 19, 2021 3:39 PM
> > To: Zhang, Chen <[e-mail]>; Eric Blake <[e-mail]>;
> > Markus Armbruster <[e-mail]>
> > Cc: qemu-dev <[e-mail]>; Li Zhijian
> > <[e-mail]>; Lukas Straub <[e-mail]>; Tao Xu
> > <[e-mail]>
> > Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when using
> > virtio-net
> >
> >
> > 在 2021/9/18 上午10:04, Zhang Chen 写道:
> > > When COLO use only one vnet_hdr_support parameter between COLO
> > network
> > > filter(filter-mirror, filter-redirector or filter-rewriter and
> > > colo-compare, packet will not be parsed correctly. Acquire network
> > > driver related to COLO, if it is nirtio-net,
> >
> >
> > Typo.
>
> Oh~ will fix in next version.
>
> >
> >
> > >   check vnet_hdr_support flag of COLO network filter and colo-compare.
> > >
> > > Signed-off-by: Tao Xu <[e-mail]>
> > > Signed-off-by: Zhang Chen <[e-mail]>
> > > ---
> > >
> > > Changelog:
> > > v3:
> > >      Fix some typos.
> > >      Rebased for Qemu 6.2.
> > >
> > > v2:
> > >      Detect virtio-net driver and apply vnet_hdr_support
> > >      automatically. (Jason)
> > > ---
> > >   net/colo-compare.c    | 57
> > +++++++++++++++++++++++++++++++++++++++++++
> > >   net/colo.c            | 20 +++++++++++++++
> > >   net/colo.h            |  4 +++
> > >   net/filter-mirror.c   | 21 ++++++++++++++++
> > >   net/filter-rewriter.c | 10 ++++++++
> > >   qapi/qom.json         |  6 +++++
> > >   qemu-options.hx       |  6 +++--
> > >   7 files changed, 122 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > b100e7b51f..870bd05a41 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -110,6 +110,7 @@ struct CompareState {
> > >       char *sec_indev;
> > >       char *outdev;
> > >       char *notify_dev;
> > > +    char *netdev;
> > >       CharBackend chr_pri_in;
> > >       CharBackend chr_sec_in;
> > >       CharBackend chr_out;
> > > @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
> > >       return COMPARE_READ_LEN_MAX;
> > >   }
> > >
> > > +static int colo_set_default_netdev(void *opaque, QemuOpts *opts,
> > > +Error **errp) {
> > > +    const char *colo_obj_type, *netdev_from_filter;
> > > +    char **netdev = (char **)opaque;
> > > +
> > > +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> > > +
> > > +    if (colo_obj_type &&
> > > +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> > > +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> > > +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> > > +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> > > +        if (*netdev == NULL) {
> > > +            *netdev = g_strdup(netdev_from_filter);
> > > +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> > > +            warn_report("%s is using a different netdev from other COLO "
> > > +                        "component", colo_obj_type);
> > > +        }
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > >   /*
> > >    * Called from the main thread on the primary for packets
> > >    * arriving over the socket from the primary.
> > > @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object *obj,
> > >       s->vnet_hdr = value;
> > >   }
> > >
> > > +static char *compare_get_netdev(Object *obj, Error **errp) {
> > > +    CompareState *s = COLO_COMPARE(obj);
> > > +
> > > +    return g_strdup(s->netdev);
> > > +}
> > > +
> > > +static void compare_set_netdev(Object *obj, const char *value, Error
> > > +**errp) {
> > > +    CompareState *s = COLO_COMPARE(obj);
> > > +
> > > +    g_free(s->netdev);
> > > +    s->netdev = g_strdup(value);
> > > +}
> > > +
> > >   static char *compare_get_notify_dev(Object *obj, Error **errp)
> > >   {
> > >       CompareState *s = COLO_COMPARE(obj); @@ -1274,6 +1312,12 @@
> > > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > >           max_queue_size = MAX_QUEUE_SIZE;
> > >       }
> > >
> > > +    if (!s->netdev) {
> > > +        /* Set default netdev as the first colo netfilter found */
> > > +        qemu_opts_foreach(qemu_find_opts("object"),
> > > +                          colo_set_default_netdev, &s->netdev, NULL);
> > > +    }
> > > +
> > >       if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
> > >           !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
> > >           return;
> > > @@ -1289,6 +1333,16 @@ static void
> > colo_compare_complete(UserCreatable *uc, Error **errp)
> > >           return;
> > >       }
> > >
> > > +    if (!s->vnet_hdr &&
> > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > +                          vnet_driver_check, s->netdev, NULL)) {
> > > +        /*
> > > +         * colo compare needs 'vnet_hdr_support' when it works on virtio-net,
> > > +         * add 'vnet_hdr_support' automatically
> > > +         */
> > > +        s->vnet_hdr = true;
> > > +    }
> > > +
> > >       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);
> > >
> > > @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
> > >       s->vnet_hdr = false;
> > >       object_property_add_bool(obj, "vnet_hdr_support",
> > compare_get_vnet_hdr,
> > >                                compare_set_vnet_hdr);
> > > +    /* colo compare can't varify that netdev is correct */
> > > +    object_property_add_str(obj, "netdev", compare_get_netdev,
> > > +                            compare_set_netdev);
> > >   }
> > >
> > >   void colo_compare_cleanup(void)
> > > diff --git a/net/colo.c b/net/colo.c
> > > index 3a3e6e89a0..4a03780f45 100644
> > > --- a/net/colo.c
> > > +++ b/net/colo.c
> > > @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable
> > > *connection_track_table,
> > >
> > >       return conn ? true : false;
> > >   }
> > > +
> > > +/* check the network driver related to COLO, return 1 if it is
> > > +virtio-net */ int vnet_driver_check(void *opaque, QemuOpts *opts,
> > > +Error **errp) {
> > > +    const char *driver_type, *netdev_from_driver;
> > > +    char *netdev_from_filter = (char *)opaque;
> > > +
> > > +    driver_type = qemu_opt_get(opts, "driver");
> > > +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> > > +
> > > +    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> > > +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> > > +        return 1;
> > > +    }
> > > +    return 0;
> > > +}
> > > diff --git a/net/colo.h b/net/colo.h
> > > index d91cd245c4..d401fc76b6 100644
> > > --- a/net/colo.h
> > > +++ b/net/colo.h
> > > @@ -18,6 +18,9 @@
> > >   #include "qemu/jhash.h"
> > >   #include "qemu/timer.h"
> > >   #include "net/eth.h"
> > > +#include "qemu/option.h"
> > > +#include "qemu/option_int.h"
> > > +#include "qemu/config-file.h"
> > >
> > >   #define HASHTABLE_MAX_SIZE 16384
> > >
> > > @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int size, int
> > vnet_hdr_len);
> > >   Packet *packet_new_nocopy(void *data, int size, int vnet_hdr_len);
> > >   void packet_destroy(void *opaque, void *user_data);
> > >   void packet_destroy_partial(void *opaque, void *user_data);
> > > +int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp);
> > >
> > >   #endif /* NET_COLO_H */
> > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > f20240cc9f..69ca9c9839 100644
> > > --- a/net/filter-mirror.c
> > > +++ b/net/filter-mirror.c
> > > @@ -12,6 +12,7 @@
> > >   #include "qemu/osdep.h"
> > >   #include "net/filter.h"
> > >   #include "net/net.h"
> > > +#include "net/colo.h"
> > >   #include "qapi/error.h"
> > >   #include "qom/object.h"
> > >   #include "qemu/main-loop.h"
> > > @@ -224,6 +225,16 @@ static void filter_mirror_setup(NetFilterState *nf,
> > Error **errp)
> > >           return;
> > >       }
> > >
> > > +    if (!s->vnet_hdr &&
> > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > +        /*
> > > +         * filter mirror needs 'vnet_hdr_support' when colo filter modules
> > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > +         */
> > > +        s->vnet_hdr = true;
> > > +    }
> > > +
> > >       qemu_chr_fe_init(&s->chr_out, chr, errp);
> > >   }
> > >
> > > @@ -252,6 +263,16 @@ static void filter_redirector_setup(NetFilterState
> > *nf, Error **errp)
> > >           }
> > >       }
> > >
> > > +    if (!s->vnet_hdr &&
> > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > +        /*
> > > +         * filter redirector needs 'vnet_hdr_support' when colo filter modules
> > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > +         */
> > > +        s->vnet_hdr = true;
> > > +    }
> > > +
> > >       net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
> > >
> > >       if (s->indev) {
> > > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c index
> > > cb3a96cde1..637ef4ce71 100644
> > > --- a/net/filter-rewriter.c
> > > +++ b/net/filter-rewriter.c
> > > @@ -388,6 +388,16 @@ static void colo_rewriter_setup(NetFilterState *nf,
> > Error **errp)
> > >   {
> > >       RewriterState *s = FILTER_REWRITER(nf);
> > >
> > > +    if (!s->vnet_hdr &&
> > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > +        /*
> > > +         * filter rewriter needs 'vnet_hdr_support' when colo filter modules
> > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > +         */
> > > +        s->vnet_hdr = true;
> > > +    }
> > > +
> > >       s->connection_track_table =
> > g_hash_table_new_full(connection_key_hash,
> > >                                                         connection_key_equal,
> > >                                                         g_free, diff
> > > --git a/qapi/qom.json b/qapi/qom.json index a25616bc7a..5760107160
> > > 100644
> > > --- a/qapi/qom.json
> > > +++ b/qapi/qom.json
> > > @@ -241,6 +241,11 @@
> > >   # @notify_dev: name of the character device backend to be used to
> > communicate
> > >   #              with the remote colo-frame (only for Xen COLO)
> > >   #
> > > +# @netdev: id of the network device backend to colo-compare. Although
> > > +#          colo-compare doesn't depend on network device directly, this
> > > +#          parameter helps colo-compare know what network driver it is
> > > +#          working on.(since 6.2)
> >
> >
> > This looks kind of not elegant. We don't want to couple COLO with netdev.
> >
> > I wonder if we can do this without adding netdev.
>
> Yes, current COLO is not bind to netdev, but here we want to auto fix configuration issue.

If my memory is correct, the current design allows an external
comparing module to work.

> To avoid wrong setting other non-colo related existing filter,  COLO need know the colo-effect netdev to setup.
> And this parameter is optional, this patch will setup default colo-effect netdev in colo_set_default_netdev().
> Or any good idea?

How about

1) adding a handshake in the protocol before receiving packets
or
2) add a header to the payload to show if it contains a vnet header
or
3) go back to let mgmt set vnet_hdr=on/off (looks like the first
proposal but it avoids the coupling).

Thanks

>
> Thanks
> Chen
>
> >
> > Thanks
> >
> >
> > > +#
> > >   # @compare_timeout: the maximum time to hold a packet from
> > @primary_in for
> > >   #                   comparison with an incoming packet on @secondary_in in
> > >   #                   milliseconds (default: 3000)
> > > @@ -264,6 +269,7 @@
> > >               'outdev': 'str',
> > >               'iothread': 'str',
> > >               '*notify_dev': 'str',
> > > +            '*netdev': 'str',
> > >               '*compare_timeout': 'uint64',
> > >               '*expired_scan_cycle': 'uint32',
> > >               '*max_queue_size': 'uint32', diff --git
> > > a/qemu-options.hx b/qemu-options.hx index 8f603cc7e6..250937fbbf
> > > 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -4964,12 +4964,14 @@ SRST
> > >           stored. The file format is libpcap, so it can be analyzed with
> > >           tools such as tcpdump or Wireshark.
> > >
> > > -    ``-object colo-
> > compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chard
> > evid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@
> > var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> > > +    ``-object
> > > + colo-
> > compare,id=id,primary_in=chardevid,secondary_in=chardevid,outde
> > > + v=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,notify
> > > +
> > _dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,
> > m
> > > + ax_queue_size=@var{size}]``
> > >           Colo-compare gets packet from primary\_in chardevid and
> > >           secondary\_in, then compare whether the payload of primary packet
> > >           and secondary packet are the same. If same, it will output
> > >           primary packet to out\_dev, else it will notify COLO-framework to do
> > > -        checkpoint and send primary packet to out\_dev. In order to
> > > +        checkpoint and send primary packet to out\_dev. Although colo-
> > compare
> > > +        does not depend on network device directly, netdevid helps
> > > +        colo-compare know what network driver it is working on. In
> > > + order to
> > >           improve efficiency, we need to put the task of comparison in
> > >           another iothread. If it has the vnet\_hdr\_support flag,
> > >           colo compare will send/recv packet with vnet\_hdr\_len.
>

Zhang Chen Oct. 20, 2021, 6:19 a.m. UTC | #6

> -----Original Message-----
> From: Jason Wang <[e-mail]>
> Sent: Wednesday, October 20, 2021 11:13 AM
> To: Zhang, Chen <[e-mail]>
> Cc: Eric Blake <[e-mail]>; Markus Armbruster
> <[e-mail]>; qemu-dev <[e-mail]>; Li Zhijian
> <[e-mail]>; Lukas Straub <[e-mail]>
> Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when using
> virtio-net
> 
> On Wed, Oct 20, 2021 at 10:53 AM Zhang, Chen <[e-mail]>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <[e-mail]>
> > > Sent: Tuesday, October 19, 2021 3:39 PM
> > > To: Zhang, Chen <[e-mail]>; Eric Blake
> > > <[e-mail]>; Markus Armbruster <[e-mail]>
> > > Cc: qemu-dev <[e-mail]>; Li Zhijian
> > > <[e-mail]>; Lukas Straub <[e-mail]>; Tao
> > > Xu <[e-mail]>
> > > Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when
> > > using virtio-net
> > >
> > >
> > > 在 2021/9/18 上午10:04, Zhang Chen 写道:
> > > > When COLO use only one vnet_hdr_support parameter between COLO
> > > network
> > > > filter(filter-mirror, filter-redirector or filter-rewriter and
> > > > colo-compare, packet will not be parsed correctly. Acquire network
> > > > driver related to COLO, if it is nirtio-net,
> > >
> > >
> > > Typo.
> >
> > Oh~ will fix in next version.
> >
> > >
> > >
> > > >   check vnet_hdr_support flag of COLO network filter and colo-compare.
> > > >
> > > > Signed-off-by: Tao Xu <[e-mail]>
> > > > Signed-off-by: Zhang Chen <[e-mail]>
> > > > ---
> > > >
> > > > Changelog:
> > > > v3:
> > > >      Fix some typos.
> > > >      Rebased for Qemu 6.2.
> > > >
> > > > v2:
> > > >      Detect virtio-net driver and apply vnet_hdr_support
> > > >      automatically. (Jason)
> > > > ---
> > > >   net/colo-compare.c    | 57
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > >   net/colo.c            | 20 +++++++++++++++
> > > >   net/colo.h            |  4 +++
> > > >   net/filter-mirror.c   | 21 ++++++++++++++++
> > > >   net/filter-rewriter.c | 10 ++++++++
> > > >   qapi/qom.json         |  6 +++++
> > > >   qemu-options.hx       |  6 +++--
> > > >   7 files changed, 122 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > b100e7b51f..870bd05a41 100644
> > > > --- a/net/colo-compare.c
> > > > +++ b/net/colo-compare.c
> > > > @@ -110,6 +110,7 @@ struct CompareState {
> > > >       char *sec_indev;
> > > >       char *outdev;
> > > >       char *notify_dev;
> > > > +    char *netdev;
> > > >       CharBackend chr_pri_in;
> > > >       CharBackend chr_sec_in;
> > > >       CharBackend chr_out;
> > > > @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
> > > >       return COMPARE_READ_LEN_MAX;
> > > >   }
> > > >
> > > > +static int colo_set_default_netdev(void *opaque, QemuOpts *opts,
> > > > +Error **errp) {
> > > > +    const char *colo_obj_type, *netdev_from_filter;
> > > > +    char **netdev = (char **)opaque;
> > > > +
> > > > +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> > > > +
> > > > +    if (colo_obj_type &&
> > > > +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> > > > +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> > > > +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> > > > +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> > > > +        if (*netdev == NULL) {
> > > > +            *netdev = g_strdup(netdev_from_filter);
> > > > +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> > > > +            warn_report("%s is using a different netdev from other COLO "
> > > > +                        "component", colo_obj_type);
> > > > +        }
> > > > +    }
> > > > +    return 0;
> > > > +}
> > > > +
> > > >   /*
> > > >    * Called from the main thread on the primary for packets
> > > >    * arriving over the socket from the primary.
> > > > @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object
> *obj,
> > > >       s->vnet_hdr = value;
> > > >   }
> > > >
> > > > +static char *compare_get_netdev(Object *obj, Error **errp) {
> > > > +    CompareState *s = COLO_COMPARE(obj);
> > > > +
> > > > +    return g_strdup(s->netdev);
> > > > +}
> > > > +
> > > > +static void compare_set_netdev(Object *obj, const char *value,
> > > > +Error
> > > > +**errp) {
> > > > +    CompareState *s = COLO_COMPARE(obj);
> > > > +
> > > > +    g_free(s->netdev);
> > > > +    s->netdev = g_strdup(value);
> > > > +}
> > > > +
> > > >   static char *compare_get_notify_dev(Object *obj, Error **errp)
> > > >   {
> > > >       CompareState *s = COLO_COMPARE(obj); @@ -1274,6 +1312,12
> @@
> > > > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > > >           max_queue_size = MAX_QUEUE_SIZE;
> > > >       }
> > > >
> > > > +    if (!s->netdev) {
> > > > +        /* Set default netdev as the first colo netfilter found */
> > > > +        qemu_opts_foreach(qemu_find_opts("object"),
> > > > +                          colo_set_default_netdev, &s->netdev, NULL);
> > > > +    }
> > > > +
> > > >       if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
> > > >           !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
> > > >           return;
> > > > @@ -1289,6 +1333,16 @@ static void
> > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > > >           return;
> > > >       }
> > > >
> > > > +    if (!s->vnet_hdr &&
> > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > +                          vnet_driver_check, s->netdev, NULL)) {
> > > > +        /*
> > > > +         * colo compare needs 'vnet_hdr_support' when it works on
> virtio-net,
> > > > +         * add 'vnet_hdr_support' automatically
> > > > +         */
> > > > +        s->vnet_hdr = true;
> > > > +    }
> > > > +
> > > >       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);
> > > >
> > > > @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
> > > >       s->vnet_hdr = false;
> > > >       object_property_add_bool(obj, "vnet_hdr_support",
> > > compare_get_vnet_hdr,
> > > >                                compare_set_vnet_hdr);
> > > > +    /* colo compare can't varify that netdev is correct */
> > > > +    object_property_add_str(obj, "netdev", compare_get_netdev,
> > > > +                            compare_set_netdev);
> > > >   }
> > > >
> > > >   void colo_compare_cleanup(void)
> > > > diff --git a/net/colo.c b/net/colo.c index 3a3e6e89a0..4a03780f45
> > > > 100644
> > > > --- a/net/colo.c
> > > > +++ b/net/colo.c
> > > > @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable
> > > > *connection_track_table,
> > > >
> > > >       return conn ? true : false;
> > > >   }
> > > > +
> > > > +/* check the network driver related to COLO, return 1 if it is
> > > > +virtio-net */ int vnet_driver_check(void *opaque, QemuOpts *opts,
> > > > +Error **errp) {
> > > > +    const char *driver_type, *netdev_from_driver;
> > > > +    char *netdev_from_filter = (char *)opaque;
> > > > +
> > > > +    driver_type = qemu_opt_get(opts, "driver");
> > > > +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> > > > +
> > > > +    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> > > > +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> > > > +        return 1;
> > > > +    }
> > > > +    return 0;
> > > > +}
> > > > diff --git a/net/colo.h b/net/colo.h index d91cd245c4..d401fc76b6
> > > > 100644
> > > > --- a/net/colo.h
> > > > +++ b/net/colo.h
> > > > @@ -18,6 +18,9 @@
> > > >   #include "qemu/jhash.h"
> > > >   #include "qemu/timer.h"
> > > >   #include "net/eth.h"
> > > > +#include "qemu/option.h"
> > > > +#include "qemu/option_int.h"
> > > > +#include "qemu/config-file.h"
> > > >
> > > >   #define HASHTABLE_MAX_SIZE 16384
> > > >
> > > > @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int size,
> > > > int
> > > vnet_hdr_len);
> > > >   Packet *packet_new_nocopy(void *data, int size, int vnet_hdr_len);
> > > >   void packet_destroy(void *opaque, void *user_data);
> > > >   void packet_destroy_partial(void *opaque, void *user_data);
> > > > +int vnet_driver_check(void *opaque, QemuOpts *opts, Error
> > > > +**errp);
> > > >
> > > >   #endif /* NET_COLO_H */
> > > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > > f20240cc9f..69ca9c9839 100644
> > > > --- a/net/filter-mirror.c
> > > > +++ b/net/filter-mirror.c
> > > > @@ -12,6 +12,7 @@
> > > >   #include "qemu/osdep.h"
> > > >   #include "net/filter.h"
> > > >   #include "net/net.h"
> > > > +#include "net/colo.h"
> > > >   #include "qapi/error.h"
> > > >   #include "qom/object.h"
> > > >   #include "qemu/main-loop.h"
> > > > @@ -224,6 +225,16 @@ static void
> > > > filter_mirror_setup(NetFilterState *nf,
> > > Error **errp)
> > > >           return;
> > > >       }
> > > >
> > > > +    if (!s->vnet_hdr &&
> > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > +        /*
> > > > +         * filter mirror needs 'vnet_hdr_support' when colo filter modules
> > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > +         */
> > > > +        s->vnet_hdr = true;
> > > > +    }
> > > > +
> > > >       qemu_chr_fe_init(&s->chr_out, chr, errp);
> > > >   }
> > > >
> > > > @@ -252,6 +263,16 @@ static void
> > > > filter_redirector_setup(NetFilterState
> > > *nf, Error **errp)
> > > >           }
> > > >       }
> > > >
> > > > +    if (!s->vnet_hdr &&
> > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > +        /*
> > > > +         * filter redirector needs 'vnet_hdr_support' when colo filter
> modules
> > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > +         */
> > > > +        s->vnet_hdr = true;
> > > > +    }
> > > > +
> > > >       net_socket_rs_init(&s->rs, redirector_rs_finalize,
> > > > s->vnet_hdr);
> > > >
> > > >       if (s->indev) {
> > > > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c index
> > > > cb3a96cde1..637ef4ce71 100644
> > > > --- a/net/filter-rewriter.c
> > > > +++ b/net/filter-rewriter.c
> > > > @@ -388,6 +388,16 @@ static void
> > > > colo_rewriter_setup(NetFilterState *nf,
> > > Error **errp)
> > > >   {
> > > >       RewriterState *s = FILTER_REWRITER(nf);
> > > >
> > > > +    if (!s->vnet_hdr &&
> > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > +        /*
> > > > +         * filter rewriter needs 'vnet_hdr_support' when colo filter
> modules
> > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > +         */
> > > > +        s->vnet_hdr = true;
> > > > +    }
> > > > +
> > > >       s->connection_track_table =
> > > g_hash_table_new_full(connection_key_hash,
> > > >                                                         connection_key_equal,
> > > >                                                         g_free,
> > > > diff --git a/qapi/qom.json b/qapi/qom.json index
> > > > a25616bc7a..5760107160
> > > > 100644
> > > > --- a/qapi/qom.json
> > > > +++ b/qapi/qom.json
> > > > @@ -241,6 +241,11 @@
> > > >   # @notify_dev: name of the character device backend to be used
> > > > to
> > > communicate
> > > >   #              with the remote colo-frame (only for Xen COLO)
> > > >   #
> > > > +# @netdev: id of the network device backend to colo-compare.
> Although
> > > > +#          colo-compare doesn't depend on network device directly, this
> > > > +#          parameter helps colo-compare know what network driver it is
> > > > +#          working on.(since 6.2)
> > >
> > >
> > > This looks kind of not elegant. We don't want to couple COLO with netdev.
> > >
> > > I wonder if we can do this without adding netdev.
> >
> > Yes, current COLO is not bind to netdev, but here we want to auto fix
> configuration issue.
> 
> If my memory is correct, the current design allows an external comparing
> module to work.

Yes, this netdev used by identify a series of comparing modules (include filters).

> 
> > To avoid wrong setting other non-colo related existing filter,  COLO need
> know the colo-effect netdev to setup.
> > And this parameter is optional, this patch will setup default colo-effect
> netdev in colo_set_default_netdev().
> > Or any good idea?
> 
> How about
> 
> 1) adding a handshake in the protocol before receiving packets or
> 2) add a header to the payload to show if it contains a vnet header or
> 3) go back to let mgmt set vnet_hdr=on/off (looks like the first proposal but
> it avoids the coupling).

Current code already have a header to the payload to show the vnet header(in net/filter-mirror.c filter_send()).
But the vnet_hdr_len is not necessary:
Disabled vnet_hdr payload:
1. size ---> 2. Real network payload.
Enbaled vnet_hdr payload:
1. size ---> 2. Vnet_hdr_len ----> 3. Real network payload.

It looks we can define the vnet_hdr_len as necessary part of protocol.
If no vnet_hdr, we set the vnet_hdr_len to 0.
1. size ---> 2. Vnet_hdr_len ----> 3. Real network payload.

How about this proposal?

Thanks
Chen


> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> > >
> > > Thanks
> > >
> > >
> > > > +#
> > > >   # @compare_timeout: the maximum time to hold a packet from
> > > @primary_in for
> > > >   #                   comparison with an incoming packet on @secondary_in in
> > > >   #                   milliseconds (default: 3000)
> > > > @@ -264,6 +269,7 @@
> > > >               'outdev': 'str',
> > > >               'iothread': 'str',
> > > >               '*notify_dev': 'str',
> > > > +            '*netdev': 'str',
> > > >               '*compare_timeout': 'uint64',
> > > >               '*expired_scan_cycle': 'uint32',
> > > >               '*max_queue_size': 'uint32', diff --git
> > > > a/qemu-options.hx b/qemu-options.hx index 8f603cc7e6..250937fbbf
> > > > 100644
> > > > --- a/qemu-options.hx
> > > > +++ b/qemu-options.hx
> > > > @@ -4964,12 +4964,14 @@ SRST
> > > >           stored. The file format is libpcap, so it can be analyzed with
> > > >           tools such as tcpdump or Wireshark.
> > > >
> > > > -    ``-object colo-
> > >
> compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=cha
> > > rd
> > > evid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout
> > > =@
> > >
> var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> > > > +    ``-object
> > > > + colo-
> > > compare,id=id,primary_in=chardevid,secondary_in=chardevid,outde
> > > > + v=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,no
> > > > + tify
> > > > +
> > >
> _dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,
> > > m
> > > > + ax_queue_size=@var{size}]``
> > > >           Colo-compare gets packet from primary\_in chardevid and
> > > >           secondary\_in, then compare whether the payload of primary
> packet
> > > >           and secondary packet are the same. If same, it will output
> > > >           primary packet to out\_dev, else it will notify COLO-framework to
> do
> > > > -        checkpoint and send primary packet to out\_dev. In order to
> > > > +        checkpoint and send primary packet to out\_dev. Although
> > > > + colo-
> > > compare
> > > > +        does not depend on network device directly, netdevid helps
> > > > +        colo-compare know what network driver it is working on.
> > > > + In order to
> > > >           improve efficiency, we need to put the task of comparison in
> > > >           another iothread. If it has the vnet\_hdr\_support flag,
> > > >           colo compare will send/recv packet with vnet\_hdr\_len.
> >

Jason Wang Oct. 21, 2021, 3:01 a.m. UTC | #7

On Wed, Oct 20, 2021 at 2:19 PM Zhang, Chen <[e-mail]> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <[e-mail]>
> > Sent: Wednesday, October 20, 2021 11:13 AM
> > To: Zhang, Chen <[e-mail]>
> > Cc: Eric Blake <[e-mail]>; Markus Armbruster
> > <[e-mail]>; qemu-dev <[e-mail]>; Li Zhijian
> > <[e-mail]>; Lukas Straub <[e-mail]>
> > Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when using
> > virtio-net
> >
> > On Wed, Oct 20, 2021 at 10:53 AM Zhang, Chen <[e-mail]>
> > wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jason Wang <[e-mail]>
> > > > Sent: Tuesday, October 19, 2021 3:39 PM
> > > > To: Zhang, Chen <[e-mail]>; Eric Blake
> > > > <[e-mail]>; Markus Armbruster <[e-mail]>
> > > > Cc: qemu-dev <[e-mail]>; Li Zhijian
> > > > <[e-mail]>; Lukas Straub <[e-mail]>; Tao
> > > > Xu <[e-mail]>
> > > > Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when
> > > > using virtio-net
> > > >
> > > >
> > > > 在 2021/9/18 上午10:04, Zhang Chen 写道:
> > > > > When COLO use only one vnet_hdr_support parameter between COLO
> > > > network
> > > > > filter(filter-mirror, filter-redirector or filter-rewriter and
> > > > > colo-compare, packet will not be parsed correctly. Acquire network
> > > > > driver related to COLO, if it is nirtio-net,
> > > >
> > > >
> > > > Typo.
> > >
> > > Oh~ will fix in next version.
> > >
> > > >
> > > >
> > > > >   check vnet_hdr_support flag of COLO network filter and colo-compare.
> > > > >
> > > > > Signed-off-by: Tao Xu <[e-mail]>
> > > > > Signed-off-by: Zhang Chen <[e-mail]>
> > > > > ---
> > > > >
> > > > > Changelog:
> > > > > v3:
> > > > >      Fix some typos.
> > > > >      Rebased for Qemu 6.2.
> > > > >
> > > > > v2:
> > > > >      Detect virtio-net driver and apply vnet_hdr_support
> > > > >      automatically. (Jason)
> > > > > ---
> > > > >   net/colo-compare.c    | 57
> > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > >   net/colo.c            | 20 +++++++++++++++
> > > > >   net/colo.h            |  4 +++
> > > > >   net/filter-mirror.c   | 21 ++++++++++++++++
> > > > >   net/filter-rewriter.c | 10 ++++++++
> > > > >   qapi/qom.json         |  6 +++++
> > > > >   qemu-options.hx       |  6 +++--
> > > > >   7 files changed, 122 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > b100e7b51f..870bd05a41 100644
> > > > > --- a/net/colo-compare.c
> > > > > +++ b/net/colo-compare.c
> > > > > @@ -110,6 +110,7 @@ struct CompareState {
> > > > >       char *sec_indev;
> > > > >       char *outdev;
> > > > >       char *notify_dev;
> > > > > +    char *netdev;
> > > > >       CharBackend chr_pri_in;
> > > > >       CharBackend chr_sec_in;
> > > > >       CharBackend chr_out;
> > > > > @@ -838,6 +839,28 @@ static int compare_chr_can_read(void *opaque)
> > > > >       return COMPARE_READ_LEN_MAX;
> > > > >   }
> > > > >
> > > > > +static int colo_set_default_netdev(void *opaque, QemuOpts *opts,
> > > > > +Error **errp) {
> > > > > +    const char *colo_obj_type, *netdev_from_filter;
> > > > > +    char **netdev = (char **)opaque;
> > > > > +
> > > > > +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> > > > > +
> > > > > +    if (colo_obj_type &&
> > > > > +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> > > > > +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> > > > > +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> > > > > +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> > > > > +        if (*netdev == NULL) {
> > > > > +            *netdev = g_strdup(netdev_from_filter);
> > > > > +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> > > > > +            warn_report("%s is using a different netdev from other COLO "
> > > > > +                        "component", colo_obj_type);
> > > > > +        }
> > > > > +    }
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >   /*
> > > > >    * Called from the main thread on the primary for packets
> > > > >    * arriving over the socket from the primary.
> > > > > @@ -1050,6 +1073,21 @@ static void compare_set_vnet_hdr(Object
> > *obj,
> > > > >       s->vnet_hdr = value;
> > > > >   }
> > > > >
> > > > > +static char *compare_get_netdev(Object *obj, Error **errp) {
> > > > > +    CompareState *s = COLO_COMPARE(obj);
> > > > > +
> > > > > +    return g_strdup(s->netdev);
> > > > > +}
> > > > > +
> > > > > +static void compare_set_netdev(Object *obj, const char *value,
> > > > > +Error
> > > > > +**errp) {
> > > > > +    CompareState *s = COLO_COMPARE(obj);
> > > > > +
> > > > > +    g_free(s->netdev);
> > > > > +    s->netdev = g_strdup(value);
> > > > > +}
> > > > > +
> > > > >   static char *compare_get_notify_dev(Object *obj, Error **errp)
> > > > >   {
> > > > >       CompareState *s = COLO_COMPARE(obj); @@ -1274,6 +1312,12
> > @@
> > > > > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > > > >           max_queue_size = MAX_QUEUE_SIZE;
> > > > >       }
> > > > >
> > > > > +    if (!s->netdev) {
> > > > > +        /* Set default netdev as the first colo netfilter found */
> > > > > +        qemu_opts_foreach(qemu_find_opts("object"),
> > > > > +                          colo_set_default_netdev, &s->netdev, NULL);
> > > > > +    }
> > > > > +
> > > > >       if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
> > > > >           !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
> > > > >           return;
> > > > > @@ -1289,6 +1333,16 @@ static void
> > > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > > > >           return;
> > > > >       }
> > > > >
> > > > > +    if (!s->vnet_hdr &&
> > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > +                          vnet_driver_check, s->netdev, NULL)) {
> > > > > +        /*
> > > > > +         * colo compare needs 'vnet_hdr_support' when it works on
> > virtio-net,
> > > > > +         * add 'vnet_hdr_support' automatically
> > > > > +         */
> > > > > +        s->vnet_hdr = true;
> > > > > +    }
> > > > > +
> > > > >       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);
> > > > >
> > > > > @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
> > > > >       s->vnet_hdr = false;
> > > > >       object_property_add_bool(obj, "vnet_hdr_support",
> > > > compare_get_vnet_hdr,
> > > > >                                compare_set_vnet_hdr);
> > > > > +    /* colo compare can't varify that netdev is correct */
> > > > > +    object_property_add_str(obj, "netdev", compare_get_netdev,
> > > > > +                            compare_set_netdev);
> > > > >   }
> > > > >
> > > > >   void colo_compare_cleanup(void)
> > > > > diff --git a/net/colo.c b/net/colo.c index 3a3e6e89a0..4a03780f45
> > > > > 100644
> > > > > --- a/net/colo.c
> > > > > +++ b/net/colo.c
> > > > > @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable
> > > > > *connection_track_table,
> > > > >
> > > > >       return conn ? true : false;
> > > > >   }
> > > > > +
> > > > > +/* check the network driver related to COLO, return 1 if it is
> > > > > +virtio-net */ int vnet_driver_check(void *opaque, QemuOpts *opts,
> > > > > +Error **errp) {
> > > > > +    const char *driver_type, *netdev_from_driver;
> > > > > +    char *netdev_from_filter = (char *)opaque;
> > > > > +
> > > > > +    driver_type = qemu_opt_get(opts, "driver");
> > > > > +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> > > > > +
> > > > > +    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
> > > > > +        return 0;
> > > > > +    }
> > > > > +
> > > > > +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> > > > > +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> > > > > +        return 1;
> > > > > +    }
> > > > > +    return 0;
> > > > > +}
> > > > > diff --git a/net/colo.h b/net/colo.h index d91cd245c4..d401fc76b6
> > > > > 100644
> > > > > --- a/net/colo.h
> > > > > +++ b/net/colo.h
> > > > > @@ -18,6 +18,9 @@
> > > > >   #include "qemu/jhash.h"
> > > > >   #include "qemu/timer.h"
> > > > >   #include "net/eth.h"
> > > > > +#include "qemu/option.h"
> > > > > +#include "qemu/option_int.h"
> > > > > +#include "qemu/config-file.h"
> > > > >
> > > > >   #define HASHTABLE_MAX_SIZE 16384
> > > > >
> > > > > @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int size,
> > > > > int
> > > > vnet_hdr_len);
> > > > >   Packet *packet_new_nocopy(void *data, int size, int vnet_hdr_len);
> > > > >   void packet_destroy(void *opaque, void *user_data);
> > > > >   void packet_destroy_partial(void *opaque, void *user_data);
> > > > > +int vnet_driver_check(void *opaque, QemuOpts *opts, Error
> > > > > +**errp);
> > > > >
> > > > >   #endif /* NET_COLO_H */
> > > > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > > > f20240cc9f..69ca9c9839 100644
> > > > > --- a/net/filter-mirror.c
> > > > > +++ b/net/filter-mirror.c
> > > > > @@ -12,6 +12,7 @@
> > > > >   #include "qemu/osdep.h"
> > > > >   #include "net/filter.h"
> > > > >   #include "net/net.h"
> > > > > +#include "net/colo.h"
> > > > >   #include "qapi/error.h"
> > > > >   #include "qom/object.h"
> > > > >   #include "qemu/main-loop.h"
> > > > > @@ -224,6 +225,16 @@ static void
> > > > > filter_mirror_setup(NetFilterState *nf,
> > > > Error **errp)
> > > > >           return;
> > > > >       }
> > > > >
> > > > > +    if (!s->vnet_hdr &&
> > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > > +        /*
> > > > > +         * filter mirror needs 'vnet_hdr_support' when colo filter modules
> > > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > > +         */
> > > > > +        s->vnet_hdr = true;
> > > > > +    }
> > > > > +
> > > > >       qemu_chr_fe_init(&s->chr_out, chr, errp);
> > > > >   }
> > > > >
> > > > > @@ -252,6 +263,16 @@ static void
> > > > > filter_redirector_setup(NetFilterState
> > > > *nf, Error **errp)
> > > > >           }
> > > > >       }
> > > > >
> > > > > +    if (!s->vnet_hdr &&
> > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > > +        /*
> > > > > +         * filter redirector needs 'vnet_hdr_support' when colo filter
> > modules
> > > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > > +         */
> > > > > +        s->vnet_hdr = true;
> > > > > +    }
> > > > > +
> > > > >       net_socket_rs_init(&s->rs, redirector_rs_finalize,
> > > > > s->vnet_hdr);
> > > > >
> > > > >       if (s->indev) {
> > > > > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c index
> > > > > cb3a96cde1..637ef4ce71 100644
> > > > > --- a/net/filter-rewriter.c
> > > > > +++ b/net/filter-rewriter.c
> > > > > @@ -388,6 +388,16 @@ static void
> > > > > colo_rewriter_setup(NetFilterState *nf,
> > > > Error **errp)
> > > > >   {
> > > > >       RewriterState *s = FILTER_REWRITER(nf);
> > > > >
> > > > > +    if (!s->vnet_hdr &&
> > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > > +        /*
> > > > > +         * filter rewriter needs 'vnet_hdr_support' when colo filter
> > modules
> > > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > > +         */
> > > > > +        s->vnet_hdr = true;
> > > > > +    }
> > > > > +
> > > > >       s->connection_track_table =
> > > > g_hash_table_new_full(connection_key_hash,
> > > > >                                                         connection_key_equal,
> > > > >                                                         g_free,
> > > > > diff --git a/qapi/qom.json b/qapi/qom.json index
> > > > > a25616bc7a..5760107160
> > > > > 100644
> > > > > --- a/qapi/qom.json
> > > > > +++ b/qapi/qom.json
> > > > > @@ -241,6 +241,11 @@
> > > > >   # @notify_dev: name of the character device backend to be used
> > > > > to
> > > > communicate
> > > > >   #              with the remote colo-frame (only for Xen COLO)
> > > > >   #
> > > > > +# @netdev: id of the network device backend to colo-compare.
> > Although
> > > > > +#          colo-compare doesn't depend on network device directly, this
> > > > > +#          parameter helps colo-compare know what network driver it is
> > > > > +#          working on.(since 6.2)
> > > >
> > > >
> > > > This looks kind of not elegant. We don't want to couple COLO with netdev.
> > > >
> > > > I wonder if we can do this without adding netdev.
> > >
> > > Yes, current COLO is not bind to netdev, but here we want to auto fix
> > configuration issue.
> >
> > If my memory is correct, the current design allows an external comparing
> > module to work.
>
> Yes, this netdev used by identify a series of comparing modules (include filters).
>
> >
> > > To avoid wrong setting other non-colo related existing filter,  COLO need
> > know the colo-effect netdev to setup.
> > > And this parameter is optional, this patch will setup default colo-effect
> > netdev in colo_set_default_netdev().
> > > Or any good idea?
> >
> > How about
> >
> > 1) adding a handshake in the protocol before receiving packets or
> > 2) add a header to the payload to show if it contains a vnet header or
> > 3) go back to let mgmt set vnet_hdr=on/off (looks like the first proposal but
> > it avoids the coupling).
>
> Current code already have a header to the payload to show the vnet header(in net/filter-mirror.c filter_send()).
> But the vnet_hdr_len is not necessary:
> Disabled vnet_hdr payload:
> 1. size ---> 2. Real network payload.
> Enbaled vnet_hdr payload:
> 1. size ---> 2. Vnet_hdr_len ----> 3. Real network payload.
>
> It looks we can define the vnet_hdr_len as necessary part of protocol.
> If no vnet_hdr, we set the vnet_hdr_len to 0.
> 1. size ---> 2. Vnet_hdr_len ----> 3. Real network payload.
>
> How about this proposal?

I think it should work.

Thanks

>
> Thanks
> Chen
>
>
> >
> > Thanks
> >
> > >
> > > Thanks
> > > Chen
> > >
> > > >
> > > > Thanks
> > > >
> > > >
> > > > > +#
> > > > >   # @compare_timeout: the maximum time to hold a packet from
> > > > @primary_in for
> > > > >   #                   comparison with an incoming packet on @secondary_in in
> > > > >   #                   milliseconds (default: 3000)
> > > > > @@ -264,6 +269,7 @@
> > > > >               'outdev': 'str',
> > > > >               'iothread': 'str',
> > > > >               '*notify_dev': 'str',
> > > > > +            '*netdev': 'str',
> > > > >               '*compare_timeout': 'uint64',
> > > > >               '*expired_scan_cycle': 'uint32',
> > > > >               '*max_queue_size': 'uint32', diff --git
> > > > > a/qemu-options.hx b/qemu-options.hx index 8f603cc7e6..250937fbbf
> > > > > 100644
> > > > > --- a/qemu-options.hx
> > > > > +++ b/qemu-options.hx
> > > > > @@ -4964,12 +4964,14 @@ SRST
> > > > >           stored. The file format is libpcap, so it can be analyzed with
> > > > >           tools such as tcpdump or Wireshark.
> > > > >
> > > > > -    ``-object colo-
> > > >
> > compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=cha
> > > > rd
> > > > evid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout
> > > > =@
> > > >
> > var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> > > > > +    ``-object
> > > > > + colo-
> > > > compare,id=id,primary_in=chardevid,secondary_in=chardevid,outde
> > > > > + v=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,no
> > > > > + tify
> > > > > +
> > > >
> > _dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,
> > > > m
> > > > > + ax_queue_size=@var{size}]``
> > > > >           Colo-compare gets packet from primary\_in chardevid and
> > > > >           secondary\_in, then compare whether the payload of primary
> > packet
> > > > >           and secondary packet are the same. If same, it will output
> > > > >           primary packet to out\_dev, else it will notify COLO-framework to
> > do
> > > > > -        checkpoint and send primary packet to out\_dev. In order to
> > > > > +        checkpoint and send primary packet to out\_dev. Although
> > > > > + colo-
> > > > compare
> > > > > +        does not depend on network device directly, netdevid helps
> > > > > +        colo-compare know what network driver it is working on.
> > > > > + In order to
> > > > >           improve efficiency, we need to put the task of comparison in
> > > > >           another iothread. If it has the vnet\_hdr\_support flag,
> > > > >           colo compare will send/recv packet with vnet\_hdr\_len.
> > >
>

Zhang Chen Oct. 21, 2021, 3:05 a.m. UTC | #8

> -----Original Message-----
> From: Jason Wang <[e-mail]>
> Sent: Thursday, October 21, 2021 11:02 AM
> To: Zhang, Chen <[e-mail]>
> Cc: Eric Blake <[e-mail]>; Markus Armbruster
> <[e-mail]>; qemu-dev <[e-mail]>; Li Zhijian
> <[e-mail]>; Lukas Straub <[e-mail]>
> Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when using
> virtio-net
> 
> On Wed, Oct 20, 2021 at 2:19 PM Zhang, Chen <[e-mail]>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <[e-mail]>
> > > Sent: Wednesday, October 20, 2021 11:13 AM
> > > To: Zhang, Chen <[e-mail]>
> > > Cc: Eric Blake <[e-mail]>; Markus Armbruster
> > > <[e-mail]>; qemu-dev <[e-mail]>; Li
> Zhijian
> > > <[e-mail]>; Lukas Straub <[e-mail]>
> > > Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag when
> > > using virtio-net
> > >
> > > On Wed, Oct 20, 2021 at 10:53 AM Zhang, Chen <[e-mail]>
> > > wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Jason Wang <[e-mail]>
> > > > > Sent: Tuesday, October 19, 2021 3:39 PM
> > > > > To: Zhang, Chen <[e-mail]>; Eric Blake
> > > > > <[e-mail]>; Markus Armbruster <[e-mail]>
> > > > > Cc: qemu-dev <[e-mail]>; Li Zhijian
> > > > > <[e-mail]>; Lukas Straub <[e-mail]>;
> > > > > Tao Xu <[e-mail]>
> > > > > Subject: Re: [PATCH V3] net/colo: check vnet_hdr_support flag
> > > > > when using virtio-net
> > > > >
> > > > >
> > > > > 在 2021/9/18 上午10:04, Zhang Chen 写道:
> > > > > > When COLO use only one vnet_hdr_support parameter between
> COLO
> > > > > network
> > > > > > filter(filter-mirror, filter-redirector or filter-rewriter and
> > > > > > colo-compare, packet will not be parsed correctly. Acquire
> > > > > > network driver related to COLO, if it is nirtio-net,
> > > > >
> > > > >
> > > > > Typo.
> > > >
> > > > Oh~ will fix in next version.
> > > >
> > > > >
> > > > >
> > > > > >   check vnet_hdr_support flag of COLO network filter and colo-
> compare.
> > > > > >
> > > > > > Signed-off-by: Tao Xu <[e-mail]>
> > > > > > Signed-off-by: Zhang Chen <[e-mail]>
> > > > > > ---
> > > > > >
> > > > > > Changelog:
> > > > > > v3:
> > > > > >      Fix some typos.
> > > > > >      Rebased for Qemu 6.2.
> > > > > >
> > > > > > v2:
> > > > > >      Detect virtio-net driver and apply vnet_hdr_support
> > > > > >      automatically. (Jason)
> > > > > > ---
> > > > > >   net/colo-compare.c    | 57
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > > >   net/colo.c            | 20 +++++++++++++++
> > > > > >   net/colo.h            |  4 +++
> > > > > >   net/filter-mirror.c   | 21 ++++++++++++++++
> > > > > >   net/filter-rewriter.c | 10 ++++++++
> > > > > >   qapi/qom.json         |  6 +++++
> > > > > >   qemu-options.hx       |  6 +++--
> > > > > >   7 files changed, 122 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > > b100e7b51f..870bd05a41 100644
> > > > > > --- a/net/colo-compare.c
> > > > > > +++ b/net/colo-compare.c
> > > > > > @@ -110,6 +110,7 @@ struct CompareState {
> > > > > >       char *sec_indev;
> > > > > >       char *outdev;
> > > > > >       char *notify_dev;
> > > > > > +    char *netdev;
> > > > > >       CharBackend chr_pri_in;
> > > > > >       CharBackend chr_sec_in;
> > > > > >       CharBackend chr_out;
> > > > > > @@ -838,6 +839,28 @@ static int compare_chr_can_read(void
> *opaque)
> > > > > >       return COMPARE_READ_LEN_MAX;
> > > > > >   }
> > > > > >
> > > > > > +static int colo_set_default_netdev(void *opaque, QemuOpts
> > > > > > +*opts, Error **errp) {
> > > > > > +    const char *colo_obj_type, *netdev_from_filter;
> > > > > > +    char **netdev = (char **)opaque;
> > > > > > +
> > > > > > +    colo_obj_type = qemu_opt_get(opts, "qom-type");
> > > > > > +
> > > > > > +    if (colo_obj_type &&
> > > > > > +        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
> > > > > > +         strcmp(colo_obj_type, "filter-redirector") == 0 ||
> > > > > > +         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
> > > > > > +        netdev_from_filter = qemu_opt_get(opts, "netdev");
> > > > > > +        if (*netdev == NULL) {
> > > > > > +            *netdev = g_strdup(netdev_from_filter);
> > > > > > +        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
> > > > > > +            warn_report("%s is using a different netdev from other
> COLO "
> > > > > > +                        "component", colo_obj_type);
> > > > > > +        }
> > > > > > +    }
> > > > > > +    return 0;
> > > > > > +}
> > > > > > +
> > > > > >   /*
> > > > > >    * Called from the main thread on the primary for packets
> > > > > >    * arriving over the socket from the primary.
> > > > > > @@ -1050,6 +1073,21 @@ static void
> compare_set_vnet_hdr(Object
> > > *obj,
> > > > > >       s->vnet_hdr = value;
> > > > > >   }
> > > > > >
> > > > > > +static char *compare_get_netdev(Object *obj, Error **errp) {
> > > > > > +    CompareState *s = COLO_COMPARE(obj);
> > > > > > +
> > > > > > +    return g_strdup(s->netdev); }
> > > > > > +
> > > > > > +static void compare_set_netdev(Object *obj, const char
> > > > > > +*value, Error
> > > > > > +**errp) {
> > > > > > +    CompareState *s = COLO_COMPARE(obj);
> > > > > > +
> > > > > > +    g_free(s->netdev);
> > > > > > +    s->netdev = g_strdup(value); }
> > > > > > +
> > > > > >   static char *compare_get_notify_dev(Object *obj, Error **errp)
> > > > > >   {
> > > > > >       CompareState *s = COLO_COMPARE(obj); @@ -1274,6 +1312,12
> > > @@
> > > > > > static void colo_compare_complete(UserCreatable *uc, Error
> **errp)
> > > > > >           max_queue_size = MAX_QUEUE_SIZE;
> > > > > >       }
> > > > > >
> > > > > > +    if (!s->netdev) {
> > > > > > +        /* Set default netdev as the first colo netfilter found */
> > > > > > +        qemu_opts_foreach(qemu_find_opts("object"),
> > > > > > +                          colo_set_default_netdev, &s->netdev, NULL);
> > > > > > +    }
> > > > > > +
> > > > > >       if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
> > > > > >           !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
> > > > > >           return;
> > > > > > @@ -1289,6 +1333,16 @@ static void
> > > > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > > > > >           return;
> > > > > >       }
> > > > > >
> > > > > > +    if (!s->vnet_hdr &&
> > > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > > +                          vnet_driver_check, s->netdev, NULL)) {
> > > > > > +        /*
> > > > > > +         * colo compare needs 'vnet_hdr_support' when it
> > > > > > + works on
> > > virtio-net,
> > > > > > +         * add 'vnet_hdr_support' automatically
> > > > > > +         */
> > > > > > +        s->vnet_hdr = true;
> > > > > > +    }
> > > > > > +
> > > > > >       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);
> > > > > >
> > > > > > @@ -1400,6 +1454,9 @@ static void colo_compare_init(Object *obj)
> > > > > >       s->vnet_hdr = false;
> > > > > >       object_property_add_bool(obj, "vnet_hdr_support",
> > > > > compare_get_vnet_hdr,
> > > > > >                                compare_set_vnet_hdr);
> > > > > > +    /* colo compare can't varify that netdev is correct */
> > > > > > +    object_property_add_str(obj, "netdev", compare_get_netdev,
> > > > > > +                            compare_set_netdev);
> > > > > >   }
> > > > > >
> > > > > >   void colo_compare_cleanup(void) diff --git a/net/colo.c
> > > > > > b/net/colo.c index 3a3e6e89a0..4a03780f45
> > > > > > 100644
> > > > > > --- a/net/colo.c
> > > > > > +++ b/net/colo.c
> > > > > > @@ -243,3 +243,23 @@ bool connection_has_tracked(GHashTable
> > > > > > *connection_track_table,
> > > > > >
> > > > > >       return conn ? true : false;
> > > > > >   }
> > > > > > +
> > > > > > +/* check the network driver related to COLO, return 1 if it
> > > > > > +is virtio-net */ int vnet_driver_check(void *opaque, QemuOpts
> > > > > > +*opts, Error **errp) {
> > > > > > +    const char *driver_type, *netdev_from_driver;
> > > > > > +    char *netdev_from_filter = (char *)opaque;
> > > > > > +
> > > > > > +    driver_type = qemu_opt_get(opts, "driver");
> > > > > > +    netdev_from_driver = qemu_opt_get(opts, "netdev");
> > > > > > +
> > > > > > +    if (!driver_type || !netdev_from_driver || !netdev_from_filter)
> {
> > > > > > +        return 0;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (g_str_has_prefix(driver_type, "virtio-net") &&
> > > > > > +        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
> > > > > > +        return 1;
> > > > > > +    }
> > > > > > +    return 0;
> > > > > > +}
> > > > > > diff --git a/net/colo.h b/net/colo.h index
> > > > > > d91cd245c4..d401fc76b6
> > > > > > 100644
> > > > > > --- a/net/colo.h
> > > > > > +++ b/net/colo.h
> > > > > > @@ -18,6 +18,9 @@
> > > > > >   #include "qemu/jhash.h"
> > > > > >   #include "qemu/timer.h"
> > > > > >   #include "net/eth.h"
> > > > > > +#include "qemu/option.h"
> > > > > > +#include "qemu/option_int.h"
> > > > > > +#include "qemu/config-file.h"
> > > > > >
> > > > > >   #define HASHTABLE_MAX_SIZE 16384
> > > > > >
> > > > > > @@ -104,5 +107,6 @@ Packet *packet_new(const void *data, int
> > > > > > size, int
> > > > > vnet_hdr_len);
> > > > > >   Packet *packet_new_nocopy(void *data, int size, int
> vnet_hdr_len);
> > > > > >   void packet_destroy(void *opaque, void *user_data);
> > > > > >   void packet_destroy_partial(void *opaque, void *user_data);
> > > > > > +int vnet_driver_check(void *opaque, QemuOpts *opts, Error
> > > > > > +**errp);
> > > > > >
> > > > > >   #endif /* NET_COLO_H */
> > > > > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > > > > f20240cc9f..69ca9c9839 100644
> > > > > > --- a/net/filter-mirror.c
> > > > > > +++ b/net/filter-mirror.c
> > > > > > @@ -12,6 +12,7 @@
> > > > > >   #include "qemu/osdep.h"
> > > > > >   #include "net/filter.h"
> > > > > >   #include "net/net.h"
> > > > > > +#include "net/colo.h"
> > > > > >   #include "qapi/error.h"
> > > > > >   #include "qom/object.h"
> > > > > >   #include "qemu/main-loop.h"
> > > > > > @@ -224,6 +225,16 @@ static void
> > > > > > filter_mirror_setup(NetFilterState *nf,
> > > > > Error **errp)
> > > > > >           return;
> > > > > >       }
> > > > > >
> > > > > > +    if (!s->vnet_hdr &&
> > > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > > > +        /*
> > > > > > +         * filter mirror needs 'vnet_hdr_support' when colo filter
> modules
> > > > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > > > +         */
> > > > > > +        s->vnet_hdr = true;
> > > > > > +    }
> > > > > > +
> > > > > >       qemu_chr_fe_init(&s->chr_out, chr, errp);
> > > > > >   }
> > > > > >
> > > > > > @@ -252,6 +263,16 @@ static void
> > > > > > filter_redirector_setup(NetFilterState
> > > > > *nf, Error **errp)
> > > > > >           }
> > > > > >       }
> > > > > >
> > > > > > +    if (!s->vnet_hdr &&
> > > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > > > +        /*
> > > > > > +         * filter redirector needs 'vnet_hdr_support' when
> > > > > > + colo filter
> > > modules
> > > > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > > > +         */
> > > > > > +        s->vnet_hdr = true;
> > > > > > +    }
> > > > > > +
> > > > > >       net_socket_rs_init(&s->rs, redirector_rs_finalize,
> > > > > > s->vnet_hdr);
> > > > > >
> > > > > >       if (s->indev) {
> > > > > > diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
> > > > > > index
> > > > > > cb3a96cde1..637ef4ce71 100644
> > > > > > --- a/net/filter-rewriter.c
> > > > > > +++ b/net/filter-rewriter.c
> > > > > > @@ -388,6 +388,16 @@ static void
> > > > > > colo_rewriter_setup(NetFilterState *nf,
> > > > > Error **errp)
> > > > > >   {
> > > > > >       RewriterState *s = FILTER_REWRITER(nf);
> > > > > >
> > > > > > +    if (!s->vnet_hdr &&
> > > > > > +        qemu_opts_foreach(qemu_find_opts("device"),
> > > > > > +                          vnet_driver_check, nf->netdev_id, NULL)) {
> > > > > > +        /*
> > > > > > +         * filter rewriter needs 'vnet_hdr_support' when colo
> > > > > > + filter
> > > modules
> > > > > > +         * work on virtio-net, add 'vnet_hdr_support' automatically
> > > > > > +         */
> > > > > > +        s->vnet_hdr = true;
> > > > > > +    }
> > > > > > +
> > > > > >       s->connection_track_table =
> > > > > g_hash_table_new_full(connection_key_hash,
> > > > > >                                                         connection_key_equal,
> > > > > >
> > > > > > g_free, diff --git a/qapi/qom.json b/qapi/qom.json index
> > > > > > a25616bc7a..5760107160
> > > > > > 100644
> > > > > > --- a/qapi/qom.json
> > > > > > +++ b/qapi/qom.json
> > > > > > @@ -241,6 +241,11 @@
> > > > > >   # @notify_dev: name of the character device backend to be
> > > > > > used to
> > > > > communicate
> > > > > >   #              with the remote colo-frame (only for Xen COLO)
> > > > > >   #
> > > > > > +# @netdev: id of the network device backend to colo-compare.
> > > Although
> > > > > > +#          colo-compare doesn't depend on network device directly,
> this
> > > > > > +#          parameter helps colo-compare know what network driver it
> is
> > > > > > +#          working on.(since 6.2)
> > > > >
> > > > >
> > > > > This looks kind of not elegant. We don't want to couple COLO with
> netdev.
> > > > >
> > > > > I wonder if we can do this without adding netdev.
> > > >
> > > > Yes, current COLO is not bind to netdev, but here we want to auto
> > > > fix
> > > configuration issue.
> > >
> > > If my memory is correct, the current design allows an external
> > > comparing module to work.
> >
> > Yes, this netdev used by identify a series of comparing modules (include
> filters).
> >
> > >
> > > > To avoid wrong setting other non-colo related existing filter,
> > > > COLO need
> > > know the colo-effect netdev to setup.
> > > > And this parameter is optional, this patch will setup default
> > > > colo-effect
> > > netdev in colo_set_default_netdev().
> > > > Or any good idea?
> > >
> > > How about
> > >
> > > 1) adding a handshake in the protocol before receiving packets or
> > > 2) add a header to the payload to show if it contains a vnet header
> > > or
> > > 3) go back to let mgmt set vnet_hdr=on/off (looks like the first
> > > proposal but it avoids the coupling).
> >
> > Current code already have a header to the payload to show the vnet
> header(in net/filter-mirror.c filter_send()).
> > But the vnet_hdr_len is not necessary:
> > Disabled vnet_hdr payload:
> > 1. size ---> 2. Real network payload.
> > Enbaled vnet_hdr payload:
> > 1. size ---> 2. Vnet_hdr_len ----> 3. Real network payload.
> >
> > It looks we can define the vnet_hdr_len as necessary part of protocol.
> > If no vnet_hdr, we set the vnet_hdr_len to 0.
> > 1. size ---> 2. Vnet_hdr_len ----> 3. Real network payload.
> >
> > How about this proposal?
> 
> I think it should work.
> 

OK~ I will do it in the V4 patch.

Thanks your comments.

Thanks
Chen

> Thanks
> 
> >
> > Thanks
> > Chen
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks
> > > > Chen
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > > > +#
> > > > > >   # @compare_timeout: the maximum time to hold a packet from
> > > > > @primary_in for
> > > > > >   #                   comparison with an incoming packet on @secondary_in
> in
> > > > > >   #                   milliseconds (default: 3000)
> > > > > > @@ -264,6 +269,7 @@
> > > > > >               'outdev': 'str',
> > > > > >               'iothread': 'str',
> > > > > >               '*notify_dev': 'str',
> > > > > > +            '*netdev': 'str',
> > > > > >               '*compare_timeout': 'uint64',
> > > > > >               '*expired_scan_cycle': 'uint32',
> > > > > >               '*max_queue_size': 'uint32', diff --git
> > > > > > a/qemu-options.hx b/qemu-options.hx index
> > > > > > 8f603cc7e6..250937fbbf
> > > > > > 100644
> > > > > > --- a/qemu-options.hx
> > > > > > +++ b/qemu-options.hx
> > > > > > @@ -4964,12 +4964,14 @@ SRST
> > > > > >           stored. The file format is libpcap, so it can be analyzed with
> > > > > >           tools such as tcpdump or Wireshark.
> > > > > >
> > > > > > -    ``-object colo-
> > > > >
> > >
> compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=cha
> > > > > rd
> > > > > evid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_tim
> > > > > eout
> > > > > =@
> > > > >
> > >
> var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> > > > > > +    ``-object
> > > > > > + colo-
> > > > > compare,id=id,primary_in=chardevid,secondary_in=chardevid,outde
> > > > > > + v=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support]
> > > > > > + [,no
> > > > > > + tify
> > > > > > +
> > > > >
> > >
> _dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,
> > > > > m
> > > > > > + ax_queue_size=@var{size}]``
> > > > > >           Colo-compare gets packet from primary\_in chardevid and
> > > > > >           secondary\_in, then compare whether the payload of
> > > > > > primary
> > > packet
> > > > > >           and secondary packet are the same. If same, it will output
> > > > > >           primary packet to out\_dev, else it will notify
> > > > > > COLO-framework to
> > > do
> > > > > > -        checkpoint and send primary packet to out\_dev. In order to
> > > > > > +        checkpoint and send primary packet to out\_dev.
> > > > > > + Although
> > > > > > + colo-
> > > > > compare
> > > > > > +        does not depend on network device directly, netdevid helps
> > > > > > +        colo-compare know what network driver it is working on.
> > > > > > + In order to
> > > > > >           improve efficiency, we need to put the task of comparison in
> > > > > >           another iothread. If it has the vnet\_hdr\_support flag,
> > > > > >           colo compare will send/recv packet with vnet\_hdr\_len.
> > > >
> >

Patch

diff --git a/net/colo-compare.c b/net/colo-compare.c
index b100e7b51f..870bd05a41 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -110,6 +110,7 @@  struct CompareState {
     char *sec_indev;
     char *outdev;
     char *notify_dev;
+    char *netdev;
     CharBackend chr_pri_in;
     CharBackend chr_sec_in;
     CharBackend chr_out;
@@ -838,6 +839,28 @@  static int compare_chr_can_read(void *opaque)
     return COMPARE_READ_LEN_MAX;
 }
 
+static int colo_set_default_netdev(void *opaque, QemuOpts *opts, Error **errp)
+{
+    const char *colo_obj_type, *netdev_from_filter;
+    char **netdev = (char **)opaque;
+
+    colo_obj_type = qemu_opt_get(opts, "qom-type");
+
+    if (colo_obj_type &&
+        (strcmp(colo_obj_type, "filter-mirror") == 0 ||
+         strcmp(colo_obj_type, "filter-redirector") == 0 ||
+         strcmp(colo_obj_type, "filter-rewriter") == 0)) {
+        netdev_from_filter = qemu_opt_get(opts, "netdev");
+        if (*netdev == NULL) {
+            *netdev = g_strdup(netdev_from_filter);
+        } else if (strcmp(*netdev, netdev_from_filter) != 0) {
+            warn_report("%s is using a different netdev from other COLO "
+                        "component", colo_obj_type);
+        }
+    }
+    return 0;
+}
+
 /*
  * Called from the main thread on the primary for packets
  * arriving over the socket from the primary.
@@ -1050,6 +1073,21 @@  static void compare_set_vnet_hdr(Object *obj,
     s->vnet_hdr = value;
 }
 
+static char *compare_get_netdev(Object *obj, Error **errp)
+{
+    CompareState *s = COLO_COMPARE(obj);
+
+    return g_strdup(s->netdev);
+}
+
+static void compare_set_netdev(Object *obj, const char *value, Error **errp)
+{
+    CompareState *s = COLO_COMPARE(obj);
+
+    g_free(s->netdev);
+    s->netdev = g_strdup(value);
+}
+
 static char *compare_get_notify_dev(Object *obj, Error **errp)
 {
     CompareState *s = COLO_COMPARE(obj);
@@ -1274,6 +1312,12 @@  static void colo_compare_complete(UserCreatable *uc, Error **errp)
         max_queue_size = MAX_QUEUE_SIZE;
     }
 
+    if (!s->netdev) {
+        /* Set default netdev as the first colo netfilter found */
+        qemu_opts_foreach(qemu_find_opts("object"),
+                          colo_set_default_netdev, &s->netdev, NULL);
+    }
+
     if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
         !qemu_chr_fe_init(&s->chr_pri_in, chr, errp)) {
         return;
@@ -1289,6 +1333,16 @@  static void colo_compare_complete(UserCreatable *uc, Error **errp)
         return;
     }
 
+    if (!s->vnet_hdr &&
+        qemu_opts_foreach(qemu_find_opts("device"),
+                          vnet_driver_check, s->netdev, NULL)) {
+        /*
+         * colo compare needs 'vnet_hdr_support' when it works on virtio-net,
+         * add 'vnet_hdr_support' automatically
+         */
+        s->vnet_hdr = true;
+    }
+
     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);
 
@@ -1400,6 +1454,9 @@  static void colo_compare_init(Object *obj)
     s->vnet_hdr = false;
     object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
                              compare_set_vnet_hdr);
+    /* colo compare can't varify that netdev is correct */
+    object_property_add_str(obj, "netdev", compare_get_netdev,
+                            compare_set_netdev);
 }
 
 void colo_compare_cleanup(void)
diff --git a/net/colo.c b/net/colo.c
index 3a3e6e89a0..4a03780f45 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -243,3 +243,23 @@  bool connection_has_tracked(GHashTable *connection_track_table,
 
     return conn ? true : false;
 }
+
+/* check the network driver related to COLO, return 1 if it is virtio-net */
+int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp)
+{
+    const char *driver_type, *netdev_from_driver;
+    char *netdev_from_filter = (char *)opaque;
+
+    driver_type = qemu_opt_get(opts, "driver");
+    netdev_from_driver = qemu_opt_get(opts, "netdev");
+
+    if (!driver_type || !netdev_from_driver || !netdev_from_filter) {
+        return 0;
+    }
+
+    if (g_str_has_prefix(driver_type, "virtio-net") &&
+        strcmp(netdev_from_driver, netdev_from_filter) == 0) {
+        return 1;
+    }
+    return 0;
+}
diff --git a/net/colo.h b/net/colo.h
index d91cd245c4..d401fc76b6 100644
--- a/net/colo.h
+++ b/net/colo.h
@@ -18,6 +18,9 @@ 
 #include "qemu/jhash.h"
 #include "qemu/timer.h"
 #include "net/eth.h"
+#include "qemu/option.h"
+#include "qemu/option_int.h"
+#include "qemu/config-file.h"
 
 #define HASHTABLE_MAX_SIZE 16384
 
@@ -104,5 +107,6 @@  Packet *packet_new(const void *data, int size, int vnet_hdr_len);
 Packet *packet_new_nocopy(void *data, int size, int vnet_hdr_len);
 void packet_destroy(void *opaque, void *user_data);
 void packet_destroy_partial(void *opaque, void *user_data);
+int vnet_driver_check(void *opaque, QemuOpts *opts, Error **errp);
 
 #endif /* NET_COLO_H */
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index f20240cc9f..69ca9c9839 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -12,6 +12,7 @@ 
 #include "qemu/osdep.h"
 #include "net/filter.h"
 #include "net/net.h"
+#include "net/colo.h"
 #include "qapi/error.h"
 #include "qom/object.h"
 #include "qemu/main-loop.h"
@@ -224,6 +225,16 @@  static void filter_mirror_setup(NetFilterState *nf, Error **errp)
         return;
     }
 
+    if (!s->vnet_hdr &&
+        qemu_opts_foreach(qemu_find_opts("device"),
+                          vnet_driver_check, nf->netdev_id, NULL)) {
+        /*
+         * filter mirror needs 'vnet_hdr_support' when colo filter modules
+         * work on virtio-net, add 'vnet_hdr_support' automatically
+         */
+        s->vnet_hdr = true;
+    }
+
     qemu_chr_fe_init(&s->chr_out, chr, errp);
 }
 
@@ -252,6 +263,16 @@  static void filter_redirector_setup(NetFilterState *nf, Error **errp)
         }
     }
 
+    if (!s->vnet_hdr &&
+        qemu_opts_foreach(qemu_find_opts("device"),
+                          vnet_driver_check, nf->netdev_id, NULL)) {
+        /*
+         * filter redirector needs 'vnet_hdr_support' when colo filter modules
+         * work on virtio-net, add 'vnet_hdr_support' automatically
+         */
+        s->vnet_hdr = true;
+    }
+
     net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
 
     if (s->indev) {
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index cb3a96cde1..637ef4ce71 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -388,6 +388,16 @@  static void colo_rewriter_setup(NetFilterState *nf, Error **errp)
 {
     RewriterState *s = FILTER_REWRITER(nf);
 
+    if (!s->vnet_hdr &&
+        qemu_opts_foreach(qemu_find_opts("device"),
+                          vnet_driver_check, nf->netdev_id, NULL)) {
+        /*
+         * filter rewriter needs 'vnet_hdr_support' when colo filter modules
+         * work on virtio-net, add 'vnet_hdr_support' automatically
+         */
+        s->vnet_hdr = true;
+    }
+
     s->connection_track_table = g_hash_table_new_full(connection_key_hash,
                                                       connection_key_equal,
                                                       g_free,
diff --git a/qapi/qom.json b/qapi/qom.json
index a25616bc7a..5760107160 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -241,6 +241,11 @@ 
 # @notify_dev: name of the character device backend to be used to communicate
 #              with the remote colo-frame (only for Xen COLO)
 #
+# @netdev: id of the network device backend to colo-compare. Although
+#          colo-compare doesn't depend on network device directly, this
+#          parameter helps colo-compare know what network driver it is
+#          working on.(since 6.2)
+#
 # @compare_timeout: the maximum time to hold a packet from @primary_in for
 #                   comparison with an incoming packet on @secondary_in in
 #                   milliseconds (default: 3000)
@@ -264,6 +269,7 @@ 
             'outdev': 'str',
             'iothread': 'str',
             '*notify_dev': 'str',
+            '*netdev': 'str',
             '*compare_timeout': 'uint64',
             '*expired_scan_cycle': 'uint32',
             '*max_queue_size': 'uint32',
diff --git a/qemu-options.hx b/qemu-options.hx
index 8f603cc7e6..250937fbbf 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4964,12 +4964,14 @@  SRST
         stored. The file format is libpcap, so it can be analyzed with
         tools such as tcpdump or Wireshark.
 
-    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
+    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,netdev=netdevid][,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
         Colo-compare gets packet from primary\_in chardevid and
         secondary\_in, then compare whether the payload of primary packet
         and secondary packet are the same. If same, it will output
         primary packet to out\_dev, else it will notify COLO-framework to do
-        checkpoint and send primary packet to out\_dev. In order to
+        checkpoint and send primary packet to out\_dev. Although colo-compare
+        does not depend on network device directly, netdevid helps
+        colo-compare know what network driver it is working on. In order to
         improve efficiency, we need to put the task of comparison in
         another iothread. If it has the vnet\_hdr\_support flag,
         colo compare will send/recv packet with vnet\_hdr\_len.