RE: [PATCH V6 4/6] net/colo-compare: Move data structure and define to .h file.

["Zhang, Chen" <chen.zhang@intel.com>]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Tuesday, May 18, 2021 4:04 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
> Gilbert <dgilbert@redhat.com>; Markus Armbruster <armbru@redhat.com>;
> Daniel P. Berrangé <berrange@redhat.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Zhang Chen
> <zhangckid@gmail.com>
> Subject: Re: [PATCH V6 4/6] net/colo-compare: Move data structure and
> define to .h file.
> 
> On Tue, 20 Apr 2021 23:15:35 +0800
> Zhang Chen <chen.zhang@intel.com> wrote:
> 
> > Rename structure with COLO index and move it to .h file, It make other
> > modules can reuse COLO code.
> 
> Hi,
> There are some definitions that don't need to be moved into the header,
> more comments below.
> 

OK.

> In general I think the new passthrough feature can be exclusive to colo-
> compare for now and that everything can remain there. If other net filters
> implement the feature, we can still move it outside of colo-compare later.
> 

Agree, This patch move some code to colo-compare.h, it still in colo-compare.

Thanks
Chen

> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  net/colo-compare.c | 134
> > +++++----------------------------------------
> >  net/colo-compare.h | 106 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 120 insertions(+), 120 deletions(-)
> >
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 9d1ad99941..b51b1437ef 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -17,44 +17,24 @@
> >  #include "qemu/error-report.h"
> >  #include "trace.h"
> >  #include "qapi/error.h"
> > -#include "net/net.h"
> >  #include "net/eth.h"
> >  #include "qom/object_interfaces.h"
> >  #include "qemu/iov.h"
> >  #include "qom/object.h"
> >  #include "net/queue.h"
> > -#include "chardev/char-fe.h"
> >  #include "qemu/sockets.h"
> > -#include "colo.h"
> > -#include "sysemu/iothread.h"
> >  #include "net/colo-compare.h"
> > -#include "migration/colo.h"
> > -#include "migration/migration.h"
> >  #include "util.h"
> >
> >  #include "block/aio-wait.h"
> >  #include "qemu/coroutine.h"
> >
> > -#define TYPE_COLO_COMPARE "colo-compare"
> > -typedef struct CompareState CompareState;
> > -DECLARE_INSTANCE_CHECKER(CompareState, COLO_COMPARE,
> > -                         TYPE_COLO_COMPARE)
> > -
> >  static QTAILQ_HEAD(, CompareState) net_compares =
> >         QTAILQ_HEAD_INITIALIZER(net_compares);
> >
> >  static NotifierList colo_compare_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(colo_compare_notifiers);
> >
> > -#define COMPARE_READ_LEN_MAX NET_BUFSIZE -#define
> MAX_QUEUE_SIZE 1024
> > -
> > -#define COLO_COMPARE_FREE_PRIMARY     0x01
> > -#define COLO_COMPARE_FREE_SECONDARY   0x02
> > -
> > -#define REGULAR_PACKET_CHECK_MS 1000
> > -#define DEFAULT_TIME_OUT_MS 3000
> > -
> 
> These 6 defines should stay here.
> 
> >  /* #define DEBUG_COLO_PACKETS */
> >
> >  static QemuMutex colo_compare_mutex;
> > @@ -64,92 +44,6 @@ static QemuCond event_complete_cond;  static int
> > event_unhandled_count;  static uint32_t max_queue_size;
> >
> > -/*
> > - *  + CompareState ++
> > - *  |               |
> > - *  +---------------+   +---------------+         +---------------+
> > - *  |   conn list   + - >      conn     + ------- >      conn     + -- > ......
> > - *  +---------------+   +---------------+         +---------------+
> > - *  |               |     |           |             |          |
> > - *  +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
> > - *                    |primary |  |secondary    |primary | |secondary
> > - *                    |packet  |  |packet  +    |packet  | |packet  +
> > - *                    +--------+  +--------+    +--------+ +--------+
> > - *                        |           |             |          |
> > - *                    +---v----+  +---v----+    +---v----+ +---v----+
> > - *                    |primary |  |secondary    |primary | |secondary
> > - *                    |packet  |  |packet  +    |packet  | |packet  +
> > - *                    +--------+  +--------+    +--------+ +--------+
> > - *                        |           |             |          |
> > - *                    +---v----+  +---v----+    +---v----+ +---v----+
> > - *                    |primary |  |secondary    |primary | |secondary
> > - *                    |packet  |  |packet  +    |packet  | |packet  +
> > - *                    +--------+  +--------+    +--------+ +--------+
> > - */
> > -
> > -typedef struct SendCo {
> > -    Coroutine *co;
> > -    struct CompareState *s;
> > -    CharBackend *chr;
> > -    GQueue send_list;
> > -    bool notify_remote_frame;
> > -    bool done;
> > -    int ret;
> > -} SendCo;
> 
> This struct should stay here.
> 
> > -typedef struct SendEntry {
> > -    uint32_t size;
> > -    uint32_t vnet_hdr_len;
> > -    uint8_t *buf;
> > -} SendEntry;
> 
> This struct should stay here.
> 
> > -struct CompareState {
> > -    Object parent;
> > -
> > -    char *pri_indev;
> > -    char *sec_indev;
> > -    char *outdev;
> > -    char *notify_dev;
> > -    CharBackend chr_pri_in;
> > -    CharBackend chr_sec_in;
> > -    CharBackend chr_out;
> > -    CharBackend chr_notify_dev;
> > -    SocketReadState pri_rs;
> > -    SocketReadState sec_rs;
> > -    SocketReadState notify_rs;
> > -    SendCo out_sendco;
> > -    SendCo notify_sendco;
> > -    bool vnet_hdr;
> > -    uint64_t compare_timeout;
> > -    uint32_t expired_scan_cycle;
> > -
> > -    /*
> > -     * Record the connection that through the NIC
> > -     * Element type: Connection
> > -     */
> > -    GQueue conn_list;
> > -    /* Record the connection without repetition */
> > -    GHashTable *connection_track_table;
> > -
> > -    IOThread *iothread;
> > -    GMainContext *worker_context;
> > -    QEMUTimer *packet_check_timer;
> > -
> > -    QEMUBH *event_bh;
> > -    enum colo_event event;
> > -
> > -    QTAILQ_ENTRY(CompareState) next;
> > -};
> > -
> > -typedef struct CompareClass {
> > -    ObjectClass parent_class;
> > -} CompareClass;
> > -
> > -enum {
> > -    PRIMARY_IN = 0,
> > -    SECONDARY_IN,
> > -};
> 
> The enum should stay here.
> 
> >  static const char *colo_mode[] = {
> >      [PRIMARY_IN] = "primary",
> >      [SECONDARY_IN] = "secondary",
> > @@ -737,19 +631,19 @@ static void colo_compare_connection(void
> > *opaque, void *user_data)
> >
> >  static void coroutine_fn _compare_chr_send(void *opaque)  {
> > -    SendCo *sendco = opaque;
> > +    COLOSendCo *sendco = opaque;
> >      CompareState *s = sendco->s;
> >      int ret = 0;
> >
> >      while (!g_queue_is_empty(&sendco->send_list)) {
> > -        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > +        COLOSendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> >          uint32_t len = htonl(entry->size);
> >
> >          ret = qemu_chr_fe_write_all(sendco->chr, (uint8_t *)&len,
> > sizeof(len));
> >
> >          if (ret != sizeof(len)) {
> >              g_free(entry->buf);
> > -            g_slice_free(SendEntry, entry);
> > +            g_slice_free(COLOSendEntry, entry);
> >              goto err;
> >          }
> >
> > @@ -766,7 +660,7 @@ static void coroutine_fn _compare_chr_send(void
> > *opaque)
> >
> >              if (ret != sizeof(len)) {
> >                  g_free(entry->buf);
> > -                g_slice_free(SendEntry, entry);
> > +                g_slice_free(COLOSendEntry, entry);
> >                  goto err;
> >              }
> >          }
> > @@ -777,12 +671,12 @@ static void coroutine_fn
> _compare_chr_send(void
> > *opaque)
> >
> >          if (ret != entry->size) {
> >              g_free(entry->buf);
> > -            g_slice_free(SendEntry, entry);
> > +            g_slice_free(COLOSendEntry, entry);
> >              goto err;
> >          }
> >
> >          g_free(entry->buf);
> > -        g_slice_free(SendEntry, entry);
> > +        g_slice_free(COLOSendEntry, entry);
> >      }
> >
> >      sendco->ret = 0;
> > @@ -790,9 +684,9 @@ static void coroutine_fn _compare_chr_send(void
> > *opaque)
> >
> >  err:
> >      while (!g_queue_is_empty(&sendco->send_list)) {
> > -        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > +        COLOSendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> >          g_free(entry->buf);
> > -        g_slice_free(SendEntry, entry);
> > +        g_slice_free(COLOSendEntry, entry);
> >      }
> >      sendco->ret = ret < 0 ? ret : -EIO;
> >  out:
> > @@ -808,8 +702,8 @@ static int compare_chr_send(CompareState *s,
> >                              bool notify_remote_frame,
> >                              bool zero_copy)  {
> > -    SendCo *sendco;
> > -    SendEntry *entry;
> > +    COLOSendCo *sendco;
> > +    COLOSendEntry *entry;
> >
> >      if (notify_remote_frame) {
> >          sendco = &s->notify_sendco;
> > @@ -821,7 +715,7 @@ static int compare_chr_send(CompareState *s,
> >          return 0;
> >      }
> >
> > -    entry = g_slice_new(SendEntry);
> > +    entry = g_slice_new(COLOSendEntry);
> >      entry->size = size;
> >      entry->vnet_hdr_len = vnet_hdr_len;
> >      if (zero_copy) {
> > @@ -1274,17 +1168,17 @@ static void
> > colo_compare_complete(UserCreatable *uc, Error **errp)
> >
> >      if (!s->compare_timeout) {
> >          /* Set default value to 3000 MS */
> > -        s->compare_timeout = DEFAULT_TIME_OUT_MS;
> > +        s->compare_timeout = COLO_DEFAULT_TIME_OUT_MS;
> >      }
> >
> >      if (!s->expired_scan_cycle) {
> >          /* Set default value to 3000 MS */
> > -        s->expired_scan_cycle = REGULAR_PACKET_CHECK_MS;
> > +        s->expired_scan_cycle = COLO_REGULAR_PACKET_CHECK_MS;
> >      }
> >
> >      if (!max_queue_size) {
> >          /* Set default queue size to 1024 */
> > -        max_queue_size = MAX_QUEUE_SIZE;
> > +        max_queue_size = MAX_COLO_QUEUE_SIZE;
> >      }
> >
> >      if (find_and_check_chardev(&chr, s->pri_indev, errp) || diff
> > --git a/net/colo-compare.h b/net/colo-compare.h index
> > 22ddd512e2..ab649c9dbe 100644
> > --- a/net/colo-compare.h
> > +++ b/net/colo-compare.h
> > @@ -17,6 +17,112 @@
> >  #ifndef QEMU_COLO_COMPARE_H
> >  #define QEMU_COLO_COMPARE_H
> >
> > +#include "net/net.h"
> > +#include "chardev/char-fe.h"
> > +#include "migration/colo.h"
> > +#include "migration/migration.h"
> > +#include "sysemu/iothread.h"
> > +#include "colo.h"
> > +
> > +#define TYPE_COLO_COMPARE "colo-compare"
> > +typedef struct CompareState CompareState;
> > +DECLARE_INSTANCE_CHECKER(CompareState, COLO_COMPARE,
> > +                         TYPE_COLO_COMPARE)
> > +
> > +#define COMPARE_READ_LEN_MAX NET_BUFSIZE #define
> MAX_COLO_QUEUE_SIZE
> > +1024
> > +
> > +#define COLO_COMPARE_FREE_PRIMARY     0x01
> > +#define COLO_COMPARE_FREE_SECONDARY   0x02
> > +
> > +#define COLO_REGULAR_PACKET_CHECK_MS 1000 #define
> > +COLO_DEFAULT_TIME_OUT_MS 3000
> > +
> > +typedef struct COLOSendCo {
> > +    Coroutine *co;
> > +    struct CompareState *s;
> > +    CharBackend *chr;
> > +    GQueue send_list;
> > +    bool notify_remote_frame;
> > +    bool done;
> > +    int ret;
> > +} COLOSendCo;
> > +
> > +typedef struct COLOSendEntry {
> > +    uint32_t size;
> > +    uint32_t vnet_hdr_len;
> > +    uint8_t *buf;
> > +} COLOSendEntry;
> > +
> > +/*
> > + *  + CompareState ++
> > + *  |               |
> > + *  +---------------+   +---------------+         +---------------+
> > + *  |   conn list   + - >      conn     + ------- >      conn     + -- > ......
> > + *  +---------------+   +---------------+         +---------------+
> > + *  |               |     |           |             |          |
> > + *  +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
> > + *                    |primary |  |secondary    |primary | |secondary
> > + *                    |packet  |  |packet  +    |packet  | |packet  +
> > + *                    +--------+  +--------+    +--------+ +--------+
> > + *                        |           |             |          |
> > + *                    +---v----+  +---v----+    +---v----+ +---v----+
> > + *                    |primary |  |secondary    |primary | |secondary
> > + *                    |packet  |  |packet  +    |packet  | |packet  +
> > + *                    +--------+  +--------+    +--------+ +--------+
> > + *                        |           |             |          |
> > + *                    +---v----+  +---v----+    +---v----+ +---v----+
> > + *                    |primary |  |secondary    |primary | |secondary
> > + *                    |packet  |  |packet  +    |packet  | |packet  +
> > + *                    +--------+  +--------+    +--------+ +--------+
> > + */
> > +struct CompareState {
> > +    Object parent;
> > +
> > +    char *pri_indev;
> > +    char *sec_indev;
> > +    char *outdev;
> > +    char *notify_dev;
> > +    CharBackend chr_pri_in;
> > +    CharBackend chr_sec_in;
> > +    CharBackend chr_out;
> > +    CharBackend chr_notify_dev;
> > +    SocketReadState pri_rs;
> > +    SocketReadState sec_rs;
> > +    SocketReadState notify_rs;
> > +    COLOSendCo out_sendco;
> > +    COLOSendCo notify_sendco;
> > +    bool vnet_hdr;
> > +    uint64_t compare_timeout;
> > +    uint32_t expired_scan_cycle;
> > +
> > +    /*
> > +     * Record the connection that through the NIC
> > +     * Element type: Connection
> > +     */
> > +    GQueue conn_list;
> > +    /* Record the connection without repetition */
> > +    GHashTable *connection_track_table;
> > +
> > +    IOThread *iothread;
> > +    GMainContext *worker_context;
> > +    QEMUTimer *packet_check_timer;
> > +
> > +    QEMUBH *event_bh;
> > +    enum colo_event event;
> > +
> > +    QTAILQ_ENTRY(CompareState) next;
> > +};
> > +
> > +typedef struct CompareClass {
> > +    ObjectClass parent_class;
> > +} CompareClass;
> > +
> > +enum {
> > +    PRIMARY_IN = 0,
> > +    SECONDARY_IN,
> > +};
> > +
> >  void colo_notify_compares_event(void *opaque, int event, Error
> > **errp);  void colo_compare_register_notifier(Notifier *notify);  void
> > colo_compare_unregister_notifier(Notifier *notify);
> 
> 
> 
> --