QEMU-gvm-vcpupin: Infinity loop if qemu_rdma_connect is failed

[ChoKyuWon]

while (1) {
    rdma = g_new0(RDMAContext, 1);
    rdma->current_index = -1;
    rdma->current_chunk = -1;
    rdma->port = atoi(addr->port);
    rdma->host = g_strdup(addr->host);
    qemu_rdma_source_init(rdma, &local_err, true);
    ret = qemu_rdma_connect(rdma, &local_err);
    if (ret == 0) {
        break;
    }
    printf("RDMA connect to %s:%s fail, retrying...\n", addr->host, addr->port);
    g_free(rdma);
    sleep(1);
}

This is part of qemu_rdma_build_outcoming_file function, in QEMU-gvm-vcpupin/migration/rdma.c. As you can see in the code, there's no escape condition if qemu_rdma_connect is always failed.

So I suggest making a limitation of number of attempt and if it failed over $LIMIT, just abort the entire process. I have no idea how to set the limitation number, but in my opinion, it can be set by compile-time by macro, like this: #define RDMA_CONNECT_TRAIAL_LIMIT 100.

[YWHyuk]

Nice report! But I think it is too many trial... I will kill -SIGKILL rather than waiting 100 seconds...

How about getting RDMA_CONNECT_TRAIAL_LIMIT as a argument? It seems like we can add new argument with some modification below

static QemuOptsList qemu_local_cpu_opts = {
    .name = "local-cpu",                                                                                                                                                                      
    .implied_opt_name = "cpus",
    .merge_lists = true,
    .head = QTAILQ_HEAD_INITIALIZER(qemu_local_cpu_opts.head),
    .desc = {  
        {         
            .name = "cpus",
            .type = QEMU_OPT_NUMBER,
            .help = "number of local CPUs",
        },{   
            .name = "start",
            .type = QEMU_OPT_NUMBER,
            .help = "start index of local CPUs",
        },{   
            .name = "iplist",
            .type = QEMU_OPT_STRING,
            .help = "list of cluster node ip address (seperated by space)",
        },    
        { /* end of list */ }
    },        
};

            case QEMU_OPTION_local_cpu:
                opts = qemu_opts_parse_noisily(qemu_find_opts("local-cpu"), optarg,                                                                                                           
                                               true);
                if (!opts) {
                    exit(1);
                }        
                local_cpus = qemu_opt_get_number(opts, "cpus", 1);
                local_cpu_start_index = qemu_opt_get_number(opts, "start", 0);
                cluster_iplist = qemu_opt_get(opts, "iplist");
                if (parse_cluster_iplist(cluster_iplist)) {
                    error_report("iplist parse failed");
                    exit(1);
                }        
                break;

[ChoKyuWon]

@YWHyuk Thanks for reviewing!

However, I think passing the maximum trial number as an argument is not good idea, because it's so minor to decide it for QEMU user. I mean, it's not dependent on QEMU user, but it depends on the system administrator.

So I think the best solution is system administrator can decide the RDMA_CONNECT_TRAIAL_LIMIT value at compile-time. For example, the administrator can build qemu by this command: ../configure --rdma-trial=10 && make -j 10.

I think this work needs some modification on configure script, but I'm not expert on C build system, so I didn't put effort into that work.

Maybe this kind of code is helpful, but I'm not sure. C code:

#ifndef RDMA_CONNECT_TRAIAL_LIMIT
#define RDMA_CONNECT_TRAIAL_LIMIT 100
#endif

compile option:

$(CC) rdma.c -c -o rdma.o $(CFLAGS) -DRDMA_CONNECT_TRAIAL_LIMIT=$(ADMIN_SET_VALUE)

[YWHyuk]

@ChoKyuWon I agree that we need some method to quit the process.

Trial is good idea. But most of users will kill the process instead of waiting. So we need more aggressive one.

So I tried to make signal works (ex Ctrl + c) and finally I made it😄

diff --git a/QEMU-gvm-vcpupin/vl.c b/QEMU-gvm-vcpupin/vl.c
index 6e20948d7..a5e1269bf 100755
--- a/QEMU-gvm-vcpupin/vl.c
+++ b/QEMU-gvm-vcpupin/vl.c
@@ -4344,6 +4344,8 @@ int main(int argc, char **argv, char **envp)
     }
     printf("]\n");

+    start_io_router();
+
     machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to UP */
     if (max_cpus > machine_class->max_cpus) {
         error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
@@ -4848,7 +4850,6 @@ int main(int argc, char **argv, char **envp)
         return 0;
     }

-    start_io_router();

     if (incoming) {
         Error *local_err = NULL;

[ChoKyuWon]

@YWHyuk I agree that signal work is very good idea. But are you sure that the patch doesn't have any side-effect? start_io_router function moved so many lines, so I little worry that there are some problems that I don't know.

Anyway, this is my first time joining an open-source project so I don't know how can I handle that patch. What can I do? Shall I commit that patch and add your name to the signed-off field? or After my PR, #17 is merged and you submit a new PR?

In my opinion, mine and your patch have the same purpose but different method, so submit new PR is a better way, but I'll respect your opinion.

[YWHyuk]

@ChoKyuWon

I agree that signal work is very good idea. But are you sure that the patch doesn't have any side-effect? start_io_router function moved so many lines, so I little worry that there are some problems that I don't know.

In my opinion, mine and your patch have the same purpose but different method, so submit new PR is a better way, but I'll respect your opinion.

Actually I'm not sure that there is no side-effect. So I have to test it for a while. After that, I'll submit it as a new PR