[PATCH 0/3] Remove res_compatible parameter

[PATCH 0/3] Remove res_compatible parameter

[Juan Quintela]

Hi

This series are the redo of the series from vfio.  Vladimir
Sementsov-Ogievskiy <vsementsov@yandex-team.ru> asked that I split the
change in ram.c (only place that set res_compatible), and the rest of
the patches.
So I ended:
- change ram.c to put the memory in res_postcopy
- remove res_compatible
- rename res_postcopy/precopy_only to not have the _only suffix.

Please review.

Juan Quintela (3):
  migration: In case of postcopy, the memory ends in res_postcopy_only
  migration: Remove unused res_compatible
  migration: Remove _only suffix for res_postcopy/precopy

 include/migration/register.h   | 21 ++++++++-------------
 migration/savevm.h             | 10 ++++------
 hw/s390x/s390-stattrib.c       |  8 +++-----
 hw/vfio/migration.c            | 11 ++++-------
 migration/block-dirty-bitmap.c |  7 +++----
 migration/block.c              |  8 +++-----
 migration/migration.c          | 18 ++++++++----------
 migration/ram.c                | 20 ++++++++------------
 migration/savevm.c             | 28 ++++++++++------------------
 hw/vfio/trace-events           |  2 +-
 migration/trace-events         |  4 ++--
 11 files changed, 54 insertions(+), 83 deletions(-)

-- 
2.39.1

[PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Juan Quintela]

So remove last assignation of res_compatible.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index b966e148c2..85ccbf88ad 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3474,7 +3474,7 @@ static void ram_state_pending_exact(void *opaque,
 
     if (migrate_postcopy_ram()) {
         /* We can do postcopy, and all the data is postcopiable */
-        *res_compatible += remaining_size;
+        *res_postcopy_only += remaining_size;
     } else {
         *res_precopy_only += remaining_size;
     }
-- 
2.39.1

Re: [PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Vladimir Sementsov-Ogievskiy]

On 08.02.23 16:57, Juan Quintela wrote:
> So remove last assignation of res_compatible.

I hoped for some description when asked to split it out :)

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/ram.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index b966e148c2..85ccbf88ad 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3474,7 +3474,7 @@ static void ram_state_pending_exact(void *opaque,
>   
>       if (migrate_postcopy_ram()) {
>           /* We can do postcopy, and all the data is postcopiable */
> -        *res_compatible += remaining_size;
> +        *res_postcopy_only += remaining_size;

Actually, these "remaining_size" bytes are still compatible, i.e. we can migrate these pending bytes in pre-copy, and we actually do it, until user call migrate-start-postcopy, yes? But we exploit the fact that, this change don't affect any logic, just name becomes wrong.. Yes? Or I don't follow:/

>       } else {
>           *res_precopy_only += remaining_size;
>       }

-- 
Best regards,
Vladimir

Re: [PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 08.02.23 16:57, Juan Quintela wrote:
>> So remove last assignation of res_compatible.
>
> I hoped for some description when asked to split it out :)
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/ram.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b966e148c2..85ccbf88ad 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3474,7 +3474,7 @@ static void ram_state_pending_exact(void *opaque,
>>         if (migrate_postcopy_ram()) {
>>           /* We can do postcopy, and all the data is postcopiable */
>> -        *res_compatible += remaining_size;
>> +        *res_postcopy_only += remaining_size;
>
> Actually, these "remaining_size" bytes are still compatible, i.e. we
> can migrate these pending bytes in pre-copy, and we actually do it,
> until user call migrate-start-postcopy, yes? But we exploit the fact
> that, this change don't affect any logic, just name becomes
> wrong.. Yes? Or I don't follow:/

My definition of the fields is: how are we going to transfer that bytes.

if they are on res_precopy_only, we transfer them with precopy, if they
are on res_postocpy_only, we transfer them with postcopy.

So, the rest of RAM, if we are in postcopy, we sent it with postcopy,
and if we are in precopy, we sent them with precopy.  See the whole
code.  This is the _estimate function.

    uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;

    if (migrate_postcopy_ram()) {
        /* We can do postcopy, and all the data is postcopiable */
        *res_postcopy_only += remaining_size;
    } else {
        *res_precopy_only += remaining_size;
    }

After the change, _exact does exactly the same.

The caller (migration_iteration_run()) does this (I remove traces and
things that don't matter for this). This is before the change.
Remember: in precopy, we add res_compat to pend_pre, and in postcopy to
pend_post.

    uint64_t pending_size = pend_pre + pend_compat + pend_post;

### pending_size is the sum of the three, so it doesn't matter.

    if (pend_pre + pend_compat <= s->threshold_size) {

###  In precopy, we add pend_compat to pend_pre, so we are ok.
###  In postcopy, we add the data to pend_postcopy, but that is right,
###  because to calculate the downtime, we only care about what we have
###  to transfer with precopy, in particular, we aren't going to send
###  more ram, so it is ok that it is in pend_post.

        qemu_savevm_state_pending_exact(&pend_pre, &pend_compat, &pend_post);
        pending_size = pend_pre + pend_compat + pend_post;
    }

    if (!pending_size || pending_size < s->threshold_size) {
        migration_completion(s);
        return MIG_ITERATE_BREAK;
    }

    /* Still a significant amount to transfer */
    if (!in_postcopy && pend_pre <= s->threshold_size &&
        qatomic_read(&s->start_postcopy)) {

#### this is what I mean.  See how we only use pend_pre to decide if we
###  are entering postcopy.

        if (postcopy_start(s)) {
            error_report("%s: postcopy failed to start", __func__);
        }
        return MIG_ITERATE_SKIP;
    }

So the only "behaviour" that we can say are having is that with the
change we are a little bit more aggressive on calling
qemu_savevm_state_pending_exact(), but I will arguee that the new
behaviour is the right one.

What do you think?

Later, Juan.

Re: [PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Vladimir Sementsov-Ogievskiy]

On 09.02.23 21:10, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>> On 08.02.23 16:57, Juan Quintela wrote:
>>> So remove last assignation of res_compatible.
>>
>> I hoped for some description when asked to split it out :)
>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>    migration/ram.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index b966e148c2..85ccbf88ad 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -3474,7 +3474,7 @@ static void ram_state_pending_exact(void *opaque,
>>>          if (migrate_postcopy_ram()) {
>>>            /* We can do postcopy, and all the data is postcopiable */
>>> -        *res_compatible += remaining_size;
>>> +        *res_postcopy_only += remaining_size;
>>
>> Actually, these "remaining_size" bytes are still compatible, i.e. we
>> can migrate these pending bytes in pre-copy, and we actually do it,
>> until user call migrate-start-postcopy, yes? But we exploit the fact
>> that, this change don't affect any logic, just name becomes
>> wrong.. Yes? Or I don't follow:/
> 
> My definition of the fields is: how are we going to transfer that bytes.
> 
> if they are on res_precopy_only, we transfer them with precopy, if they
> are on res_postocpy_only, we transfer them with postcopy.
> 
> So, the rest of RAM, if we are in postcopy, we sent it with postcopy,
> and if we are in precopy, we sent them with precopy.  See the whole
> code.  This is the _estimate function.
> 
>      uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
> 
>      if (migrate_postcopy_ram()) {
>          /* We can do postcopy, and all the data is postcopiable */
>          *res_postcopy_only += remaining_size;
>      } else {
>          *res_precopy_only += remaining_size;
>      }
> 
> After the change, _exact does exactly the same.
> 
> The caller (migration_iteration_run()) does this (I remove traces and
> things that don't matter for this). This is before the change.
> Remember: in precopy, we add res_compat to pend_pre, and in postcopy to
> pend_post.
> 
>      uint64_t pending_size = pend_pre + pend_compat + pend_post;
> 
> ### pending_size is the sum of the three, so it doesn't matter.
> 
>      if (pend_pre + pend_compat <= s->threshold_size) {
> 
> ###  In precopy, we add pend_compat to pend_pre, so we are ok.
> ###  In postcopy, we add the data to pend_postcopy, but that is right,
> ###  because to calculate the downtime, we only care about what we have
> ###  to transfer with precopy, in particular, we aren't going to send
> ###  more ram, so it is ok that it is in pend_post.
> 
>          qemu_savevm_state_pending_exact(&pend_pre, &pend_compat, &pend_post);
>          pending_size = pend_pre + pend_compat + pend_post;
>      }
> 
>      if (!pending_size || pending_size < s->threshold_size) {
>          migration_completion(s);
>          return MIG_ITERATE_BREAK;
>      }
> 
>      /* Still a significant amount to transfer */
>      if (!in_postcopy && pend_pre <= s->threshold_size &&
>          qatomic_read(&s->start_postcopy)) {
> 
> #### this is what I mean.  See how we only use pend_pre to decide if we
> ###  are entering postcopy.
> 
>          if (postcopy_start(s)) {
>              error_report("%s: postcopy failed to start", __func__);
>          }
>          return MIG_ITERATE_SKIP;
>      }
> 
> So the only "behaviour" that we can say are having is that with the

actualy, this one was already "changed", as _estimate never return compat other than zero.

So, The patch really changes nothing

> change we are a little bit more aggressive on calling
> qemu_savevm_state_pending_exact(), but I will arguee that the new
> behaviour is the right one.
> 
> What do you think?
> 


I think, that the order of logic and documentation changing since introducing _estimate is a bit confused.

But I agree now, that we are safe to unite old compat and old postcopy_only into one variable, as we want only

1. the total sum, to probably go to migration_completion()
2. pend_pre to probably go to postcopy_start()

So, patch is OK, and seems it changes absolutely nothing in logic. Thanks for explanations!


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 09.02.23 21:10, Juan Quintela wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>>> On 08.02.23 16:57, Juan Quintela wrote:
>>>> So remove last assignation of res_compatible.
>> 
>
>
> I think, that the order of logic and documentation changing since introducing _estimate is a bit confused.
>
> But I agree now, that we are safe to unite old compat and old postcopy_only into one variable, as we want only
>
> 1. the total sum, to probably go to migration_completion()
> 2. pend_pre to probably go to postcopy_start()
>
> So, patch is OK, and seems it changes absolutely nothing in logic. Thanks for explanations!
>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks.

You are welcome.

Re: [PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 08.02.23 16:57, Juan Quintela wrote:
>> So remove last assignation of res_compatible.
>
> I hoped for some description when asked to split it out :)
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/ram.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b966e148c2..85ccbf88ad 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3474,7 +3474,7 @@ static void ram_state_pending_exact(void *opaque,
>>         if (migrate_postcopy_ram()) {
>>           /* We can do postcopy, and all the data is postcopiable */
>> -        *res_compatible += remaining_size;
>> +        *res_postcopy_only += remaining_size;
>
> Actually, these "remaining_size" bytes are still compatible, i.e. we
> can migrate these pending bytes in pre-copy, and we actually do it,
> until user call migrate-start-postcopy, yes? But we exploit the fact
> that, this change don't affect any logic, just name becomes
> wrong.. Yes? Or I don't follow:/

I think of this from this different angle:
- if we are on precopy, we return on res_precopy everything (and nothing
  on res_postcopy)
- if we are on postcopy, we return on res_precopy what we _must_ sent
  through precopy, and in res_postcopy what we can sent through
  postcopy.

i.e. if we stop the guest and do the migration right now, what are we
going to send through each channel.

Later, Juan.

Re: [PATCH 1/3] migration: In case of postcopy, the memory ends in res_postcopy_only

[Vladimir Sementsov-Ogievskiy]

On 15.02.23 12:08, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>> On 08.02.23 16:57, Juan Quintela wrote:
>>> So remove last assignation of res_compatible.
>>
>> I hoped for some description when asked to split it out :)
>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>    migration/ram.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index b966e148c2..85ccbf88ad 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -3474,7 +3474,7 @@ static void ram_state_pending_exact(void *opaque,
>>>          if (migrate_postcopy_ram()) {
>>>            /* We can do postcopy, and all the data is postcopiable */
>>> -        *res_compatible += remaining_size;
>>> +        *res_postcopy_only += remaining_size;
>>
>> Actually, these "remaining_size" bytes are still compatible, i.e. we
>> can migrate these pending bytes in pre-copy, and we actually do it,
>> until user call migrate-start-postcopy, yes? But we exploit the fact
>> that, this change don't affect any logic, just name becomes
>> wrong.. Yes? Or I don't follow:/
> 
> I think of this from this different angle:
> - if we are on precopy, we return on res_precopy everything (and nothing
>    on res_postcopy)
> - if we are on postcopy, we return on res_precopy what we _must_ sent
>    through precopy, and in res_postcopy what we can sent through
>    postcopy.
> 
> i.e. if we stop the guest and do the migration right now, what are we
> going to send through each channel.
> 

I understand.

I've introduced the division into three parts together with block-dirty-bitmap implementation, and it seemed significant to me that block-dirty-bitmap pending is postcopy_only in the sense that it can't be migrated before source stop, unlike RAM. But yes, it turns out that that's not significant for the generic migration algorithm, it works the same way for RAM and block-dirty-bitmap not distinguishing postcopy_only vs comaptible.

Anyway final documentation and new field names that you proposed are clean and correspond to the meaning which you have expected. And it avoids extra variable that I've introduced.

Haha. Looking at my old commit 4799502640e6a29d3 "migration: introduce postcopy-only pending" I see

-                              uint64_t *non_postcopiable_pending,
-                              uint64_t *postcopiable_pending);
+                              uint64_t *res_precopy_only,
+                              uint64_t *res_compatible,
+                              uint64_t *res_postcopy_only);

so, we just rollback my change, which actually was never necessary. And it was called like I've proposed in 03 discussion thread :) Still, must_precopy and can_postcopy are nicer.

-- 
Best regards,
Vladimir

[PATCH 2/3] migration: Remove unused res_compatible

[Juan Quintela]

Nothing assigns to it after previous commit.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/register.h   |  7 ++-----
 migration/savevm.h             |  2 --
 hw/s390x/s390-stattrib.c       |  1 -
 hw/vfio/migration.c            |  3 +--
 migration/block-dirty-bitmap.c |  1 -
 migration/block.c              |  1 -
 migration/migration.c          | 18 ++++++++----------
 migration/ram.c                |  2 --
 migration/savevm.c             |  8 ++------
 hw/vfio/trace-events           |  2 +-
 migration/trace-events         |  4 ++--
 11 files changed, 16 insertions(+), 33 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index b91a0cdbf8..a958a92a0f 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -49,22 +49,19 @@ typedef struct SaveVMHandlers {
     /* Note for save_live_pending:
      * - res_precopy_only is for data which must be migrated in precopy phase
      *     or in stopped state, in other words - before target vm start
-     * - res_compatible is for data which may be migrated in any phase
      * - res_postcopy_only is for data which must be migrated in postcopy phase
      *     or in stopped state, in other words - after source vm stop
      *
-     * Sum of res_postcopy_only, res_compatible and res_postcopy_only is the
-     * whole amount of pending data.
+     * Sum of res_postcopy_only and res_postcopy_only is the whole
+     * amount of pending data.
      */
     /* This estimates the remaining data to transfer */
     void (*state_pending_estimate)(void *opaque,
                                    uint64_t *res_precopy_only,
-                                   uint64_t *res_compatible,
                                    uint64_t *res_postcopy_only);
     /* This calculate the exact remaining data to transfer */
     void (*state_pending_exact)(void *opaque,
                                 uint64_t *res_precopy_only,
-                                uint64_t *res_compatible,
                                 uint64_t *res_postcopy_only);
     LoadStateHandler *load_state;
     int (*load_setup)(QEMUFile *f, void *opaque);
diff --git a/migration/savevm.h b/migration/savevm.h
index b1901e68d5..bd625a644b 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -41,10 +41,8 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f);
 int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
                                        bool inactivate_disks);
 void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only,
-                                     uint64_t *res_compatible,
                                      uint64_t *res_postcopy_only);
 void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only,
-                                        uint64_t *res_compatible,
                                         uint64_t *res_postcopy_only);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 3e32002eab..c7ae9184ab 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -184,7 +184,6 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
 
 static void cmma_state_pending(void *opaque,
                                uint64_t *res_precopy_only,
-                               uint64_t *res_compatible,
                                uint64_t *res_postcopy_only)
 {
     S390StAttribState *sas = S390_STATTRIB(opaque);
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index b3318f0f20..fb0dd9571d 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -458,7 +458,6 @@ static void vfio_save_cleanup(void *opaque)
 
 static void vfio_state_pending(void *opaque,
                                uint64_t *res_precopy_only,
-                               uint64_t *res_compatible,
                                uint64_t *res_postcopy_only)
 {
     VFIODevice *vbasedev = opaque;
@@ -473,7 +472,7 @@ static void vfio_state_pending(void *opaque,
     *res_precopy_only += migration->pending_bytes;
 
     trace_vfio_state_pending(vbasedev->name, *res_precopy_only,
-                            *res_postcopy_only, *res_compatible);
+                            *res_postcopy_only);
 }
 
 static int vfio_save_iterate(QEMUFile *f, void *opaque)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 5a621419d3..9c6655e11a 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -764,7 +764,6 @@ static int dirty_bitmap_save_complete(QEMUFile *f, void *opaque)
 
 static void dirty_bitmap_state_pending(void *opaque,
                                        uint64_t *res_precopy_only,
-                                       uint64_t *res_compatible,
                                        uint64_t *res_postcopy_only)
 {
     DBMSaveState *s = &((DBMState *)opaque)->save;
diff --git a/migration/block.c b/migration/block.c
index 29f69025af..99d149904c 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -865,7 +865,6 @@ static int block_save_complete(QEMUFile *f, void *opaque)
 
 static void block_state_pending(void *opaque,
                                 uint64_t *res_precopy_only,
-                                uint64_t *res_compatible,
                                 uint64_t *res_postcopy_only)
 {
     /* Estimate pending number of bytes to send */
diff --git a/migration/migration.c b/migration/migration.c
index 7a14aa98d8..b74fb360df 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3820,20 +3820,18 @@ typedef enum {
  */
 static MigIterateState migration_iteration_run(MigrationState *s)
 {
-    uint64_t pend_pre, pend_compat, pend_post;
+    uint64_t pend_pre, pend_post;
     bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
 
-    qemu_savevm_state_pending_estimate(&pend_pre, &pend_compat, &pend_post);
-    uint64_t pending_size = pend_pre + pend_compat + pend_post;
+    qemu_savevm_state_pending_estimate(&pend_pre, &pend_post);
+    uint64_t pending_size = pend_pre + pend_post;
 
-    trace_migrate_pending_estimate(pending_size,
-                                   pend_pre, pend_compat, pend_post);
+    trace_migrate_pending_estimate(pending_size, pend_pre, pend_post);
 
-    if (pend_pre + pend_compat <= s->threshold_size) {
-        qemu_savevm_state_pending_exact(&pend_pre, &pend_compat, &pend_post);
-        pending_size = pend_pre + pend_compat + pend_post;
-        trace_migrate_pending_exact(pending_size,
-                                    pend_pre, pend_compat, pend_post);
+    if (pend_pre <= s->threshold_size) {
+        qemu_savevm_state_pending_exact(&pend_pre, &pend_post);
+        pending_size = pend_pre + pend_post;
+        trace_migrate_pending_exact(pending_size, pend_pre, pend_post);
     }
 
     if (!pending_size || pending_size < s->threshold_size) {
diff --git a/migration/ram.c b/migration/ram.c
index 85ccbf88ad..abd960659c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3437,7 +3437,6 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 
 static void ram_state_pending_estimate(void *opaque,
                                        uint64_t *res_precopy_only,
-                                       uint64_t *res_compatible,
                                        uint64_t *res_postcopy_only)
 {
     RAMState **temp = opaque;
@@ -3455,7 +3454,6 @@ static void ram_state_pending_estimate(void *opaque,
 
 static void ram_state_pending_exact(void *opaque,
                                     uint64_t *res_precopy_only,
-                                    uint64_t *res_compatible,
                                     uint64_t *res_postcopy_only)
 {
     RAMState **temp = opaque;
diff --git a/migration/savevm.c b/migration/savevm.c
index e9cf4999ad..2943b8301b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1542,13 +1542,11 @@ flush:
  * for units that can't do postcopy.
  */
 void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only,
-                                        uint64_t *res_compatible,
                                         uint64_t *res_postcopy_only)
 {
     SaveStateEntry *se;
 
     *res_precopy_only = 0;
-    *res_compatible = 0;
     *res_postcopy_only = 0;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
@@ -1561,19 +1559,17 @@ void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only,
             }
         }
         se->ops->state_pending_exact(se->opaque,
-                                     res_precopy_only, res_compatible,
+                                     res_precopy_only,
                                      res_postcopy_only);
     }
 }
 
 void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only,
-                                     uint64_t *res_compatible,
                                      uint64_t *res_postcopy_only)
 {
     SaveStateEntry *se;
 
     *res_precopy_only = 0;
-    *res_compatible = 0;
     *res_postcopy_only = 0;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
@@ -1586,7 +1582,7 @@ void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only,
             }
         }
         se->ops->state_pending_estimate(se->opaque,
-                                        res_precopy_only, res_compatible,
+                                        res_precopy_only,
                                         res_postcopy_only);
     }
 }
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 52de1c84f8..90a8aecb37 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -157,7 +157,7 @@ vfio_save_cleanup(const char *name) " (%s)"
 vfio_save_buffer(const char *name, uint64_t data_offset, uint64_t data_size, uint64_t pending) " (%s) Offset 0x%"PRIx64" size 0x%"PRIx64" pending 0x%"PRIx64
 vfio_update_pending(const char *name, uint64_t pending) " (%s) pending 0x%"PRIx64
 vfio_save_device_config_state(const char *name) " (%s)"
-vfio_state_pending(const char *name, uint64_t precopy, uint64_t postcopy, uint64_t compatible) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64" compatible 0x%"PRIx64
+vfio_state_pending(const char *name, uint64_t precopy, uint64_t postcopy) " (%s) precopy 0x%"PRIx64" postcopy 0x%"PRIx64
 vfio_save_iterate(const char *name, int data_size) " (%s) data_size %d"
 vfio_save_complete_precopy(const char *name) " (%s)"
 vfio_load_device_config_state(const char *name) " (%s)"
diff --git a/migration/trace-events b/migration/trace-events
index 67b65a70ff..422823b1db 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -150,8 +150,8 @@ migrate_fd_cleanup(void) ""
 migrate_fd_error(const char *error_desc) "error=%s"
 migrate_fd_cancel(void) ""
 migrate_handle_rp_req_pages(const char *rbname, size_t start, size_t len) "in %s at 0x%zx len 0x%zx"
-migrate_pending_exact(uint64_t size, uint64_t pre, uint64_t compat, uint64_t post) "exact pending size %" PRIu64 " (pre = %" PRIu64 " compat=%" PRIu64 " post=%" PRIu64 ")"
-migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t compat, uint64_t post) "estimate pending size %" PRIu64 " (pre = %" PRIu64 " compat=%" PRIu64 " post=%" PRIu64 ")"
+migrate_pending_exact(uint64_t size, uint64_t pre, uint64_t post) "exact pending size %" PRIu64 " (pre = %" PRIu64 " post=%" PRIu64 ")"
+migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate pending size %" PRIu64 " (pre = %" PRIu64 " post=%" PRIu64 ")"
 migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
 migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64
 migration_completion_file_err(void) ""
-- 
2.39.1

Re: [PATCH 2/3] migration: Remove unused res_compatible

[Vladimir Sementsov-Ogievskiy]

On 08.02.23 16:57, Juan Quintela wrote:
>   {
> -    uint64_t pend_pre, pend_compat, pend_post;
> +    uint64_t pend_pre, pend_post;
>       bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
>   
> -    qemu_savevm_state_pending_estimate(&pend_pre, &pend_compat, &pend_post);
> -    uint64_t pending_size = pend_pre + pend_compat + pend_post;
> +    qemu_savevm_state_pending_estimate(&pend_pre, &pend_post);
> +    uint64_t pending_size = pend_pre + pend_post;

Mixed declarations are "gnerally not allowed" by devel/style.rst.. Preexisting, but we may fix it now.

Anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH 2/3] migration: Remove unused res_compatible

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 08.02.23 16:57, Juan Quintela wrote:
>>   {
>> -    uint64_t pend_pre, pend_compat, pend_post;
>> +    uint64_t pend_pre, pend_post;
>>       bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
>>   -    qemu_savevm_state_pending_estimate(&pend_pre, &pend_compat,
>> &pend_post);
>> -    uint64_t pending_size = pend_pre + pend_compat + pend_post;
>> +    qemu_savevm_state_pending_estimate(&pend_pre, &pend_post);
>> +    uint64_t pending_size = pend_pre + pend_post;
>
> Mixed declarations are "gnerally not allowed" by devel/style.rst.. Preexisting, but we may fix it now.

They are used left and right.
But you are right.  Instead to change my code, I have sent a proposal to
change devel/style.rst.

Discuss it there O:-)

> Anyway:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks.

[PATCH 3/3] migration: Remove _only suffix for res_postcopy/precopy

[Juan Quintela]

Once that res_compatible is removed, they don't make sense anymore.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/register.h   | 18 ++++++++----------
 migration/savevm.h             |  8 ++++----
 hw/s390x/s390-stattrib.c       |  7 +++----
 hw/vfio/migration.c            | 10 ++++------
 migration/block-dirty-bitmap.c |  6 +++---
 migration/block.c              |  7 +++----
 migration/ram.c                | 18 ++++++++----------
 migration/savevm.c             | 24 ++++++++++--------------
 8 files changed, 43 insertions(+), 55 deletions(-)

diff --git a/include/migration/register.h b/include/migration/register.h
index a958a92a0f..4a4a6d7174 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -47,22 +47,20 @@ typedef struct SaveVMHandlers {
     /* This runs outside the iothread lock!  */
     int (*save_setup)(QEMUFile *f, void *opaque);
     /* Note for save_live_pending:
-     * - res_precopy_only is for data which must be migrated in precopy phase
+     * - res_precopy is for data which must be migrated in precopy phase
      *     or in stopped state, in other words - before target vm start
-     * - res_postcopy_only is for data which must be migrated in postcopy phase
+     * - res_postcopy is for data which must be migrated in postcopy phase
      *     or in stopped state, in other words - after source vm stop
      *
-     * Sum of res_postcopy_only and res_postcopy_only is the whole
-     * amount of pending data.
+     * Sum of res_postcopy and res_postcopy is the whole amount of
+     * pending data.
      */
     /* This estimates the remaining data to transfer */
-    void (*state_pending_estimate)(void *opaque,
-                                   uint64_t *res_precopy_only,
-                                   uint64_t *res_postcopy_only);
+    void (*state_pending_estimate)(void *opaque, uint64_t *res_precopy,
+                                   uint64_t *res_postcopy);
     /* This calculate the exact remaining data to transfer */
-    void (*state_pending_exact)(void *opaque,
-                                uint64_t *res_precopy_only,
-                                uint64_t *res_postcopy_only);
+    void (*state_pending_exact)(void *opaque, uint64_t *res_precopy,
+                                uint64_t *res_postcopy);
     LoadStateHandler *load_state;
     int (*load_setup)(QEMUFile *f, void *opaque);
     int (*load_cleanup)(void *opaque);
diff --git a/migration/savevm.h b/migration/savevm.h
index bd625a644b..24f2d2a28b 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -40,10 +40,10 @@ void qemu_savevm_state_cleanup(void);
 void qemu_savevm_state_complete_postcopy(QEMUFile *f);
 int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
                                        bool inactivate_disks);
-void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only,
-                                     uint64_t *res_postcopy_only);
-void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only,
-                                        uint64_t *res_postcopy_only);
+void qemu_savevm_state_pending_exact(uint64_t *res_precopy,
+                                     uint64_t *res_postcopy);
+void qemu_savevm_state_pending_estimate(uint64_t *res_precopy,
+                                        uint64_t *res_postcopy);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
 int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index c7ae9184ab..dfb95eb20c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -182,16 +182,15 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
     return 0;
 }
 
-static void cmma_state_pending(void *opaque,
-                               uint64_t *res_precopy_only,
-                               uint64_t *res_postcopy_only)
+static void cmma_state_pending(void *opaque, uint64_t *res_precopy,
+                               uint64_t *res_postcopy)
 {
     S390StAttribState *sas = S390_STATTRIB(opaque);
     S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
     long long res = sac->get_dirtycount(sas);
 
     if (res >= 0) {
-        *res_precopy_only += res;
+        *res_precopy += res;
     }
 }
 
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index fb0dd9571d..d8c913acde 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -456,9 +456,8 @@ static void vfio_save_cleanup(void *opaque)
     trace_vfio_save_cleanup(vbasedev->name);
 }
 
-static void vfio_state_pending(void *opaque,
-                               uint64_t *res_precopy_only,
-                               uint64_t *res_postcopy_only)
+static void vfio_state_pending(void *opaque, uint64_t *res_precopy,
+                               uint64_t *res_postcopy)
 {
     VFIODevice *vbasedev = opaque;
     VFIOMigration *migration = vbasedev->migration;
@@ -469,10 +468,9 @@ static void vfio_state_pending(void *opaque,
         return;
     }
 
-    *res_precopy_only += migration->pending_bytes;
+    *res_precopy += migration->pending_bytes;
 
-    trace_vfio_state_pending(vbasedev->name, *res_precopy_only,
-                            *res_postcopy_only);
+    trace_vfio_state_pending(vbasedev->name, *res_precopy, *res_postcopy);
 }
 
 static int vfio_save_iterate(QEMUFile *f, void *opaque)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 9c6655e11a..793bf4347d 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -763,8 +763,8 @@ static int dirty_bitmap_save_complete(QEMUFile *f, void *opaque)
 }
 
 static void dirty_bitmap_state_pending(void *opaque,
-                                       uint64_t *res_precopy_only,
-                                       uint64_t *res_postcopy_only)
+                                       uint64_t *res_precopy,
+                                       uint64_t *res_postcopy)
 {
     DBMSaveState *s = &((DBMState *)opaque)->save;
     SaveBitmapState *dbms;
@@ -784,7 +784,7 @@ static void dirty_bitmap_state_pending(void *opaque,
 
     trace_dirty_bitmap_state_pending(pending);
 
-    *res_postcopy_only += pending;
+    *res_postcopy += pending;
 }
 
 /* First occurrence of this bitmap. It should be created if doesn't exist */
diff --git a/migration/block.c b/migration/block.c
index 99d149904c..21bcf560e0 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -863,9 +863,8 @@ static int block_save_complete(QEMUFile *f, void *opaque)
     return 0;
 }
 
-static void block_state_pending(void *opaque,
-                                uint64_t *res_precopy_only,
-                                uint64_t *res_postcopy_only)
+static void block_state_pending(void *opaque, uint64_t *res_precopy,
+                                uint64_t *res_postcopy)
 {
     /* Estimate pending number of bytes to send */
     uint64_t pending;
@@ -886,7 +885,7 @@ static void block_state_pending(void *opaque,
 
     trace_migration_block_state_pending(pending);
     /* We don't do postcopy */
-    *res_precopy_only += pending;
+    *res_precopy += pending;
 }
 
 static int block_load(QEMUFile *f, void *opaque, int version_id)
diff --git a/migration/ram.c b/migration/ram.c
index abd960659c..add5c5edfc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3435,9 +3435,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     return 0;
 }
 
-static void ram_state_pending_estimate(void *opaque,
-                                       uint64_t *res_precopy_only,
-                                       uint64_t *res_postcopy_only)
+static void ram_state_pending_estimate(void *opaque, uint64_t *res_precopy,
+                                       uint64_t *res_postcopy)
 {
     RAMState **temp = opaque;
     RAMState *rs = *temp;
@@ -3446,15 +3445,14 @@ static void ram_state_pending_estimate(void *opaque,
 
     if (migrate_postcopy_ram()) {
         /* We can do postcopy, and all the data is postcopiable */
-        *res_postcopy_only += remaining_size;
+        *res_postcopy += remaining_size;
     } else {
-        *res_precopy_only += remaining_size;
+        *res_precopy += remaining_size;
     }
 }
 
-static void ram_state_pending_exact(void *opaque,
-                                    uint64_t *res_precopy_only,
-                                    uint64_t *res_postcopy_only)
+static void ram_state_pending_exact(void *opaque, uint64_t *res_precopy,
+                                    uint64_t *res_postcopy)
 {
     RAMState **temp = opaque;
     RAMState *rs = *temp;
@@ -3472,9 +3470,9 @@ static void ram_state_pending_exact(void *opaque,
 
     if (migrate_postcopy_ram()) {
         /* We can do postcopy, and all the data is postcopiable */
-        *res_postcopy_only += remaining_size;
+        *res_postcopy += remaining_size;
     } else {
-        *res_precopy_only += remaining_size;
+        *res_precopy += remaining_size;
     }
 }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 2943b8301b..9d3d971a50 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1541,13 +1541,13 @@ flush:
  * the result is split into the amount for units that can and
  * for units that can't do postcopy.
  */
-void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only,
-                                        uint64_t *res_postcopy_only)
+void qemu_savevm_state_pending_estimate(uint64_t *res_precopy,
+                                        uint64_t *res_postcopy)
 {
     SaveStateEntry *se;
 
-    *res_precopy_only = 0;
-    *res_postcopy_only = 0;
+    *res_precopy = 0;
+    *res_postcopy = 0;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->state_pending_exact) {
@@ -1558,19 +1558,17 @@ void qemu_savevm_state_pending_estimate(uint64_t *res_precopy_only,
                 continue;
             }
         }
-        se->ops->state_pending_exact(se->opaque,
-                                     res_precopy_only,
-                                     res_postcopy_only);
+        se->ops->state_pending_exact(se->opaque, res_precopy, res_postcopy);
     }
 }
 
-void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only,
-                                     uint64_t *res_postcopy_only)
+void qemu_savevm_state_pending_exact(uint64_t *res_precopy,
+                                     uint64_t *res_postcopy)
 {
     SaveStateEntry *se;
 
-    *res_precopy_only = 0;
-    *res_postcopy_only = 0;
+    *res_precopy = 0;
+    *res_postcopy = 0;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops || !se->ops->state_pending_estimate) {
@@ -1581,9 +1579,7 @@ void qemu_savevm_state_pending_exact(uint64_t *res_precopy_only,
                 continue;
             }
         }
-        se->ops->state_pending_estimate(se->opaque,
-                                        res_precopy_only,
-                                        res_postcopy_only);
+        se->ops->state_pending_estimate(se->opaque, res_precopy, res_postcopy);
     }
 }
 
-- 
2.39.1

Re: [PATCH 3/3] migration: Remove _only suffix for res_postcopy/precopy

[Vladimir Sementsov-Ogievskiy]

On 08.02.23 16:57, Juan Quintela wrote:
> Once that res_compatible is removed, they don't make sense anymore.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   include/migration/register.h   | 18 ++++++++----------
>   migration/savevm.h             |  8 ++++----
>   hw/s390x/s390-stattrib.c       |  7 +++----
>   hw/vfio/migration.c            | 10 ++++------
>   migration/block-dirty-bitmap.c |  6 +++---
>   migration/block.c              |  7 +++----
>   migration/ram.c                | 18 ++++++++----------
>   migration/savevm.c             | 24 ++++++++++--------------
>   8 files changed, 43 insertions(+), 55 deletions(-)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index a958a92a0f..4a4a6d7174 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers {
>       /* This runs outside the iothread lock!  */
>       int (*save_setup)(QEMUFile *f, void *opaque);
>       /* Note for save_live_pending:
> -     * - res_precopy_only is for data which must be migrated in precopy phase
> +     * - res_precopy is for data which must be migrated in precopy phase
>        *     or in stopped state, in other words - before target vm start
> -     * - res_postcopy_only is for data which must be migrated in postcopy phase
> +     * - res_postcopy is for data which must be migrated in postcopy phase
>        *     or in stopped state, in other words - after source vm stop


That's now wrong. "postcopy" is everything except "precopy", as it includes "compat". Really, for RAM, it can be copied in precopy too, and it is copied in precopy until user run command migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot migrate in precopy at all, it migrate only in stopped state or in postcopy).

So, finally:

"precopy"

   definition:
   - must be migrated in precopy or in stopped state
   - in other words: must be migrated before target start
   - in other words: can't be migrated in postcopy
   - in other words: can't be migrated after target start

"postcopy"

   definition:
   - can migrate in postcopy
   - in other words: can migrate after target start
   
   some properties:
   - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap)
   - of course, can be migrated in stopped state


To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable".


-- 
Best regards,
Vladimir

unsubscribe RE: [PATCH 3/3] migration: Remove _only suffix for res_postcopy/precopy

[McDowell, Jadon]

-----Original Message-----
From: qemu-s390x-bounces+jadon.mcdowell=optum.com@nongnu.org <qemu-s390x-bounces+jadon.mcdowell=optum.com@nongnu.org> On Behalf Of Vladimir Sementsov-Ogievskiy
Sent: Tuesday, February 14, 2023 9:27 AM
To: Juan Quintela <quintela@redhat.com>; qemu-devel@nongnu.org
Cc: Ilya Leoshkevich <iii@linux.ibm.com>; qemu-block@nongnu.org; Alex Williamson <alex.williamson@redhat.com>; Fam Zheng <fam@euphon.net>; Eric Blake <eblake@redhat.com>; Dr. David Alan Gilbert <dgilbert@redhat.com>; Stefan Hajnoczi <stefanha@redhat.com>; Halil Pasic <pasic@linux.ibm.com>; David Hildenbrand <david@redhat.com>; Thomas Huth <thuth@redhat.com>; Eric Farman <farman@linux.ibm.com>; qemu-s390x@nongnu.org; John Snow <jsnow@redhat.com>; Christian Borntraeger <borntraeger@linux.ibm.com>; Richard Henderson <richard.henderson@linaro.org>
Subject: Re: [PATCH 3/3] migration: Remove _only suffix for res_postcopy/precopy

On 08.02.23 16:57, Juan Quintela wrote:
> Once that res_compatible is removed, they don't make sense anymore.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   include/migration/register.h   | 18 ++++++++----------
>   migration/savevm.h             |  8 ++++----
>   hw/s390x/s390-stattrib.c       |  7 +++----
>   hw/vfio/migration.c            | 10 ++++------
>   migration/block-dirty-bitmap.c |  6 +++---
>   migration/block.c              |  7 +++----
>   migration/ram.c                | 18 ++++++++----------
>   migration/savevm.c             | 24 ++++++++++--------------
>   8 files changed, 43 insertions(+), 55 deletions(-)
> 
> diff --git a/include/migration/register.h 
> b/include/migration/register.h index a958a92a0f..4a4a6d7174 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers {
>       /* This runs outside the iothread lock!  */
>       int (*save_setup)(QEMUFile *f, void *opaque);
>       /* Note for save_live_pending:
> -     * - res_precopy_only is for data which must be migrated in precopy phase
> +     * - res_precopy is for data which must be migrated in precopy 
> + phase
>        *     or in stopped state, in other words - before target vm start
> -     * - res_postcopy_only is for data which must be migrated in postcopy phase
> +     * - res_postcopy is for data which must be migrated in postcopy 
> + phase
>        *     or in stopped state, in other words - after source vm stop


That's now wrong. "postcopy" is everything except "precopy", as it includes "compat". Really, for RAM, it can be copied in precopy too, and it is copied in precopy until user run command migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot migrate in precopy at all, it migrate only in stopped state or in postcopy).

So, finally:

"precopy"

   definition:
   - must be migrated in precopy or in stopped state
   - in other words: must be migrated before target start
   - in other words: can't be migrated in postcopy
   - in other words: can't be migrated after target start

"postcopy"

   definition:
   - can migrate in postcopy
   - in other words: can migrate after target start
   
   some properties:
   - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap)
   - of course, can be migrated in stopped state


To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable".


--
Best regards,
Vladimir

This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or intended recipient’s authorized agent, the reader is hereby
notified that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

Re: [PATCH 3/3] migration: Remove _only suffix for res_postcopy/precopy

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> On 08.02.23 16:57, Juan Quintela wrote:
>> Once that res_compatible is removed, they don't make sense anymore.
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   include/migration/register.h   | 18 ++++++++----------
>>   migration/savevm.h             |  8 ++++----
>>   hw/s390x/s390-stattrib.c       |  7 +++----
>>   hw/vfio/migration.c            | 10 ++++------
>>   migration/block-dirty-bitmap.c |  6 +++---
>>   migration/block.c              |  7 +++----
>>   migration/ram.c                | 18 ++++++++----------
>>   migration/savevm.c             | 24 ++++++++++--------------
>>   8 files changed, 43 insertions(+), 55 deletions(-)
>> diff --git a/include/migration/register.h
>> b/include/migration/register.h
>> index a958a92a0f..4a4a6d7174 100644
>> --- a/include/migration/register.h
>> +++ b/include/migration/register.h
>> @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers {
>>       /* This runs outside the iothread lock!  */
>>       int (*save_setup)(QEMUFile *f, void *opaque);
>>       /* Note for save_live_pending:
>> -     * - res_precopy_only is for data which must be migrated in precopy phase
>> +     * - res_precopy is for data which must be migrated in precopy phase
>>        *     or in stopped state, in other words - before target vm start
>> -     * - res_postcopy_only is for data which must be migrated in postcopy phase
>> +     * - res_postcopy is for data which must be migrated in postcopy phase
>>        *     or in stopped state, in other words - after source vm stop
>
>
> That's now wrong. "postcopy" is everything except "precopy", as it
> includes "compat". Really, for RAM, it can be copied in precopy too,
> and it is copied in precopy until user run command
> migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot
> migrate in precopy at all, it migrate only in stopped state or in
> postcopy).
>
> So, finally:
>
> "precopy"
>
>   definition:
>   - must be migrated in precopy or in stopped state
>   - in other words: must be migrated before target start
>   - in other words: can't be migrated in postcopy
>   - in other words: can't be migrated after target start
>
> "postcopy"
>
>   definition:
>   - can migrate in postcopy
>   - in other words: can migrate after target start
>      some properties:
>   - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap)
>   - of course, can be migrated in stopped state

What about (with latest naming)

must_precopy:
* must be migrated in precopy or in stopped state
* i.e. must be migrated before target start

can_postcopy:
* can migrate in postcopy or in stopped state
* i.e. can migrate after target start
* some can also be migrated during precopy (RAM)
* some must be migrated after source stops (block-dirty-bitmap)

> To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable".

I hate negatives when naming variables.

What about: must_precopy and can_postcopy?

Later, Juan.

Re: [PATCH 3/3] migration: Remove _only suffix for res_postcopy/precopy

[Vladimir Sementsov-Ogievskiy]

On 14.02.23 21:22, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>> On 08.02.23 16:57, Juan Quintela wrote:
>>> Once that res_compatible is removed, they don't make sense anymore.
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>    include/migration/register.h   | 18 ++++++++----------
>>>    migration/savevm.h             |  8 ++++----
>>>    hw/s390x/s390-stattrib.c       |  7 +++----
>>>    hw/vfio/migration.c            | 10 ++++------
>>>    migration/block-dirty-bitmap.c |  6 +++---
>>>    migration/block.c              |  7 +++----
>>>    migration/ram.c                | 18 ++++++++----------
>>>    migration/savevm.c             | 24 ++++++++++--------------
>>>    8 files changed, 43 insertions(+), 55 deletions(-)
>>> diff --git a/include/migration/register.h
>>> b/include/migration/register.h
>>> index a958a92a0f..4a4a6d7174 100644
>>> --- a/include/migration/register.h
>>> +++ b/include/migration/register.h
>>> @@ -47,22 +47,20 @@ typedef struct SaveVMHandlers {
>>>        /* This runs outside the iothread lock!  */
>>>        int (*save_setup)(QEMUFile *f, void *opaque);
>>>        /* Note for save_live_pending:
>>> -     * - res_precopy_only is for data which must be migrated in precopy phase
>>> +     * - res_precopy is for data which must be migrated in precopy phase
>>>         *     or in stopped state, in other words - before target vm start
>>> -     * - res_postcopy_only is for data which must be migrated in postcopy phase
>>> +     * - res_postcopy is for data which must be migrated in postcopy phase
>>>         *     or in stopped state, in other words - after source vm stop
>>
>>
>> That's now wrong. "postcopy" is everything except "precopy", as it
>> includes "compat". Really, for RAM, it can be copied in precopy too,
>> and it is copied in precopy until user run command
>> migrate-start-postcopy. (In contrast: block-dirty-bitmap cannot
>> migrate in precopy at all, it migrate only in stopped state or in
>> postcopy).
>>
>> So, finally:
>>
>> "precopy"
>>
>>    definition:
>>    - must be migrated in precopy or in stopped state
>>    - in other words: must be migrated before target start
>>    - in other words: can't be migrated in postcopy
>>    - in other words: can't be migrated after target start
>>
>> "postcopy"
>>
>>    definition:
>>    - can migrate in postcopy
>>    - in other words: can migrate after target start
>>       some properties:
>>    - probably can be migrated in precopy (like RAM), or, may be not (like block-dirty-bitmap)
>>    - of course, can be migrated in stopped state
> 
> What about (with latest naming)
> 
> must_precopy:
> * must be migrated in precopy or in stopped state
> * i.e. must be migrated before target start
> 
> can_postcopy:
> * can migrate in postcopy or in stopped state
> * i.e. can migrate after target start
> * some can also be migrated during precopy (RAM)
> * some must be migrated after source stops (block-dirty-bitmap)

Sounds very good, I'm for!

> 
>> To be absolutely clear, we may rename them to "not_postcopyable" and "postcopyable".
> 
> I hate negatives when naming variables.

Agree.

> 
> What about: must_precopy and can_postcopy?
> 

Sounds good!)

-- 
Best regards,
Vladimir