Re: [Qemu-devel] [PATCH RFC 3/4] Curling: the sender

[junqing.wang@cs2c.com.cn]

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

hi,


>> +    bool create = false;
> >This variable is never set.

It is set in the following 'if' block.
 +        create = true;    <<=======

>> -    migration_bitmap = bitmap_new(ram_pages);
>> -    bitmap_set(migration_bitmap, 0, ram_pages);
>> -    migration_dirty_pages = ram_pages;
>> +    if (!ft_enabled() || !migration_bitmap)  {
>> +        migration_bitmap = bitmap_new(ram_pages);
>> +        bitmap_set(migration_bitmap, 0, ram_pages);
>> +        migration_dirty_pages = ram_pages;
>> +        create = true;       <==========
>> +    }

>Nothing in this patch sets the migration_bitmap to anything.

Let me explain all the odd 'if'  block:
1 >> +    if (!ft_enabled() || !migration_bitmap)  {
2 >> +    if (!ft_enabled() || create) {
3 >> +    if (!ft_enabled()) {

As I mentioned in the commit log: 
>> We need to handle the variables related to live migration very
>> carefully. So the new migration does not restart from the very
>> begin of the migration, instead, it continues the previous
>> migration.

Some variables should not be reset after one migration, because
the next one need these variables to continue the migration.
This explains all the "if ft_enabled()"

Besides, some variables need to be initialized at the first migration of curling.
That explains the "if create" and "if  !migration_bitmap"

>> +                if (ft_enabled()) {
>> +                    if (old_vm_running) {
>> +                        qemu_mutex_lock_iothread();
>> +                        vm_start();
>> +                        qemu_mutex_unlock_iothread();
>> +
>> +                        current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> +                        time_spent = current_time - migration_start_time;
>> +                        DPRINTF("this migration lasts for %" PRId64 "ms\n",
>> +                                time_spent);
>> +                        if (time_spent < time_window) {
>> +                            g_usleep((time_window - time_spent)*1000);
>
>Why are we waiting here?  If we are migration faster than allowed,  why
>we are waiting?

Looping fast is not good, that means we enter iothread lock and do vm stop more frequently. The performance will drop and vm user will experience input stall if we do not sleep.

How to deal with this is a difficult issue, any suggestion is welcomed.

THIS IS ONE OF THE TWO MAIN PROBLEMS.  The other one is related to the magic number 0xfeedcafe.



[-- Attachment #2: Type: text/html, Size: 5060 bytes --]