Re: [Qemu-devel] [PATCH v10 6/7] migration/ram.c: add a function to disable the bulk stage

[Wei Wang <wei.w.wang@intel.com>]

On 12/03/2018 01:31 PM, Peter Xu wrote:
> On Mon, Dec 03, 2018 at 10:18:30AM +0800, Wei Wang wrote:
>> This patch adds a function to enable a precopy notifier callback outside
>> the migration subsystem to disable the bulk stage flag. This is needed by
>> the free page optimization offered by virtio-balloon.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> CC: Peter Xu <peterx@redhat.com>
>> ---
>>   include/migration/misc.h | 1 +
>>   migration/ram.c          | 9 +++++++++
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>> index 15f8d00..47e7ff5 100644
>> --- a/include/migration/misc.h
>> +++ b/include/migration/misc.h
>> @@ -37,6 +37,7 @@ void precopy_infrastructure_init(void);
>>   void precopy_add_notifier(NotifierWithReturn *n);
>>   void precopy_remove_notifier(NotifierWithReturn *n);
>>   int precopy_notify(PrecopyNotifyReason reason, Error **errp);
>> +void precopy_disable_bulk_stage(void);
>>   
>>   void ram_mig_init(void);
>>   void qemu_guest_free_page_hint(void *addr, size_t len);
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b90a3f2..739dc97 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -354,6 +354,15 @@ int precopy_notify(PrecopyNotifyReason reason, Error **errp)
>>       return notifier_with_return_list_notify(&precopy_notifier_list, &pnd);
>>   }
>>   
>> +void precopy_disable_bulk_stage(void)
>> +{
>> +    if (!ram_state) {
>> +        return;
>> +    }
>> +
>> +    ram_state->ram_bulk_stage = false;
>> +}
>> +
> This one is a bit tricky.  E.g., I think it'll at least affect XBZRLE
> and compression somehow.  Since we will have the on-off switch for
> balloon free page hinting so the user can at least decide what
> features to use, with that I don't see much issue with it so far. But
> I'd also like to see how other people see this change.
>

Yes. I think it would be better that this optimization could co-exist 
with the
compression feature.

How about adding a new flag "ram_state->free_page_optimization_enabled"?

We will then need a change in migration_bitmap_find_dirty:
  - if (rs->ram_bulk_stage && start > 0) {
+ if (!rs->free_page_optimization_enabled && rs->ram_bulk_stage && start 
 > 0) {
         next = start + 1;
     } else {
         next = find_next_bit(bitmap, size, start);
     }

Best,
Wei