[PATCH 0/7] Fixed some bugs and optimized some codes for COLO

[PATCH 0/7] Fixed some bugs and optimized some codes for COLO

[Lei Rao]

From: Rao, Lei <lei.rao@intel.com>

The series of patches include:
        Fixed some bugs of qemu crash and segment fault.
        Optimized the function of fill_connection_key.
        Remove some unnecessary code to improve COLO.

Rao, Lei (7):
  Some minor optimizations for COLO
  Fixed qemu crash when guest power off in COLO mode
  Fixed SVM hang when do failover before PVM crash
  colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff
  Removed the qemu_fclose() in colo_process_incoming_thread
  Changed the last-mode to none of first start COLO
  Optimized the function of fill_connection_key.

 migration/colo.c      | 20 +++++++-------------
 migration/migration.c |  6 +++++-
 net/colo-compare.c    |  4 ++--
 net/colo.c            | 31 ++++++++++++-------------------
 net/colo.h            |  6 +++---
 net/filter-rewriter.c | 10 +---------
 6 files changed, 30 insertions(+), 47 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] Some minor optimizations for COLO

[Lei Rao]

From: "Rao, Lei" <lei.rao@intel.com>

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/colo.c   | 2 +-
 net/colo-compare.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 79fa1f6..616dc00 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -152,7 +152,7 @@ static void primary_vm_do_failover(void)
      * kick COLO thread which might wait at
      * qemu_sem_wait(&s->colo_checkpoint_sem).
      */
-    colo_checkpoint_notify(migrate_get_current());
+    colo_checkpoint_notify(s);
 
     /*
      * Wake up COLO thread which may blocked in recv() or send(),
diff --git a/net/colo-compare.c b/net/colo-compare.c
index b100e7b..4a64a5d 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -170,7 +170,7 @@ static bool packet_matches_str(const char *str,
         return false;
     }
 
-    return !memcmp(str, buf, strlen(str));
+    return !memcmp(str, buf, packet_len);
 }
 
 static void notify_remote_frame(CompareState *s)
-- 
1.8.3.1

[PATCH 2/7] Fixed qemu crash when guest power off in COLO mode

[Lei Rao]

From: "Rao, Lei" <lei.rao@intel.com>

This patch fixes the following:
qemu-system-x86_64: invalid runstate transition: 'shutdown' -> 'running'
Aborted (core dumped)
The gdb bt as following:
0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
1  0x00007faa3d613859 in __GI_abort () at abort.c:79
2  0x000055c5a21268fd in runstate_set (new_state=RUN_STATE_RUNNING) at vl.c:723
3  0x000055c5a1f8cae4 in vm_prepare_start () at /home/workspace/colo-qemu/cpus.c:2206
4  0x000055c5a1f8cb1b in vm_start () at /home/workspace/colo-qemu/cpus.c:2213
5  0x000055c5a2332bba in migration_iteration_finish (s=0x55c5a4658810) at migration/migration.c:3376
6  0x000055c5a2332f3b in migration_thread (opaque=0x55c5a4658810) at migration/migration.c:3527
7  0x000055c5a251d68a in qemu_thread_start (args=0x55c5a5491a70) at util/qemu-thread-posix.c:519
8  0x00007faa3d7e9609 in start_thread (arg=<optimized out>) at pthread_create.c:477
9  0x00007faa3d710293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/migration.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 4228635..c2c84c7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3561,7 +3561,9 @@ static void migration_iteration_finish(MigrationState *s)
     case MIGRATION_STATUS_CANCELLED:
     case MIGRATION_STATUS_CANCELLING:
         if (s->vm_was_running) {
-            vm_start();
+            if (!runstate_check(RUN_STATE_SHUTDOWN)) {
+                vm_start();
+            }
         } else {
             if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
                 runstate_set(RUN_STATE_POSTMIGRATE);
-- 
1.8.3.1

[PATCH 3/7] Fixed SVM hang when do failover before PVM crash

[Lei Rao]

From: "Rao, Lei" <lei.rao@intel.com>

This patch fixed as follows:
    Thread 1 (Thread 0x7f34ee738d80 (LWP 11212)):
    #0 __pthread_clockjoin_ex (threadid=139847152957184, thread_return=0x7f30b1febf30, clockid=<optimized out>, abstime=<optimized out>, block=<optimized out>) at pthread_join_common.c:145
    #1 0x0000563401998e36 in qemu_thread_join (thread=0x563402d66610) at util/qemu-thread-posix.c:587
    #2 0x00005634017a79fa in process_incoming_migration_co (opaque=0x0) at migration/migration.c:502
    #3 0x00005634019b59c9 in coroutine_trampoline (i0=63395504, i1=22068) at util/coroutine-ucontext.c:115
    #4 0x00007f34ef860660 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91 from /lib/x86_64-linux-gnu/libc.so.6
    #5 0x00007f30b21ee730 in ?? ()
    #6 0x0000000000000000 in ?? ()

    Thread 13 (Thread 0x7f30b3dff700 (LWP 11747)):
    #0  __lll_lock_wait (futex=futex@entry=0x56340218ffa0 <qemu_global_mutex>, private=0) at lowlevellock.c:52
    #1  0x00007f34efa000a3 in _GI__pthread_mutex_lock (mutex=0x56340218ffa0 <qemu_global_mutex>) at ../nptl/pthread_mutex_lock.c:80
    #2  0x0000563401997f99 in qemu_mutex_lock_impl (mutex=0x56340218ffa0 <qemu_global_mutex>, file=0x563401b7a80e "migration/colo.c", line=806) at util/qemu-thread-posix.c:78
    #3  0x0000563401407144 in qemu_mutex_lock_iothread_impl (file=0x563401b7a80e "migration/colo.c", line=806) at /home/workspace/colo-qemu/cpus.c:1899
    #4  0x00005634017ba8e8 in colo_process_incoming_thread (opaque=0x563402d664c0) at migration/colo.c:806
    #5  0x0000563401998b72 in qemu_thread_start (args=0x5634039f8370) at util/qemu-thread-posix.c:519
    #6  0x00007f34ef9fd609 in start_thread (arg=<optimized out>) at pthread_create.c:477
    #7  0x00007f34ef924293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

    The QEMU main thread is holding the lock:
    (gdb) p qemu_global_mutex
    $1 = {lock = {_data = {lock = 2, __count = 0, __owner = 11212, __nusers = 9, __kind = 0, __spins = 0, __elision = 0, __list = {_prev = 0x0, __next = 0x0}},
     __size = "\002\000\000\000\000\000\000\000\314+\000\000\t", '\000' <repeats 26 times>, __align = 2}, file = 0x563401c07e4b "util/main-loop.c", line = 240,
    initialized = true}

From the call trace, we can see it is a deadlock bug. and the QEMU main thread holds the global mutex to wait until the COLO thread ends. and the colo thread
wants to acquire the global mutex, which will cause a deadlock. So, we should release the qemu_global_mutex before waiting colo thread ends.

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/migration.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index c2c84c7..6debb8b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -593,8 +593,10 @@ static void process_incoming_migration_co(void *opaque)
         mis->have_colo_incoming_thread = true;
         qemu_coroutine_yield();
 
+        qemu_mutex_unlock_iothread();
         /* Wait checkpoint incoming thread exit before free resource */
         qemu_thread_join(&mis->colo_incoming_thread);
+        qemu_mutex_lock_iothread();
         /* We hold the global iothread lock, so it is safe here */
         colo_release_ram_cache();
     }
-- 
1.8.3.1

[PATCH 4/7] colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff

[Lei Rao]

From: "Rao, Lei" <lei.rao@intel.com>

When a PVM completed its SVM failover steps and begins to run in
the simplex mode, QEMU would encounter a 'Segmentation fault' if
the guest poweroff with the following calltrace:

Program received signal SIGSEGV, Segmentation fault.

This is because primary_vm_do_failover() would call "qemu_file_shutdown
(s->rp_state.from_dst_file);" and later the migration_shutdown() would
do it again. So, we should set the s->rp_state.from_dst_file to NULL.

Signed-off-by: Like Xu <like.xu@linux.intel.com>
Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/colo.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 616dc00..c25e488 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -156,14 +156,15 @@ static void primary_vm_do_failover(void)
 
     /*
      * Wake up COLO thread which may blocked in recv() or send(),
-     * The s->rp_state.from_dst_file and s->to_dst_file may use the
-     * same fd, but we still shutdown the fd for twice, it is harmless.
+     * The s->to_dst_file may use the same fd, but we still shutdown
+     * the fd for twice, it is harmless.
      */
     if (s->to_dst_file) {
         qemu_file_shutdown(s->to_dst_file);
     }
     if (s->rp_state.from_dst_file) {
         qemu_file_shutdown(s->rp_state.from_dst_file);
+        s->rp_state.from_dst_file = NULL;
     }
 
     old_state = failover_set_state(FAILOVER_STATUS_ACTIVE,
-- 
1.8.3.1

[PATCH 5/7] Removed the qemu_fclose() in colo_process_incoming_thread

[Lei Rao]

From: "Rao, Lei" <lei.rao@intel.com>

After the live migration, the related fd will be cleanup in
migration_incoming_state_destroy(). So, the qemu_close()
in colo_process_incoming_thread is not necessary.

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/colo.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index c25e488..e2ab349 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -919,11 +919,6 @@ out:
     /* Hope this not to be too long to loop here */
     qemu_sem_wait(&mis->colo_incoming_sem);
     qemu_sem_destroy(&mis->colo_incoming_sem);
-    /* Must be called after failover BH is completed */
-    if (mis->to_src_file) {
-        qemu_fclose(mis->to_src_file);
-        mis->to_src_file = NULL;
-    }
 
     rcu_unregister_thread();
     return NULL;
-- 
1.8.3.1

[PATCH 6/7] Changed the last-mode to none of first start COLO

[Lei Rao]

From: "Rao, Lei" <lei.rao@intel.com>

When we first stated the COLO, the last-mode is as follows:
{ "execute": "query-colo-status" }
{"return": {"last-mode": "primary", "mode": "primary", "reason": "none"}}

The last-mode is unreasonable. After the patch, will be changed to the
following:
{ "execute": "query-colo-status" }
{"return": {"last-mode": "none", "mode": "primary", "reason": "none"}}

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/colo.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index e2ab349..69a2501 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -206,7 +206,7 @@ void colo_do_failover(void)
         vm_stop_force_state(RUN_STATE_COLO);
     }
 
-    switch (get_colo_mode()) {
+    switch (last_colo_mode = get_colo_mode()) {
     case COLO_MODE_PRIMARY:
         primary_vm_do_failover();
         break;
@@ -531,8 +531,7 @@ static void colo_process_checkpoint(MigrationState *s)
     Error *local_err = NULL;
     int ret;
 
-    last_colo_mode = get_colo_mode();
-    if (last_colo_mode != COLO_MODE_PRIMARY) {
+    if (get_colo_mode() != COLO_MODE_PRIMARY) {
         error_report("COLO mode must be COLO_MODE_PRIMARY");
         return;
     }
@@ -830,8 +829,7 @@ void *colo_process_incoming_thread(void *opaque)
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_COLO);
 
-    last_colo_mode = get_colo_mode();
-    if (last_colo_mode != COLO_MODE_SECONDARY) {
+    if (get_colo_mode() != COLO_MODE_SECONDARY) {
         error_report("COLO mode must be COLO_MODE_SECONDARY");
         return NULL;
     }
-- 
1.8.3.1

[PATCH 7/7] Optimized the function of fill_connection_key.

[Lei Rao]

From: "Rao, Lei" <lei.rao@intel.com>

Remove some unnecessary code to improve the performance of
the filter-rewriter module.

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 net/colo-compare.c    |  2 +-
 net/colo.c            | 31 ++++++++++++-------------------
 net/colo.h            |  6 +++---
 net/filter-rewriter.c | 10 +---------
 4 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 4a64a5d..6a1354d 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -264,7 +264,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
         pkt = NULL;
         return -1;
     }
-    fill_connection_key(pkt, &key);
+    fill_connection_key(pkt, &key, 0);
 
     conn = connection_get(s->connection_track_table,
                           &key,
diff --git a/net/colo.c b/net/colo.c
index 3a3e6e8..5e7232c 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -83,19 +83,26 @@ int parse_packet_early(Packet *pkt)
     return 0;
 }
 
-void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key, Packet *pkt)
+void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key,
+                         Packet *pkt, int reverse)
 {
+    if (reverse) {
+        key->src = pkt->ip->ip_dst;
+        key->dst = pkt->ip->ip_src;
+        key->src_port = ntohs(tmp_ports & 0xffff);
+        key->dst_port = ntohs(tmp_ports >> 16);
+    } else {
         key->src = pkt->ip->ip_src;
         key->dst = pkt->ip->ip_dst;
         key->src_port = ntohs(tmp_ports >> 16);
         key->dst_port = ntohs(tmp_ports & 0xffff);
+    }
 }
 
-void fill_connection_key(Packet *pkt, ConnectionKey *key)
+void fill_connection_key(Packet *pkt, ConnectionKey *key, int reverse)
 {
-    uint32_t tmp_ports;
+    uint32_t tmp_ports = 0;
 
-    memset(key, 0, sizeof(*key));
     key->ip_proto = pkt->ip->ip_p;
 
     switch (key->ip_proto) {
@@ -106,29 +113,15 @@ void fill_connection_key(Packet *pkt, ConnectionKey *key)
     case IPPROTO_SCTP:
     case IPPROTO_UDPLITE:
         tmp_ports = *(uint32_t *)(pkt->transport_header);
-        extract_ip_and_port(tmp_ports, key, pkt);
         break;
     case IPPROTO_AH:
         tmp_ports = *(uint32_t *)(pkt->transport_header + 4);
-        extract_ip_and_port(tmp_ports, key, pkt);
         break;
     default:
         break;
     }
-}
-
-void reverse_connection_key(ConnectionKey *key)
-{
-    struct in_addr tmp_ip;
-    uint16_t tmp_port;
-
-    tmp_ip = key->src;
-    key->src = key->dst;
-    key->dst = tmp_ip;
 
-    tmp_port = key->src_port;
-    key->src_port = key->dst_port;
-    key->dst_port = tmp_port;
+    extract_ip_and_port(tmp_ports, key, pkt, reverse);
 }
 
 Connection *connection_new(ConnectionKey *key)
diff --git a/net/colo.h b/net/colo.h
index d91cd24..5f4d502 100644
--- a/net/colo.h
+++ b/net/colo.h
@@ -89,9 +89,9 @@ typedef struct Connection {
 uint32_t connection_key_hash(const void *opaque);
 int connection_key_equal(const void *opaque1, const void *opaque2);
 int parse_packet_early(Packet *pkt);
-void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key, Packet *pkt);
-void fill_connection_key(Packet *pkt, ConnectionKey *key);
-void reverse_connection_key(ConnectionKey *key);
+void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key,
+                         Packet *pkt, int reverse);
+void fill_connection_key(Packet *pkt, ConnectionKey *key, int reverse);
 Connection *connection_new(ConnectionKey *key);
 void connection_destroy(void *opaque);
 Connection *connection_get(GHashTable *connection_track_table,
diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index cb3a96c..bf05023 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -279,15 +279,7 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
      */
     if (pkt && is_tcp_packet(pkt)) {
 
-        fill_connection_key(pkt, &key);
-
-        if (sender == nf->netdev) {
-            /*
-             * We need make tcp TX and RX packet
-             * into one connection.
-             */
-            reverse_connection_key(&key);
-        }
+        fill_connection_key(pkt, &key, sender == nf->netdev);
 
         /* After failover we needn't change new TCP packet */
         if (s->failover_mode &&
-- 
1.8.3.1

Re: [PATCH 3/7] Fixed SVM hang when do failover before PVM crash

[lizhijian]

On 17/06/2021 10:47, Lei Rao wrote:
> From: "Rao, Lei" <lei.rao@intel.com>
>
> This patch fixed as follows:
>      Thread 1 (Thread 0x7f34ee738d80 (LWP 11212)):
>      #0 __pthread_clockjoin_ex (threadid=139847152957184, thread_return=0x7f30b1febf30, clockid=<optimized out>, abstime=<optimized out>, block=<optimized out>) at pthread_join_common.c:145
>      #1 0x0000563401998e36 in qemu_thread_join (thread=0x563402d66610) at util/qemu-thread-posix.c:587
>      #2 0x00005634017a79fa in process_incoming_migration_co (opaque=0x0) at migration/migration.c:502
>      #3 0x00005634019b59c9 in coroutine_trampoline (i0=63395504, i1=22068) at util/coroutine-ucontext.c:115
>      #4 0x00007f34ef860660 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91 from /lib/x86_64-linux-gnu/libc.so.6
>      #5 0x00007f30b21ee730 in ?? ()
>      #6 0x0000000000000000 in ?? ()
>
>      Thread 13 (Thread 0x7f30b3dff700 (LWP 11747)):
>      #0  __lll_lock_wait (futex=futex@entry=0x56340218ffa0 <qemu_global_mutex>, private=0) at lowlevellock.c:52
>      #1  0x00007f34efa000a3 in _GI__pthread_mutex_lock (mutex=0x56340218ffa0 <qemu_global_mutex>) at ../nptl/pthread_mutex_lock.c:80
>      #2  0x0000563401997f99 in qemu_mutex_lock_impl (mutex=0x56340218ffa0 <qemu_global_mutex>, file=0x563401b7a80e "migration/colo.c", line=806) at util/qemu-thread-posix.c:78
>      #3  0x0000563401407144 in qemu_mutex_lock_iothread_impl (file=0x563401b7a80e "migration/colo.c", line=806) at /home/workspace/colo-qemu/cpus.c:1899
>      #4  0x00005634017ba8e8 in colo_process_incoming_thread (opaque=0x563402d664c0) at migration/colo.c:806
>      #5  0x0000563401998b72 in qemu_thread_start (args=0x5634039f8370) at util/qemu-thread-posix.c:519
>      #6  0x00007f34ef9fd609 in start_thread (arg=<optimized out>) at pthread_create.c:477
>      #7  0x00007f34ef924293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
>
>      The QEMU main thread is holding the lock:
>      (gdb) p qemu_global_mutex
>      $1 = {lock = {_data = {lock = 2, __count = 0, __owner = 11212, __nusers = 9, __kind = 0, __spins = 0, __elision = 0, __list = {_prev = 0x0, __next = 0x0}},
>       __size = "\002\000\000\000\000\000\000\000\314+\000\000\t", '\000' <repeats 26 times>, __align = 2}, file = 0x563401c07e4b "util/main-loop.c", line = 240,
>      initialized = true}
>
>  From the call trace, we can see it is a deadlock bug. and the QEMU main thread holds the global mutex to wait until the COLO thread ends. and the colo thread
> wants to acquire the global mutex, which will cause a deadlock. So, we should release the qemu_global_mutex before waiting colo thread ends.
>
> Signed-off-by: Lei Rao <lei.rao@intel.com>
Reviewed-by: Li Zhijian <lizhijian@cn.fujitsu.com>


> ---
>   migration/migration.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index c2c84c7..6debb8b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -593,8 +593,10 @@ static void process_incoming_migration_co(void *opaque)
>           mis->have_colo_incoming_thread = true;
>           qemu_coroutine_yield();
>   
> +        qemu_mutex_unlock_iothread();
>           /* Wait checkpoint incoming thread exit before free resource */
>           qemu_thread_join(&mis->colo_incoming_thread);
> +        qemu_mutex_lock_iothread();
>           /* We hold the global iothread lock, so it is safe here */
>           colo_release_ram_cache();
>       }

Re: [PATCH 1/7] Some minor optimizations for COLO

[Dr. David Alan Gilbert]

* Lei Rao (lei.rao@intel.com) wrote:
> From: "Rao, Lei" <lei.rao@intel.com>
> 
> Signed-off-by: Lei Rao <lei.rao@intel.com>
> ---

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


OK, although to be honest, I think the compiler will probably do this
for you.

>  migration/colo.c   | 2 +-
>  net/colo-compare.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index 79fa1f6..616dc00 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -152,7 +152,7 @@ static void primary_vm_do_failover(void)
>       * kick COLO thread which might wait at
>       * qemu_sem_wait(&s->colo_checkpoint_sem).
>       */
> -    colo_checkpoint_notify(migrate_get_current());
> +    colo_checkpoint_notify(s);
>  
>      /*
>       * Wake up COLO thread which may blocked in recv() or send(),
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index b100e7b..4a64a5d 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -170,7 +170,7 @@ static bool packet_matches_str(const char *str,
>          return false;
>      }
>  
> -    return !memcmp(str, buf, strlen(str));
> +    return !memcmp(str, buf, packet_len);
>  }
>  
>  static void notify_remote_frame(CompareState *s)
> -- 
> 1.8.3.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 4/7] colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2518 bytes --]

On Thu, 17 Jun 2021 10:47:12 +0800
Lei Rao <lei.rao@intel.com> wrote:

> From: "Rao, Lei" <lei.rao@intel.com>
> 
> When a PVM completed its SVM failover steps and begins to run in
> the simplex mode, QEMU would encounter a 'Segmentation fault' if
> the guest poweroff with the following calltrace:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 
> This is because primary_vm_do_failover() would call "qemu_file_shutdown
> (s->rp_state.from_dst_file);" and later the migration_shutdown() would
> do it again. So, we should set the s->rp_state.from_dst_file to NULL.

Hello,
Please provide a backtrace to such bugfixes. It helps the reviewers to
better understand the bug and the fix and it helps yourself to check if
it is actually correct. I suggest you to enable coredumps in your test
(or even production) system, so for every crash you definitely have a
backtrace.

> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> Signed-off-by: Lei Rao <lei.rao@intel.com>
> ---
>  migration/colo.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index 616dc00..c25e488 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -156,14 +156,15 @@ static void primary_vm_do_failover(void)
>  
>      /*
>       * Wake up COLO thread which may blocked in recv() or send(),
> -     * The s->rp_state.from_dst_file and s->to_dst_file may use the
> -     * same fd, but we still shutdown the fd for twice, it is harmless.
> +     * The s->to_dst_file may use the same fd, but we still shutdown
> +     * the fd for twice, it is harmless.
>       */

This change to the comment is incorrect. Shutting down a file multiple
times is safe and not a bug in it self. If it leads to a crash anyway,
that points to a bug in the qemu_file_shutdown() function or similar.

>      if (s->to_dst_file) {
>          qemu_file_shutdown(s->to_dst_file);
>      }
>      if (s->rp_state.from_dst_file) {
>          qemu_file_shutdown(s->rp_state.from_dst_file);
> +        s->rp_state.from_dst_file = NULL;
>      }

This is incorrect, as the file won't be closed after this and is
leaked. And indeed, when applying the patch to master, qemu crashes
when triggering failover on the primary, due to the leak:

qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance: Assertion `QLIST_EMPTY(&entry->yankfns)' failed.

>      old_state = failover_set_state(FAILOVER_STATUS_ACTIVE,



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 4/7] colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff

[Rao, Lei]

It's been a long time since this bug, I will reproduce it to get the GDB stack, but it may take some time.

Thanks,
Lei.

-----Original Message-----
From: Lukas Straub <lukasstraub2@web.de> 
Sent: Sunday, July 4, 2021 4:36 AM
To: Rao, Lei <lei.rao@intel.com>
Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; jasowang@redhat.com; zhang.zhanghailiang@huawei.com; quintela@redhat.com; dgilbert@redhat.com; like.xu.linux@gmail.com; qemu-devel@nongnu.org; Like Xu <like.xu@linux.intel.com>
Subject: Re: [PATCH 4/7] colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff

On Thu, 17 Jun 2021 10:47:12 +0800
Lei Rao <lei.rao@intel.com> wrote:

> From: "Rao, Lei" <lei.rao@intel.com>
> 
> When a PVM completed its SVM failover steps and begins to run in the 
> simplex mode, QEMU would encounter a 'Segmentation fault' if the guest 
> poweroff with the following calltrace:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 
> This is because primary_vm_do_failover() would call 
> "qemu_file_shutdown (s->rp_state.from_dst_file);" and later the 
> migration_shutdown() would do it again. So, we should set the s->rp_state.from_dst_file to NULL.

Hello,
Please provide a backtrace to such bugfixes. It helps the reviewers to better understand the bug and the fix and it helps yourself to check if it is actually correct. I suggest you to enable coredumps in your test (or even production) system, so for every crash you definitely have a backtrace.

> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> Signed-off-by: Lei Rao <lei.rao@intel.com>
> ---
>  migration/colo.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index 
> 616dc00..c25e488 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -156,14 +156,15 @@ static void primary_vm_do_failover(void)
>  
>      /*
>       * Wake up COLO thread which may blocked in recv() or send(),
> -     * The s->rp_state.from_dst_file and s->to_dst_file may use the
> -     * same fd, but we still shutdown the fd for twice, it is harmless.
> +     * The s->to_dst_file may use the same fd, but we still shutdown
> +     * the fd for twice, it is harmless.
>       */

This change to the comment is incorrect. Shutting down a file multiple times is safe and not a bug in it self. If it leads to a crash anyway, that points to a bug in the qemu_file_shutdown() function or similar.

>      if (s->to_dst_file) {
>          qemu_file_shutdown(s->to_dst_file);
>      }
>      if (s->rp_state.from_dst_file) {
>          qemu_file_shutdown(s->rp_state.from_dst_file);
> +        s->rp_state.from_dst_file = NULL;
>      }

This is incorrect, as the file won't be closed after this and is leaked. And indeed, when applying the patch to master, qemu crashes when triggering failover on the primary, due to the leak:

qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance: Assertion `QLIST_EMPTY(&entry->yankfns)' failed.

>      old_state = failover_set_state(FAILOVER_STATUS_ACTIVE,



-- 

RE: [PATCH 4/7] colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff

[Rao, Lei]

I can reproduce this bug successfully and the GDB stack is as follows:
Program terminated with signal SIGSEGV, Segmentation fault.
#0  object_class_dynamic_cast (class=0x55c8f5d2bf50, typename=0x55c8f2f7379e "qio-channel") at qom/object.c:832
832         if (type->class->interfaces &&
[Current thread is 1 (Thread 0x7f756e97eb00 (LWP 1811577))]
(gdb) bt
#0  object_class_dynamic_cast (class=0x55c8f5d2bf50, typename=0x55c8f2f7379e "qio-channel") at qom/object.c:832
#1  0x000055c8f2c3dd14 in object_dynamic_cast (obj=0x55c8f543ac00, typename=0x55c8f2f7379e "qio-channel") at qom/object.c:763
#2  0x000055c8f2c3ddce in object_dynamic_cast_assert (obj=0x55c8f543ac00, typename=0x55c8f2f7379e "qio-channel",
    file=0x55c8f2f73780 "migration/qemu-file-channel.c", line=117, func=0x55c8f2f73800 <__func__.18724> "channel_shutdown") at qom/object.c:786
#3  0x000055c8f2bbc6ac in channel_shutdown (opaque=0x55c8f543ac00, rd=true, wr=true, errp=0x0) at migration/qemu-file-channel.c:117
#4  0x000055c8f2bba56e in qemu_file_shutdown (f=0x7f7558070f50) at migration/qemu-file.c:67
#5  0x000055c8f2ba5373 in migrate_fd_cancel (s=0x55c8f4ccf3f0) at migration/migration.c:1699
#6  0x000055c8f2ba1992 in migration_shutdown () at migration/migration.c:187
#7  0x000055c8f29a5b77 in main (argc=69, argv=0x7fff3e9e8c08, envp=0x7fff3e9e8e38) at vl.c:4512

From the call trace we can see that the reason for core dump is due to QIOChannel *ioc = QIO_CHANNEL(opaque) in the channel_shutdown();
After analysis, I think what you said is right, Shutting down a file multiple times is safe and not a bug in it self. The reason for the segmentation fault 
Is that after doing failover, the COLO thread will qemu_fclose(s->rp_state.from_dst_file) in colo_process_checkpoint();
if we power off the guest at the same time. This will cause qemu_file_shutdown after qemu_close(from_dst_file). As a result, the qemu will crash.
So, I think the better way is as follows:
  /*
     * Must be called after failover BH is completed,
     * Or the failover BH may shutdown the wrong fd that
     * re-used by other threads after we release here.
     */
     if (s->rp_state.from_dst_file) {
            qemu_fclose(s->rp_state.from_dst_file);
+          s->rp_state.from_dst_file = NULL;
      }
After qemu_close(s->rp_state.from_dst_file), we set the from_dst_file = NULL;

Ways to reproduce bugs:
You can kill SVM, after executing the failover QMP command, immediately execute the power off command in the guest, which will have a high probability to reproduce this bug.

Thanks,
Lei.


-----Original Message-----
From: Rao, Lei 
Sent: Monday, July 5, 2021 10:54 AM
To: Lukas Straub <lukasstraub2@web.de>
Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; jasowang@redhat.com; zhang.zhanghailiang@huawei.com; quintela@redhat.com; dgilbert@redhat.com; like.xu.linux@gmail.com; qemu-devel@nongnu.org; Like Xu <like.xu@linux.intel.com>
Subject: RE: [PATCH 4/7] colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff

It's been a long time since this bug, I will reproduce it to get the GDB stack, but it may take some time.

Thanks,
Lei.

-----Original Message-----
From: Lukas Straub <lukasstraub2@web.de>
Sent: Sunday, July 4, 2021 4:36 AM
To: Rao, Lei <lei.rao@intel.com>
Cc: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com; jasowang@redhat.com; zhang.zhanghailiang@huawei.com; quintela@redhat.com; dgilbert@redhat.com; like.xu.linux@gmail.com; qemu-devel@nongnu.org; Like Xu <like.xu@linux.intel.com>
Subject: Re: [PATCH 4/7] colo: fixed 'Segmentation fault' when the simplex mode PVM poweroff

On Thu, 17 Jun 2021 10:47:12 +0800
Lei Rao <lei.rao@intel.com> wrote:

> From: "Rao, Lei" <lei.rao@intel.com>
> 
> When a PVM completed its SVM failover steps and begins to run in the 
> simplex mode, QEMU would encounter a 'Segmentation fault' if the guest 
> poweroff with the following calltrace:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 
> This is because primary_vm_do_failover() would call 
> "qemu_file_shutdown (s->rp_state.from_dst_file);" and later the
> migration_shutdown() would do it again. So, we should set the s->rp_state.from_dst_file to NULL.

Hello,
Please provide a backtrace to such bugfixes. It helps the reviewers to better understand the bug and the fix and it helps yourself to check if it is actually correct. I suggest you to enable coredumps in your test (or even production) system, so for every crash you definitely have a backtrace.

> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> Signed-off-by: Lei Rao <lei.rao@intel.com>
> ---
>  migration/colo.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index
> 616dc00..c25e488 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -156,14 +156,15 @@ static void primary_vm_do_failover(void)
>  
>      /*
>       * Wake up COLO thread which may blocked in recv() or send(),
> -     * The s->rp_state.from_dst_file and s->to_dst_file may use the
> -     * same fd, but we still shutdown the fd for twice, it is harmless.
> +     * The s->to_dst_file may use the same fd, but we still shutdown
> +     * the fd for twice, it is harmless.
>       */

This change to the comment is incorrect. Shutting down a file multiple times is safe and not a bug in it self. If it leads to a crash anyway, that points to a bug in the qemu_file_shutdown() function or similar.

>      if (s->to_dst_file) {
>          qemu_file_shutdown(s->to_dst_file);
>      }
>      if (s->rp_state.from_dst_file) {
>          qemu_file_shutdown(s->rp_state.from_dst_file);
> +        s->rp_state.from_dst_file = NULL;
>      }

This is incorrect, as the file won't be closed after this and is leaked. And indeed, when applying the patch to master, qemu crashes when triggering failover on the primary, due to the leak:

qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance: Assertion `QLIST_EMPTY(&entry->yankfns)' failed.

>      old_state = failover_set_state(FAILOVER_STATUS_ACTIVE,



-- 

RE: [PATCH 7/7] Optimized the function of fill_connection_key.

[Zhang, Chen]

> -----Original Message-----
> From: Rao, Lei <lei.rao@intel.com>
> Sent: Thursday, June 17, 2021 10:47 AM
> To: Zhang, Chen <chen.zhang@intel.com>; lizhijian@cn.fujitsu.com;
> jasowang@redhat.com; zhang.zhanghailiang@huawei.com;
> quintela@redhat.com; dgilbert@redhat.com; lukasstraub2@web.de
> Cc: like.xu.linux@gmail.com; qemu-devel@nongnu.org; Rao, Lei
> <lei.rao@intel.com>
> Subject: [PATCH 7/7] Optimized the function of fill_connection_key.
> 
> From: "Rao, Lei" <lei.rao@intel.com>
> 
> Remove some unnecessary code to improve the performance of the filter-
> rewriter module.
> 
> Signed-off-by: Lei Rao <lei.rao@intel.com>

Looks good to me.
Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
>  net/colo-compare.c    |  2 +-
>  net/colo.c            | 31 ++++++++++++-------------------
>  net/colo.h            |  6 +++---
>  net/filter-rewriter.c | 10 +---------
>  4 files changed, 17 insertions(+), 32 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 4a64a5d..6a1354d 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -264,7 +264,7 @@ static int packet_enqueue(CompareState *s, int
> mode, Connection **con)
>          pkt = NULL;
>          return -1;
>      }
> -    fill_connection_key(pkt, &key);
> +    fill_connection_key(pkt, &key, 0);
> 
>      conn = connection_get(s->connection_track_table,
>                            &key,
> diff --git a/net/colo.c b/net/colo.c
> index 3a3e6e8..5e7232c 100644
> --- a/net/colo.c
> +++ b/net/colo.c
> @@ -83,19 +83,26 @@ int parse_packet_early(Packet *pkt)
>      return 0;
>  }
> 
> -void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key, Packet
> *pkt)
> +void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key,
> +                         Packet *pkt, int reverse)
>  {
> +    if (reverse) {
> +        key->src = pkt->ip->ip_dst;
> +        key->dst = pkt->ip->ip_src;
> +        key->src_port = ntohs(tmp_ports & 0xffff);
> +        key->dst_port = ntohs(tmp_ports >> 16);
> +    } else {
>          key->src = pkt->ip->ip_src;
>          key->dst = pkt->ip->ip_dst;
>          key->src_port = ntohs(tmp_ports >> 16);
>          key->dst_port = ntohs(tmp_ports & 0xffff);
> +    }
>  }
> 
> -void fill_connection_key(Packet *pkt, ConnectionKey *key)
> +void fill_connection_key(Packet *pkt, ConnectionKey *key, int reverse)
>  {
> -    uint32_t tmp_ports;
> +    uint32_t tmp_ports = 0;
> 
> -    memset(key, 0, sizeof(*key));
>      key->ip_proto = pkt->ip->ip_p;
> 
>      switch (key->ip_proto) {
> @@ -106,29 +113,15 @@ void fill_connection_key(Packet *pkt,
> ConnectionKey *key)
>      case IPPROTO_SCTP:
>      case IPPROTO_UDPLITE:
>          tmp_ports = *(uint32_t *)(pkt->transport_header);
> -        extract_ip_and_port(tmp_ports, key, pkt);
>          break;
>      case IPPROTO_AH:
>          tmp_ports = *(uint32_t *)(pkt->transport_header + 4);
> -        extract_ip_and_port(tmp_ports, key, pkt);
>          break;
>      default:
>          break;
>      }
> -}
> -
> -void reverse_connection_key(ConnectionKey *key) -{
> -    struct in_addr tmp_ip;
> -    uint16_t tmp_port;
> -
> -    tmp_ip = key->src;
> -    key->src = key->dst;
> -    key->dst = tmp_ip;
> 
> -    tmp_port = key->src_port;
> -    key->src_port = key->dst_port;
> -    key->dst_port = tmp_port;
> +    extract_ip_and_port(tmp_ports, key, pkt, reverse);
>  }
> 
>  Connection *connection_new(ConnectionKey *key) diff --git a/net/colo.h
> b/net/colo.h index d91cd24..5f4d502 100644
> --- a/net/colo.h
> +++ b/net/colo.h
> @@ -89,9 +89,9 @@ typedef struct Connection {  uint32_t
> connection_key_hash(const void *opaque);  int
> connection_key_equal(const void *opaque1, const void *opaque2);  int
> parse_packet_early(Packet *pkt); -void extract_ip_and_port(uint32_t
> tmp_ports, ConnectionKey *key, Packet *pkt); -void
> fill_connection_key(Packet *pkt, ConnectionKey *key); -void
> reverse_connection_key(ConnectionKey *key);
> +void extract_ip_and_port(uint32_t tmp_ports, ConnectionKey *key,
> +                         Packet *pkt, int reverse); void
> +fill_connection_key(Packet *pkt, ConnectionKey *key, int reverse);
>  Connection *connection_new(ConnectionKey *key);  void
> connection_destroy(void *opaque);  Connection
> *connection_get(GHashTable *connection_track_table, diff --git
> a/net/filter-rewriter.c b/net/filter-rewriter.c index cb3a96c..bf05023 100644
> --- a/net/filter-rewriter.c
> +++ b/net/filter-rewriter.c
> @@ -279,15 +279,7 @@ static ssize_t
> colo_rewriter_receive_iov(NetFilterState *nf,
>       */
>      if (pkt && is_tcp_packet(pkt)) {
> 
> -        fill_connection_key(pkt, &key);
> -
> -        if (sender == nf->netdev) {
> -            /*
> -             * We need make tcp TX and RX packet
> -             * into one connection.
> -             */
> -            reverse_connection_key(&key);
> -        }
> +        fill_connection_key(pkt, &key, sender == nf->netdev);
> 
>          /* After failover we needn't change new TCP packet */
>          if (s->failover_mode &&
> --
> 1.8.3.1