Re: [PATCH v1 2/6] migration: Add last stage indicator to global dirty log synchronization

[Gavin Shan <gshan@redhat.com>]

On 2/22/23 4:36 AM, Peter Xu wrote:
> On Mon, Feb 13, 2023 at 08:39:21AM +0800, Gavin Shan wrote:
>> The global dirty log synchronization is used when KVM and dirty ring
>> are enabled. There is a particularity for ARM64 where the backup
>> bitmap is used to track dirty pages in non-running-vcpu situations.
>> It means the dirty ring works with the combination of ring buffer
>> and backup bitmap. The dirty bits in the backup bitmap needs to
>> collected in the last stage of live migration.
>>
>> In order to identify the last stage of live migration and pass it
>> down, an extra parameter is added to the relevant functions and
>> callbacks. This last stage indicator isn't used until the dirty
>> ring is enabled in the subsequent patches.
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> One trivial thing to mention below.
> 
>> ---
>>   accel/kvm/kvm-all.c   |  2 +-
>>   include/exec/memory.h |  5 +++--
>>   migration/dirtyrate.c |  4 ++--
>>   migration/ram.c       | 20 ++++++++++----------
>>   softmmu/memory.c      | 10 +++++-----
>>   5 files changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 9b26582655..01a6a026af 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1554,7 +1554,7 @@ static void kvm_log_sync(MemoryListener *listener,
>>       kvm_slots_unlock();
>>   }
>>   
>> -static void kvm_log_sync_global(MemoryListener *l)
>> +static void kvm_log_sync_global(MemoryListener *l, bool last_stage)
>>   {
>>       KVMMemoryListener *kml = container_of(l, KVMMemoryListener, listener);
>>       KVMState *s = kvm_state;
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 2e602a2fad..75b2fd9f48 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -929,8 +929,9 @@ struct MemoryListener {
>>        * its @log_sync must be NULL.  Vice versa.
>>        *
>>        * @listener: The #MemoryListener.
>> +     * @last_stage: The last stage to synchronize the log during migration
> 
> IMHO it may be important to mention the vcpu status here that the caller
> guarantees to call the last_stage==true only once, only after all vcpus are
> stopped (and vcpus will not be started again if migration succeeded).
> 

Yes, I will update the comments in next revision accordingly.

>>        */
>> -    void (*log_sync_global)(MemoryListener *listener);
>> +    void (*log_sync_global)(MemoryListener *listener, bool last_stage);

Thanks,
Gavin