Re: [Qemu-devel] [PATCH V2 2/3] colo-compare: track connection and enqueue packet

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Li Zhijian (lizhijian@cn.fujitsu.com) wrote:
> 
> 
> On 03/30/2016 06:36 PM, Dr. David Alan Gilbert wrote:
> >* Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:

...

> >>+static inline void colo_flush_connection(void *opaque, void *user_data)
> >>+{
> >
> >Is this used?
> Yes, it isn't used currently.
> Actually this is needed after compare module is integrated to COLO frame.

OK, leave it out for the moment; I think some compilers complain when
you compile with an unused static.

Dave

> 
> 
> >
> >>+    Connection *conn = opaque;
> >>+    Packet *pkt = NULL;
> >>+
> >>+    qemu_mutex_lock(&conn->list_lock);
> >>+    while (!g_queue_is_empty(&conn->primary_list)) {
> >>+        pkt = g_queue_pop_head(&conn->primary_list);
> >>+        compare_chr_send(pkt->s->chr_out, pkt->data, pkt->size);
> >>+        /* FIXME: destroy pkt ?*/
> >>+    }
> >>+    while (!g_queue_is_empty(&conn->secondary_list)) {
> >>+        pkt = g_queue_pop_head(&conn->secondary_list);
> >>+        packet_destroy(pkt, NULL);
> >>+    }
> >>+    qemu_mutex_unlock(&conn->list_lock);
> >>+}
> >>+
> >>  static int compare_chr_send(CharDriverState *out, const uint8_t *buf, int size)
> >>  {
> >>      int ret = 0;
> >>@@ -142,8 +431,10 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
> >>
> >>      ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
> >>      if (ret == 1) {
> >>-        /* FIXME: enqueue to primary packet list */
> >>-        compare_chr_send(s->chr_out, buf, size);
> >>+        if (packet_enqueue(s, PRIMARY_IN)) {
> >>+            error_report("primary: unsupported packet in");
> >
> >Is this for non-IP packets?  If so you don't want an error_report - because non-IP are
> >quite common; a trace would be useful giving the packet type etc
> Agree. And further more, IMO release the packet to client is not always correct for all non-IP
> but at current stage, this looks fine.
> 
> Thanks
> Li Zhijian
> 
> >
> >>+            compare_chr_send(s->chr_out, buf, size);
> >>+        }
> >>      } else if (ret == -1) {
> >>          qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> >>      }
> >>@@ -156,7 +447,9 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
> >>
> >>      ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
> >>      if (ret == 1) {
> >>-        /* TODO: enqueue to secondary packet list*/
> >>+        if (packet_enqueue(s, SECONDARY_IN)) {
> >>+            error_report("secondary: unsupported packet in");
> >>+        }
> >>      } else if (ret == -1) {
> >>          qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> >>      }
> >>@@ -210,6 +503,7 @@ static void compare_set_outdev(Object *obj, const char *value, Error **errp)
> >>  static void colo_compare_complete(UserCreatable *uc, Error **errp)
> >>  {
> >>      CompareState *s = COLO_COMPARE(uc);
> >>+    struct sysinfo si;
> >>
> >>      if (!s->pri_indev || !s->sec_indev || !s->outdev) {
> >>          error_setg(errp, "colo compare needs 'primary_in' ,"
> >>@@ -255,6 +549,29 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> >>
> >>      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> >>
> >>+    g_queue_init(&s->conn_list);
> >>+    qemu_mutex_init(&s->conn_list_lock);
> >>+
> >>+    s->hashtable_size = 0;
> >>+    /*
> >>+     * Idea from kernel tcp.c: use 1/16384 of memory.  On i386: 32MB
> >>+     * machine has 512 buckets. >= 1GB machines have 16384 buckets.
> >>+     */
> >>+    sysinfo(&si);
> >>+    hashtable_max_size = si.totalram / 16384;
> >>+    if (si.totalram > (1024 * 1024 * 1024 / PAGE_SIZE)) {
> >>+        hashtable_max_size = 16384;
> >>+    }
> >>+    if (hashtable_max_size < 32) {
> >>+        hashtable_max_size = 32;
> >>+    }
> >>+    hashtable_max_size = hashtable_max_size * 8; /* default factor = 8 */
> >
> >Make this a lot simpler; just pick a size and if it's a problem then we'll worry
> >about it later, or make it an option on the filter if you want it changeable.
> >
> >>+    s->connection_track_table = g_hash_table_new_full(connection_key_hash,
> >>+                                                      connection_key_equal,
> >>+                                                      g_free,
> >>+                                                      connection_destroy);
> >>+
> >>      return;
> >>
> >>  out:
> >>@@ -297,6 +614,7 @@ static void colo_compare_class_finalize(ObjectClass *oc, void *data)
> >>      if (!QTAILQ_EMPTY(&net_compares)) {
> >>          QTAILQ_REMOVE(&net_compares, s, next);
> >>      }
> >>+    qemu_mutex_destroy(&s->conn_list_lock);
> >>  }
> >>
> >>  static void colo_compare_init(Object *obj)
> >>--
> >>1.9.1
> >>
> >>
> >>
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> >.
> >
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK