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