diff mbox series

[RFC,1/6] migration: skipped field is really obsolete.

Message ID 20230612193344.3796-2-quintela@redhat.com
State New
Headers show
Series Migration deprecated parts | expand

Commit Message

Juan Quintela June 12, 2023, 7:33 p.m. UTC
Has return zero for more than 10 years.  Just mark it deprecated.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 docs/about/deprecated.rst | 10 ++++++++++
 qapi/migration.json       | 12 ++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

Comments

Daniel P. Berrangé June 20, 2023, 12:01 p.m. UTC | #1
On Mon, Jun 12, 2023 at 09:33:39PM +0200, Juan Quintela wrote:
> Has return zero for more than 10 years.  Just mark it deprecated.

Specifically we introduced the field in 1.5.0

commit f1c72795af573b24a7da5eb52375c9aba8a37972
Author: Peter Lieven <pl@kamp.de>
Date:   Tue Mar 26 10:58:37 2013 +0100

    migration: do not sent zero pages in bulk stage
    
    during bulk stage of ram migration if a page is a
    zero page do not send it at all.
    the memory at the destination reads as zero anyway.
    
    even if there is an madvise with QEMU_MADV_DONTNEED
    at the target upon receipt of a zero page I have observed
    that the target starts swapping if the memory is overcommitted.
    it seems that the pages are dropped asynchronously.
    
    this patch also updates QMP to return the number of
    skipped pages in MigrationStats.
    


but removed its usage in 1.5.3

commit 9ef051e5536b6368a1076046ec6c4ec4ac12b5c6
Author: Peter Lieven <pl@kamp.de>
Date:   Mon Jun 10 12:14:19 2013 +0200

    Revert "migration: do not sent zero pages in bulk stage"
    
    Not sending zero pages breaks migration if a page is zero
    at the source but not at the destination. This can e.g. happen
    if different BIOS versions are used at source and destination.
    It has also been reported that migration on pseries is completely
    broken with this patch.
    
    This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.


> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  docs/about/deprecated.rst | 10 ++++++++++
>  qapi/migration.json       | 12 ++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> diff --git a/qapi/migration.json b/qapi/migration.json
> index cb7cd3e578..bcae193733 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -23,7 +23,8 @@
>  #
>  # @duplicate: number of duplicate (zero) pages (since 1.2)
>  #
> -# @skipped: number of skipped zero pages (since 1.5)
> +# @skipped: number of skipped zero pages. Don't use, only provided for
> +#     compatibility (since 1.5)

I'd say

   @skipped: number of skipped zero pages. Always zero, only provided for
   compatibility (since 1.5)

>  #
>  # @normal: number of normal pages (since 1.2)
>  #
> @@ -62,11 +63,18 @@
>  #     between 0 and @dirty-sync-count * @multifd-channels.  (since
>  #     7.1)
>  #
> +# Features:
> +#
> +# @deprecated: Member @skipped has not been used for a long time.

  @deprecated: Member @skipped is always zero since 1.5.3


With regards,
Daniel
Juan Quintela June 22, 2023, 5:49 p.m. UTC | #2
Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Jun 12, 2023 at 09:33:39PM +0200, Juan Quintela wrote:
>> Has return zero for more than 10 years.  Just mark it deprecated.
>
> Specifically we introduced the field in 1.5.0
>
> commit f1c72795af573b24a7da5eb52375c9aba8a37972
> Author: Peter Lieven <pl@kamp.de>
> Date:   Tue Mar 26 10:58:37 2013 +0100
>
>     migration: do not sent zero pages in bulk stage
>     
>     during bulk stage of ram migration if a page is a
>     zero page do not send it at all.
>     the memory at the destination reads as zero anyway.
>     
>     even if there is an madvise with QEMU_MADV_DONTNEED
>     at the target upon receipt of a zero page I have observed
>     that the target starts swapping if the memory is overcommitted.
>     it seems that the pages are dropped asynchronously.
>     
>     this patch also updates QMP to return the number of
>     skipped pages in MigrationStats.
>     
>
>
> but removed its usage in 1.5.3
>
> commit 9ef051e5536b6368a1076046ec6c4ec4ac12b5c6
> Author: Peter Lieven <pl@kamp.de>
> Date:   Mon Jun 10 12:14:19 2013 +0200
>
>     Revert "migration: do not sent zero pages in bulk stage"
>     
>     Not sending zero pages breaks migration if a page is zero
>     at the source but not at the destination. This can e.g. happen
>     if different BIOS versions are used at source and destination.
>     It has also been reported that migration on pseries is completely
>     broken with this patch.
>     
>     This effectively reverts commit f1c72795af573b24a7da5eb52375c9aba8a37972.


Thanks for the history O:-)

>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  docs/about/deprecated.rst | 10 ++++++++++
>>  qapi/migration.json       | 12 ++++++++++--
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index cb7cd3e578..bcae193733 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -23,7 +23,8 @@
>>  #
>>  # @duplicate: number of duplicate (zero) pages (since 1.2)
>>  #
>> -# @skipped: number of skipped zero pages (since 1.5)
>> +# @skipped: number of skipped zero pages. Don't use, only provided for
>> +#     compatibility (since 1.5)
>
> I'd say
>
>    @skipped: number of skipped zero pages. Always zero, only provided for
>    compatibility (since 1.5)

Changed.

>>  #
>>  # @normal: number of normal pages (since 1.2)
>>  #
>> @@ -62,11 +63,18 @@
>>  #     between 0 and @dirty-sync-count * @multifd-channels.  (since
>>  #     7.1)
>>  #
>> +# Features:
>> +#
>> +# @deprecated: Member @skipped has not been used for a long time.
>
>   @deprecated: Member @skipped is always zero since 1.5.3

Changed.

Thanks.
diff mbox series

Patch

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 0743459862..e1aa0eafc8 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -423,3 +423,13 @@  both, older and future versions of QEMU.
 The ``blacklist`` config file option has been renamed to ``block-rpcs``
 (to be in sync with the renaming of the corresponding command line
 option).
+
+Migration
+---------
+
+``skipped`` MigrationStats field (since 8.1)
+''''''''''''''''''''''''''''''''''''''''''''
+
+``skipped`` field in Migration stats has been deprecated.  It hasn't
+been used for more than 10 years.
+
diff --git a/qapi/migration.json b/qapi/migration.json
index cb7cd3e578..bcae193733 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -23,7 +23,8 @@ 
 #
 # @duplicate: number of duplicate (zero) pages (since 1.2)
 #
-# @skipped: number of skipped zero pages (since 1.5)
+# @skipped: number of skipped zero pages. Don't use, only provided for
+#     compatibility (since 1.5)
 #
 # @normal: number of normal pages (since 1.2)
 #
@@ -62,11 +63,18 @@ 
 #     between 0 and @dirty-sync-count * @multifd-channels.  (since
 #     7.1)
 #
+# Features:
+#
+# @deprecated: Member @skipped has not been used for a long time.
+#
 # Since: 0.14
+#
 ##
 { 'struct': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
-           'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
+           'duplicate': 'int',
+           'skipped': { 'type': 'int', 'features': ['deprecated'] },
+           'normal': 'int',
            'normal-bytes': 'int', 'dirty-pages-rate': 'int',
            'mbps': 'number', 'dirty-sync-count': 'int',
            'postcopy-requests': 'int', 'page-size': 'int',