This patch adds an API to clear bits corresponding to guest free pages
from the dirty bitmap. Spilt the free page block if it crosses the QEMU
RAMBlock boundary.

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>
---
 include/migration/misc.h |  2 ++
 migration/ram.c          | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 4ebf24c..113320e 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -14,11 +14,13 @@
 #ifndef MIGRATION_MISC_H
 #define MIGRATION_MISC_H
 
+#include "exec/cpu-common.h"
 #include "qemu/notify.h"
 
 /* migration/ram.c */
 
 void ram_mig_init(void);
+void qemu_guest_free_page_hint(void *addr, size_t len);
 
 /* migration/block.c */
 
diff --git a/migration/ram.c b/migration/ram.c
index 9a72b1a..0147548 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2198,6 +2198,50 @@ static int ram_init_all(RAMState **rsp)
 }
 
 /*
+ * This function clears bits of the free pages reported by the caller from the
+ * migration dirty bitmap. @addr is the host address corresponding to the
+ * start of the continuous guest free pages, and @len is the total bytes of
+ * those pages.
+ */
+void qemu_guest_free_page_hint(void *addr, size_t len)
+{
+    RAMBlock *block;
+    ram_addr_t offset;
+    size_t used_len, start, npages;
+
+    for (; len > 0; len -= used_len) {
+        block = qemu_ram_block_from_host(addr, false, &offset);
+        if (unlikely(!block)) {
+            return;
+        }
+
+        /*
+         * This handles the case that the RAMBlock is resized after the free
+         * page hint is reported.
+         */
+        if (unlikely(offset > block->used_length)) {
+            return;
+        }
+
+        if (len <= block->used_length - offset) {
+            used_len = len;
+        } else {
+            used_len = block->used_length - offset;
+            addr += used_len;
+        }
+
+        start = offset >> TARGET_PAGE_BITS;
+        npages = used_len >> TARGET_PAGE_BITS;
+
+        qemu_mutex_lock(&ram_state->bitmap_mutex);
+        ram_state->migration_dirty_pages -=
+                      bitmap_count_one_with_offset(block->bmap, start, npages);
+        bitmap_clear(block->bmap, start, npages);
+        qemu_mutex_unlock(&ram_state->bitmap_mutex);
+    }
+}
+
+/*
  * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
  * long-running RCU critical section.  When rcu-reclaims in the code
  * start to become numerous it will be necessary to reduce the
-- 
1.8.3.1

On Tue, Apr 24, 2018 at 02:13:46PM +0800, Wei Wang wrote:
> This patch adds an API to clear bits corresponding to guest free pages
> from the dirty bitmap. Spilt the free page block if it crosses the QEMU
> RAMBlock boundary.
> 
> 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>
> ---
>  include/migration/misc.h |  2 ++
>  migration/ram.c          | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 4ebf24c..113320e 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -14,11 +14,13 @@
>  #ifndef MIGRATION_MISC_H
>  #define MIGRATION_MISC_H
>  
> +#include "exec/cpu-common.h"
>  #include "qemu/notify.h"
>  
>  /* migration/ram.c */
>  
>  void ram_mig_init(void);
> +void qemu_guest_free_page_hint(void *addr, size_t len);
>  
>  /* migration/block.c */
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 9a72b1a..0147548 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2198,6 +2198,50 @@ static int ram_init_all(RAMState **rsp)
>  }
>  
>  /*
> + * This function clears bits of the free pages reported by the caller from the
> + * migration dirty bitmap. @addr is the host address corresponding to the
> + * start of the continuous guest free pages, and @len is the total bytes of
> + * those pages.
> + */
> +void qemu_guest_free_page_hint(void *addr, size_t len)
> +{
> +    RAMBlock *block;
> +    ram_addr_t offset;
> +    size_t used_len, start, npages;

Do we need to check here on whether a migration is in progress?  Since
if not I'm not sure whether this hint still makes any sense any more,
and more importantly it seems to me that block->bmap below at [1] is
only valid during a migration.  So I'm not sure whether QEMU will
crash if this function is called without a running migration.

> +
> +    for (; len > 0; len -= used_len) {
> +        block = qemu_ram_block_from_host(addr, false, &offset);
> +        if (unlikely(!block)) {
> +            return;

We should never reach here, should we?  Assuming the callers of this
function should always pass in a correct host address. If we are very
sure that the host addr should be valid, could we just assert?

> +        }
> +
> +        /*
> +         * This handles the case that the RAMBlock is resized after the free
> +         * page hint is reported.
> +         */
> +        if (unlikely(offset > block->used_length)) {
> +            return;
> +        }
> +
> +        if (len <= block->used_length - offset) {
> +            used_len = len;
> +        } else {
> +            used_len = block->used_length - offset;
> +            addr += used_len;
> +        }
> +
> +        start = offset >> TARGET_PAGE_BITS;
> +        npages = used_len >> TARGET_PAGE_BITS;
> +
> +        qemu_mutex_lock(&ram_state->bitmap_mutex);

So now I think I understand the lock can still be meaningful since
this function now can be called outside the migration thread (e.g., in
vcpu thread).  But still it would be nice to mention it somewhere on
the truth of the lock.

Regards,

> +        ram_state->migration_dirty_pages -=
> +                      bitmap_count_one_with_offset(block->bmap, start, npages);
> +        bitmap_clear(block->bmap, start, npages);

[1]

> +        qemu_mutex_unlock(&ram_state->bitmap_mutex);
> +    }
> +}
> +
> +/*
>   * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>   * long-running RCU critical section.  When rcu-reclaims in the code
>   * start to become numerous it will be necessary to reduce the
> -- 
> 1.8.3.1
> 
> 

-- 
Peter Xu

On 06/01/2018 12:00 PM, Peter Xu wrote:
> On Tue, Apr 24, 2018 at 02:13:46PM +0800, Wei Wang wrote:
>> This patch adds an API to clear bits corresponding to guest free pages
>> from the dirty bitmap. Spilt the free page block if it crosses the QEMU
>> RAMBlock boundary.
>>
>> 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>
>> ---
>>   include/migration/misc.h |  2 ++
>>   migration/ram.c          | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>> index 4ebf24c..113320e 100644
>> --- a/include/migration/misc.h
>> +++ b/include/migration/misc.h
>> @@ -14,11 +14,13 @@
>>   #ifndef MIGRATION_MISC_H
>>   #define MIGRATION_MISC_H
>>   
>> +#include "exec/cpu-common.h"
>>   #include "qemu/notify.h"
>>   
>>   /* migration/ram.c */
>>   
>>   void ram_mig_init(void);
>> +void qemu_guest_free_page_hint(void *addr, size_t len);
>>   
>>   /* migration/block.c */
>>   
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 9a72b1a..0147548 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2198,6 +2198,50 @@ static int ram_init_all(RAMState **rsp)
>>   }
>>   
>>   /*
>> + * This function clears bits of the free pages reported by the caller from the
>> + * migration dirty bitmap. @addr is the host address corresponding to the
>> + * start of the continuous guest free pages, and @len is the total bytes of
>> + * those pages.
>> + */
>> +void qemu_guest_free_page_hint(void *addr, size_t len)
>> +{
>> +    RAMBlock *block;
>> +    ram_addr_t offset;
>> +    size_t used_len, start, npages;
> Do we need to check here on whether a migration is in progress?  Since
> if not I'm not sure whether this hint still makes any sense any more,
> and more importantly it seems to me that block->bmap below at [1] is
> only valid during a migration.  So I'm not sure whether QEMU will
> crash if this function is called without a running migration.

OK. How about just adding comments above to have users noted that this 
function should be used during migration?

If we want to do a sanity check here, I think it would be easier to just 
check !block->bmap here.


>
>> +
>> +    for (; len > 0; len -= used_len) {
>> +        block = qemu_ram_block_from_host(addr, false, &offset);
>> +        if (unlikely(!block)) {
>> +            return;
> We should never reach here, should we?  Assuming the callers of this
> function should always pass in a correct host address. If we are very
> sure that the host addr should be valid, could we just assert?

Probably not the case, because of the corner case that the memory would 
be hot unplugged after the free page is reported to QEMU.



>
>> +        }
>> +
>> +        /*
>> +         * This handles the case that the RAMBlock is resized after the free
>> +         * page hint is reported.
>> +         */
>> +        if (unlikely(offset > block->used_length)) {
>> +            return;
>> +        }
>> +
>> +        if (len <= block->used_length - offset) {
>> +            used_len = len;
>> +        } else {
>> +            used_len = block->used_length - offset;
>> +            addr += used_len;
>> +        }
>> +
>> +        start = offset >> TARGET_PAGE_BITS;
>> +        npages = used_len >> TARGET_PAGE_BITS;
>> +
>> +        qemu_mutex_lock(&ram_state->bitmap_mutex);
> So now I think I understand the lock can still be meaningful since
> this function now can be called outside the migration thread (e.g., in
> vcpu thread).  But still it would be nice to mention it somewhere on
> the truth of the lock.
>

Yes. Thanks for the reminder. I will add some explanation to the patch 2 
commit log.


Best,
Wei

On Fri, Jun 01, 2018 at 03:36:01PM +0800, Wei Wang wrote:
> On 06/01/2018 12:00 PM, Peter Xu wrote:
> > On Tue, Apr 24, 2018 at 02:13:46PM +0800, Wei Wang wrote:
> > > This patch adds an API to clear bits corresponding to guest free pages
> > > from the dirty bitmap. Spilt the free page block if it crosses the QEMU
> > > RAMBlock boundary.
> > > 
> > > 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>
> > > ---
> > >   include/migration/misc.h |  2 ++
> > >   migration/ram.c          | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 46 insertions(+)
> > > 
> > > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > > index 4ebf24c..113320e 100644
> > > --- a/include/migration/misc.h
> > > +++ b/include/migration/misc.h
> > > @@ -14,11 +14,13 @@
> > >   #ifndef MIGRATION_MISC_H
> > >   #define MIGRATION_MISC_H
> > > +#include "exec/cpu-common.h"
> > >   #include "qemu/notify.h"
> > >   /* migration/ram.c */
> > >   void ram_mig_init(void);
> > > +void qemu_guest_free_page_hint(void *addr, size_t len);
> > >   /* migration/block.c */
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 9a72b1a..0147548 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -2198,6 +2198,50 @@ static int ram_init_all(RAMState **rsp)
> > >   }
> > >   /*
> > > + * This function clears bits of the free pages reported by the caller from the
> > > + * migration dirty bitmap. @addr is the host address corresponding to the
> > > + * start of the continuous guest free pages, and @len is the total bytes of
> > > + * those pages.
> > > + */
> > > +void qemu_guest_free_page_hint(void *addr, size_t len)
> > > +{
> > > +    RAMBlock *block;
> > > +    ram_addr_t offset;
> > > +    size_t used_len, start, npages;
> > Do we need to check here on whether a migration is in progress?  Since
> > if not I'm not sure whether this hint still makes any sense any more,
> > and more importantly it seems to me that block->bmap below at [1] is
> > only valid during a migration.  So I'm not sure whether QEMU will
> > crash if this function is called without a running migration.
> 
> OK. How about just adding comments above to have users noted that this
> function should be used during migration?
> 
> If we want to do a sanity check here, I think it would be easier to just
> check !block->bmap here.

I think the faster way might be that we check against the migration
state.

> 
> 
> > 
> > > +
> > > +    for (; len > 0; len -= used_len) {
> > > +        block = qemu_ram_block_from_host(addr, false, &offset);
> > > +        if (unlikely(!block)) {
> > > +            return;
> > We should never reach here, should we?  Assuming the callers of this
> > function should always pass in a correct host address. If we are very
> > sure that the host addr should be valid, could we just assert?
> 
> Probably not the case, because of the corner case that the memory would be
> hot unplugged after the free page is reported to QEMU.

Question: Do we allow to do hot plug/unplug for memory during
migration?

> 
> 
> 
> > 
> > > +        }
> > > +
> > > +        /*
> > > +         * This handles the case that the RAMBlock is resized after the free
> > > +         * page hint is reported.
> > > +         */
> > > +        if (unlikely(offset > block->used_length)) {
> > > +            return;
> > > +        }
> > > +
> > > +        if (len <= block->used_length - offset) {
> > > +            used_len = len;
> > > +        } else {
> > > +            used_len = block->used_length - offset;
> > > +            addr += used_len;
> > > +        }
> > > +
> > > +        start = offset >> TARGET_PAGE_BITS;
> > > +        npages = used_len >> TARGET_PAGE_BITS;
> > > +
> > > +        qemu_mutex_lock(&ram_state->bitmap_mutex);
> > So now I think I understand the lock can still be meaningful since
> > this function now can be called outside the migration thread (e.g., in
> > vcpu thread).  But still it would be nice to mention it somewhere on

(Actually after read the next patch I think it's in iothread, so I'd
 better reply with all the series read over next time :)

> > the truth of the lock.
> > 
> 
> Yes. Thanks for the reminder. I will add some explanation to the patch 2
> commit log.

Thanks,

-- 
Peter Xu

On 06/01/2018 06:06 PM, Peter Xu wrote:
> On Fri, Jun 01, 2018 at 03:36:01PM +0800, Wei Wang wrote:
>> On 06/01/2018 12:00 PM, Peter Xu wrote:
>>> On Tue, Apr 24, 2018 at 02:13:46PM +0800, Wei Wang wrote:
>>>>    /*
>>>> + * This function clears bits of the free pages reported by the caller from the
>>>> + * migration dirty bitmap. @addr is the host address corresponding to the
>>>> + * start of the continuous guest free pages, and @len is the total bytes of
>>>> + * those pages.
>>>> + */
>>>> +void qemu_guest_free_page_hint(void *addr, size_t len)
>>>> +{
>>>> +    RAMBlock *block;
>>>> +    ram_addr_t offset;
>>>> +    size_t used_len, start, npages;
>>> Do we need to check here on whether a migration is in progress?  Since
>>> if not I'm not sure whether this hint still makes any sense any more,
>>> and more importantly it seems to me that block->bmap below at [1] is
>>> only valid during a migration.  So I'm not sure whether QEMU will
>>> crash if this function is called without a running migration.
>> OK. How about just adding comments above to have users noted that this
>> function should be used during migration?
>>
>> If we want to do a sanity check here, I think it would be easier to just
>> check !block->bmap here.
> I think the faster way might be that we check against the migration
> state.
>

Sounds good. We can do a sanity check:

     MigrationState *s = migrate_get_current();
     if (!migration_is_setup_or_active(s->state))
         return;



>>
>>>> +
>>>> +    for (; len > 0; len -= used_len) {
>>>> +        block = qemu_ram_block_from_host(addr, false, &offset);
>>>> +        if (unlikely(!block)) {
>>>> +            return;
>>> We should never reach here, should we?  Assuming the callers of this
>>> function should always pass in a correct host address. If we are very
>>> sure that the host addr should be valid, could we just assert?
>> Probably not the case, because of the corner case that the memory would be
>> hot unplugged after the free page is reported to QEMU.
> Question: Do we allow to do hot plug/unplug for memory during
> migration?

I think so. From the code, I don't find where it forbids memory hotplug 
during migration.

>>
>>
>>>> +        }
>>>> +
>>>> +        /*
>>>> +         * This handles the case that the RAMBlock is resized after the free
>>>> +         * page hint is reported.
>>>> +         */
>>>> +        if (unlikely(offset > block->used_length)) {
>>>> +            return;
>>>> +        }
>>>> +
>>>> +        if (len <= block->used_length - offset) {
>>>> +            used_len = len;
>>>> +        } else {
>>>> +            used_len = block->used_length - offset;
>>>> +            addr += used_len;
>>>> +        }
>>>> +
>>>> +        start = offset >> TARGET_PAGE_BITS;
>>>> +        npages = used_len >> TARGET_PAGE_BITS;
>>>> +
>>>> +        qemu_mutex_lock(&ram_state->bitmap_mutex);
>>> So now I think I understand the lock can still be meaningful since
>>> this function now can be called outside the migration thread (e.g., in
>>> vcpu thread).  But still it would be nice to mention it somewhere on
> (Actually after read the next patch I think it's in iothread, so I'd
>   better reply with all the series read over next time :)

That's fine actually :) Whether it is called by an iothread or a vcpu 
thread doesn't affect our discussion here.

I think we could just focus on the interfaces here and the usage in live 
migration. I can explain more when needed.

Best,
Wei

On Fri, Jun 01, 2018 at 08:32:27PM +0800, Wei Wang wrote:
> On 06/01/2018 06:06 PM, Peter Xu wrote:
> > On Fri, Jun 01, 2018 at 03:36:01PM +0800, Wei Wang wrote:
> > > On 06/01/2018 12:00 PM, Peter Xu wrote:
> > > > On Tue, Apr 24, 2018 at 02:13:46PM +0800, Wei Wang wrote:
> > > > >    /*
> > > > > + * This function clears bits of the free pages reported by the caller from the
> > > > > + * migration dirty bitmap. @addr is the host address corresponding to the
> > > > > + * start of the continuous guest free pages, and @len is the total bytes of
> > > > > + * those pages.
> > > > > + */
> > > > > +void qemu_guest_free_page_hint(void *addr, size_t len)
> > > > > +{
> > > > > +    RAMBlock *block;
> > > > > +    ram_addr_t offset;
> > > > > +    size_t used_len, start, npages;
> > > > Do we need to check here on whether a migration is in progress?  Since
> > > > if not I'm not sure whether this hint still makes any sense any more,
> > > > and more importantly it seems to me that block->bmap below at [1] is
> > > > only valid during a migration.  So I'm not sure whether QEMU will
> > > > crash if this function is called without a running migration.
> > > OK. How about just adding comments above to have users noted that this
> > > function should be used during migration?
> > > 
> > > If we want to do a sanity check here, I think it would be easier to just
> > > check !block->bmap here.
> > I think the faster way might be that we check against the migration
> > state.
> > 
> 
> Sounds good. We can do a sanity check:
> 
>     MigrationState *s = migrate_get_current();
>     if (!migration_is_setup_or_active(s->state))
>         return;

Yes.

> 
> 
> 
> > > 
> > > > > +
> > > > > +    for (; len > 0; len -= used_len) {
> > > > > +        block = qemu_ram_block_from_host(addr, false, &offset);
> > > > > +        if (unlikely(!block)) {
> > > > > +            return;
> > > > We should never reach here, should we?  Assuming the callers of this
> > > > function should always pass in a correct host address. If we are very
> > > > sure that the host addr should be valid, could we just assert?
> > > Probably not the case, because of the corner case that the memory would be
> > > hot unplugged after the free page is reported to QEMU.
> > Question: Do we allow to do hot plug/unplug for memory during
> > migration?
> 
> I think so. From the code, I don't find where it forbids memory hotplug
> during migration.

I don't play with that much; do we need to do "device_add" after all?

  (qemu) object_add memory-backend-file,id=mem1,size=1G,mem-path=/mnt/hugepages-1GB
  (qemu) device_add pc-dimm,id=dimm1,memdev=mem1

If so, we may not allow that since in qdev_device_add() we don't allow
that:

    if (!migration_is_idle()) {
        error_setg(errp, "device_add not allowed while migrating");
        return NULL;
    }

> 
> > > 
> > > 
> > > > > +        }
> > > > > +
> > > > > +        /*
> > > > > +         * This handles the case that the RAMBlock is resized after the free
> > > > > +         * page hint is reported.
> > > > > +         */
> > > > > +        if (unlikely(offset > block->used_length)) {
> > > > > +            return;
> > > > > +        }
> > > > > +
> > > > > +        if (len <= block->used_length - offset) {
> > > > > +            used_len = len;
> > > > > +        } else {
> > > > > +            used_len = block->used_length - offset;
> > > > > +            addr += used_len;
> > > > > +        }
> > > > > +
> > > > > +        start = offset >> TARGET_PAGE_BITS;
> > > > > +        npages = used_len >> TARGET_PAGE_BITS;
> > > > > +
> > > > > +        qemu_mutex_lock(&ram_state->bitmap_mutex);
> > > > So now I think I understand the lock can still be meaningful since
> > > > this function now can be called outside the migration thread (e.g., in
> > > > vcpu thread).  But still it would be nice to mention it somewhere on
> > (Actually after read the next patch I think it's in iothread, so I'd
> >   better reply with all the series read over next time :)
> 
> That's fine actually :) Whether it is called by an iothread or a vcpu thread
> doesn't affect our discussion here.
> 
> I think we could just focus on the interfaces here and the usage in live
> migration. I can explain more when needed.

Ok.  Thanks!

-- 
Peter Xu

On 06/04/2018 10:49 AM, Peter Xu wrote:
>
>>
>>
>>>>>> +
>>>>>> +    for (; len > 0; len -= used_len) {
>>>>>> +        block = qemu_ram_block_from_host(addr, false, &offset);
>>>>>> +        if (unlikely(!block)) {
>>>>>> +            return;
>>>>> We should never reach here, should we?  Assuming the callers of this
>>>>> function should always pass in a correct host address. If we are very
>>>>> sure that the host addr should be valid, could we just assert?
>>>> Probably not the case, because of the corner case that the memory would be
>>>> hot unplugged after the free page is reported to QEMU.
>>> Question: Do we allow to do hot plug/unplug for memory during
>>> migration?
>> I think so. From the code, I don't find where it forbids memory hotplug
>> during migration.
> I don't play with that much; do we need to do "device_add" after all?
>
>    (qemu) object_add memory-backend-file,id=mem1,size=1G,mem-path=/mnt/hugepages-1GB
>    (qemu) device_add pc-dimm,id=dimm1,memdev=mem1
>
> If so, we may not allow that since in qdev_device_add() we don't allow
> that:
>
>      if (!migration_is_idle()) {
>          error_setg(errp, "device_add not allowed while migrating");
>          return NULL;
>      }
>

OK, I missed that part, and thanks for correcting it. I'll use an assert 
there if no objections from others.


Best,
Wei