Prasad Pandit <ppan...@redhat.com> writes:
> Hello Fabiano,
>
> On Tue, 18 Feb 2025 at 19:52, Fabiano Rosas <faro...@suse.de> wrote:
>> Do you concede that this code has a hidden assumption? Either that
>> migrate_multifd() != migrate_postcopy_preempt() or that multifd channels
>> must be set up before postcopy preempt channel? Because that is enough
>> for us to have to do something about it. Either restructuring or a
>> comment explaining.
>
> * Not a hidden assumption, but it is an observation that 'main' and
> 'multifd' channels are established before 'postcopy' ones. And for new
> migration to start, it is necessary that 'main' and 'multifd' channels
> (when enabled) are established before migration starts.
>
>> > * When does postcopy preempt channel creation race with the multifd
>> > channel creation?
>>
>> For instance, postcopy_do_resume() has this comment:
>> /*
>> * If preempt is enabled, re-establish the preempt channel. Note that
>> * we do it after resume prepare to make sure the main channel will be
>> * created before the preempt channel. E.g. with weak network, the
>> * dest QEMU may get messed up with the preempt and main channels on
>> * the order of connection setup. This guarantees the correct order.
>> */
>> It looks like if the main channel can race, so do the multifd channels,
>> no? In any case, I'm fine with just documenting any assumption for now.
>
> * The first requirement for this race to occur is that two types of
> channels are created together at the same time. Let's see:
>
> * Postcopy migration: without multifd enabled
> - 'main' channel is created before the migration starts. And
> 'postcopy' channels are created towards the end of precopy migration,
> when the Postcopy phase starts. So in this scenario the race does not
> happen.
>
> * Postcopy resume: without multifd enabled
> - As described in the comment above, preempt channel is created
> _after_ the 'main' channel to avoid the race condition.
>
> * Postcopy migration: with multifd enabled
> - 'main' and 'multifd' channels are created before migration
> starts. And 'postcopy' channels are created towards the end of precopy
> migration, when the Postcopy phase starts. No race occurs.
>
> * Postcopy resume: with multifd enabled
> - 'multifd' channels are shutdown before Postcopy starts, ie. no
> 'multifd' channels exist during Postcopy resume. So no race between
> 'postcopy' and 'multifd' channels.
> - And 'postcopy' channels are created after the 'main' channel
> to avoid the race between them.
> - postcopy_do_resume() does not seem to create 'multifd' channels.
>
> * Multifd migration: without Postcopy enabled
> - 'main' and 'multifd' channels are created before the migration
> starts. They both send 'magic value' bytes, so are easier to
> differentiate. No race occurs.
>
I don't see anything stopping postcopy_start() from being called in the
source in relation to multifd recv threads being setup in the
destination. So far it seems possible that the source is opening the
preempt channel while multifd still hasn't seen all threads. There's
also pre-7.2 machines which create the postcopy channel early.
>
>> > * migration_needs_multiple_sockets() => return migrate_multifd() ||
>> > migrate_postcopy_preempt();
>> >
>> Nope, this is just saying whether a single channel is expected, or more
>> than one.
>
> * If we read it as a question:
> - migration_needs_multiple_sockets() ? True => Yes, migration
> needs multiple sockets.
> - migration_needs_multiple_sockets() ? False => No, migration does
> not need multiple sockets.
>
> Then it should return 'True' when both migrate_multifd() and
> postcopy_preempt() are enabled.
>
Why?
>>That's why I think it would be a good gate for this peeking
>> code. Since postcopy preempt could be a peekable channel, it's
>> misleading to put it all behind QIO_CHANNEL_FEATURE_READ_MSG_PEEK
>> only. This is a time-bomb for the next person to refactor this code.
>
> * Postcopy preempt could be a peekable channel ? Currently it does not
> send magic value, does it?
>
Peekable means it can read ahead and rollback without consuming that
part of the stream. We need it because there's code later on that will
validate the MAGIC.
>> Right, but that's not what we have today. Changing this requires
>> figuring out how to keep the stream compatible when channels now start
>> sending extra stuff at the start. It's not trivial. There's also
>> mapped-ram which is asynchronous and there might be something special to
>> be done about the TLS handshake, I'm not sure.
>
> * True, it's not trivial.
>
>> Well, aside from preempt, they're *not* dependent on the order. That's
>> the point of having to do all of this dance. In fact we might be better
>> off if we could serialize the connections somehow.
>>
>> I havent't followed this series closely, could you point me to the
>> discussion that led to the channels concept being introduced?
>
> * Channels concept was not introduced in this series. It has been
> there since the beginning, no?
>
I thought you meant the CH_MAIN stuff. So now I don't know what you
mean. You want to do away with multifd?
>> Yes. They *can* be used without multifd. The comment would explain that
>> at that point in the code, these are the only types possible. So as to
>> not mislead future readers that whenever tls/file, then multifd must be
>> used.
> ....
>> See? Multifd mutually exclusive with postcopy preempt. You carried that
>> assumption (well done), but made it more subtle (not good), since
>> if/else is by definition showing the relationship between the two while
>> migration_has_main_and_multifd_channels() makes it hidden under the
>> multifd check allowing the last return true to happen.
>>
>> If we're enabling multifd along with postcopy, we need to be aware that
>> the relationship with preempt might not hold true anymore.
>
> * Sorry, I did not get that. Enabling them together means that they
> are _not_ exclusive, no? It is not Either 'multifd' OR 'postcopy'
> case, anymore.
>
Yes, as we're seeing, there's this assumption that multifd is never
enabled along with postcopy_preempt. Now with multifd+postcopy we need
to be careful to stop the places where that assumption was made.
>>>Not sure if/how that works really. It is possible that currently
>>> these (tls/file) channels are used only with migrate_multifd() enabled
>>> and so are processed with multifd_recv_new_channel() function. The
>>> current patch handles them the same way.
>>
>> That's the entire point I'm making when I ask to not omit the else
>> clauses.
>
> * ie. we set 'channel = CH_MAIN' in the final else clause as well? - Okay.
>
>> Do you think this series can work without touching the channel discovery
>> code? As I said earlier, I'm missing a bit of context, but to me it
>> seems it cannot.
>
> * The reason we need to touch the channel discovery part is: with
> 'multifd' and 'postcopy' both enabled, towards the end of migration,
> when 'postcopy' connection comes in
> migration_ioc_process_incoming(...)
> {
> if (migrate_multifd() && !migrate_mapped_ram() &&
> !migrate_postcopy_ram() &&
> qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK))
> {
> ...
> } else {
> default_channel = !mis->from_src_file;
> }
> ...
> if (migrate_multifd()) {
> multifd_recv_new_channel(ioc, &local_err);
> } else {
> postcopy_preempt_new_channel(mis, f);
> }
> }
>
> * The first 'if (migrate_multifd() ... !migrate_postcopy_ram()')
> evaluates to false, in the else part 'default_channel' also evaluates
> to false, because the 'main' channel is established. Now the new
> incoming connection falls in the second migrate_multifd() block and
> gets processed via - multifd_recv_new_channel(ioc, &local_err); call
> and migration would not complete/finish.
>
> * To identify the incoming postcopy connection, in the very first
> version of this series, a magic value for the postcopy channel was
> introduced and everything else remained the same.
>
> * Would that be an acceptable solution for now?
>
Continue with this patch and fix the stuff I mentioned. You can ignore
the first two paragraphs of that reply.
https://lore.kernel.org/r/87y0y4tf5q....@suse.de
I still think we need to test that preempt + multifd scenario, but it
should be easy to write a test for that once the series is in more of a
final shape.
>> If instead of this refactoring you want to start working on a model for
>> consistent channel advertisement, then that's fine. But we'll have to
>> put this series on hold (which is also fine). It also looks like it
>> could be considerably more work, although I haven't looked at it in
>> detail. Granted, it's work that makes sense, instead of the heuristics
>> we have today.
>
> * IMHO, we need not put this series on hold, for now we could go ahead
> with the postcopy magic value patch if that works. And the larger
> overhaul of the channel discovery part could be done as a separate
> series of its own.
>
We can't add magic values, as we've discussed.
> Thank you.
> ---
> - Prasad
