Re: [PATCH v1 3/4] migration: refactor ram_save_target_page functions

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> On Wed, 27 Nov 2024 at 02:49, Fabiano Rosas <farosas@suse.de> wrote:
>> This patch should be just the actual refactoring on top of master, with
>> no mention to postcopy at all.
>
> * Okay. We'll have to ensure that it is merged before multifd+postcopy change.

That's ok, just put it at the start of the series.

>
>> > +            if (migrate_multifd() && !migration_in_postcopy()
>> > +                && (!migrate_multifd_flush_after_each_section()
>> > +                    || migrate_mapped_ram())) {
>>
>> This is getting out of hand. We can't keep growing this condition every
>> time something new comes up. Any ideas?
>
> * In 'v0' this series, !migration_in_postcopy() was added to
> migrate_multifd(), which worked, but was not okay.
>
> * Another could be to define a new helper/macro to include above 3-4
> checks. Because migrate_multifd(),
> migrate_multifd_flush_after_each_section() and migrate_mapped_ram()
> appear together at multiple points. But strangely the equation is not
> the same. Sometimes migrate_mapped_ram() is 'true' and sometimes it is
> 'false', so a combined helper may not work.  It is all to accommodate
> different workings of multifd IIUC, if it and precopy used the same
> protocol/stream, maybe such conditionals would go away automatically.
>
>> Yes, although I wonder if we should keep documenting this when we only
>> call this function for a single page and it always returns at most 1.
>> >          if (page_dirty) {
>> >              /* Be strict to return code; it must be 1, or what else? */
>>
>> ... this^ comment, for instance.
>>
>
> * Okay, can remove them.
>
> So to confirm: this refactoring patch should be a separate one by
> itself? And then in the following multifd+postcopy series we just add
> !migration_in_postcopy() check to it?

It doesn't need to be a single patch submission, it could be a patch at
the start of this series. It's just that it needs to logically do only
one thing, which is to move the code around without adding new bits that
don't exist in current master (a strict definition of refactoring). The
multifd+postcopy changes come in a subsequent patch so it's clear that
one patch was just shuffling code around while the rest is part of the
feature enablement.

>
> Thank you.
> ---
>   - Prasad